413250 - chrome directory traversal (local disk access via "flat" addons)

Status: RESOLVED FIXED

(Core :: General, defect, P1)

Description

[Daniel Veditz [:dveditz]]

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]]

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]

See also bug 394075, a similar-sounding bug for resource: URLs.

Comment 3

[Benjamin Smedberg]

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]

Bug 413451 explains why this is [sg:high].

Comment 5

[Devon Jensen]

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]

(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]

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]

is there a reason not to kill all access from content to chrome:// and resource://?

Comment 9

[Benjamin Smedberg]

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 [:ma1]]

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 [:ma1]]

(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]

My understanding was that standardurl unescaped everything at parse-time... biesi is that understanding incorrect?

Comment 13

[Giorgio Maone [:ma1]]

(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 [:ma1]]

(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]

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 [:ma1]]

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]]

Attachment: 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]]

Attachment: 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

[[e-mail]]

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]

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]]

Attachment: Fix length after changing string

Carrying over Neil's r+
Added call to SetLength() after modifying the string.

Comment 22

[Daniel Veditz [:dveditz]]

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]]

(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]]

checked into 1.8 branch, waiting for trunk tree to open.

Comment 25

[Daniel Veditz [:dveditz]]

checked in on trunk

Comment 26

[georgi - hopefully not receiving bugspam]

(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 - ex-MoCo]]

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]]

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 - ex-MoCo]]

?? 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][⌚️UTC+2]]

(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]

Is this a separate issue?

http://www.hiredhacker.com/2008/01/31/more-chrome-directory-traversing/

Comment 32

[Al Billings [:abillings - ex-MoCo]]

Devon, that has been dealt with elsewhere. I don't have the bug number handy.

Comment 33

[Al Billings [:abillings - ex-MoCo]]

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 [:ma1]]

@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]]

Attachment: 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)]

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]]

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]]

Comment on attachment 300720 [details] [diff] [review]
catch both levels of unescaping

a=beltzner

Comment 39

[Benjamin Smedberg]

Mossop, can you write a testcase for this?

Comment 40

[Robert Kaiser]

Do we need to patch rdf/chrome for this second part as well?

Comment 41

[Daniel Veditz [:dveditz]]

Attachment: mozilla/rdf/chrome version

Comment 42

[Daniel Veditz [:dveditz]]

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]]

Attachment: flexible online testcase (derived from second hiredhacker example)

I didn't feel like installing sessionstore here.  :)

Comment 44

[[e-mail]]

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]

 ( *(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]

(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]]

> [...] will stop reading if the string is null terminated.

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

Comment 48

[Daniel Veditz [:dveditz]]

Attachment: 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]]

Attachment: 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)]

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

[(no longer active)]

Attachment: 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 - ex-MoCo]]

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 - ex-MoCo]]

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]

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]

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]

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]]

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