866825 - (CVE-2013-1675) nsDOMSVGZoomEvent::m{Previous,New}Scale are used uninitialized

Status: VERIFIED FIXED

(Core :: DOM: Events, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Test

Noticed while reviewing bug 866679. Attached test shows that we return garbage to JS.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 2

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2cf77e8ab713

Comment 3

[Daniel Holbert [:dholbert]]

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?

Comment 4

[Daniel Holbert [:dholbert]]

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.

Comment 5

[Daniel Holbert [:dholbert]]

[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 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2cf77e8ab713

Comment 8

[Andrea Marchesini [:baku]]

Thanks, ms2ger. Should I do something else?

Comment 9

[bhavana bajaj [:bajaj]]

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.

Comment 10

[bhavana bajaj [:bajaj]]

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.

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(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.)

Comment 12

[Daniel Holbert [:dholbert]]

(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 13

[Daniel Holbert [:dholbert]]

Comment on attachment 743181 [details] [diff] [review]
patch

[extending approval request to include b2g18 and esr17]

Comment 14

[Daniel Holbert [:dholbert]]

[clearing needinfo per comment 11 & comment 12, and setting in-testsuite to remind us to eventually land an automated test for this]

Comment 15

[bhavana bajaj [:bajaj]]

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.

Comment 16

[Daniel Holbert [:dholbert]]

Landed on Aurora and Beta:
 https://hg.mozilla.org/releases/mozilla-aurora/rev/17231a0a0cac
 https://hg.mozilla.org/releases/mozilla-beta/rev/e2100251b520

Comment 17

[Daniel Holbert [:dholbert]]

Landed on ESR17:
 https://hg.mozilla.org/releases/mozilla-esr17/rev/1aeefef4b5cb

Comment 18

[Daniel Holbert [:dholbert]]

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.

Comment 19

[Daniel Holbert [:dholbert]]

[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.]

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Needs tef+ blocking status to land on b2g18_v1_0_1. b2g18_v1_1_0 isn't in use yet. You're good.

Comment 21

[Matt Wobensmith [:mwobensmith][:matt:]]

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

Comment 22

[Daniel Veditz [:dveditz]]

(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?

Comment 23

[Daniel Holbert [:dholbert]]

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.)

Comment 24

[Daniel Veditz [:dveditz]]

Thanks. That's sec-high at worst then.