Fix Off-By-One Stack Buffer Overflows in XML Parser

[S4N-T0S]

Bug Description:
The functions XmlGetAttributeText and XmlGetNodeText in Xml.c contain off-by-one buffer overflows. They check if the parsed length l is strictly greater than size (l > size). If l == size, the functions execute buffer[l] = 0;, which writes a null byte exactly one byte past the allocated bounds of the array.

Security Impact:
Because VeraCrypt parses Configuration.xml and History.xml from the user-controlled %APPDATA% folder, a low-privileged malware could place a malicious XML file there. If VeraCrypt is later launched with Administrative privileges (e.g., via UAC for device mounting), this memory corruption occurs in an elevated context. Of course this would be difficult to exploit because of Stack Canaries etc in Windows, either way no reason not to fix. At the very least it results in an application crash.

Fix:
Changed the boundary limits from > to >= to ensure 1 byte is always cleanly reserved for the null terminator.

[idrassi]

@S4N-T0S
Thank you for the report and patch.
Indeed, when the parsed XML attribute/text length is exactly equal to the destination buffer size, the current code writes the terminating NUL one byte past the caller-provided buffer.

The proposed >= checks fix that immediate bug and the patch is clean. However, before merging, I would like the fix to be slightly more complete around the same parser code:

• XmlGetAttributeText should check that the closing quote was found before computing e - t.
• It should also ensure the closing quote belongs to the current tag, not to later XML content.
• XmlGetNodeText should bound-check the decoded output index while expanding <, >, and &, so we reserve space for the NUL terminator without unnecessarily rejecting escaped text that would decode safely.
• A few boundary tests would be useful: length size - 1 accepted, length size rejected for plain text/attributes, escaped text handled correctly, malformed missing quote returns NULL.

I would classify the impact as local memory corruption and possible crash from user-controlled XML, with security relevance when the file is parsed by an elevated VeraCrypt process, but not as a demonstrated privilege escalation.

Can you please update the PR with the broader parser hardening described above? I can also amend this before merging while preserving attribution.

[S4N-T0S]

@idrassi

Hopefully the commit I have just pushed will suffice in terms of tests and the parser hardening. Here is a quick breakdown of how the latest changes address your feedback:

• Checking for the closing quote & current tag bounds (XmlGetAttributeText):
  I introduced a nodeEnd pointer that locates the > character of the current tag. The function now explicitly looks for both the opening (quote1) and closing (quote2) quotes and verifies that neither is NULL and that both appear before nodeEnd. This prevents it from computing invalid lengths or falsely grabbing a quote from a subsequent XML tag.
• Bounding the decoded output index (XmlGetNodeText):
  I removed the raw length check prior to the loop and instead added a if (j >= xmlTextSize - 1) check inside the decoding while loop. This ensures we are strictly bounding against the final decoded string length (safely preserving 1 byte for the NUL terminator) without prematurely rejecting valid escaped text where the raw length exceeds the buffer but the decoded length fits.
• General robustness:
  Added <= 0 early-exit checks for the passed buffer sizes in both functions just to be perfectly safe.
• Boundary Tests (Tests.c / Tests.h):
  Added a new XmlTest() suite covering all the requested scenarios:
  • 1 & 5: size - 1 lengths accepted for both attributes and nodes.
  • 2 & 6: size lengths correctly rejected (preventing the original off-by-one).
  • 3: Malformed tags with missing quotes returning NULL.
  • 4: Cross-tag quote constraints (closing quote belonging to a later tag).
  • 7 & 8: Escaped text handling, ensuring decoded output is checked correctly against the buffer bounds.

[idrassi]

@S4N-T0S
Thank you, this is much closer and the quote bound handling in XmlGetAttributeText goes in the right direction.

I found two issues that still need to be fixed before merge:

1. XmlGetNodeText can now return NULL after partially writing to xmlText when the decoded output exceeds the destination size. Several existing callers don't check the return value, so on failure they may consume a truncated or non terminated buffer. Please preserve the previous failure behavior: on any failure, the output buffer should remain an empty string. A pre-pass to compute decoded length, or clearing xmlText[0] = 0 before returning from the capacity failure path, would address this. Please also add a test that seeds the buffer and verifies overflow failure leaves it empty.

2. XmlTest is now called from AutoTestAlgorithms but AutoTestAlgorithms is compiled into the Windows driver. The driver project includes Common\Tests.c but not Common\Xml.c, so this will break the driver build.
   Please move the XML tests out of the common crypto self-test path.

After those two changes and successful checks, I think this will be ready to merge.

[S4N-T0S]

@S4N-T0S Thank you, this is much closer and the quote bound handling in XmlGetAttributeText goes in the right direction.

I found two issues that still need to be fixed before merge:

1. `XmlGetNodeText` can now return NULL after partially writing to `xmlText` when the decoded output exceeds the destination size. Several existing callers don't check the return value, so on failure they may consume a truncated or non terminated buffer. Please preserve the previous failure behavior: on any failure, the output buffer should remain an empty string. A pre-pass to compute decoded length, or clearing `xmlText[0] = 0` before returning from the capacity failure path, would address this. Please also add a test that seeds the buffer and verifies overflow failure leaves it empty.

2. `XmlTest` is now called from `AutoTestAlgorithms` but `AutoTestAlgorithms` is compiled into the Windows driver. The driver project includes `Common\Tests.c` but not `Common\Xml.c`, so this will break the driver build.
   Please move the XML tests out of the common crypto self-test path.

After those two changes and successful checks, I think this will be ready to merge.

@idrassi
My apologies for that, I had assumed the Tests.c file could be used for this XmlTest function. Since XmlTest() is no longer called from AutoTestAlgorithms(), could you point me to the preferred user-mode initialization/test path where I should hook XmlTest() so that it still gets executed?

[idrassi]

@S4N-T0S
The preferred place is the existing user mode test dialog path in Dlgcode.c, around the handler that currently calls AutoTestAlgorithms for the "Auto-Test All" action. That code is usermode only, so it can safely call XML parser tests without pulling Xml.c into the driver.

Please structure it like this:

• keep XmlTest out of Tests.c and Tests.h
• put it in Xml.c
• expose it via a guarded declaration, e.g. only when !defined(TC_WINDOWS_DRIVER) && !defined(_UEFI)
• call it from the Dlgcode.c test-dialog auto-test handler after AutoTestAlgorithms

So the flow would be:

BOOL testsPassed = AutoTestAlgorithms();

#if !defined(TC_WINDOWS_DRIVER) && !defined(_UEFI)
if (testsPassed && !XmlTest())
    testsPassed = FALSE;
#endif

This keeps XML testing in a user-mode test path.

Please also add the sentinel test discussed earlier: when XmlGetNodeText fails due to insufficient output size, the destination buffer should be left as an empty string and not partially decoded text.