URL parsing treats leading whitespace inconsistently

RESOLVED FIXED

Status

()

Core
Networking
P2
normal
RESOLVED FIXED
9 years ago
4 years ago

People

(Reporter: smontagu, Assigned: dveditz)

Tracking

({fixed1.8.1.19, fixed1.9.0.5, fixed1.9.1})

Trunk
fixed1.8.1.19, fixed1.9.0.5, fixed1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
blocking1.8.1.19 -
wanted1.8.1.x +
blocking1.8.0.next +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:investigate])

Attachments

(1 attachment)

4.60 KB, patch
jst
: review+
Samuel Sidler (old account; do not CC)
: approval1.8.1.19+
Samuel Sidler (old account; do not CC)
: approval1.9.0.5+
Alexander Sack
: approval1.8.0.next+
Details | Diff | Splinter Review
(Reporter)

Description

9 years ago
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.
(Reporter)

Updated

9 years ago
Blocks: 443288
Whiteboard: [sg:low]
(Assignee)

Comment 1

9 years ago
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?]
(Assignee)

Updated

9 years ago
Flags: blocking1.9.1?
Flags: blocking1.9.0.3+
Flags: blocking1.8.1.18+
Flags: blocking1.8.1.17?
(Assignee)

Updated

9 years ago
Assignee: nobody → dveditz
(Assignee)

Updated

9 years ago
Flags: blocking1.9.0.5+
Flags: blocking1.9.0.4+
Flags: blocking1.8.1.19+
Flags: blocking1.8.1.18+

Updated

9 years ago
Flags: blocking1.9.1? → blocking1.9.1+

Comment 2

9 years ago
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?]
(Assignee)

Comment 5

9 years ago
Created attachment 349143 [details] [diff] [review]
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.
Attachment #349143 - Flags: review?(jst)

Comment 6

9 years ago
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.)
(Assignee)

Comment 7

9 years ago
(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)
Blocks: 425046

Comment 8

9 years ago
I see. Sorry to disturb...  :)

Updated

9 years ago
Attachment #349143 - Flags: review?(jst) → review+
Comment on attachment 349143 [details] [diff] [review]
Fix offsets, only strip whitespace not controls

Looks good, r=jst
(Assignee)

Comment 10

9 years ago
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]
(Assignee)

Updated

9 years ago
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+
(Assignee)

Comment 12

9 years ago
Fix checked into 1.8 and 1.9.0 branches
Keywords: fixed1.8.1.19, fixed1.9.0.5
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.
(Assignee)

Updated

9 years ago
Group: core-security

Comment 15

9 years ago
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+

Updated

9 years ago
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
Last Resolved: 9 years ago
Keywords: fixed1.9.1
Resolution: --- → FIXED
Keywords: checkin-needed
Depends on: 982302
You need to log in before you can comment on or make changes to this bug.