Can someone attach the test case to this bug?  I don't have access to cluster-fuzz.appspot.com.  Thanks!

attaching minimized testcase.

Deleted: fuzz-0-libxml_xml_read_memory_fuzzer 146 bytes

fuzz-0-libxml_xml_read_memory_fuzzer 146 bytes View Download

This is definitely an int overflow but I don't believe this can be leveraged into a crash. However, since int overflows are undefined this could lead to divergent behaviours for the same input document under different compilers/architectures.

Here's a potential patch (with a test case).

Note that I don't have a version of clang on OS X that has UBSan enabled, so this fix was done via code inspection and lldb.  I have not tested this change with UBSan directly.

Deleted: 0001-Integer-overflow-in-xmlParse3986Port.patch 12.0 KB

0001-Integer-overflow-in-xmlParse3986Port.patch 12.0 KB Download

Thanks for the patch. We want to try and upstream any changes and I think there are three issues that may be relevant here:
1. RFC3986 does not restrict the port part of a URI to a 16 bit value.
2. The use of C11 strict_assert() may be an issue for the upstream as the current code doesn't require a C11 compiler.
3. We should probably leave the header comment as "Parse" rather than "Parses" for consistency with the rest of the code.

I've tried to craft something similar to your patch, but addressing the above points. Please let me know what you think.

Deleted: 602280-mwp.patch 1.1 KB

602280-mwp.patch 1.1 KB Download

> I've tried to craft something similar to your patch, but addressing the above points. Please let me know what you think.

Thanks!  I agree with all your points, but I think there are three issues with your patch:

1. It does not preserve the invariant that uri->port gets initialized to zero if uri is non-NULL.  (I realize this was done to use the field for math, but still seems better to initialize the field in the case of an overflow error as well.)

2. It does not preserve the invariant that **str points to the text after the string of digits used for the port (even if the port value overflows), so parsing after the port won't continue as expected.

3. A silly port number like '0' or '07' or '01024' will look like overflow due to the 'tmp > port' check not also checking for equality.

Attached a revised patch that's built on top of yours.  :)

Deleted: 0001-Integer-overflow-in-xmlParse3986Port-v2.patch 12.0 KB

0001-Integer-overflow-in-xmlParse3986Port-v2.patch 12.0 KB Download

Thanks for taking on board my feedback.

You are 100% correct on point (3). Great catch.

Re points (1) and (2), the correct action is unclear to me. The existing code does not set uri->port or update **str unless it parses a port. If we bail out due to overflow should we avoid any side effects? I'm good with either approach, so if you would consider this and make a final call, I would appreciate it.

Last niggle :-) It might probably be marginally cleaner to return(1) rather than return(hasOverflow). It avoids conflating a boolean flag with an error return value.

I think we are good to roll with the latest version of the patch as is. I'll update the README.chromium file and submit in it the review queue.

NACK from me, for the test, it should test the error a stake not
a zillion different other error messages produced because the fuzzer 
output garbage, so make it a correct doc with just the broken URI namespace
to be tested with a extraordinary large number.

The RFC allows unbounded values for port at the syntactic level:

port        = *DIGIT

so returning a failure on overflow makes me uneasy
but it also says in the security section:

Applications should prevent dereference of a URI that specifies a TCP
   port number within the "well-known port" range (0 - 1023) unless the
   protocol being used to dereference that URI is compatible with the
   protocol expected on that well-known port. 

The point however is that URIs are also used for namespace names, which
are *not* intended to be dereferenced (and libvirt doesn't), we should
not fail on parsing here even if there is an overflow. The port is part of
the URI used only as a string for the identification of the namespace, and that
string is compliant to rfc3986, so second NACK on not returnng 0 in that
case (but emitting a warning is fine !)

  Won't apply as is upstream, sorry it's broken :) but we are close,
    thanks !

Daniel

> Re points (1) and (2), the correct action is unclear to me. The existing code does not set uri->port or update **str unless it parses a port. If we bail out due to overflow should we avoid any side effects? I'm good with either approach, so if you would consider this and make a final call, I would appreciate it.

My goal here was to preserve existing behavior as much as possible (since we will backport this change to three shipping versions of OS X), although per Daniel's comment, apparently this parsing code is used for namespace names, so we shouldn't return 1 even if we overflow.

> Last niggle :-) It might probably be marginally cleaner to return(1) rather than return(hasOverflow). It avoids conflating a boolean flag with an error return value.

This becomes a moot point with Daniel's feedback.  (I think this method actually returns a boolean value, but uses int to do that for compatibility reasons.)

Thanks for the feedback Daniel. How about something like the attached patch which accepts any valid port and interprets the value modulo INT_MAX+1? This essentially preserves all defined behavior of the original code, while removing the overflow.

It wasn't clear to me how to correctly emit a warning, but since the current code doesn't warn there is no regression.

Deleted: 602280-mwp-01.patch 1003 bytes

602280-mwp-01.patch 1003 bytes Download

Update. I've tested the proposed patch using ./testURI.

Pre-patch:
Input: http://example.com:2147483649
Output: http://example.com [UNDEFINED BEHAVIOR!]

Post-patch:
Input: http://example.com:2147483649
Output: http://example.com:1 [DEFINED BEHAVIOR]

So I am convinced the patch works. However, uri->port is an int so it can be different lengths on different machines. Thus I don't think adding test URIs to the regression tests can be done in architecture independent way.

Upstream bug logged. https://bugzilla.gnome.org/show_bug.cgi?id=765566

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bc5dfe3dbb61e497438849dbe909520128f5bbac

commit bc5dfe3dbb61e497438849dbe909520128f5bbac
Author: mwp <mwp@chromium.org>
Date: Tue Apr 26 01:09:59 2016

Fix undefined behavior due to integer overflow when parsing uri port.

BUG= 602280 

Review URL: https://codereview.chromium.org/1914903002

Cr-Commit-Position: refs/heads/master@{#389650}

[modify] https://crrev.com/bc5dfe3dbb61e497438849dbe909520128f5bbac/third_party/libxml/README.chromium
[modify] https://crrev.com/bc5dfe3dbb61e497438849dbe909520128f5bbac/third_party/libxml/src/uri.c

Thanks Michael!  I like your final patch.  Sorry, got pulled off on some other issues in the meantime.

ClusterFuzz has detected this issue as fixed in range 389574:389973.

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=5389085794893824

Fuzzer: libfuzzer_libxml_xml_read_memory_fuzzer
Job Type: libfuzzer_chrome_ubsan
Platform Id: linux

Crash Type: Integer-overflow
Crash Address: 
Crash State:
  xmlParse3986Port
  xmlParse3986Authority
  xmlParse3986RelativeRef
  
Fixed: https://cluster-fuzz.appspot.com/revisions?job=libfuzzer_chrome_ubsan&range=389574:389973

Minimized Testcase (0.14 Kb): https://cluster-fuzz.appspot.com/download/AMIfv956YpvO6fNOLoG8h5ddsVF1E9fcKycQ_614dg812_zT8lXVwW56WlzURPmL_LajvbsKYd_IPe1LcdWFXgSskP_sWZsGG2oWqmlfpdP4f2QDgarEO_CdVb2edndCbjQeVEpIElSHgM92j5gYUuia7Al6wKoyjQ

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reproducing.md for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.

This is upstream in 846cf015.

Removing EditIssue view restrictions from ClusterFuzz filed bugs. If you believe that this issue should still be restricted, please reapply the label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

In case this is useful to anyone, this issue was assigned CVE-2016-4616.