Created attachment 743178 [details] Test Noticed while reviewing bug 866679. Attached test shows that we return garbage to JS.
Created attachment 743181 [details] [diff] [review] patch
4 years ago
Does this affects branches? (I'd bet that it does, since this class & these member-vars have been around for a long long time?) If so, technically this should've received sec-approval before landing... But I guess there's no sense requesting that flag now, given that the cat's out of the bag. We need to see if this affects aurora/beta, and backport soonish if so. (I suspect it does, as I get random values from loading the testcase in beta) baku, can you take care of that?
Not sure if webdeveloper-exposed uninitialized-memory-read bugs are sec-high vs. sec-critical; assuming they might be critical for now, and CC'ing dveditz for possible followup assessment.
[for future reference, please don't land security bugs' patches (or mark them as "checkin-needed") unless you're *sure* they don't affect branches. If they do affect branches (or if you aren't sure), then you should first request sec-approval on the patch, per the policy laid out at https://wiki.mozilla.org/Security/Bug_Approval_Process ]
Comment on attachment 743181 [details] [diff] [review] patch [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 302103 (landed 2005-08-25) User impact if declined: unprivileged JS can read uninitialized memory Testing completed (on m-c, etc.): on inbound Risk to taking this patch (and alternatives if risky): low risk String or IDL/UUID changes made by this patch: none
Thanks, ms2ger. Should I do something else?
Although, this landed without sec-approval can you please help answer questions which generally come-up when nomination a patch for sec-approval : [Security approval request comment] How easily can the security issue be deduced from the patch? Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Which older supported branches are affected by this flaw? If not all supported branches, which bug introduced the flaw? Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? How likely is this patch to cause regressions; how much testing does it need? This will help us understand how easy discoverable the issue is .We are concerned about uplifting it all the way to beta, since this is our second last beta and any regression/fallouts from this(SVG) may not be very obvious before release, keeping in mind we are touching old code here.
Adding needsinfo on Ms2ger,Olli to see if they can help with comment# 9, keeping in mind we need to decide if this needs to get in our beta build which is going to build in a couple of hours. Please redirect me if you think someone else who I may have missed can better address this comment here.
(In reply to bhavana bajaj [:bajaj] from comment #9) > Although, this landed without sec-approval can you please help answer > questions which generally come-up when nomination a patch for sec-approval : > > [Security approval request comment] > How easily can the security issue be deduced from the patch? Probably not too hard. > Do comments in the patch, the check-in comment, or tests included in the > patch paint a bulls-eye on the security problem? I guess the checkin comment kinda does. > Which older supported branches are affected by this flaw? All. > If not all supported branches, which bug introduced the flaw? N/A > Do you have backports for the affected branches? If not, how different, > hard to create, and risky will they be? Should be pretty similar / easy / low-risk. > How likely is this patch to cause regressions; how much testing does it > need? Unlikely to cause regressions; running the testcase from comment 0 should be sufficient. (Going offline for the night now.)
(In reply to :Ms2ger from comment #11) > > How likely is this patch to cause regressions; how much testing does it > > need? > > Unlikely to cause regressions; running the testcase from comment 0 should be > sufficient. Agreed, extremely unlikely to cause regressions. The patch just initializes two previously-uninitialized values, producing deterministic behavior where previously we'd do something random. I'd strongly support landing this on branches wherever possible, including ESR. This code is strictly safer & saner with the patch applied.
Comment on attachment 743181 [details] [diff] [review] patch [extending approval request to include b2g18 and esr17]
[clearing needinfo per comment 11 & comment 12, and setting in-testsuite to remind us to eventually land an automated test for this]
Comment on attachment 743181 [details] [diff] [review] patch Keeping in mind the patch is super-safe based on the recent bug comments & a quick talk with :dholbert,I am going to approve this on branches as the check-in comment and the patch itself will leave us exposed which is risky as well.
Landed on Aurora and Beta: https://hg.mozilla.org/releases/mozilla-aurora/rev/17231a0a0cac https://hg.mozilla.org/releases/mozilla-beta/rev/e2100251b520
Landed on ESR17: https://hg.mozilla.org/releases/mozilla-esr17/rev/1aeefef4b5cb
Landed on b2g18: https://hg.mozilla.org/releases/mozilla-b2g18/rev/c13f5cbbfb23 Do we need to land on the other b2g repos as well (1.0.1 / 1.1.0), and is approval-mozilla-b2g18+ all that's needed to land there? I'm fuzzy on the nature of those repos at the moment.
[I'm assuming ryanvm knows the answer to comment 18. Ryan, please let me know where else I should land & if there are any additional flags I should request; or alternately, feel free to go ahead and push it if you know what to do.]
Needs tef+ blocking status to land on b2g18_v1_0_1. b2g18_v1_1_0 isn't in use yet. You're good.
Reproduced on FF17.0.5esr Confirmed fixed on FF17.0.6esr candidate build 1 Confirmed fixed on FF21 candidate build 2 Confirmed fixed on FF22 2013-05-09 Confirmed fixed on FF23 2013-05-09
(In reply to Daniel Holbert [:dholbert] from comment #4) > Not sure if webdeveloper-exposed uninitialized-memory-read bugs are > sec-high vs. sec-critical; What bad things happen if some float scaling values are nonsense? These aren't pointers and not a size or index used to bound a chunk of memory. Could it do worse than cause the page to draw funny?
It lets the web developer read out a chunk of uninitialized memory, which (depending on what object overlayed this memory just before now) could give away something like a password or a cookie or something like that. (I'm not sure how easy or reliable that sort of attack would be, but IIUC that's the general security concern with uninitialized-memory-reads -- it's not a buffer-overflow sort of thing, it's an information-leak sort of thing.)
Thanks. That's sec-high at worst then.