451613 - URL parsing treats leading whitespace inconsistently

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

[Simon Montagu :smontagu]

Spun off from bug 443288

Description from attachment 327885 [details]:

The code that cracks the URL into segments treats leading whitespace inconsistently. This allows the following non-whitespace characters to pass through parsing: 0x01-0x08, 0x0B, 0x0C, 0x0E-0x1F.
	
netwerk\base\src\nsURLParsers.cpp:99-103
        if (*p > '\0' && *p <= ' ') {
            spec++;
            specLen--;
            continue;
        }

Because the whitespace handling is inconsistent, the terminal offsets of the different URL segments can be made to point inside multi-byte character sequences.

Comment 1

[Daniel Veditz [:dveditz]]

raising severity just because we depend so heavily on URIs being parsed sanely and consistently -- surely this will bite us badly elsewhere

Comment 2

[Bjarne (:bjarne)]

Looks like this would be resolved by a fix to bug #425046? And the answer to bz's question in bug #425046 comment #6 is pretty relevant here...

Comment 3

[Samuel Sidler (old account; do not CC)]

Dan, how's a patch coming for this bug? Code freeze is in one week.

Comment 4

[Samuel Sidler (old account; do not CC)]

This isn't going to make 1.9.0.6 or 1.8.1.19. Pushing out.

Comment 5

[Daniel Veditz [:dveditz]]

Attachment: Fix offsets, only strip whitespace not controls

This patch fixes two problems: the inconsistency about what kind of characters we strip from the front of a URL, and a problem in the offsets returned to the caller if we did find anything to strip.

Control characters are still stripped from the end. I guess that's right since they aren't valid characters. It doesn't seem to be causing any problems but I'd appreciate feedback from reviewers on that.

Will work on a testcase in a separate patch.

Comment 6

[Bjarne (:bjarne)]

Sorry for barging in, but is there any reason for not using nsURLHelper::net_FilterURIString() to pre-process the string instead of implementing a new version of this loop?

(I stumbled across this issue in a different context a couple of weeks ago and it occurred to me that doing so would be more aligned with the rest of the URL processing.)

Comment 7

[Daniel Veditz [:dveditz]]

(In reply to comment #6)
> Sorry for barging in, but is there any reason for not using
> nsURLHelper::net_FilterURIString() to pre-process the string instead of
> implementing a new version of this loop?

A lot of the callers are already pre-processing the spec using that before handing it off to nsBaseURLParser::ParseURL, so switching to that would fix the narrow issue of stripping non-whitespace control characters. But net_FilterURIString potentially changes the string which would make it impossible to set the offsets correctly, the primary problem of bug 425046. (The limited testcase in that bug would be fixed by the control-char thing, but if you call nsIURLParser->ParseURL directly with leading whitespace you get the same effects. At least 4 addons appear to use nsIURLParser directly and it doesn't look like they pre-filter their URIs. test-enabled builds produce a TestURLParser program in dist/bin, Try passing it "  http://foo.com/bar.txt")
 
And the need to potentially change the string would mean we'd have to allocate and copy the string first, and the whole point of the goofy offset thing is to avoid allocs. Apparently this was a perf hotspot when this code was re-written this way (bug 103916)

Comment 8

[Bjarne (:bjarne)]

I see. Sorry to disturb...  :)

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 349143 [details] [diff] [review]
Fix offsets, only strip whitespace not controls

Looks good, r=jst

Comment 10

[Daniel Veditz [:dveditz]]

I'm going to dial down my paranoia, sg:moderate means we found a pretty bad exploit with some mitigating circumstances. We haven't really found an exploit here, just a correctness problem we're worried might be a useful trick in some future exploit.

Comment 11

[Samuel Sidler (old account; do not CC)]

Comment on attachment 349143 [details] [diff] [review]
Fix offsets, only strip whitespace not controls

Approved for 1.8.1.19 and 1.9.0.5. a=ss for release-drivers

Comment 12

[Daniel Veditz [:dveditz]]

Fix checked into 1.8 and 1.9.0 branches

Comment 13

[Al Billings [:abillings - ex-MoCo]]

The manual tests here are not entirely clear (one claims that everything should be green in a fixed version of Firefox but that isn't the case for 1.8.1.19 or 1.9.0.5).

I did verify a variant of this with bug 425046. That doesn't cover all of the scenarios though.

Comment 14

[Al Billings [:abillings - ex-MoCo]]

Doh, I put that comment in the wrong bug. Please ignore #13.

That being said, I don't know how to reproduce the "inconsistency" of the white space handling outside of my verification of bug 425046.

Comment 15

[Alexander Sack]

Comment on attachment 349143 [details] [diff] [review]
Fix offsets, only strip whitespace not controls

a=asac for 1.8.0 (distros already had that)

Comment 16

[Samuel Sidler (old account; do not CC)]

This should've been blocking 1.9.0.5, not 1.9.0.6, but either way it's landed.

Comment 17

[Jason Duell]

I've been assigned by Johnny to look at networking bugs marked with SG:investigate.   This one looks essentially solved, no? (URLs now parsed consistently, patch landed).  Is there any reason we shouldn't just close this, and/or remove the SG: whiteboard label?

Comment 18

[Samuel Sidler (old account; do not CC)]

This one is solved, but it hasn't landed on trunk or 1.9.1 yet. Dan, wanna land it?

Comment 19

[Johnny Stenback (:jst)]

Fix checked in for trunk and 1.9.1.