Dynamic this binding is biting PAC

VERIFIED FIXED in mozilla1.9alpha1

Status

()

Core
Networking
P1
normal
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: mrbkap, Assigned: mrbkap)

Tracking

({verified1.8.0.4, verified1.8.1})

Trunk
mozilla1.9alpha1
verified1.8.0.4, verified1.8.1
Points:
---
Bug Flags:
blocking1.7.14 ?
blocking-aviary1.0.9 ?
blocking1.8.0.4 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:moderate][patch], URL)

Attachments

(1 attachment)

(Assignee)

Description

11 years ago
I'm splitting this bug off of bug 321101 since there are two bugs reported there, and they're unrelated. This bug is about attachment 206496 [details], which I described as simple "highway robbery".

moz_bug_r_a4's clever testcase exploits the fact that when we call FindProxyForURL |this| is the nsProxyAutoConfig object itself and since there aren't any security checks once you're in JS-only land, the evil PAC script is able to trick the proxy auto config object into calling eval with its evil code (oh, how I hate seeing Components.stack in an alert ;-)).
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [patch]
(Assignee)

Comment 1

11 years ago
Created attachment 220560 [details] [diff] [review]
Proposed minimal fix

This is the smallest fix I can come up with for this bug. It uses Function.prototype.call to force the |this| object to be the sandbox object, so doing evil things to the |this| object doesn't end up in privilege escalation.
Attachment #220560 - Flags: review?(darin)
(Assignee)

Comment 2

11 years ago
By the way, I also considered a patch to call this._sandBox.FindProxyForURL directly instead of storing the function in a member property; however, that would have changed behavior (if the PAC script did something weird), so I decided against it.
(Assignee)

Updated

11 years ago
Flags: blocking1.8.0.4?

Updated

11 years ago
Attachment #220560 - Flags: review?(darin) → review+
(Assignee)

Updated

11 years ago
Attachment #220560 - Flags: superreview?(dveditz)
Comment on attachment 220560 [details] [diff] [review]
Proposed minimal fix

sr=dveditz

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #220560 - Flags: superreview?(dveditz)
Attachment #220560 - Flags: superreview+
Attachment #220560 - Flags: approval1.8.0.4+
Attachment #220560 - Flags: approval-branch-1.8.1+
Flags: blocking1.8.0.4? → blocking1.8.0.4+
(Assignee)

Comment 4

11 years ago
Fix checked in everywhere.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.0.4, fixed1.8.1
Resolution: --- → FIXED
Blocks: 321101
Whiteboard: [patch] → [sg:moderate][patch]

Updated

11 years ago
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.0.4, fixed1.8.1 → verified1.8.0.4, verified1.8.1
Flags: blocking1.7.14?
Flags: blocking-aviary1.0.9?
Group: security
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.