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
Created attachment 8497671 [details] [diff] [review] patch
Is this trunk only? Otherwise, this should have gotten a security rating and sec-approval+. https://wiki.mozilla.org/Security/Bug_Approval_Process
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.
https://hg.mozilla.org/mozilla-central/rev/d927d3102472 Not sure if this affects B2G or not.
Markus, can we get an Aurora patch and a nomination for it, please?
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?
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
(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.)
Sounds good, thanks a lot for doing the research.
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.
Please nominate this for esr31 as well.
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.
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
Marking qe-verify- flag. Issues found via audit, custom tool etc., not enough time/resources for us to verify.