Last Comment Bug 413250 - chrome directory traversal (local disk access via "flat" addons)
: chrome directory traversal (local disk access via "flat" addons)
Status: RESOLVED FIXED
[sg:high]
: fixed1.8.0.15, verified1.8.1.12, verified1.8.1.13
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: All All
: P1 normal with 3 votes (vote)
: ---
Assigned To: Daniel Veditz [:dveditz]
:
Mentors:
http://www.hiredhacker.com/2008/01/19...
: 182176 (view as bug list)
Depends on: 415367 417086
Blocks: 413451 414327 415116 415191 415292
  Show dependency treegraph
 
Reported: 2008-01-20 13:00 PST by Daniel Veditz [:dveditz]
Modified: 2009-06-16 09:01 PDT (History)
52 users (show)
mtschrep: blocking1.9+
dveditz: blocking1.8.1.12+
dveditz: wanted1.8.1.x+
asac: blocking1.8.0.next+
benjamin: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
prevent directory traversal in mozilla/chrome (870 bytes, patch)
2008-01-24 04:07 PST, Daniel Veditz [:dveditz]
no flags Details | Diff | Review
rdf/chrome version (1.89 KB, patch)
2008-01-24 04:45 PST, Daniel Veditz [:dveditz]
neil: review+
Details | Diff | Review
Fix length after changing string (2.87 KB, patch)
2008-01-24 13:01 PST, Daniel Veditz [:dveditz]
dveditz: review+
benjamin: superreview+
dveditz: approval1.8.1.12+
asac: approval1.8.0.next+
Details | Diff | Review
catch both levels of unescaping (2.45 KB, patch)
2008-01-31 13:15 PST, Daniel Veditz [:dveditz]
benjamin: review+
benjamin: superreview+
samuel.sidler+old: approval1.8.1.12+
mbeltzner: approval1.9b3+
asac: approval1.8.0.next+
Details | Diff | Review
mozilla/rdf/chrome version (1.82 KB, patch)
2008-01-31 16:28 PST, Daniel Veditz [:dveditz]
neil: review+
neil: superreview+
Details | Diff | Review
flexible online testcase (derived from second hiredhacker example) (1.13 KB, text/html)
2008-01-31 18:08 PST, Alex Vincent [:WeirdAl]
no flags Details
skip the query ('?') part (3.58 KB, patch)
2008-02-01 14:59 PST, Daniel Veditz [:dveditz]
neil: review+
neil: superreview+
Details | Diff | Review
1.8 version of above minus the bug 415116 bit (3.90 KB, patch)
2008-02-01 19:18 PST, Daniel Veditz [:dveditz]
samuel.sidler+old: approval1.8.1.12+
samuel.sidler+old: approval1.8.1.13+
asac: approval1.8.0.next+
Details | Diff | Review
Handle query string part on trunk (5.23 KB, patch)
2008-02-02 11:35 PST, :Ehsan Akhgari (busy, don't ask for review please)
no flags Details | Diff | Review

Description Daniel Veditz [:dveditz] 2008-01-20 13:00:21 PST
The hirehacker.com blog had a post by Gerry Eisenhaur demonstrating that the chrome: URI scheme does not prevent directory traversal. When a chrome package is "flat" rather than contained in a .jar the directory traversal allows escaping the extensions directory and reading random files on the disk. Several popular addons are packaged in this way

http://www.hiredhacker.com/2008/01/19/firefox-chrome-url-handling-directory-traversal/

Web content cannot in general link to chrome: uris, but there are exceptions for images, scripts and stylesheets. There's not a whole lot in those formats in known locations, but given the starting point is inside the salted profile directory you could at least steal the user's prefs.js (probably more interesting in SeaMonkey and Thunderbird than in Firefox)

The PoC requires Download Statusbar: https://addons.mozilla.org/en-US/firefox/addon/26
Comment 1 Daniel Veditz [:dveditz] 2008-01-20 13:29:40 PST
Of course that's exactly why the first line of prefs.js is
  # Mozilla User Preferences

You can't actually steal anything from prefs.js, the parser craps out right away on the illegal character. Still, there might be other sensitive files that might be readable this way, or at the very least the fact that certain files exist or not could be interesting information (via load errors).
Comment 2 Jesse Ruderman 2008-01-22 11:20:11 PST
See also bug 394075, a similar-sounding bug for resource: URLs.
Comment 3 Benjamin Smedberg [:bsmedberg] 2008-01-22 15:50:57 PST
So, we've had directory-traversal protection in place for a long time:

http://mxr.mozilla.org/mozilla/source/chrome/src/nsChromeRegistry.cpp#623
combined with
http://mxr.mozilla.org/mozilla/source/chrome/src/nsChromeRegistry.cpp#686

should prevent this attack, though apparently it's not. See bug 338037 for additional protections that were put in place.
Comment 4 Jesse Ruderman 2008-01-22 20:25:42 PST
Bug 413451 explains why this is [sg:high].
Comment 5 Devon Jensen 2008-01-22 23:59:24 PST
Note:
I just released a JARred version of Download Statusbar 0.9.5.3

If you want to test this bug in FF2, you can use Download Statusbar 0.9.5.2
https://addons.mozilla.org/en-US/firefox/addons/versions/26

(Yes, I realize that this is only one of many 'flat' extensions but considering it is the main example and the large user base, I thought it best to JAR it up for now)

I prefer the flat file structure so I hope this can get fixed - 
Comment 6 georgi - hopefully not receiving bugspam 2008-01-23 00:16:28 PST
(In reply to comment #1)
> or at the very least the fact that certain files
> exist or not could be interesting information (via load errors).
> 

you don't need traversal for this. in the wild there is something like "extension checker" that does "img src='chrome://ext1/img.png' onload='exists' onerror='noexists'"
Comment 7 Jesse Ruderman 2008-01-23 00:45:33 PST
This bug lets a malicious website find out whether I have applications such as Google Earth installed.  That's not the same as finding out whether I have a given extension installed.
Comment 8 georgi - hopefully not receiving bugspam 2008-01-23 03:41:02 PST
is there a reason not to kill all access from content to chrome:// and resource://?
Comment 9 Benjamin Smedberg [:bsmedberg] 2008-01-23 06:34:50 PST
Yes, we've discussed this in several bugs that killing access to chrome:// and resource:// completely would break remote XUL as well as several kinds of about: pages.

Has anyone breakpointed the code above to see why the existing traversal checks are not enough?
Comment 10 Giorgio Maone [:mao] 2008-01-23 09:31:43 PST
Just for the sake of completeness, NoScript prevents this bug from being exploited, no matter if the site is trusted or not, by blocking chrome JS load attempts originated from content.

IIRC, Adblock Plus should have a similar feature as well.
Comment 11 Giorgio Maone [:mao] 2008-01-23 11:11:19 PST
(In reply to comment #3)
> http://mxr.mozilla.org/mozilla/source/chrome/src/nsChromeRegistry.cpp#623
> http://mxr.mozilla.org/mozilla/source/chrome/src/nsChromeRegistry.cpp#686
> 
(In reply to comment #9)
> Has anyone breakpointed the code above to see why the existing traversal
> checks are not enough?

Correct me if I'm missing something, but even without breakpointing I'd say you need to *percent decode* GetFilePath() output before searching for dangerous stuff...


Comment 12 Benjamin Smedberg [:bsmedberg] 2008-01-23 11:34:45 PST
My understanding was that standardurl unescaped everything at parse-time... biesi is that understanding incorrect?
Comment 13 Giorgio Maone [:mao] 2008-01-23 12:38:55 PST
(In reply to comment #12)
> My understanding was that standardurl unescaped everything at parse-time...
> biesi is that understanding incorrect?

Apparently not, and if it did I would regard it as a bug...

Try this on the Error Console:

var CI = Components.interfaces; var u = Components.classes["@mozilla.org/network/standard-url;1"].createInstance(CI.nsIStandardURL); u.init(1, -1, "chrome://foo/%2e%2e%2f%bar", null, null); alert(u.QueryInterface(CI.nsIURL).filePath);
Comment 14 Giorgio Maone [:mao] 2008-01-23 12:47:37 PST
(In reply to comment #13)
> (In reply to comment #12)
> > My understanding was that standardurl unescaped everything at parse-time...
> > biesi is that understanding incorrect?
> 
> Apparently not, and if it did I would regard it as a bug...

I obviously meant that StandardURL does *not* seem to escape anything at parse time -- I wasn't commenting on your understanding :)
Comment 15 Sam Quigley 2008-01-23 12:54:43 PST
Just thought I'd point out that there's an 8-month old, very similar-sounding
bug regarding resource:// urls at bug 380994.  (380994 is different from the
previously-mentioned bug 394075...)

-sq
Comment 16 Giorgio Maone [:mao] 2008-01-23 13:08:19 PST
On a side note, the fact you should expect escaped chars is "even" documented:

http://mxr.mozilla.org/mozilla/source/netwerk/base/public/nsIURL.idl#71
http://mxr.mozilla.org/mozilla/source/netwerk/base/src/nsStandardURL.cpp#1954
Comment 17 Daniel Veditz [:dveditz] 2008-01-24 04:07:16 PST
Created attachment 298911 [details] [diff] [review]
prevent directory traversal in mozilla/chrome

We'll need a different patch for rdf/chrome -- it handles %2e%2e fine but is fooled by "%2e." and ".%2e". I can't figure out if that's used on the trunk anymore, but it's definitely used by Seamonkey on the 1.8 branch.
Comment 18 Daniel Veditz [:dveditz] 2008-01-24 04:45:49 PST
Created attachment 298916 [details] [diff] [review]
rdf/chrome version

Similar change for the suite. Seemed better to make it match Firefox and just kill suspicious URIs than to try to support relative chrome URIs as long as they don't go "too far".
Comment 19 neil@parkwaycc.co.uk 2008-01-24 05:20:28 PST
Comment on attachment 298916 [details] [diff] [review]
rdf/chrome version

I think Camino currently still uses this code on trunk.
Comment 20 georgi - hopefully not receiving bugspam 2008-01-24 10:39:25 PST
does the patch protect against <script src="resource://"></script>

data:text/html;,<script%20src="resource://"></script>

error console reveals install location
Comment 21 Daniel Veditz [:dveditz] 2008-01-24 13:01:24 PST
Created attachment 299017 [details] [diff] [review]
Fix length after changing string

Carrying over Neil's r+
Added call to SetLength() after modifying the string.
Comment 22 Daniel Veditz [:dveditz] 2008-01-24 13:15:13 PST
Comment on attachment 299017 [details] [diff] [review]
Fix length after changing string

approved for 1.8.1.12, a=dveditz for release-drivers
Comment 23 Daniel Veditz [:dveditz] 2008-01-24 13:16:33 PST
(In reply to comment #20)
> does the patch protect against [...]

No, that's completely unrelated. Is there an existing bug on that?
Comment 24 Daniel Veditz [:dveditz] 2008-01-24 21:01:36 PST
checked into 1.8 branch, waiting for trunk tree to open.
Comment 25 Daniel Veditz [:dveditz] 2008-01-24 22:58:30 PST
checked in on trunk
Comment 26 georgi - hopefully not receiving bugspam 2008-01-24 23:29:37 PST
(In reply to comment #23)
> (In reply to comment #20)
> > does the patch protect against [...]
> 
> No, that's completely unrelated. Is there an existing bug on that?
> 

it is Bug 394075
can't see the sg:high bug, but "resource:" causes similar trouble to this bug
Comment 27 Al Billings [:abillings] 2008-01-29 16:23:29 PST
I've verified both testcases from the blog. The first is Windows specific because of the path.

I've verified them with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.12) Gecko/2008012820 Firefox/2.0.0.12.

I also verified the sessionstore on on Mac (it repros in 2.0.0.11) with Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.12) Gecko/2008012822 Firefox/2.0.0.12.

All looks good.
Comment 28 Daniel Veditz [:dveditz] 2008-01-29 21:38:29 PST
Al: Just double-checking, but what version of the download statusbar did you use? The latest, 0.5.9.3 released Jan 22, cannot be used in this exploit. You have to use the current version when the blog came out on Jan 19, version 0.5.9.2

You can get the older version from AMO's "Complete version history" link.

I've verified the fix myself so I'm not worried, but I wrote the patch so I probably don't count.
Comment 29 Al Billings [:abillings] 2008-01-29 22:01:47 PST
?? I verified that the fix *worked*, Dan. :-) 

There is no issue here. We're done unless something horribly goes wrong. It's all good.

I used the older version of the extension specifically because of the comments above noting that it had been changed to a jar format recently. 

The bug is still marked unverified because it isn't clear to me if this needs to land in trunk since "version" is "unspecified".
Comment 30 Henrik Skupin (:whimboo) 2008-01-29 23:32:01 PST
(In reply to comment #29)
> The bug is still marked unverified because it isn't clear to me if this needs
> to land in trunk since "version" is "unspecified".

Al, the trunk check is noted in comment 25. So updating version field.
Comment 31 Devon Jensen 2008-01-30 21:58:25 PST
Is this a separate issue?

http://www.hiredhacker.com/2008/01/31/more-chrome-directory-traversing/
Comment 32 Al Billings [:abillings] 2008-01-30 22:08:59 PST
Devon, that has been dealt with elsewhere. I don't have the bug number handy.
Comment 33 Al Billings [:abillings] 2008-01-30 22:12:02 PST
Ack, no, I misspoke. You are right, this is new. I'll contact people about this right now to get it investigated.
Comment 34 Giorgio Maone [:mao] 2008-01-30 22:58:08 PST
@Daniel Veditz:
I can see double escaping happening.
Either somewhere something performs deep escaping before using the URL, or it's your new escaping that should not have been done in place.

We can fix it quickly and safely by doing deep escaping (i.e. escaping until no "%" character is found) ON A COPY before checking. The only disadvantage of this approach is that we won't allow any URI containing a literal sequence among [%2E%2E|.%2E|%2E.], but I don't believe it's a trouble in chrome: URIs...

Otherwise we must ensure NO double escaping can happen anywhere, and still check ON A COPY to prevent our check itself from causing a double escaping.
Comment 35 Daniel Veditz [:dveditz] 2008-01-31 13:15:29 PST
Created attachment 300720 [details] [diff] [review]
catch both levels of unescaping

The original (and this) patch unescape on a copy in Firefox (in the old chrome registry used by seamonkey it did modify the chrome URI). The multiple unescaping is happening after it's turned into a file: uri elsewhere. Both demos from hiredhacker.com work in Firefox 2.0.0.11, the previous patch did not "cause" the second demonstration it just didn't fix it.

This patch takes advantage of the already unescaped path instead of doing it twice, and then checks for further escaping.

I also fixed bug 415116 out of paranoia, in case there were tricks you could play with the unchecked provider string.
Comment 36 Samuel Sidler (old account; do not CC) 2008-01-31 13:32:40 PST
Comment on attachment 300720 [details] [diff] [review]
catch both levels of unescaping

Approved for 1.8.1.12; a=ss.
Comment 37 Daniel Veditz [:dveditz] 2008-01-31 13:34:38 PST
Comment on attachment 300720 [details] [diff] [review]
catch both levels of unescaping

Given the high profile of this PoC we should fix FF3b3 also.
Comment 38 Mike Beltzner [:beltzner, not reading bugmail] 2008-01-31 13:50:04 PST
Comment on attachment 300720 [details] [diff] [review]
catch both levels of unescaping

a=beltzner
Comment 39 Benjamin Smedberg [:bsmedberg] 2008-01-31 14:26:07 PST
Mossop, can you write a testcase for this?
Comment 40 Robert Kaiser (not working on stability any more) 2008-01-31 14:58:01 PST
Do we need to patch rdf/chrome for this second part as well?
Comment 41 Daniel Veditz [:dveditz] 2008-01-31 16:28:56 PST
Created attachment 300771 [details] [diff] [review]
mozilla/rdf/chrome version
Comment 42 Daniel Veditz [:dveditz] 2008-01-31 18:00:56 PST
mozilla/chrome fix checked in to trunk, 1.8 branch and 1.8.1.12 relbranch. Filed bug 415191 to track landing the mozilla/rdf/chrome version once it's gotten appropriate reviews.
Comment 43 Alex Vincent [:WeirdAl] 2008-01-31 18:08:52 PST
Created attachment 300797 [details]
flexible online testcase (derived from second hiredhacker example)

I didn't feel like installing sessionstore here.  :)
Comment 44 neil@parkwaycc.co.uk 2008-02-01 04:06:44 PST
Comment on attachment 300771 [details] [diff] [review]
mozilla/rdf/chrome version

>+          if (*(curChar+1) == '.')
Personally I'd find the code easier to read if you renamed the variable curPos and used curPos[1] here (and similarly below).
Comment 45 georgi - hopefully not receiving bugspam 2008-02-01 04:23:44 PST
 ( *(curChar+2) == 'e' || *(curChar+2) == 'E' || 

is *(curChar+2) guaranteed to be within the string?

if not, it may trigger valgrind warning or even crash
Comment 46 georgi - hopefully not receiving bugspam 2008-02-01 05:39:58 PST
(In reply to comment #45)
>  ( *(curChar+2) == 'e' || *(curChar+2) == 'E' || 
> 
> is *(curChar+2) guaranteed to be within the string?
> 
> if not, it may trigger valgrind warning or even crash
> 

ooops, this comment is wrong.
if (*(curChar+1) == '2' will stop reading if the string is null terminated.
Comment 47 Daniel Veditz [:dveditz] 2008-02-01 11:40:56 PST
> [...] will stop reading if the string is null terminated.

ns[C]AutoString is guaranteed to be null-terminated.

Comment 48 Daniel Veditz [:dveditz] 2008-02-01 14:59:27 PST
Created attachment 300944 [details] [diff] [review]
skip the query ('?') part

attachment 300771 [details] [diff] [review] hasn't landed for seamonkey, but the earlier patch did and caused problems with the advanced search sidebar:
   http://forums.mozillazine.org/viewtopic.php?t=625043

Since it uses URIs of the form
chrome://communicator/content/search/internetresults.xul?internetsearch:engine=engine://C%253A%255Cmozilla%255Cdist%255Cbin%255Csearchplugins%255Cdmoz.src&engine=engine://C%253A%255Cmozilla%255Cdist%255Cbin%255Csearchplugins%255Cgoogle.src&engine=engine://C%253A%255Cmozilla%255Cdist%255Cbin%255Csearchplugins%255Cjeeves.src&text=chrome
the newer patch will have the same problem (note double-escaped %253A).

New patch stops vetting the path when we hit the query part since that part isn't used to open any local files.
Comment 49 Daniel Veditz [:dveditz] 2008-02-01 19:18:48 PST
Created attachment 300976 [details] [diff] [review]
1.8 version of above minus the bug 415116 bit

The fix for bug 415116 was breaking the French translation for FF 2.0.0.12. We could fix the bug in the french help file, but seemed safer to eliminate the new check rather than worry what addons might have similarly sloppy links.

Also want to adopt the "stop at the query params" fix in Firefox: if it broke Seamonkey there might likewise be addons doing the same trick.
Comment 50 Samuel Sidler (old account; do not CC) 2008-02-01 19:55:42 PST
Comment on attachment 300976 [details] [diff] [review]
1.8 version of above minus the bug 415116 bit

Approved for 1.8.1.12; a=ss.

Please land on the relbranch and on the 1.8 branch.
Comment 51 :Ehsan Akhgari (busy, don't ask for review please) 2008-02-02 11:35:24 PST
Created attachment 301036 [details] [diff] [review]
Handle query string part on trunk

A copy of the patch posted in bug 415367 to handle the query string part on trunk.
Comment 52 Al Billings [:abillings] 2008-02-05 13:17:29 PST
I've verified this again with rc4 for branch: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.12) Gecko/2008020121 Firefox/2.0.0.12.
Comment 53 Al Billings [:abillings] 2008-03-17 17:38:25 PDT
Verified again for 1.8.1.13 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.13) Gecko/2008031114 Firefox/2.0.0.13. 
Comment 54 Alexander Sack 2008-03-18 04:49:43 PDT
Comment on attachment 299017 [details] [diff] [review]
Fix length after changing string

a=asac for 1.8.0.15

(distro patch). remember to commit the other two patches here at the same time.
Comment 55 Alexander Sack 2008-03-18 04:50:21 PDT
Comment on attachment 300720 [details] [diff] [review]
catch both levels of unescaping

a=asac for 1.8.0.15

(distro patch). remember to commit the other two patches here at the same time.
Comment 56 Alexander Sack 2008-03-18 04:50:56 PDT
Comment on attachment 300976 [details] [diff] [review]
1.8 version of above minus the bug 415116 bit

a=asac for 1.8.0.15

(distro patch). remember to commit the other two patches here at the same time.
Comment 57 Reed Loden [:reed] (use needinfo?) 2008-03-22 00:56:21 PDT
MOZILLA_1_8_0_BRANCH:

Checking in chrome/src/nsChromeRegistry.cpp;
/cvsroot/mozilla/chrome/src/nsChromeRegistry.cpp,v  <--  nsChromeRegistry.cpp
new revision: 1.338.2.1.4.4; previous revision: 1.338.2.1.4.3
done
Checking in rdf/chrome/src/nsChromeRegistry.cpp;
/cvsroot/mozilla/rdf/chrome/src/nsChromeRegistry.cpp,v  <--  nsChromeRegistry.cpp
new revision: 1.323.10.3; previous revision: 1.323.10.2
done

Checking in chrome/src/nsChromeRegistry.cpp;
/cvsroot/mozilla/chrome/src/nsChromeRegistry.cpp,v  <--  nsChromeRegistry.cpp
new revision: 1.338.2.1.4.5; previous revision: 1.338.2.1.4.4
done

Checking in chrome/src/nsChromeRegistry.cpp;
/cvsroot/mozilla/chrome/src/nsChromeRegistry.cpp,v  <--  nsChromeRegistry.cpp
new revision: 1.338.2.1.4.6; previous revision: 1.338.2.1.4.5
done
Checking in rdf/chrome/src/nsChromeRegistry.cpp;
/cvsroot/mozilla/rdf/chrome/src/nsChromeRegistry.cpp,v  <--  nsChromeRegistry.cpp
new revision: 1.323.10.4; previous revision: 1.323.10.3
done
Comment 58 Daniel Veditz [:dveditz] 2009-06-16 09:01:52 PDT
*** Bug 182176 has been marked as a duplicate of this bug. ***

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