959100 - ParseChunkRemaining doesn't detect chunk size overflow

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Daniel Stenberg [:bagder]]

Attachment: nsHttpChunkedDecoder-ParseChunkRemaining.patch

The nsHttpChunkedDecoder::ParseChunkRemaining function uses sscanf(..."%x"...) to parse the hexadecimal chunk size. The code assumes a 32bit limited value (although RFC2616 doesn't explicitly say that this is the maximum).

sscanf() doesn't provide any overflow detection, so when given a stream with >32 bit numbers in the chunked sizes, this function will silently cap the value in however way your particular sscanf() implementation decides to do it, and that will greatly confuse everything from that point on. (a recent glibc on Linux seems to prefer to use the last 8 hex digits specified, so 123456789 becomes 0x23456789)

The proper solution is to make sure that a plain unsigned 32bit value is parsed and that overflows are detected and bailed out from.

A first stab at a patch is attached. It uses strtoul() instead of sscanf(). My main hesitation there is how to detect the overflow in a platform agnostic way since I'm not sure the errno == ERANGE way works universally and I will appreciate some comments/help on that.

Comment 1

[Daniel Stenberg [:bagder]]

This microsoft.com page (http://msdn.microsoft.com/en-us/library/5k9xb7x1.aspx) says strtoul there also reports overflow using ULONG_MAX and errno == ERANGE, so the patch should work fine I think.

Comment 2

[Jason Duell]

Comment on attachment 8359134 [details] [diff] [review]
nsHttpChunkedDecoder-ParseChunkRemaining.patch

Review of attachment 8359134 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good.  I checked on OSX and its strtoul function seems to work the same as well, and has since 2001.  

See if you can push this to try and it builds OK.

I'm on the fence here about testing--AFAICT we don't have any tests for chunked encoding, so it would nice to have.  It's probably not hard to write one using seizePower()

   http://mxr.mozilla.org/mozilla-central/source/netwerk/test/unit/test_duplicate_headers.js#62

You could write one test case that passes a valid length and chunks, and one that passes in a >4GB value and some small body (don't send 4 GB or test will be slow!).  Make sure we fail the channel for the overflow case.  Please use a name like "test_chunked_responses.js", rather than "test_bug959100.js" (we've had inconclusive wars on dev.platform about which naming convention is better: I think the former, as it makes the test extensible if it can be used for other bugs, and also easier to understand from a simple 'ls' int the directory).  Make sure to add the test name to xpcshell.ini.  And you might as well add a _wrap version in unit_ipc (directory for multiple-process tests), so we have e10s (shorthand for "electrolysis", the multiprocess version of gecko) coverage too.

If writing a test here becomes a problem or takes too much time I'd be ok with landing this as is.

Comment 3

[Daniel Stenberg [:bagder]]

Thanks, will work on making up a few tests for this!

Comment 4

[Daniel Stenberg [:bagder]]

Attachment: test_chunked_responses.patch

This patch introduces test_chunked_responses.js with three individual tests. One overflow, one with junk before the size and one with junk after the size.

I found lots of inspiration in other test files so I hope I didn't violate too many rules here! The test case runs fine with the fixed chunked parser.

Comment 5

[Jason Duell]

Comment on attachment 8361054 [details] [diff] [review]
test_chunked_responses.patch

Review of attachment 8361054 [details] [diff] [review]:
-----------------------------------------------------------------

Good stuff! Just a couple nits and this is ready to be checked in.  Feel free to mark the next version of the patch +r w/o asking me for another review (this is called "carrying a review forward", and exposes the interesting fact that Bugzilla doesn't stop you from marking your own work +r:  do it only when your reviewer tells you it's OK :)

Add a unit_ipc/test_chunked_responses_wrap.js file too (per my comment about e10s testing).

Make sure to mark the old patch 'obsolete' when you add the new patch.  Generally we give them clever names like 'v2', 'v3' etc.

After you've attached the new patch, mark 'checkin-needed' in the Keywords field at the top of the bug, and one of the sheriffs will check it in for you.

::: netwerk/test/unit/test_chunked_responses.js
@@ +104,5 @@
> +test_flags[3] = CL_ALLOW_UNKNOWN_CL;
> +
> +function handler3(metadata, response)
> +{
> +  var body = "c junkafter\r\ndata reached";

Maybe add a 4th test where the server actually behaves per RFC 2616, i.e. sends a last chunk of 0-length?

Comment 6

[Daniel Stenberg [:bagder]]

Attachment: version 2 (extended tests, improved code)

It struck me that "mChunkRemaining" is an uint32_t even on 64 bit machines so I added a little more code to see if the long strtoul() return value is too large for 32 bits.

I added two test cases too, one to just see that a fine and complete chunked flow works and then that a >32bit value overflow gets detected.

This is taken into account the v2 version of the patch.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a5f36d486b4

Thanks for the patch, Daniel! One request - please make sure that future patches have the needed commit information included per the directions below. Makes life much easier for those landing on your behalf :)
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b0a1d41b80f

https://tbpl.mozilla.org/php/getParsedLog.php?id=33171577&tree=Mozilla-Inbound

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Pasted the wrong cset, this was the backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d8510edd4e6

Comment 10

[Patrick McManus [:mcmanus]]

Comment on attachment 8361626 [details] [diff] [review]
version 2 (extended tests, improved code)

Review of attachment 8361626 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/nsHttpChunkedDecoder.cpp
@@ +132,5 @@
> +            parsedval = strtoul(buf, &endptr, 16);
> +            mChunkRemaining = (uint32_t) parsedval;
> +
> +            if ((endptr == buf) ||
> +                (errno == ERANGE) ||

not related to the compile fail - but you can't check errno without first checking that the function returned an error.. otherwise it may be uninitialized.

Comment 11

[Daniel Stenberg [:bagder]]

ah, right :mcmanus, I lost the other part of the errno check.:-( grrr, will fix together with the missing include. (slightly surprised that I don't get that compiler error but...)

Comment 12

[Daniel Stenberg [:bagder]]

Attachment: version 3, fixed errno include and conditional

New patch version. Generated with hg.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Also a Windows xpcshell failure:
https://tbpl.mozilla.org/php/getParsedLog.php?id=33174846&tree=Mozilla-Inbound

Comment 14

[Daniel Stenberg [:bagder]]

Thanks, I'll need to digest that to figure out what's going on there!

Comment 15

[Daniel Stenberg [:bagder]]

I suspect that error happens exactly because of the errno checking error :mcmanus pointed out in the v2 patch and that's fixed in v3. A false positive.

Comment 16

[Patrick McManus [:mcmanus]]

just to be on the safe side, let's see a link to a green try run and a fresh review. thanks!

Comment 17

[Daniel Stenberg [:bagder]]

Yeps, will do as soon as I get access.

Comment 18

[Patrick McManus [:mcmanus]]

Comment on attachment 8361754 [details] [diff] [review]
version 3, fixed errno include and conditional

https://tbpl.mozilla.org/?tree=Try&rev=cef4af1c6ba0

Comment 19

[Daniel Stenberg [:bagder]]

Lots of "install-tests" errors there. Did my patch really cause them or what are we seeing?

Comment 20

[Patrick McManus [:mcmanus]]

my bad on the try run - didn't git add the new tests in the patch

new one
https://tbpl.mozilla.org/?tree=Try&rev=0b78a53dbef0

Comment 21

[Jason Duell]

Comment on attachment 8361754 [details] [diff] [review]
version 3, fixed errno include and conditional

Review of attachment 8361754 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good.  The try run has a bunch of reds on B2G emulators only.  I suspect that's not our fault, and hopefully it's something transient.

Comment 22

[Patrick McManus [:mcmanus]]

https://tbpl.mozilla.org/?tree=Try&rev=2897cafd891e

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5238a0b63a59

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5238a0b63a59