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

RESOLVED FIXED

Status

()

Core
General
P1
normal
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: dveditz, Assigned: dveditz)

Tracking

({fixed1.8.0.15, verified1.8.1.12, verified1.8.1.13})

Trunk
fixed1.8.0.15, verified1.8.1.12, verified1.8.1.13
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
blocking1.8.1.12 +
wanted1.8.1.x +
blocking1.8.0.next +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:high], URL)

Attachments

(4 attachments, 5 obsolete attachments)

2.45 KB, patch
bsmedberg
: review+
bsmedberg
: superreview+
Samuel Sidler (old account; do not CC)
: approval1.8.1.12+
beltzner
: approval1.9b3+
Alexander Sack
: approval1.8.0.next+
Details | Diff | Splinter Review
1.13 KB, text/html
Details
3.90 KB, patch
Samuel Sidler (old account; do not CC)
: approval1.8.1.12+
Samuel Sidler (old account; do not CC)
: approval1.8.1.13+
Alexander Sack
: approval1.8.0.next+
Details | Diff | Splinter Review
5.23 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

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

Updated

10 years ago
Flags: wanted1.8.1.x+
Flags: blocking1.9?
Flags: blocking1.8.1.12?
(Assignee)

Comment 1

10 years ago
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).
Whiteboard: [sg:low]

Comment 2

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

Updated

10 years ago
Assignee: nobody → dveditz
(Assignee)

Updated

10 years ago
Flags: blocking1.8.1.12? → blocking1.8.1.12+

Updated

10 years ago
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
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.

Updated

10 years ago
Priority: P2 → P1

Comment 4

10 years ago
Bug 413451 explains why this is [sg:high].
Blocks: 413451
Whiteboard: [sg:low] → [sg:high]

Comment 5

10 years ago
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 - 
(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

10 years ago
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.
is there a reason not to kill all access from content to chrome:// and resource://?
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

10 years ago
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

10 years ago
(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...


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

Comment 13

10 years ago
(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

10 years ago
(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

10 years ago
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

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

Updated

10 years ago
No longer blocks: 413451
(Assignee)

Comment 17

10 years ago
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.
Attachment #298911 - Flags: superreview?
Attachment #298911 - Flags: review?
(Assignee)

Comment 18

10 years ago
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".
Attachment #298916 - Flags: superreview?
Attachment #298916 - Flags: review?
(Assignee)

Updated

10 years ago
Attachment #298911 - Flags: superreview?(benjamin)
Attachment #298911 - Flags: superreview?
Attachment #298911 - Flags: review?(benjamin)
Attachment #298911 - Flags: review?
(Assignee)

Updated

10 years ago
Attachment #298916 - Flags: superreview?(benjamin)
Attachment #298916 - Flags: superreview?
Attachment #298916 - Flags: review?(neil)
Attachment #298916 - Flags: review?

Comment 19

10 years ago
Comment on attachment 298916 [details] [diff] [review]
rdf/chrome version

I think Camino currently still uses this code on trunk.
Attachment #298916 - Flags: review?(neil) → review+
does the patch protect against <script src="resource://"></script>

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

error console reveals install location
(Assignee)

Comment 21

10 years ago
Created attachment 299017 [details] [diff] [review]
Fix length after changing string

Carrying over Neil's r+
Added call to SetLength() after modifying the string.
Attachment #298911 - Attachment is obsolete: true
Attachment #298916 - Attachment is obsolete: true
Attachment #299017 - Flags: superreview?(benjamin)
Attachment #299017 - Flags: review+
Attachment #298911 - Flags: superreview?(benjamin)
Attachment #298911 - Flags: review?(benjamin)
Attachment #298916 - Flags: superreview?(benjamin)
Attachment #299017 - Flags: superreview?(benjamin) → superreview+
(Assignee)

Comment 22

10 years ago
Comment on attachment 299017 [details] [diff] [review]
Fix length after changing string

approved for 1.8.1.12, a=dveditz for release-drivers
Attachment #299017 - Flags: approval1.8.1.12?
(Assignee)

Updated

10 years ago
Attachment #299017 - Flags: approval1.8.1.12? → approval1.8.1.12+
(Assignee)

Comment 23

10 years ago
(In reply to comment #20)
> does the patch protect against [...]

No, that's completely unrelated. Is there an existing bug on that?
(Assignee)

Comment 24

10 years ago
checked into 1.8 branch, waiting for trunk tree to open.
Keywords: fixed1.8.1.12
(Assignee)

Comment 25

10 years ago
checked in on trunk
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(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
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.
Keywords: fixed1.8.1.12 → verified1.8.1.12
Blocks: 413451
(Assignee)

Comment 28

10 years ago
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.
?? 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".
(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.
Version: unspecified → Trunk

Comment 31

10 years ago
Is this a separate issue?

http://www.hiredhacker.com/2008/01/31/more-chrome-directory-traversing/
Devon, that has been dealt with elsewhere. I don't have the bug number handy.
Ack, no, I misspoke. You are right, this is new. I'll contact people about this right now to get it investigated.

Comment 34

10 years ago
@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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Keywords: verified1.8.1.12
(Assignee)

Comment 35

10 years ago
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.
Attachment #299017 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Attachment #300720 - Flags: superreview?(benjamin)
Attachment #300720 - Flags: review?(benjamin)
Attachment #300720 - Flags: approval1.8.1.12?
Attachment #300720 - Flags: superreview?(benjamin)
Attachment #300720 - Flags: superreview+
Attachment #300720 - Flags: review?(benjamin)
Attachment #300720 - Flags: review+
Comment on attachment 300720 [details] [diff] [review]
catch both levels of unescaping

Approved for 1.8.1.12; a=ss.
Attachment #300720 - Flags: approval1.8.1.12? → approval1.8.1.12+
(Assignee)

Comment 37

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

Given the high profile of this PoC we should fix FF3b3 also.
Attachment #300720 - Flags: approval1.9b3?
Comment on attachment 300720 [details] [diff] [review]
catch both levels of unescaping

a=beltzner
Attachment #300720 - Flags: approval1.9b3? → approval1.9b3+
Mossop, can you write a testcase for this?
Flags: in-testsuite?
(Assignee)

Updated

10 years ago
Keywords: fixed1.8.1.12, fixed1.8.1.13
(Assignee)

Updated

10 years ago
Blocks: 415116

Comment 40

10 years ago
Do we need to patch rdf/chrome for this second part as well?
(Assignee)

Comment 41

10 years ago
Created attachment 300771 [details] [diff] [review]
mozilla/rdf/chrome version
Attachment #300771 - Flags: superreview?(neil)
Attachment #300771 - Flags: review?(neil)
(Assignee)

Comment 42

10 years ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
(Assignee)

Updated

10 years ago
Blocks: 415191
Created attachment 300797 [details]
flexible online testcase (derived from second hiredhacker example)

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

Updated

10 years ago
Attachment #300797 - Attachment description: flexible online testcase (derived from second hiddenhacks example) → flexible online testcase (derived from second hiredhacker example)

Comment 44

10 years ago
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).
Attachment #300771 - Flags: superreview?(neil)
Attachment #300771 - Flags: superreview+
Attachment #300771 - Flags: review?(neil)
Attachment #300771 - Flags: review+
 ( *(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
(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.
(Assignee)

Comment 47

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

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

(Assignee)

Comment 48

10 years ago
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.
Attachment #300771 - Attachment is obsolete: true
Attachment #300944 - Flags: superreview?(neil)
Attachment #300944 - Flags: review?(neil)

Updated

10 years ago
Attachment #300944 - Flags: superreview?(neil)
Attachment #300944 - Flags: superreview+
Attachment #300944 - Flags: review?(neil)
Attachment #300944 - Flags: review+

Updated

10 years ago
Blocks: 415292
Blocks: 414327
(Assignee)

Comment 49

10 years ago
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.
Attachment #300944 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Attachment #300976 - Flags: approval1.8.1.12?
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.
Attachment #300976 - Flags: approval1.8.1.13+
Attachment #300976 - Flags: approval1.8.1.12?
Attachment #300976 - Flags: approval1.8.1.12+
Depends on: 415367

Updated

10 years ago
Blocks: 415367
No longer depends on: 415367
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.
(Assignee)

Updated

10 years ago
No longer blocks: 415367
Depends on: 415367
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.
Keywords: fixed1.8.1.12 → verified1.8.1.12
(Assignee)

Updated

10 years ago
Depends on: 417086
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. 
Keywords: fixed1.8.1.13 → verified1.8.1.13

Updated

9 years ago
Flags: blocking1.8.0.15+

Comment 54

9 years ago
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.
Attachment #299017 - Flags: approval1.8.0.15+

Comment 55

9 years ago
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.
Attachment #300720 - Flags: approval1.8.0.15+

Comment 56

9 years ago
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.
Attachment #300976 - Flags: approval1.8.0.15+
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
Keywords: fixed1.8.0.15
(Assignee)

Updated

8 years ago
Duplicate of this bug: 182176
You need to log in before you can comment on or make changes to this bug.