Blob Blame History Raw
commit 22fe2da8e2bc0625d3c492f42d6b716adb36d5c2
Author: Tomas Korbar <tkorbar@redhat.com>
Date:   Mon Feb 14 12:09:42 2022 +0100

    CVE-2022-23852

diff --git a/lib/xmlparse.c b/lib/xmlparse.c
index 85ee0a8..4552680 100644
--- a/lib/xmlparse.c
+++ b/lib/xmlparse.c
@@ -161,6 +161,9 @@ typedef char ICHAR;
 /* Round up n to be a multiple of sz, where sz is a power of 2. */
 #define ROUND_UP(n, sz) (((n) + ((sz) - 1)) & ~((sz) - 1))
 
+/* Do safe (NULL-aware) pointer arithmetic */
+#define EXPAT_SAFE_PTR_DIFF(p, q) (((p) && (q)) ? ((p) - (q)) : 0)
+
 /* Handle the case where memmove() doesn't exist. */
 #ifndef HAVE_MEMMOVE
 #ifdef HAVE_BCOPY
@@ -2026,39 +2029,54 @@ XML_GetBuffer(XML_Parser parser, int len)
   default: ;
   }
 
-  if (len > parser->m_bufferLim - parser->m_bufferEnd) {
-#ifdef XML_CONTEXT_BYTES
+  if (len > EXPAT_SAFE_PTR_DIFF(parser->m_bufferLim, parser->m_bufferEnd)) {
     int keep;
-#endif  /* defined XML_CONTEXT_BYTES */
     /* Do not invoke signed arithmetic overflow: */
-    int neededSize = (int) ((unsigned)len + (unsigned)(parser->m_bufferEnd - parser->m_bufferPtr));
+    int neededSize = (int)((unsigned)len
+                           + (unsigned)EXPAT_SAFE_PTR_DIFF(
+                               parser->m_bufferEnd, parser->m_bufferPtr));
     if (neededSize < 0) {
       parser->m_errorCode = XML_ERROR_NO_MEMORY;
       return NULL;
     }
-#ifdef XML_CONTEXT_BYTES
-    keep = (int)(parser->m_bufferPtr - parser->m_buffer);
+
+    keep = (int)EXPAT_SAFE_PTR_DIFF(parser->m_bufferPtr, parser->m_buffer);
     if (keep > XML_CONTEXT_BYTES)
       keep = XML_CONTEXT_BYTES;
+    /* Detect and prevent integer overflow */
+    if (keep > INT_MAX - neededSize) {
+      parser->m_errorCode = XML_ERROR_NO_MEMORY;
+      return NULL;
+    }
     neededSize += keep;
-#endif  /* defined XML_CONTEXT_BYTES */
-    if (neededSize  <= parser->m_bufferLim - parser->m_buffer) {
+    if (neededSize
+        <= EXPAT_SAFE_PTR_DIFF(parser->m_bufferLim, parser->m_buffer)) {
 #ifdef XML_CONTEXT_BYTES
-      if (keep < parser->m_bufferPtr - parser->m_buffer) {
-        int offset = (int)(parser->m_bufferPtr - parser->m_buffer) - keep;
-        memmove(parser->m_buffer, &parser->m_buffer[offset], parser->m_bufferEnd - parser->m_bufferPtr + keep);
+      if (keep < EXPAT_SAFE_PTR_DIFF(parser->m_bufferPtr, parser->m_buffer)) {
+        int offset
+            = (int)EXPAT_SAFE_PTR_DIFF(parser->m_bufferPtr, parser->m_buffer)
+              - keep;
+        /* The buffer pointers cannot be NULL here; we have at least some bytes
+         * in the buffer */
+        memmove(parser->m_buffer, &parser->m_buffer[offset],
+                parser->m_bufferEnd - parser->m_bufferPtr + keep);
         parser->m_bufferEnd -= offset;
         parser->m_bufferPtr -= offset;
       }
 #else
-      memmove(parser->m_buffer, parser->m_bufferPtr, parser->m_bufferEnd - parser->m_bufferPtr);
-      parser->m_bufferEnd = parser->m_buffer + (parser->m_bufferEnd - parser->m_bufferPtr);
-      parser->m_bufferPtr = parser->m_buffer;
-#endif  /* not defined XML_CONTEXT_BYTES */
-    }
-    else {
+      if (parser->m_buffer && parser->m_bufferPtr) {
+        memmove(parser->m_buffer, parser->m_bufferPtr,
+                EXPAT_SAFE_PTR_DIFF(parser->m_bufferEnd, parser->m_bufferPtr));
+        parser->m_bufferEnd
+            = parser->m_buffer
+              + EXPAT_SAFE_PTR_DIFF(parser->m_bufferEnd, parser->m_bufferPtr);
+        parser->m_bufferPtr = parser->m_buffer;
+      }
+#endif /* not defined XML_CONTEXT_BYTES */
+    } else {
       char *newBuf;
-      int bufferSize = (int)(parser->m_bufferLim - parser->m_bufferPtr);
+      int bufferSize
+          = (int)EXPAT_SAFE_PTR_DIFF(parser->m_bufferLim, parser->m_bufferPtr);
       if (bufferSize == 0)
         bufferSize = INIT_BUFFER_SIZE;
       do {
@@ -2077,25 +2095,33 @@ XML_GetBuffer(XML_Parser parser, int len)
       parser->m_bufferLim = newBuf + bufferSize;
 #ifdef XML_CONTEXT_BYTES
       if (parser->m_bufferPtr) {
-        int keep = (int)(parser->m_bufferPtr - parser->m_buffer);
-        if (keep > XML_CONTEXT_BYTES)
-          keep = XML_CONTEXT_BYTES;
-        memcpy(newBuf, &parser->m_bufferPtr[-keep], parser->m_bufferEnd - parser->m_bufferPtr + keep);
+        memcpy(newBuf, &parser->m_bufferPtr[-keep],
+               EXPAT_SAFE_PTR_DIFF(parser->m_bufferEnd, parser->m_bufferPtr)
+                   + keep);
         FREE(parser, parser->m_buffer);
         parser->m_buffer = newBuf;
-        parser->m_bufferEnd = parser->m_buffer + (parser->m_bufferEnd - parser->m_bufferPtr) + keep;
+        parser->m_bufferEnd
+            = parser->m_buffer
+              + EXPAT_SAFE_PTR_DIFF(parser->m_bufferEnd, parser->m_bufferPtr)
+              + keep;
         parser->m_bufferPtr = parser->m_buffer + keep;
-      }
-      else {
-        parser->m_bufferEnd = newBuf + (parser->m_bufferEnd - parser->m_bufferPtr);
+      } else {
+        /* This must be a brand new buffer with no data in it yet */
+        parser->m_bufferEnd = newBuf;
         parser->m_bufferPtr = parser->m_buffer = newBuf;
       }
 #else
       if (parser->m_bufferPtr) {
-        memcpy(newBuf, parser->m_bufferPtr, parser->m_bufferEnd - parser->m_bufferPtr);
+        memcpy(newBuf, parser->m_bufferPtr,
+               EXPAT_SAFE_PTR_DIFF(parser->m_bufferEnd, parser->m_bufferPtr));
         FREE(parser, parser->m_buffer);
+        parser->m_bufferEnd
+            = newBuf
+              + EXPAT_SAFE_PTR_DIFF(parser->m_bufferEnd, parser->m_bufferPtr);
+      } else {
+        /* This must be a brand new buffer with no data in it yet */
+        parser->m_bufferEnd = newBuf;
       }
-      parser->m_bufferEnd = newBuf + (parser->m_bufferEnd - parser->m_bufferPtr);
       parser->m_bufferPtr = parser->m_buffer = newBuf;
 #endif  /* not defined XML_CONTEXT_BYTES */
     }
diff --git a/tests/runtests.c b/tests/runtests.c
index e1f1ad1..ecc6f47 100644
--- a/tests/runtests.c
+++ b/tests/runtests.c
@@ -4116,6 +4116,31 @@ START_TEST(test_get_buffer_2)
 }
 END_TEST
 
+/* Test for signed integer overflow CVE-2022-23852 */
+#if defined(XML_CONTEXT_BYTES)
+START_TEST(test_get_buffer_3_overflow) {
+  XML_Parser parser = XML_ParserCreate(NULL);
+  assert(parser != NULL);
+
+  const char *const text = "\n";
+  const int expectedKeepValue = (int)strlen(text);
+
+  // After this call, variable "keep" in XML_GetBuffer will
+  // have value expectedKeepValue
+  if (XML_Parse(parser, text, (int)strlen(text), XML_FALSE /* isFinal */)
+      == XML_STATUS_ERROR)
+    xml_failure(parser);
+
+  assert(expectedKeepValue > 0);
+  if (XML_GetBuffer(parser, INT_MAX - expectedKeepValue + 1) != NULL)
+    fail("enlarging buffer not failed");
+
+  XML_ParserFree(parser);
+}
+END_TEST
+#endif // defined(XML_CONTEXT_BYTES)
+
+
 /* Test position information macros */
 START_TEST(test_byte_info_at_end)
 {
@@ -12117,6 +12142,9 @@ make_suite(void)
     tcase_add_test(tc_basic, test_empty_parse);
     tcase_add_test(tc_basic, test_get_buffer_1);
     tcase_add_test(tc_basic, test_get_buffer_2);
+#if defined(XML_CONTEXT_BYTES)
+    tcase_add_test(tc_basic, test_get_buffer_3_overflow);
+#endif
     tcase_add_test(tc_basic, test_byte_info_at_end);
     tcase_add_test(tc_basic, test_byte_info_at_error);
     tcase_add_test(tc_basic, test_byte_info_at_cdata);