Closed Bug 267645 Opened 20 years ago Closed 4 years ago

Page can obtain path to Mozilla installation or possibly profile by examining JavaScript exceptions

Categories

(Core :: Security, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox75 --- fixed

People

(Reporter: bugzilla, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

(Keywords: privacy, sec-want, Whiteboard: [sg:want] stepping-stone [fingerprinting][fp-triaged][adv-main75-])

Attachments

(3 files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; cs-CZ; rv:1.7.3) Gecko/20040910
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; cs-CZ; rv:1.7.3) Gecko/20040910

I am not sure if this should be considered as security bug. If you don't think
so, open it.

Page which call window.sidebar.addSearchEngine() with bad agruments (bad suffix,
protocol) can catch the exception of the Mozilla code and from it obtain path to
the Mozilla installlation.

Problem is in the nsSidebar.js (in Mozilla and Firefox too) containing line:
this.promptService.alert(null, "Failed to add the search engine.");

there is missing the 3th alert argument (text of the dialog), which raises
expception e.g.:
[Exception... "Not enough arguments [nsIPromptService.alert]"  nsresult:
"0x80570001 (NS_ERROR_XPC_NOT_ENOUGH_ARGS)"  location: "JS frame ::
file:///C:/Program%20Files/mozilla.org/Mozilla/components/nsSidebar.js ::
anonymous :: line 266"  data: no]

HTML page can catch this exception and obtain the path of the problematic file
(i.e. the path of Mozilla installation).

Revealing the path to the nsSidebar.js is not probably critical problem, but
what if the exception raises in some extension in the user profile and the page
can catch path to the user profile (and the user login)?

So problem is propably more general. Should be the HTML page able to catch
exceptions from the Mozilla code? And if should, should it obtain a full text of
the expections (including file path)?

Reproducible: Always
Steps to Reproduce:
Attached file Testcase
Note that there are other ways that sidebar can throw exceptions, e.g.
sidebar.addPanel(null, null, null);
Perhaps that should be a chrome URI, not a file one?
nsSidebar.js doesn't live in chrome, it's a component.
I'm not too worried about revealing the installation directory (it's generally
in an easily guessed default location), but Met is absolutely right that this
could potentially reveal the profile location. Would require:

- user installs Firefox extension (profile-based)
- extension uses a javascript component (chrome js exceptions would just reveal
  that the extension exists)
- extension exposes their component to web content somehow -- probably rare

Then the attacker would also have to know of a local-file-stealing exploit for
the profile location to be useful. In limited situations just knowing the
OS-login name might be useful to an attacker and that would usually be revealed
(unless the user chose a non-default profile location, and that's not generally
possible in Firefox without secret command-line args).

One "fix" for this bug would be to prevent the sidebar component from throwing
exceptions into user scripts. That seems like the friendly thing to do in any
case for sidebar, but that doesn't fix the potential for this to occur in some
popular but badly coded extension. Rather we need to fix this at the JS
exception level somehow.

Blue skying: if the exception is going to put a file: url in there just list the
filename, but strip the rest of the path. That's a little severe and hurts
people trying to develop apps on their local disk. Could perhaps check whether
the top level script is priveged or not, or what scheme it came from. Or just
strip paths by default and let developers flip a pref to get full paths like the
plugin.expose_full_path pref.

Nominating for 1.0 just to get consideration, but without knowing of a specific
profile-installed extension that can be induced to throw an exception into web
content I don't think we need to take this kind of change so late. Good 1.1/1.8
fodder.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-aviary1.0?
Whiteboard: [sg:fix]
gonna work on this for the next release
Flags: blocking-aviary1.0? → blocking-aviary1.0-
Summary: nsSidebar.js - page can obtain path to Mozilla installation → Page can obtain path to Mozilla installation or possibly profile by examining JavaScript exceptions
Group: security
Keywords: privacy
Whiteboard: [sg:fix] → [sg:want] stepping-stone
Flags: testcase+
Also true for FF 1.5.0.1RC1 on Mac OS X
OS: Windows 2000 → All
Hardware: PC → All
Flags: blocking1.9a1?
Flags: blocking1.8.1?
I marked this as blocked by bug 268370 because it fixes the specific case mentioned here, but re-reading this bug, I see that's it's looking for a general solution to the problem, so I'll revert that change.
No longer depends on: 268370
Information about the testcase was posted to Bugtraq list on May 22th listing it as PoC:
http://www.securityfocus.com/archive/1/434696/30/30/threaded

Original time stamp at posting mwntioned is May 21 2006 because of moderating process of Symantec.
also now posted as secunia 
http://secunia.com/advisories/20256/ (suite) and http://secunia.com/advisories/20244/ firefox
I've posted a 1.8.0-compatible branch patch for the one known case of this problem in bug 268370. I've posted a better patch for 1.8 and trunk in bug 338989.
Also true for Firefox 2.0a2 (Bon Echo) on Fedora Core 4, kernel 2.6.14-1.1653_FC4, j2sdk-1.5.0_05-1_FC4.
Could we have our exception objects implement nsISecurityCheckedComponent and do a CanAccess check from the caller principal to the filename URI?

For DOMException this could be tough because it has classinfo claiming it's a DOM object and therefore would short-circuit the nsISecurityCheckedComponent check, but maybe we can change said classinfo?
Flags: blocking1.8.0.5?
Flags: blocking1.9a1?
Flags: blocking1.9a1+
Flags: blocking1.8.1?
Flags: blocking1.8.1+
Flags: blocking1.8.0.5?
Flags: blocking1.8.0.5+
(In reply to comment #11)
> I've posted a 1.8.0-compatible branch patch for the one known case of this
> problem in bug 268370.

That's a perfectly fine fix that we want, but we should not claim it fixes this bug (I don't think it even fixes neil's comment 2 testcase).
(In reply to comment #14)
> (In reply to comment #11)
> > I've posted a 1.8.0-compatible branch patch for the one known case of this
> > problem in bug 268370.
> 
> That's a perfectly fine fix that we want, but we should not claim it fixes
> this bug (I don't think it even fixes neil's comment 2 testcase).

You're right, I should have said "for one known case of this" instead of "for the one known case of this". It does not fix the case from comment 2. I'll post a patch that does, because it's a trivial fix, but getting a better general solution to this bug would obviously be better than trying to play whack-a-mole with all the places this can happen.
Depends on: 268370
With the only known instance fixed (bug 268370) we can push the more general solution out to a more appropriate place like trunk or 1.8 branch.
Flags: blocking1.8.0.5+ → blocking1.8.0.5-
Target Milestone: --- → mozilla1.8.1beta2
Not going to block on this for release of Firefox 2, but we'd take a baked patch if one comes to us (nominate for approval1.8.1)
Flags: blocking1.8.1+ → blocking1.8.1-
Flags: blocking1.9-
Whiteboard: [sg:want] stepping-stone → [sg:want] stepping-stone [wanted-1.9]
Blocks: 370018
Flags: in-testsuite+ → in-testsuite?
Flags: wanted1.9+
Whiteboard: [sg:want] stepping-stone [wanted-1.9] → [sg:want] stepping-stone
The following test case is not able to catch anything on trunk.

try {
  window.sidebar.addPanel(null,null,null);
}
catch(e) {
  alert(e);
}

And the following is logged to the error console:

Error: Invalid argument passed to window.sidebar.addPanel: Unsupported panel URL.
Source File: file:///C:/Program%20Files/Mozilla%20Firefox%203%20Beta%203/components/nsSidebar.js
Line: 47

Is this bug still valid on trunk?
(In reply to comment #18)
> Is this bug still valid on trunk?

Yes, see comment 16. There may be other unknown errors that will expose similar problems, so we need a general solution, rather than relying on being able to identify all the potential problem areas.
Flags: blocking1.9a1+
Assignee: dveditz → nobody
Depends on: 503228
QA Contact: toolkit
Flags: in-testsuite?
Whiteboard: [sg:want] stepping-stone → [sg:want] stepping-stone [fingerprinting]
Marking as P2 to investigate for fingerprinting
Priority: -- → P2
I wanted to give a quick update on the current state of this, which has changed a lot since comment 19.

Once bug 888600 lands (matter of days, I hope), all web-exposed things will be either WebIDL or some sort of direct injection via exportFunction.

When calling through WebIDL, if a JS exception is thrown as part of the _implementation_, one of the following things happens:

* If this is JS-implemented WebIDL, then CallbackObject::CallSetup::ShouldRethrowException does a security check and will not expose exceptions thrown by chrome to webpages.  They will be reported to the console and a sanitized exception thrown to the webpage.  We have tests for this behavior.

* If this is C++-implemented WebIDL that calls a JS XPCOM component that throws, we will end up in dom::ThrowExceptionValueIfSafe and again do a security check to make sure it's OK to expose the exception.  See bug 1295322 for where we changed that.

So I believe we have now put in place the general solution this bug was about, for WebIDL.

For the direct-injection case... I'm not sure what the situation is.
Can we close this bug according to comment 21?
Flags: needinfo?(tom)
Flags: needinfo?(tihuang)
That's hard to say since the situation of the direct-injection case is still unclear. I think we should also make sure that the installation path won't be leaked through direct-injection codes before we close this.
Flags: needinfo?(tihuang)
Flags: needinfo?(tom)
Priority: P2 → P3
Whiteboard: [sg:want] stepping-stone [fingerprinting] → [sg:want] stepping-stone [fingerprinting][fp-triaged]

We talked to :baku about this and he thinks we need to sanitize the direct injection case. We can look for an Exception in the JSContext, take it out, sanitize it if it's there, and then put it back. We can reuse the same type of sanitization we already have. But first we need to write a test to show it failing.

Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED

OK, I tried editing the testcase to from comment 25 make sense: to make the sandbox less privileged than the exported function, and to examine the state the sandbox sees, not the state the privileged caller sees.

As things stand, the sandbox ends up with a direct reference (via CCWs, of course) to the privileged exception, and attempting to get properties off it results in security errors.

Is that good enough, or do we still want to do some sort of sanitizing behavior to prevent those security errors?

Attachment #9074137 - Attachment description: add test case; → Bug 267645. Sanitize exceptions from exported functions.

Anyway, I posted a proposed fix that does the sanitizing at https://phabricator.services.mozilla.com/D35996

Attachment #9074137 - Attachment description: Bug 267645. Sanitize exceptions from exported functions. → Bug 267645 part 2. Sanitize exceptions from exported functions.

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #26)

Is that good enough, or do we still want to do some sort of sanitizing behavior to prevent those security errors?

It looks good to me.
Tom, can you take a look?

Flags: needinfo?(tom)

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #26)

OK, I tried editing the testcase to from comment 25 make sense: to make the sandbox less privileged than the exported function, and to examine the state the sandbox sees, not the state the privileged caller sees.

As things stand, the sandbox ends up with a direct reference (via CCWs, of course) to the privileged exception, and attempting to get properties off it results in security errors.

Is that good enough, or do we still want to do some sort of sanitizing behavior to prevent those security errors?

Truthfully I don't have a solid grasp of everything here, but it seems like it would be okay to me. Could one probe to see if a property exists (returns a security error) vs doesn't and infer something about the user's machine from it? That seems unlikely - they would probably just be able to infer something about the exception thrown which isn't concerning, because they caused it to be thrown.

Flags: needinfo?(tom)
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/942a92874526
part 1.  Add a way to ask for DOMException to be exposed in sandboxes. r=bholley
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/039e8fd2928d
part 2.  Sanitize exceptions from exported functions. r=xeonchen,bholley
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.8.1beta2 → mozilla69
Regressions: 1617527
Whiteboard: [sg:want] stepping-stone [fingerprinting][fp-triaged] → [sg:want] stepping-stone [fingerprinting][fp-triaged][adv-main75-]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: