Last Comment Bug 451613 - URL parsing treats leading whitespace inconsistently
: URL parsing treats leading whitespace inconsistently
Status: RESOLVED FIXED
[sg:investigate]
: fixed1.8.1.19, fixed1.9.0.5, fixed1.9.1
Product: Core
Classification: Components
Component: Networking (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: ---
Assigned To: Daniel Veditz [:dveditz]
:
: Patrick McManus [:mcmanus]
Mentors:
Depends on: 982302
Blocks: CVE-2008-5508 CVE-2008-0016
  Show dependency treegraph
 
Reported: 2008-08-21 12:39 PDT by Simon Montagu :smontagu
Modified: 2014-03-12 02:43 PDT (History)
17 users (show)
benjamin: blocking1.9.1+
samuel.sidler+old: blocking1.8.1.19-
dveditz: wanted1.8.1.x+
asac: blocking1.8.0.next+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix offsets, only strip whitespace not controls (4.60 KB, patch)
2008-11-20 00:37 PST, Daniel Veditz [:dveditz]
jst: review+
samuel.sidler+old: approval1.8.1.19+
samuel.sidler+old: approval1.9.0.5+
asac: approval1.8.0.next+
Details | Diff | Splinter Review

Description Simon Montagu :smontagu 2008-08-21 12:39:58 PDT
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] 2008-08-22 11:25:19 PDT
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) 2008-11-10 04:31:48 PST
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) 2008-11-10 09:40:33 PST
Dan, how's a patch coming for this bug? Code freeze is in one week.
Comment 4 Samuel Sidler (old account; do not CC) 2008-11-19 15:13:07 PST
This isn't going to make 1.9.0.6 or 1.8.1.19. Pushing out.
Comment 5 Daniel Veditz [:dveditz] 2008-11-20 00:37:17 PST
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.
Comment 6 Bjarne (:bjarne) 2008-11-20 00:54:26 PST
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] 2008-11-20 10:21:18 PST
(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) 2008-11-20 11:08:50 PST
I see. Sorry to disturb...  :)
Comment 9 Johnny Stenback (:jst, jst@mozilla.com) 2008-11-20 17:27:43 PST
Comment on attachment 349143 [details] [diff] [review]
Fix offsets, only strip whitespace not controls

Looks good, r=jst
Comment 10 Daniel Veditz [:dveditz] 2008-11-20 21:33:24 PST
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) 2008-11-21 11:16:59 PST
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] 2008-11-24 14:47:12 PST
Fix checked into 1.8 and 1.9.0 branches
Comment 13 Al Billings [:abillings] 2008-11-25 17:37:51 PST
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] 2008-11-25 17:40:26 PST
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 2008-12-17 05:56:55 PST
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) 2009-01-05 07:33:17 PST
This should've been blocking 1.9.0.5, not 1.9.0.6, but either way it's landed.
Comment 17 Jason Duell [:jduell] (needinfo me) 2009-01-07 15:18:32 PST
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) 2009-01-09 22:20:14 PST
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, jst@mozilla.com) 2009-01-12 17:23:54 PST
Fix checked in for trunk and 1.9.1.

Note You need to log in before you can comment on or make changes to this bug.