Last Comment Bug 336313 - Dynamic this binding is biting PAC
: Dynamic this binding is biting PAC
: verified1.8.0.4, verified1.8.1
Product: Core
Classification: Components
Component: Networking (show other bugs)
: Trunk
: All All
P1 normal (vote)
: mozilla1.9alpha1
Assigned To: Blake Kaplan (:mrbkap)
: Patrick McManus [:mcmanus]
Depends on:
Blocks: 321101
  Show dependency treegraph
Reported: 2006-05-02 14:53 PDT by Blake Kaplan (:mrbkap)
Modified: 2007-08-06 14:50 PDT (History)
5 users (show)
dveditz: blocking1.7.14?
dveditz: blocking‑aviary1.0.9?
dveditz: blocking1.8.0.4+
jwalden+bmo: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Proposed minimal fix (1.23 KB, patch)
2006-05-02 14:56 PDT, Blake Kaplan (:mrbkap)
darin.moz: review+
dveditz: superreview+
dveditz: approval‑branch‑1.8.1+
dveditz: approval1.8.0.4+
Details | Diff | Splinter Review

Description User image Blake Kaplan (:mrbkap) 2006-05-02 14:53:54 PDT
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 ;-)).
Comment 1 User image Blake Kaplan (:mrbkap) 2006-05-02 14:56:38 PDT
Created attachment 220560 [details] [diff] [review]
Proposed minimal fix

This is the smallest fix I can come up with for this bug. It uses 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.
Comment 2 User image Blake Kaplan (:mrbkap) 2006-05-02 14:57:39 PDT
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.
Comment 3 User image Daniel Veditz [:dveditz] 2006-05-02 16:40:13 PDT
Comment on attachment 220560 [details] [diff] [review]
Proposed minimal fix


approved for 1.8.0 branch, a=dveditz for drivers
Comment 4 User image Blake Kaplan (:mrbkap) 2006-05-02 17:22:44 PDT
Fix checked in everywhere.

Note You need to log in before you can comment on or make changes to this bug.