Bug 1074280 (CVE-2014-1594)

Bad casting: From BasicThebesLayer to BasicContainerLayer

RESOLVED FIXED in Firefox 34, Firefox OS v1.4

Status

()

Core
Graphics: Layers
RESOLVED FIXED
3 years ago
9 months ago

People

(Reporter: lifeasageek, Assigned: mstange)

Tracking

({sec-high})

Trunk
mozilla35
x86_64
Linux
sec-high
Points:
---
Bug Flags:
sec-bounty +
qe-verify -

Firefox Tracking Flags

(firefox32 wontfix, firefox33- wontfix, firefox34+ fixed, firefox35- fixed, firefox-esr3134+ fixed, b2g-v1.4 fixed, b2g-v2.0 fixed, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

(Whiteboard: [adv-main34+][adv-esr31.3+])

Attachments

(3 attachments)

(Reporter)

Description

3 years ago
Created attachment 8496944 [details]
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

Updated

3 years ago
Component: Untriaged → Graphics: Layers
Product: Firefox → Core
(Assignee)

Comment 1

3 years ago
Created attachment 8497671 [details] [diff] [review]
patch
Assignee: nobody → mstange
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8497671 - Flags: review?(roc)
Attachment #8497671 - Flags: review?(roc) → review+
(Assignee)

Comment 2

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d927d3102472
Is this trunk only? Otherwise, this should have gotten a security rating and sec-approval+.

https://wiki.mozilla.org/Security/Bug_Approval_Process
Flags: needinfo?(mstange)
(Assignee)

Comment 4

3 years ago
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.
Flags: needinfo?(mstange)
https://hg.mozilla.org/mozilla-central/rev/d927d3102472

Not sure if this affects B2G or not.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox32: --- → wontfix
status-firefox33: --- → wontfix
status-firefox34: --- → affected
status-firefox35: --- → fixed
status-firefox-esr31: --- → affected
tracking-firefox33: --- → ?
tracking-firefox34: --- → ?
tracking-firefox35: --- → ?
tracking-firefox-esr31: --- → ?
Flags: needinfo?(mstange)
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Markus, can we get an Aurora patch and a nomination for it, please?
tracking-firefox35: ? → -
tracking-firefox33: ? → -
tracking-firefox34: ? → +
(Assignee)

Comment 7

3 years ago
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?
Flags: needinfo?(mstange)
(Assignee)

Comment 8

3 years ago
Created attachment 8502734 [details] [diff] [review]
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
Attachment #8502734 - Flags: approval-mozilla-beta?
Attachment #8502734 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

3 years ago
Attachment #8502734 - Flags: approval-mozilla-beta?
(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.
(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.)
Keywords: sec-critical
(Assignee)

Comment 11

3 years ago
Sounds good, thanks a lot for doing the research.
status-b2g-v1.4: --- → affected
status-b2g-v2.0: --- → affected
status-b2g-v2.0M: --- → affected
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → fixed
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.
tracking-firefox-esr31: ? → 34+
Keywords: sec-critical → sec-high
Attachment #8502734 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Please nominate this for esr31 as well.
Flags: needinfo?(mstange)
https://hg.mozilla.org/releases/mozilla-aurora/rev/0fe683c5cabc
status-b2g-v2.1: affected → fixed
status-firefox34: affected → fixed
(Assignee)

Comment 15

3 years ago
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.
Attachment #8502734 - Flags: approval-mozilla-esr31?
Flags: needinfo?(mstange)
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
status-b2g-v1.4: affected → fixed
status-b2g-v2.0: affected → fixed
status-b2g-v2.0M: affected → fixed
Attachment #8502734 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
https://hg.mozilla.org/releases/mozilla-esr31/rev/26ec6429b69b
status-firefox-esr31: affected → fixed
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
Whiteboard: [adv-main34+][adv-esr31.3+]
Alias: CVE-2014-1594
Marking qe-verify- flag. Issues found via audit, custom tool etc., not enough time/resources for us to verify.
Flags: qe-verify-
Group: core-security
You need to log in before you can comment on or make changes to this bug.