Last Comment Bug 577512 - (CVE-2010-3171) (more) cross-domain information leakage with Math.random()
: (more) cross-domain information leakage with Math.random()
[sg:low], fixed-in-tracemonkey [qa-ne...
: privacy
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
-- normal (vote)
: ---
Assigned To: Andreas Gal :gal
: Jason Orendorff [:jorendorff]
Depends on:
  Show dependency treegraph
Reported: 2010-07-08 10:13 PDT by Daniel Veditz [:dveditz]
Modified: 2015-10-16 11:47 PDT (History)
13 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

reporter's paper (pdf) (95.57 KB, application/pdf)
2010-07-08 10:13 PDT, Daniel Veditz [:dveditz]
no flags Details
patch (2.22 KB, patch)
2010-07-08 11:10 PDT, Andreas Gal :gal
bzbarsky: review-
Details | Diff | Splinter Review
patch (598 bytes, patch)
2010-07-08 12:25 PDT, Andreas Gal :gal
bzbarsky: review+
Details | Diff | Splinter Review

Description User image Daniel Veditz [:dveditz] 2010-07-08 10:13:57 PDT
Created attachment 456463 [details]
reporter's paper (pdf)

Amit Klein of Trusteer reports that our previous fixes for bug 464071 and bug 475585 were insufficient and sends the attached paper.

He would like to know "within one (1) week from today, whether you confirm and acknowledge the technical nature of the vulnerabilities described in the attachment. Of course, I'm available for questions/discussions/suggestions at any time. If you need an extension, please let me know in advance."

He may be disclosing this issue in the future at "a security conference, in which case I would ask you to defer your own disclosure to a date as close to the conference as possible."

While Mozilla attempted to address the issues of cross domain information leakage (through Math.random) in Firefox 3.6.4 (and above), Firefox 3.5.10 and Firefox 4.0 (alpha and beta), there is still a security vulnerability in the way the isolation is implemented, which enables cross domain leakage. In fact, it may make it easier to attack Firefox in some cases, compared to previous versions.

Additionally, a concerned is raised on the entropy provided in the seed to the Math.random PRNG, which may enable more powerful attacks.
Comment 1 User image Boris Zbarsky [:bz] (still a bit busy) 2010-07-08 10:20:07 PDT
Gah.  The patch in bug 475585 could really have used a DOM reviewer...

Shouldn't we reseed the rng at the point where we clear the outer window scope during page navigation?
Comment 2 User image Andreas Gal :gal 2010-07-08 10:57:00 PDT
Yes, we should re-seed. And I also have very serious doubts about the severity of this attack. Math.random is a terrible PRNG, its completely underspecified and doesn't give you any guarantees what kind of randomness or entropy you get out of it. It shouldn't be used for anything but casual random number generation in the first place.
Comment 3 User image Andreas Gal :gal 2010-07-08 10:58:41 PDT
bz, we currently have the rng state in cx, but really it should be in the scope actually (global object). I can clear the cx the clear scope request comes in on, but thats conceptually not guaranteed to be the right one (its in our embedding I am pretty sure though).
Comment 4 User image Boris Zbarsky [:bz] (still a bit busy) 2010-07-08 11:06:07 PDT
> but really it should be in the scope actually 

Conceptually, yes.

If there is api to reseed the rng, we could just call that in the DOM code...
Comment 5 User image Andreas Gal :gal 2010-07-08 11:10:39 PDT
Created attachment 456472 [details] [diff] [review]

Attached is a patch that re-seeds when the scope is cleared. Should we seed from /dev/random? I initialize libc's prng and seed with that, but stacking prngs is usually a bad idea.
Comment 6 User image Boris Zbarsky [:bz] (still a bit busy) 2010-07-08 11:44:32 PDT
Comment on attachment 456472 [details] [diff] [review]

Please make this compile on Windows.  As discussed, the seeding is a side-show; might not be worth worrying about.
Comment 7 User image Andreas Gal :gal 2010-07-08 12:25:28 PDT
Created attachment 456479 [details] [diff] [review]

minimal fix
Comment 8 User image Andreas Gal :gal 2010-07-08 16:01:41 PDT
Comment 9 User image Andreas Gal :gal 2010-07-08 16:05:02 PDT
I landed the patch on tracemonkey with a harmless commit message.
Comment 10 User image Daniel Veditz [:dveditz] 2010-07-16 10:16:57 PDT
Comment on attachment 456479 [details] [diff] [review]

Presumably this simple patch applies to the branches, and if so please request approval. Otherwise please attach branch versions.
Comment 12 User image Daniel Veditz [:dveditz] 2010-07-22 19:25:33 PDT
Is there a reason this can't go into the next set of 1.9.2.x and 1.9.1.x releases?
Comment 15 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2010-08-16 19:40:54 PDT
Comment on attachment 456479 [details] [diff] [review]

Requesting on this patch as well, since Gavin informs me this is needed for bug 475585 to be useful.
Comment 16 User image Al Billings [:abillings] 2010-08-17 15:56:20 PDT
What is the STR here for reproducing the bug? I want to verify the fix.
Comment 17 User image Daniel Veditz [:dveditz] 2010-09-01 12:24:13 PDT
Comment on attachment 456479 [details] [diff] [review]

Approved for, a=dveditz for release-drivers
Comment 18 User image Reed Loden [:reed] (use needinfo?) 2010-09-08 00:13:08 PDT
This bug was forgotten when writing the security advisories, most likely because it didn't have the proper flags set on the attachment. :(

Please be mindful of this in the future and ensure proper branch-landing procedure is always followed.

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