Closed Bug 451613 Opened 16 years ago Closed 15 years ago

URL parsing treats leading whitespace inconsistently

Categories

(Core :: Networking, defect, P2)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: smontagu, Assigned: dveditz)

References

Details

(Keywords: fixed1.8.1.19, fixed1.9.0.5, fixed1.9.1, Whiteboard: [sg:investigate])

Attachments

(1 file)

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.
Whiteboard: [sg:low]
raising severity just because we depend so heavily on URIs being parsed sanely and consistently -- surely this will bite us badly elsewhere
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.17?
Whiteboard: [sg:low] → [sg:moderate?]
Flags: blocking1.9.1?
Flags: blocking1.9.0.3+
Flags: blocking1.8.1.18+
Flags: blocking1.8.1.17?
Assignee: nobody → dveditz
Flags: blocking1.9.0.5+
Flags: blocking1.9.0.4+
Flags: blocking1.8.1.19+
Flags: blocking1.8.1.18+
Flags: blocking1.9.1? → blocking1.9.1+
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...
Dan, how's a patch coming for this bug? Code freeze is in one week.
Whiteboard: [sg:moderate?] → [sg:moderate?][needs 1.8/1.9 patches]
This isn't going to make 1.9.0.6 or 1.8.1.19. Pushing out.
Flags: blocking1.9.0.6+
Flags: blocking1.9.0.5+
Flags: blocking1.8.1.19-
Flags: blocking1.8.1.19+
Whiteboard: [sg:moderate?][needs 1.8/1.9 patches] → [sg:moderate?]
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.
Attachment #349143 - Flags: review?(jst)
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.)
(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)
I see. Sorry to disturb...  :)
Attachment #349143 - Flags: review?(jst) → review+
Comment on attachment 349143 [details] [diff] [review]
Fix offsets, only strip whitespace not controls

Looks good, r=jst
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.
Whiteboard: [sg:moderate?] → [sg:investigate]
Attachment #349143 - Flags: approval1.9.0.5?
Attachment #349143 - Flags: approval1.8.1.19?
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
Attachment #349143 - Flags: approval1.9.0.5?
Attachment #349143 - Flags: approval1.9.0.5+
Attachment #349143 - Flags: approval1.8.1.19?
Attachment #349143 - Flags: approval1.8.1.19+
Fix checked into 1.8 and 1.9.0 branches
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.
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.
Group: core-security
Comment on attachment 349143 [details] [diff] [review]
Fix offsets, only strip whitespace not controls

a=asac for 1.8.0 (distros already had that)
Attachment #349143 - Flags: approval1.8.0.next+
Flags: blocking1.8.0.next+
This should've been blocking 1.9.0.5, not 1.9.0.6, but either way it's landed.
Flags: blocking1.9.0.6+
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?
This one is solved, but it hasn't landed on trunk or 1.9.1 yet. Dan, wanna land it?
Keywords: checkin-needed
Priority: -- → P2
Fix checked in for trunk and 1.9.1.
Status: NEW → RESOLVED
Closed: 15 years ago
Keywords: fixed1.9.1
Resolution: --- → FIXED
Depends on: 982302
You need to log in before you can comment on or make changes to this bug.