Fix Off-By-One Stack Buffer Overflows in XML Parser (#1717)

* Off-By-One Null Byte Fix

* Add XML parser tests and improve XmlGetAttributeText handling

* Refactor XML testing: integrate XmlTest into AutoTestAlgorithms, add sentinel test for XmlGetNodeText insuficient output size.

* Remove no-op Tests.c change

---------

Co-authored-by: Mounir IDRASSI <mounir.idrassi@amcrypto.jp>

src/Common/Dlgcode.c

@@ -7705,8 +7705,14 @@ CipherTestDialogProc (HWND hwndDlg, UINT uMsg, WPARAM wParam, LPARAM lParam)
 
 		if (lw == IDC_AUTO)
 		{
+			BOOL testsPassed;
 			WaitCursor ();
-			if (!AutoTestAlgorithms())
+			testsPassed = AutoTestAlgorithms();
+			#if !defined(TC_WINDOWS_DRIVER) && !defined(_UEFI)
+			if (testsPassed && !XmlTest())
+				testsPassed = FALSE;
+			#endif
+			if (!testsPassed)
 			{
 				ShowWindow(GetDlgItem(hwndDlg, IDC_TESTS_MESSAGE), SW_SHOWNORMAL);
 				SetWindowTextW(GetDlgItem(hwndDlg, IDC_TESTS_MESSAGE), GetString ("TESTS_FAILED"));

src/Common/Xml.c

@@ -84,16 +84,20 @@ char *XmlFindElementByAttributeValue (char *xml, char *nodeName, const char *att
 char *XmlGetAttributeText (char *xmlNode, const char *xmlAttrName, char *xmlAttrValue, int xmlAttrValueSize)
 {
 	char *t = xmlNode;
-	char *e = xmlNode;
+	char *nodeEnd = xmlNode;
+	char *quote1, *quote2;
 	int l = 0;
 
+	if (xmlAttrValueSize <= 0)
+		return NULL;
+
 	xmlAttrValue[0] = 0;
 	if (t[0] != '<') return NULL;
 
-	e = strchr (e, '>');
+	nodeEnd = strchr (nodeEnd, '>');
-	if (e == NULL) return NULL;
+	if (nodeEnd == NULL) return NULL;
 
-	while ((t = strstr (t, xmlAttrName)) && t < e)
+	while ((t = strstr (t, xmlAttrName)) && t < nodeEnd)
 	{
 		char *o = t + strlen (xmlAttrName);
 		if (t[-1] == ' '
@@ -108,12 +112,17 @@ char *XmlGetAttributeText (char *xmlNode, const char *xmlAttrName, char *xmlAttr
 		t++;
 	}
 
-	if (t == NULL || t > e) return NULL;
+	if (t == NULL || t > nodeEnd) return NULL;
 
-	t = ((char*)strchr (t, '"')) + 1;
+	quote1 = strchr (t, '"');
-	e = strchr (t, '"');
+	if (quote1 == NULL || quote1 > nodeEnd) return NULL;
-	l = (int)(e - t);
+	t = quote1 + 1;
-	if (e == NULL || l > xmlAttrValueSize) return NULL;
+
+	quote2 = strchr (t, '"');
+	if (quote2 == NULL || quote2 > nodeEnd) return NULL;
+
+	l = (int)(quote2 - t);
+	if (l < 0 || l >= xmlAttrValueSize) return NULL;
 
 	memcpy (xmlAttrValue, t, l);
 	xmlAttrValue[l] = 0;
@@ -128,6 +137,9 @@ char *XmlGetNodeText (char *xmlNode, char *xmlText, int xmlTextSize)
 	char *e = xmlNode + 1;
 	int l = 0, i = 0, j = 0;
 
+	if (xmlTextSize <= 0)
+		return NULL;
+
 	xmlText[0] = 0;
 
 	if (t[0] != '<')
@@ -141,10 +153,16 @@ char *XmlGetNodeText (char *xmlNode, char *xmlText, int xmlTextSize)
 	if (e == NULL) return NULL;
 
 	l = (int)(e - t);
-	if (e == NULL || l > xmlTextSize) return NULL;
+	if (l < 0) return NULL;
 
 	while (i < l)
 	{
+		if (j >= xmlTextSize - 1)
+		{
+			xmlText[0] = 0;
+			return NULL;
+		}
+
 		if (BeginsWith (&t[i], "&lt;"))
 		{
 			xmlText[j++] = '<';
@@ -282,3 +300,69 @@ int XmlWriteFooter (FILE *file)
 	return fputws (L"\n</VeraCrypt>", file);
 }
 #endif !defined(_UEFI)
+
+#if !defined(TC_WINDOWS_DRIVER) && !defined(_UEFI)
+BOOL XmlTest (void)
+{
+	char buffer[10];
+
+	/* XmlGetAttributeText tests */
+
+	/* 1. length size - 1 accepted */
+	char xmlAttrValid[] = "<Node attr=\"123456789\"></Node>";
+	if (XmlGetAttributeText (xmlAttrValid, "attr", buffer, sizeof (buffer)) == NULL
+		|| strcmp (buffer, "123456789") != 0)
+		return FALSE;
+
+	/* 2. length size rejected (off-by-one: would write NUL past buffer end) */
+	char xmlAttrOverflow[] = "<Node attr=\"1234567890\"></Node>";
+	if (XmlGetAttributeText (xmlAttrOverflow, "attr", buffer, sizeof (buffer)) != NULL)
+		return FALSE;
+
+	/* 3. malformed: closing quote absent returns NULL */
+	char xmlAttrMissingQuote[] = "<Node attr=\"123456789></Node>";
+	if (XmlGetAttributeText (xmlAttrMissingQuote, "attr", buffer, sizeof (buffer)) != NULL)
+		return FALSE;
+
+	/* 4. closing quote belongs to a later tag, not the current one */
+	char xmlAttrCrossTag[] = "<Node attr=\"123456789></Node><Other attr=\"test\"></Other>";
+	if (XmlGetAttributeText (xmlAttrCrossTag, "attr", buffer, sizeof (buffer)) != NULL)
+		return FALSE;
+
+
+	/* XmlGetNodeText tests */
+
+	/* 5. length size - 1 accepted */
+	char xmlNodeValid[] = "<Node>123456789</Node>";
+	if (XmlGetNodeText (xmlNodeValid, buffer, sizeof (buffer)) == NULL
+		|| strcmp (buffer, "123456789") != 0)
+		return FALSE;
+
+	/* 6. length size rejected (off-by-one: would write NUL past buffer end) */
+	char xmlNodeOverflow[] = "<Node>1234567890</Node>";
+	if (XmlGetNodeText (xmlNodeOverflow, buffer, sizeof (buffer)) != NULL)
+		return FALSE;
+
+	/* 7. escaped text accepted: raw input is larger than buffer but decoded
+	   output fits. Decoded: "<>&456789" (9 chars), buffer is 10 bytes. */
+	char xmlNodeEscaped[] = "<Node>&lt;&gt;&amp;456789</Node>";
+	if (XmlGetNodeText (xmlNodeEscaped, buffer, sizeof (buffer)) == NULL
+		|| strcmp (buffer, "<>&456789") != 0)
+		return FALSE;
+
+	/* 8. escaped text rejected: decoded output is exactly size (10 chars),
+	   leaving no room for the NUL terminator. Decoded: "<>&4567890" (10 chars). */
+	char xmlNodeEscapedOverflow[] = "<Node>&lt;&gt;&amp;4567890</Node>";
+	if (XmlGetNodeText (xmlNodeEscapedOverflow, buffer, sizeof (buffer)) != NULL)
+		return FALSE;
+
+	/* 9. seed the buffer and verify overflow failure leaves it empty */
+	char xmlNodeOverflowSeed[] = "<Node>1234567890</Node>";
+	buffer[0] = 's';
+	buffer[1] = 0;
+	if (XmlGetNodeText (xmlNodeOverflowSeed, buffer, sizeof (buffer)) != NULL || buffer[0] != 0)
+		return FALSE;
+
+	return TRUE;
+}
+#endif

src/Common/Xml.h

@@ -21,6 +21,10 @@ char *XmlGetNodeText (char *xmlNode, char *xmlText, int xmlTextSize);
 char *XmlFindElementByAttributeValue (char *xml, char *nodeName, const char *attrName, const char *attrValue);
 char *XmlQuoteText (const char *textSrc, char *textDst, int textDstMaxSize);
 
+#if !defined(TC_WINDOWS_DRIVER) && !defined(_UEFI)
+BOOL XmlTest (void);
+#endif
+
 #if !defined(_UEFI)
 wchar_t *XmlQuoteTextW(const wchar_t *textSrc, wchar_t *textDst, int textDstMaxSize);
 int XmlWriteHeader (FILE *file);