Skip to content

[CVE-2026-56410] xmlwf: protect resolveSystemId from integer overflow#1252

Merged
hartwork merged 2 commits into
libexpat:masterfrom
netliomax25-code:resolvesystemid-int-overflow
May 31, 2026
Merged

[CVE-2026-56410] xmlwf: protect resolveSystemId from integer overflow#1252
hartwork merged 2 commits into
libexpat:masterfrom
netliomax25-code:resolvesystemid-int-overflow

Conversation

@netliomax25-code

Copy link
Copy Markdown
Contributor

resolveSystemId builds an absolute path by allocating (tcslen(base) + tcslen(systemId) + 2) * sizeof(XML_Char), then copies base and systemId in. systemId comes from an external entity SYSTEM identifier, so on a wide-char build the multiply can wrap and under-allocate, leaving the following tcscpy/tcscat to overflow the heap. Guard the count against SIZE_MAX / sizeof(XML_Char) before the malloc, like copyString does, and fall back to systemId as the malloc-failure path already does.

@hartwork hartwork left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@netliomax25-code the suggested patch may not be enough, please see below:

Comment thread expat/xmlwf/xmlfile.c Outdated
@Smattr

Smattr commented May 30, 2026

Copy link
Copy Markdown
Contributor

This might be a silly question, but where does the + 2 come from? git blame tells me, originally, 5bfaa29, but this still does not explain. AFAICT this code copies at most the length of the other two strings + 1 character.

(I realise this is not being changed in this PR, but just wondering while we’re here adding overflow checks if the value overflowing is even the correct sum.)

@netliomax25-code

Copy link
Copy Markdown
Contributor Author

@Smattr you're right, the worst case is tcslen(base) (the part kept before the last separator) + tcslen(systemId) + 1 for the NUL, so +1 would be enough and +2 over-allocates by one char. Looks like a harmless off-by-one from the original commit rather than something load-bearing. I left it alone here to keep this PR scoped to the overflow guard, but happy to trim it to +1 in a separate change if you'd prefer.

@hartwork hartwork left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@netliomax25-code thanks for the adjustments!

@hartwork

Copy link
Copy Markdown
Member

@Smattr @netliomax25-code I believe the additional "plus 1" was intended for insertion of a delimiter, just the code after at…

tcscpy(*toFree, base);
s = *toFree;
if (tcsrchr(s, T('/')))
s = tcsrchr(s, T('/')) + 1;
#if defined(_WIN32)
if (tcsrchr(s, T('\\')))
s = tcsrchr(s, T('\\')) + 1;
#endif
tcscpy(s, systemId);

…does not add a delimiter (but look for existing ones). My vote for a dedicated follow-up pull request.

@netliomax25-code

Copy link
Copy Markdown
Contributor Author

Agreed, follow-up it is. I'll open a separate PR to trim the +2 to +1 since nothing inserts a delimiter, so base + systemId + 1 for the NUL is all we need. Keeping this one scoped to the overflow guard.

@hartwork hartwork merged commit 84d5209 into libexpat:master May 31, 2026
51 checks passed
@hartwork hartwork added this to the 2.8.2 milestone May 31, 2026
@hartwork hartwork changed the title xmlwf: protect resolveSystemId from integer overflow [CVE-2026-56410] xmlwf: protect resolveSystemId from integer overflow Jun 21, 2026
@hartwork

Copy link
Copy Markdown
Member

CVE-2026-56410 has been assigned

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants