Last Comment Bug 380994 - (CVE-2007-3073) Fix for bug 367428 lets through escaped slashes on Linux (windows too on trunk)
(CVE-2007-3073)
: Fix for bug 367428 lets through escaped slashes on Linux (windows too on trunk)
Status: RESOLVED FIXED
[sg:dos]
: fixed1.9.1, privacy, verified1.8.1.17, verified1.9.0.2, verified1.9.0.4
Product: Core
Classification: Components
Component: Networking (show other bugs)
: Trunk
: x86 Linux
P1 normal with 1 vote (vote)
: mozilla1.9
Assigned To: Daniel Veditz [:dveditz]
:
: Patrick McManus [:mcmanus]
Mentors:
Depends on:
Blocks: 395343 CVE-2007-3072 394075 416318
  Show dependency treegraph
 
Reported: 2007-05-17 00:49 PDT by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2009-05-08 13:57 PDT (History)
30 users (show)
benjamin: blocking1.9.1+
dsicore: blocking1.9-
dsicore: wanted1.9.0.x+
dveditz: blocking1.8.1.17+
dveditz: wanted1.8.1.x+
asac: blocking1.8.0.next+
dveditz: wanted1.8.0.x+
reed: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
coalesce escaped slashes too (3.27 KB, patch)
2008-08-26 05:04 PDT, Daniel Veditz [:dveditz]
benjamin: review-
Details | Diff | Splinter Review
updated, with testcase (4.53 KB, patch)
2008-08-26 18:02 PDT, Daniel Veditz [:dveditz]
no flags Details | Diff | Splinter Review
Address biesi's comments (4.73 KB, patch)
2008-08-27 00:16 PDT, Daniel Veditz [:dveditz]
cbiesinger: review+
samuel.sidler+old: approval1.8.1.17+
samuel.sidler+old: approval1.9.0.2+
asac: approval1.8.0.next+
Details | Diff | Splinter Review
incremental patch on top of what's checked in (1.10 KB, patch)
2008-08-27 11:37 PDT, Daniel Veditz [:dveditz]
cbiesinger: review+
dveditz: approval1.8.1.17+
dveditz: approval1.9.0.2+
asac: approval1.8.0.next+
Details | Diff | Splinter Review
iterator hack (on top) for 1.8.0 (1.25 KB, patch)
2008-08-31 06:53 PDT, Alexander Sack
no flags Details | Diff | Splinter Review
iterator hack (on top) for 1.8.0 (1.25 KB, patch)
2008-08-31 07:07 PDT, Alexander Sack
cbiesinger: review+
asac: approval1.8.0.next+
Details | Diff | Splinter Review

Description User image Boris Zbarsky [:bz] (still a bit busy) 2007-05-17 00:49:49 PDT
STEPS TO REPRODUCE:

1)  Open browser on Linux
2)  Load resource:///..%2F..%2F..%2F in it

EXPECTED RESULTS:  Don't see file list for an ancestor of the install directory

ACTUAL RESULTS: I'm looking at a listing of ~bzbarsky (since the browser is installed under ~bzbarsky/mozilla/nightly/install-dir).
Comment 1 User image Benjamin Smedberg [:bsmedberg] 2007-05-25 04:38:40 PDT
This was reported publicly, so opening.
Comment 2 User image Boris Zbarsky [:bz] (still a bit busy) 2007-05-25 07:10:04 PDT
It was?  Where?
Comment 3 User image Benjamin Smedberg [:bsmedberg] 2007-05-25 07:19:53 PDT
bug 367428 comment 16
Comment 4 User image Benjamin Smedberg [:bsmedberg] 2007-06-08 08:58:09 PDT
Taking for now, but biesi if you have time for this I'd much appreciate it.
Comment 5 User image Daniel Veditz [:dveditz] 2007-07-09 11:20:26 PDT
taking to get a fix in.
Comment 6 User image Daniel Veditz [:dveditz] 2007-07-12 10:36:52 PDT
On trunk it looks like windows can get fooled by ..%2f.. too.
Comment 7 User image Ben Bucksch (:BenB) 2007-07-14 04:51:02 PDT
resource:///..%2F..%2F..%2F..%2F/etc/host.conf
Comment 8 User image georgi - hopefully not receiving bugspam 2008-01-29 02:34:43 PST
macosx on ppc seems partially affected - can traverse up to /Volumes (when started from .dmg)
Comment 9 User image Mike Beltzner [:beltzner, not reading bugmail] 2008-02-27 14:11:02 PST
Moving bugs that aren't beta 4 blockers to target final release.
Comment 10 User image Samuel Sidler (old account; do not CC) 2008-02-29 11:42:38 PST
Dan says this should block. It's blocking on branch as well.
Comment 11 User image Mike Schroepfer 2008-03-31 13:09:55 PDT
Dveditz you on this?
Comment 12 User image Mike Schroepfer 2008-03-31 16:19:50 PDT
Given this is sg:low/dos taking off the blocker list - we'd certainly take a patch..
Comment 13 User image Boris Zbarsky [:bz] (still a bit busy) 2008-03-31 20:37:15 PDT
This isn't quite limited to a dos.  It has potential for data leakage, since any website can link to resource: scripts and stylesheets and then try to glean data from the result.  For example, if you happen to have any interesting data stored in JSON on your hard drive, you lose.

I agree that it's not stop-ship, but I think it should be a very high priority for fixing as soon as we possibly can....
Comment 14 User image Damon Sicore (:damons) 2008-04-01 14:06:35 PDT
Not blocking, but wanted1.9.0.x+, based on comment #13. 
Comment 15 User image Daniel Veditz [:dveditz] 2008-08-26 05:04:22 PDT
Created attachment 335516 [details] [diff] [review]
coalesce escaped slashes too

The line numbers in the patch are for the 1.8 branch, but it applies cleanly to cvs trunk and mozilla-central
Comment 16 User image Benjamin Smedberg [:bsmedberg] 2008-08-26 10:56:06 PDT
Comment on attachment 335516 [details] [diff] [review]
coalesce escaped slashes too

Well, this will work but will almost certainly cause multiple allocations for URIs over 40 chars... can you at least SetCapacity on spec before entering the loop?

Also, this needs a unit-test... you should be able to add a trivial one to http://mxr.mozilla.org/mozilla-central/source/netwerk/test/unit/test_bug337744.js or write a new test copying that code.
Comment 17 User image Daniel Veditz [:dveditz] 2008-08-26 18:02:45 PDT
Created attachment 335650 [details] [diff] [review]
updated, with testcase
Comment 18 User image Daniel Veditz [:dveditz] 2008-08-26 18:04:35 PDT
Worked a fix for related bug 394075 into this patch.
Comment 19 User image Christian :Biesinger (don't email me, ping me on IRC) 2008-08-26 20:40:53 PDT
Comment on attachment 335650 [details] [diff] [review]
updated, with testcase

+        if (*src == '%' && *(src+1) == '2') {

*(src+1) mighht not be valid. You need to check that src+1 != end.

Similar on the next line

(an nsACString isn't necessarily nullterminated)

I also don't think that appending character-by-character is particularly efficient code... but that probably doesn't matter much. (other code keeps track of how many characters were left unchanged and append them in once call. cf. http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsEscape.cpp#516)

I'd suggest spaces around your + operators but this file doesn't really seem to do that

r+sr=biesi if you fix the src+1 thing
Comment 20 User image georgi - hopefully not receiving bugspam 2008-08-26 23:21:57 PDT
> *(src+1) mighht not be valid.

so may be:
char ch = *(src+2);
Comment 21 User image Daniel Veditz [:dveditz] 2008-08-27 00:16:33 PDT
Created attachment 335671 [details] [diff] [review]
Address biesi's comments
Comment 22 User image Samuel Sidler (old account; do not CC) 2008-08-27 00:19:26 PDT
Comment on attachment 335671 [details] [diff] [review]
Address biesi's comments

Approved for 1.8.1.17. a=ss
Comment 23 User image Samuel Sidler (old account; do not CC) 2008-08-27 00:20:06 PDT
Comment on attachment 335671 [details] [diff] [review]
Address biesi's comments

And approved for 1.9.0.2 as well. a=ss
Comment 24 User image Daniel Veditz [:dveditz] 2008-08-27 00:27:41 PDT
Fix checked in to 1.8.1 and 1.9.0 branches. Will seek retroactive r= from biesi to make sure this is what he wanted
Comment 25 User image georgi - hopefully not receiving bugspam 2008-08-27 02:51:04 PDT
+           if (*(src+2) == 'f' || *(src+***1***) == 'F')
+             ch = '/';
+           else if (*(src+2) == 'e' || *(src+2) == 'E')

is the only |+1| on purpose ?
Comment 26 User image Christian :Biesinger (don't email me, ping me on IRC) 2008-08-27 11:34:24 PDT
Comment on attachment 335671 [details] [diff] [review]
Address biesi's comments

+           if (*(src+2) == 'f' || *(src+1) == 'F')

I agree with georgi, that should be +2

maybe the test should verify the final URL's spec, instead of just verifying that it doesn't contain ..?
Comment 27 User image Daniel Veditz [:dveditz] 2008-08-27 11:37:07 PDT
Created attachment 335741 [details] [diff] [review]
incremental patch on top of what's checked in
Comment 28 User image Mike Beltzner [:beltzner, not reading bugmail] 2008-08-27 11:52:42 PDT
This needs to land both on the branch and on the GECKO190_20080827_RELBRANCH
Comment 29 User image Mike Beltzner [:beltzner, not reading bugmail] 2008-08-27 11:53:11 PDT
Er, make that both branches (1.8.1 and 1.9) and GECKO190_20080827_RELBRANCH
Comment 30 User image Daniel Veditz [:dveditz] 2008-08-27 12:23:16 PDT
incremental fix checked into 1.8, 1.9 and _RELBRANCH. Filed bug 452470 for improving the testcase.
Comment 31 User image juan becerra [:juanb] 2008-08-29 15:50:56 PDT
Verified on latest build candidates for 20017 and 3.0.2. When I load resource:///..%2F..%2F..%2F I see the contents of these directories: 

On 20016
Index of file:///Users/user/Desktop/Firefox.app/Contents/MacOS/../../../
Index of file:///home/mozilla/Desktop/firefox/../../../
WinXP/Vista (nothing happens)

On 20017
Index of file:///Users/user/Desktop/Firefox.app/Contents/MacOS/
Index of file:///home/mozilla/Desktop/firefox/
Index of file:///C:/Program Files/Mozilla Firefox/

On 3.0.1
Index of file:///Users/user/Desktop/Firefox.app/Contents/MacOS/../../../
Index of file:///home/mozilla/Desktop/firefox/../../../
WinXP/Vista (nothing happens)

On 3.0.2
Index of file:///Users/user/Desktop/Firefox.app/Contents/MacOS/
Index of file:///home/mozilla/Desktop/firefox/
Index of file:///C:/Program Files/Mozilla Firefox/
Comment 32 User image Alexander Sack 2008-08-31 06:27:20 PDT
Comment on attachment 335671 [details] [diff] [review]
Address biesi's comments

a=asac for 1.8.0.15 (with attachment 335741 [details] [diff] [review])
Comment 33 User image Alexander Sack 2008-08-31 06:50:56 PDT
Comment on attachment 335741 [details] [diff] [review]
incremental patch on top of what's checked in

a=asac for 1.8.0.15 (with attachment 335741 [details] [diff] [review] and iterator backport patch)
Comment 34 User image Alexander Sack 2008-08-31 06:53:34 PDT
Created attachment 336249 [details] [diff] [review]
iterator hack (on top) for 1.8.0

biesi, you think this 1.8.0 fix is too ugly for the sake of keeping the other patches the unmodified?
Comment 35 User image Alexander Sack 2008-08-31 07:07:18 PDT
Created attachment 336251 [details] [diff] [review]
iterator hack (on top) for 1.8.0 

err, here the one that was meant to be attached :)
Comment 36 User image Christian :Biesinger (don't email me, ping me on IRC) 2008-09-01 18:39:02 PDT
Comment on attachment 336251 [details] [diff] [review]
iterator hack (on top) for 1.8.0 

this is for 180 only? fine with me :)
Comment 37 User image Alexander Sack 2008-09-02 01:29:54 PDT
Comment on attachment 336251 [details] [diff] [review]
iterator hack (on top) for 1.8.0 

a=asac for 1.8.0.15

thanks biesi!
Comment 38 User image Marc Bejarano 2008-09-29 16:04:29 PDT
can't we resolve this as fixed, now?
Comment 39 User image Daniel Veditz [:dveditz] 2008-09-30 11:29:01 PDT
I need someone to check this into mozilla-central to call it "fixed"
Comment 40 User image Reed Loden [:reed] (use needinfo?) 2008-09-30 22:18:22 PDT
http://hg.mozilla.org/mozilla-central/rev/6dad95d60106
Comment 41 User image Reed Loden [:reed] (use needinfo?) 2008-09-30 22:21:31 PDT
http://hg.mozilla.org/mozilla-central/rev/1eccc541661c
Comment 42 User image Al Billings [:abillings] 2008-10-23 15:19:51 PDT
Verified for 1.9.0.4 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.4pre) Gecko/2008102304 GranParadiso/3.0.4pre.
Comment 43 User image Benjamin Smedberg [:bsmedberg] 2009-01-28 07:34:25 PST
This landed before 1.9.1 branched
Comment 44 User image Mike Beltzner [:beltzner, not reading bugmail] 2009-01-30 14:41:53 PST
(In reply to comment #43)
> This landed before 1.9.1 branched

Adding fixed1.9.1

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