1074280 - (CVE-2014-1594) Bad casting: From BasicThebesLayer to BasicContainerLayer

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[lifeasageek]

Attachment: layout.txt

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/37.0.2062.120 Safari/537.36

Steps to reproduce:

Hi Firefox,

We have recently developed a runtime detection tool to identify undefined behaviors in static_cast (similar to -fsanitize=object-size/-fsanitize=vptr in Clang, but we improved to find undefined behaviors). Please see our confirmed other bug reports in libstdc++ (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63345). We have built/run Firefox using our instrumentation tool, and also found several bad castings in Firefox (let us report an issue one by one). 

-----------------------------------------------------------
// gfx/layers/basic/BasicLayerManager.cpp,

// aLayer might not be a container layer, but if so we take care not to use                                                                                                
// the container variable                                                                                                                                                  
BasicContainerLayer* container = static_cast<BasicContainerLayer*>(aLayer);
-----------------------------------------------------------
Here, aLayer is pointing to the object allocated as BasicThebesLayer, and it is casted into BasicContainerLayer. However, since BasicContainerLayer is not a subobject of BasicThebesLayer, it is violating C++ standard rules 5.2.9/11--down casting is undefined if the object that the pointer to be casted points to is not a suboject of down casting type-- and causes undefined behaviors.

Class layouts for BasicThebesLayer and BasicContainerLayer are attached, and pasted the error reports from our tool.



Actual results:


==  Bad-casting reports
/home/blee/project/firefox/firefox/gfx/layers/basic/BasicLayerManager.cpp:800:36: Casting from 'mozilla::layers::BasicThebesLayer' to 'mozilla::layers::BasicContainerLayer'
         Pointer         0x62a000093d00
         Alloc base      0x62a000093d00
         User base       0x62a000093d00
         Offset          0x000000000000
         TypeTable       0x7f3f47d14d40
    #0 0x7f3f43edb057 in PaintLayer /home/blee/project/firefox/firefox/gfx/layers/basic/BasicLayerManager.cpp:800
    #1 0x7f3f43edbe09 in PaintSelfOrChildren /home/blee/project/firefox/firefox/gfx/layers/basic/BasicLayerManager.cpp:744
    #2 0x7f3f43edb2f8 in PaintLayer /home/blee/project/firefox/firefox/gfx/layers/basic/BasicLayerManager.cpp:847
    #3 0x7f3f43edbe09 in PaintSelfOrChildren /home/blee/project/firefox/firefox/gfx/layers/basic/BasicLayerManager.cpp:744
    #4 0x7f3f43edb2f8 in PaintLayer /home/blee/project/firefox/firefox/gfx/layers/basic/BasicLayerManager.cpp:847
    #5 0x7f3f43eda13f in EndTransactionInternal /home/blee/project/firefox/firefox/gfx/layers/basic/BasicLayerManager.cpp:524
    #6 0x7f3f450cb55e in PaintFramesWithEffects /home/blee/project/firefox/firefox/layout/svg/nsSVGIntegrationUtils.cpp:544
    #7 0x7f3f44f4b073 in PaintInactiveLayer /home/blee/project/firefox/firefox/layout/base/FrameLayerBuilder.cpp:2592
    #8 0x7f3f44f4ba8d in DrawThebesLayer /home/blee/project/firefox/firefox/layout/base/FrameLayerBuilder.cpp:4497
    #9 0x7f3f43ef325a in PaintBuffer /home/blee/project/firefox/firefox/gfx/layers/basic/BasicThebesLayer.h:116
    #10 0x7f3f43ee3001 in Validate /home/blee/project/firefox/firefox/gfx/layers/basic/BasicThebesLayer.cpp:184
    #11 0x7f3f43ed90e2 in Validate /home/blee/project/firefox/firefox/gfx/layers/basic/BasicContainerLayer.cpp:128
    #12 0x7f3f43ed9feb in EndTransactionInternal /home/blee/project/firefox/firefox/gfx/layers/basic/BasicLayerManager.cpp:493

Comment 1

[Markus Stange [:mstange]]

Attachment: patch

Comment 2

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d927d3102472

Comment 3

[Al Billings [:abillings - ex-MoCo]]

Is this trunk only? Otherwise, this should have gotten a security rating and sec-approval+.

https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 4

[Markus Stange [:mstange]]

Argh, I forgot about this again. Sorry!

This is not trunk only. The cast was added in https://hg.mozilla.org/integration/mozilla-inbound/rev/0b6e26a32304 which shipped in Firefox 7.

Is the wrong cast itself dangerous? Or it only dangerous to access the casted pointer? In the latter case, the old code was harmless because the "container" variable was only accessed if aLayer->GetFirstChild() was non-null, and that's only possible for container layers, i.e. in the case that the cast was valid.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d927d3102472

Not sure if this affects B2G or not.

Comment 6

[Al Billings [:abillings - ex-MoCo]]

Markus, can we get an Aurora patch and a nomination for it, please?

Comment 7

[Markus Stange [:mstange]]

Sure, but I'm pretty sure the security risk from this bug is zero and the patch is only a cosmetic fix that silences the (false-positive) warning. So I don't think we need to uplift the patch.

(In reply to Markus Stange [:mstange] from comment #4)
> Is the wrong cast itself dangerous? Or [is] it only dangerous to access the
> casted pointer?

The answers I found on the internet were in agreement that the bad cast itself does not result in undefined behavior. The undefined behavior only starts once you de-reference the resulting pointer. Can somebody confirm this, please, and give this bug the appropriate security rating?

Comment 8

[Markus Stange [:mstange]]

Attachment: aurora/beta patch

Approval Request Comment
[Feature/regressing bug #]: bad cast, introduced by bug 647560
[User impact if declined]: no user impact as far as I can tell
[Describe test coverage new/current, TBPL]: none
[Risks and why]: low
[String/UUID change made/needed]: none

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Markus Stange [:mstange] from comment #7)
> The answers I found on the internet were in agreement that the bad cast
> itself does not result in undefined behavior. The undefined behavior only
> starts once you de-reference the resulting pointer. Can somebody confirm
> this, please, and give this bug the appropriate security rating?

I don't think that's quite right. From searching online for the spec section referenced in comment 0 (5.2.9/11), I found two different formulations of this C++ spec-text.  The first formulation seems to agree with you (the undefined behavior starts when the pointer is dereferenced/used); the second, newer formulation does not.

Formulation #1:
http://www.open-std.org/JTC1/SC22/WG21/docs/papers/2011/n3242.pdf (see bottom of page 104)
> Otherwise, the result of the cast is undefined.

Formulation #2:
http://www.open-std.org/JTC1/SC22/WG21/docs/papers/2013/n3797.pdf (see top of page 105)
> Otherwise, the behavior is undefined.

This newer text ("the behavior is undefined", RE the static_cast) is more clearly-permissive for letting compilers do crazy stuff, and suggests to me that we should consider this dangerous.

So, I think we should err on the side of considering this a security bug, and get the fix backported to affected branches.

Comment 10

[Daniel Holbert [:dholbert]]

(In reality, it's likely that the compilers we use *don't* really do crazy stuff in this scenario; but until/unless we've confirmed that, we should probably consider this sec-critical. --> Adding keyword.)

Comment 11

[Markus Stange [:mstange]]

Sounds good, thanks a lot for doing the research.

Comment 12

[Daniel Veditz [:dveditz]]

This is unfortunate, but at this point we're not looking to rebuild the world so let's not land on Beta just now and catch this in Firefox 34. Especially since we don't know that the compilers we actually use do anything crazy.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for esr31 as well.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0fe683c5cabc

Comment 15

[Markus Stange [:mstange]]

Comment on attachment 8502734 [details] [diff] [review]
aurora/beta patch

[Approval Request Comment]
User impact if declined: theoretical security risk depending on compiler behavior
Fix Landed on Version: 35 + uplift to 34
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/54ec9cb26b59
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/54ec9cb26b59
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/f188835b2495

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr31/rev/26ec6429b69b

Comment 18

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

Marking qe-verify- flag. Issues found via audit, custom tool etc., not enough time/resources for us to verify.