PAC privilege escalation using Array.prototype methods

RESOLVED FIXED

Status

()

Core
Security
RESOLVED FIXED
11 years ago
5 years ago

People

(Reporter: moz_bug_r_a4, Assigned: mrbkap)

Tracking

Trunk
x86
Windows XP
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 ?
blocking1.8.1.1 -
wanted1.8.1.x +
blocking1.8.0.9 -
blocking1.8.0.14 -
wanted1.8.0.x +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:moderate] need SJsOW)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

11 years ago
When a PAC script calls dnsResolve(obj) or alert(obj), obj.toString and
obj.valueOf can be called by chrome script.  I had thought that PAC scripts
could not abuse this with Array.prototype methods trick (bug 344495) since PAC
scripts could get neither a privileged eval nor a window object.  But, I
noticed that there is a way that PAC scripts get a privileged eval, as
described in bug 355214.  Thus, a PAC script can run arbitrary code with chrome
privileges by using Array.prototype methods.

The trunk, 2.0, 1.5.0.x, 1.0.x and moz1.7.x are affected.

Steps to Reproduce:
1. Setup a PAC file testcase on web server, or save it to local hard disk.
2. Set Automatic proxy configuration URL to its URL.
3. Open any web site.

Alert dialogs that show Components.stack will appear.
(Reporter)

Comment 1

11 years ago
Created attachment 241037 [details]
testcase
Assignee: dveditz → mrbkap
Depends on: 355214
Flags: blocking1.8.1.1?
Flags: blocking1.8.0.9?
Whiteboard: [sg:moderate]
Flags: blocking1.9?
Blake: any thoughts on these? Is there an alternative assignee that makes sense?
(Assignee)

Comment 3

11 years ago
This is the same as the other Array.prototype bugs, right? I think I might have a fix for those, but I need to test it more.
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.1?
Flags: blocking1.8.1.1-
Flags: blocking1.8.0.9?
Flags: blocking1.8.0.9-
Flags: blocking1.8.1.2+
Flags: blocking1.8.0.10+

Comment 4

11 years ago
Brian: Similar to bug 355214, any chance you can take a look?
(Assignee)

Comment 5

11 years ago
Perhaps we should push a pseudo-frame for getters and setters as well as watchpoints? I'm suddenly unsure as to why we do for one but not the other.
(Reporter)

Comment 6

11 years ago
There is another place that a PAC script can exploit by using Array.prototype
methods.

  this._findProxyForURL = this._sandBox.FindProxyForURL;

A PAC script can run arbitrary code when chrome code calls FindProxyForURL
getter function.
(Reporter)

Comment 7

11 years ago
Created attachment 252888 [details]
testcase 2
(Assignee)

Comment 8

11 years ago
(In reply to comment #6)
>   this._findProxyForURL = this._sandBox.FindProxyForURL;

This is pretty trivially fixable by using the safe object wrapper that jst wrote in bug 355766. Once that lands, I'll attach the trivial patch here to fix it.
(Assignee)

Updated

11 years ago
Depends on: 355766
Flags: blocking1.8.1.3+
Flags: blocking1.8.1.2+
Flags: blocking1.8.0.11+
Flags: blocking1.8.0.10+
(Reporter)

Comment 9

11 years ago
If the safe object wrapper is not yet available, I wonder if the remaining
issue could be fixed by using the same way as the fix in bug 355214?

  var safeGetProperty = null;
  function myGet(thisp, id) {
      return thisp[id];
  }

  safeGetProperty =
            Components.utils.evalInSandbox("(" + myGet.toSource() + ")",
                                           this._sandBox);

  this._findProxyForURL = safeGetProperty(this._sandBox, "FindProxyForURL");
(Assignee)

Comment 10

11 years ago
(In reply to comment #9)
> If the safe object wrapper is not yet available, I wonder if the remaining
> issue could be fixed by using the same way as the fix in bug 355214?

Yeah, that'll work. The reason I wanted to use the safe object wrapper is because this is *exactly* what it does under the hood. I'll cut a patch once I check the patch for bug 355214 in.
(Assignee)

Comment 11

11 years ago
Created attachment 253793 [details] [diff] [review]
Fix
Attachment #253793 - Flags: superreview?(dveditz)
Attachment #253793 - Flags: review?(crowder)

Comment 12

11 years ago
Comment on attachment 253793 [details] [diff] [review]
Fix

Basically a rubberstamp, but this looks good to me.
Attachment #253793 - Flags: review?(crowder) → review+
Comment on attachment 253793 [details] [diff] [review]
Fix

sr=dveditz
Attachment #253793 - Flags: superreview?(dveditz) → superreview+
(Assignee)

Comment 14

11 years ago
Created attachment 254173 [details] [diff] [review]
Patch that applies cleanly

The last patch applied over a messed up checkin. This patch applies over the current trunk. I'll land it there once the trunk reopens.
Attachment #253793 - Attachment is obsolete: true
(Assignee)

Comment 15

11 years ago
Fixed on trunk.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Moving out per Blake
Flags: blocking1.8.1.5+
Flags: blocking1.8.1.4+
Flags: blocking1.8.0.13+
Flags: blocking1.8.0.12+
Flags: blocking1.8.1.5+ → blocking1.8.1.6+
Flags: blocking1.8.0.13+ → blocking1.8.0.14+
Is this ready to land in 1.8.1.8?
Flags: blocking1.8.1.8+ → blocking1.8.1.9+
Whiteboard: [sg:moderate] → [sg:moderate] need SJsOW
Flags: blocking1.8.0.14+ → blocking1.8.0.14-
Flags: blocking1.8.1.12+
Group: core-security
You need to log in before you can comment on or make changes to this bug.