959093 - nsHttp::ParseInt64 integer overflow (Content-Length: parser)

Status: NEW

(Core :: Networking: HTTP, defect, P5)

Description

[Daniel Stenberg [:bagder]]

Attachment: nsHttp-ParseInt64.patch

The function nsHttp::ParseInt64() in netwerk/protocol/http/nsHttp.cpp is primarily used to convert a specified Content-Length into a signed 64 bit integer.

The current code seems to assume that an integer overflow will make a smaller value than the previous one, which would be an incorrect assumption based on the standards and relies on undefined behavior and thus may break at some point when a compiler decides to act differently.

It would be suitable to have a strtoll() function to call here instead.

I'm attaching my first stab at rewriting the function to not rely on undefined behavior.

Comment 1

[Daniel Stenberg [:bagder]]

I've since noted that we have a few places in the netwerk code where we use PR_sscanf( ... "%lld") to parse 64 bit integers and I think it would make sense to unify to a single parsing method.

However, PR_sscanf() makes *no* attempts to detect overflows (which thus is bound to cause some funny bugs for crap input). At least ParseInt64() has overflow detection that works for most cases.

I would like us to introduce a global 64 bit number parser with overflow detection capabilities. strtoll() and _strtoi64() are readily available on our major platforms...

Comment 2

[qz1]

Can someone look at this bug, please? There is 400 crash reports related to it in last 28 days at crash-stats.mozilla.com.

Comment 3

[Daniel Stenberg [:bagder]]

Not helping a lot but some observations from me:

The crash reports are about SIGSEGVs in ParseInt64(), right? It seems unlikely that it is actually related to a 64bit overflow. Not only because larger-than-64bit values are very rare in HTTP headers but also because a typical overflow should not result in a SIGSEGV anyway.

Further: all of them seem to have happened on 32bit Ubuntu Linux, at least one with Thunderbird.

Comment 4

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258