Merge pull request #446 from ThisIsFineTM/439-fix-normalize_link

Fix buffer overrun in link normalisation
openzim · Jan 17, 2025 · b4a1fa0 · b4a1fa0
2 parents 768cbe5 + 491caf4
commit b4a1fa0
Show file tree

Hide file tree

Showing 2 changed files with 39 additions and 8 deletions.
diff --git a/src/tools.cpp b/src/tools.cpp
@@ -404,23 +404,24 @@ std::string normalize_link(const std::string& input, const std::string& baseUrl)
     output.reserve(baseUrl.size() + input.size() + 1);
 
     bool check_rel = false;
-    const char* p = input.c_str();
+    auto p = input.cbegin();
     if ( *(p) == '/') {
       // This is an absolute url.
       p++;
     } else {
       //This is a relative url, use base url
       output = baseUrl;
-      if (!output.empty() && output.back() != '/')
+      if (!output.empty() && output.back() != '/') {
           output += '/';
+      }
       check_rel = true;
     }
 
     //URL Decoding.
-    while (*p)
+    while (p < input.cend())
     {
         if ( check_rel ) {
-            if (strncmp(p, "../", 3) == 0) {
+            if (std::strncmp(&*p, "../", 3) == 0) {
                 // We must go "up"
                 // Remove the '/' at the end of output.
                 output.resize(output.size()-1);
@@ -432,7 +433,7 @@ std::string normalize_link(const std::string& input, const std::string& baseUrl)
                 check_rel = false;
                 continue;
             }
-            if (strncmp(p, "./", 2) == 0) {
+            if (std::strncmp(&*p, "./", 2) == 0) {
                 // We must simply skip this part
                 // Simply move after the ".".
                 p += 2;
@@ -449,8 +450,13 @@ std::string normalize_link(const std::string& input, const std::string& baseUrl)
 
         if ( *p == '%')
         {
-            char ch;
-            sscanf(p+1, "%2hhx", &ch);
+            if( (p+3) >= input.cend()){
+                // if the %XX token would go off the end of the string, just break
+                break;
+            }
+            // hhx only officially supports hex unsigned char
+            unsigned char ch = 0;
+            std::sscanf(&*(p+1), "%2hhx", &ch);
             output += ch;
             p += 3;
             continue;

diff --git a/test/tools-test.cpp b/test/tools-test.cpp
@@ -236,6 +236,10 @@ TEST(tools, isOutofBounds)
 
 TEST(tools, normalize_link)
 {
+    ASSERT_EQ(normalize_link("", ""), "");
+    ASSERT_EQ(normalize_link("/", ""), "");
+    ASSERT_EQ(normalize_link("", "/"), "/");
+
     ASSERT_EQ(normalize_link("/a", "/b"), "a");
 
     // not absolute
@@ -250,6 +254,28 @@ TEST(tools, normalize_link)
 
     // URI-decoding is performed
     ASSERT_EQ(normalize_link("/%41%62c", "/"), "Abc");
+
+    // #439: normalized link reading off end of buffer
+    // small-string-opt sizes, so sanitizers and valgrind don't pick this up
+    ASSERT_EQ(normalize_link("%", "/"), "/");
+    ASSERT_EQ(normalize_link("%1", ""), "");
+
+    // ../test/tools-test.cpp:260: Failure
+    // Expected equality of these values:
+    //   normalize_link("%", "/")
+    //     Which is: "/\01bc"
+    //   "/"
+    //
+    // ../test/tools-test.cpp:261: Failure
+    // Expected equality of these values:
+    //   normalize_link("%1", "")
+    //     Which is: "\x1" "1bc"
+    //   ""
+
+    // test outside of small-string-opt
+    // valgrind will pick up on the error in this one
+    ASSERT_EQ(normalize_link("qrstuvwxyz%", "/abcdefghijklmnop"), "/abcdefghijklmnop/qrstuvwxyz");
+    ASSERT_EQ(normalize_link("qrstuvwxyz%1", "/abcdefghijklmnop"), "/abcdefghijklmnop/qrstuvwxyz");
 }
 
 TEST(tools, addler32)
@@ -400,7 +426,6 @@ TEST(tools, getLinks)
       "abcd href = qwerty src={123} xyz",
       ""
     );
-
 }
 #undef EXPECT_LINKS