GNOME Bugzilla – Bug 765566
Integer overflow parsing port number in uri.c
Last modified: 2021-07-05 13:23:17 UTC
Created attachment 326717 [details] [review] Proposed patch. In uri.c:xmlParse3986Port(), uri->port can overflow when parsing a the port number. The type of uri->port is int, so the consequent behavior is undefined and may differ between compilers and architectures. This may have security implications as the same document may exhibit different behaviors on different platforms. The attached patch uses an unsigned to parse the port number and stores the value modulo INT_MAX in uri->port. The patched code preserves all behavior that was previously defined.
RFCs 3986 doesn't limit the port value ! URI can be used for namespace, where libxml2 will parse the URI string used as a name for the namespace without ever trying to access the port. So a ridiculously large port, is okay in that context and we should try to preserve those. That said if it's over INT_MAX I think it's reasonable to fail to save the uri->port. I reused the patch as is, I was tempted ti use unsigned long but since for ABI we need to keep the field as int, that's pointless anyway, applied as https://git.gnome.org/browse/libxml2/commit/?id=846cf015a77b9bca7b90c17c1f608ece3e268dad thanks ! Daniel
I don't think this issue is fixed. At the very least, the port number should be mapped to some illegal value if an overflow occurs. Using modulo arithmetic results in almost the same behavior as before and only avoids error messages from sanitizers. The behavior is still platform-dependent and, more importantly, it still allows to disguise port numbers. A value of (INT_MAX + 81) will be mapped to 80, for example. Ideally, the port should be stored as a string and only converted to a number by the protocol handlers.
*** Bug 784014 has been marked as a duplicate of this bug. ***
(In reply to Nick Wellnhofer from comment #2) > I don't think this issue is fixed. At the very least, the port number should > be mapped to some illegal value if an overflow occurs. Using modulo > arithmetic results in almost the same behavior as before and only avoids > error messages from sanitizers. > > The behavior is still platform-dependent and, more importantly, it still > allows to disguise port numbers. A value of (INT_MAX + 81) will be mapped to > 80, for example. > > Ideally, the port should be stored as a string and only converted to a > number by the protocol handlers. Hi Nick, The specific issue addressed by my patch was undefined behaviour when a signed integer overflows. The behaviour is now defined, closing off a potential vulnerability. Your broader point is quite valid; an out of range port number would best be treated as an error rather than mapped to a modulo value. This would require larger code changes (with a risk of introducing new bugs). Your proposed approach, in particular, may require altering the ABI and would result in externally visible behavioural change. If there is consensus from the project owners that this is OK I would be happy to work on a patch.
> The specific issue addressed by my patch was undefined behaviour when a signed > integer overflows. The behaviour is now defined, closing off a potential > vulnerability. That's correct. But typically, signed integer overflow only leads to wrapping the value around, much like the code does now explicitly. > If there is consensus from the project owners that this is OK I would be happy > to work on a patch. I think this is a minor issue that doesn't require immediate attention. By reopening, I mainly wanted to make sure that this bug isn't forgotten. It's slightly dangerous to make fixes just to silence sanitizers without addressing the underlying issue.
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new ticket at https://gitlab.gnome.org/GNOME/libxml2/-/issues/ Thank you for your understanding and your help.