720103 - (CVE-2012-0457) ASAN: heap-use-after-free READ of size 8 at nsSMILTimeValueSpec::ConvertBetweenTimeContainers

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Atte Kettunen]

Attachment: repro-file

Repro-file as attachment.

==21012== ERROR: AddressSanitizer heap-use-after-free on address 0x7fa967597e88 at pc 0x7fa98a245854 bp 0x7fffc4ef2160 sp 0x7fffc4ef2158
READ of size 8 at 0x7fa967597e88 thread T0
    #0 0x7fa98a245854 (/home/ouspg/firefox/objdir-ff-asan/toolkit/library/libxul.so+0x1df2854)
    #1 0x7fa98a256c18 (/home/ouspg/firefox/objdir-ff-asan/toolkit/library/libxul.so+0x1e03c18)
0x7fa967597e88 is located 8 bytes inside of 32-byte region [0x7fa967597e80,0x7fa967597ea0)
freed by thread T0 here:
    #0 0x40ad84 (/home/ouspg/firefox/objdir-ff-asan/dist/bin/firefox+0x40ad84)
    #1 0x7fa98a2548d5 (/home/ouspg/firefox/objdir-ff-asan/toolkit/library/libxul.so+0x1e018d5)
    #2 0x7fa98a24f4c6 (/home/ouspg/firefox/objdir-ff-asan/toolkit/library/libxul.so+0x1dfc4c6)
    #3 0x7fa98a24ef4e (/home/ouspg/firefox/objdir-ff-asan/toolkit/library/libxul.so+0x1dfbf4e)
    #4 0x7fa98a259776 (/home/ouspg/firefox/objdir-ff-asan/toolkit/library/libxul.so+0x1e06776)
    #5 0x7fa98a256c18 (/home/ouspg/firefox/objdir-ff-asan/toolkit/library/libxul.so+0x1e03c18)
previously allocated by thread T0 here:
    #0 0x40ae64 (/home/ouspg/firefox/objdir-ff-asan/dist/bin/firefox+0x40ae64)
    #1 0x7fa98e37613d (/home/ouspg/firefox/objdir-ff-asan/memory/mozalloc/libmozalloc.so+0x113d)
    #2 0x7fa98a24f8cd (/home/ouspg/firefox/objdir-ff-asan/toolkit/library/libxul.so+0x1dfc8cd)
    #3 0x7fa98a24473f (/home/ouspg/firefox/objdir-ff-asan/toolkit/library/libxul.so+0x1df173f)
    #4 0x7fa98a244e05 (/home/ouspg/firefox/objdir-ff-asan/toolkit/library/libxul.so+0x1df1e05)
==21012== ABORTING
Stats: 54M malloced (77M for red zones) by 226646 calls
Stats: 3M realloced by 10807 calls
Stats: 29M freed by 100041 calls
Stats: 0M really freed by 0 calls
Stats: 160M (40979 full pages) mmaped in 40 calls
  mmaps   by size class: 8:196596; 9:32764; 10:8190; 11:6141; 12:2048; 13:1024; 14:512; 15:256; 16:256; 17:32; 18:64; 19:8; 20:4; 
  mallocs by size class: 8:187646; 9:23438; 10:7261; 11:5113; 12:1486; 13:776; 14:463; 15:169; 16:213; 17:23; 18:51; 19:5; 20:2; 
  frees   by size class: 8:78129; 9:12741; 10:4539; 11:2633; 12:830; 13:600; 14:244; 15:132; 16:163; 17:17; 18:10; 19:2; 20:1; 
  rfrees  by size class: 
Stats: malloc large: 81 small slow: 942
Shadow byte and word:
  0x1ff52ceb2fd1: fd
  0x1ff52ceb2fd0: fd fd fd fd fd fd fd fd
More shadow bytes:
  0x1ff52ceb2fb0: 00 00 00 00 00 00 fb fb
  0x1ff52ceb2fb8: fb fb fb fb fb fb fb fb
  0x1ff52ceb2fc0: fa fa fa fa fa fa fa fa
  0x1ff52ceb2fc8: fa fa fa fa fa fa fa fa
=>0x1ff52ceb2fd0: fd fd fd fd fd fd fd fd
  0x1ff52ceb2fd8: fd fd fd fd fd fd fd fd
  0x1ff52ceb2fe0: fa fa fa fa fa fa fa fa
  0x1ff52ceb2fe8: fa fa fa fa fa fa fa fa
  0x1ff52ceb2ff0: fd fd fd fd fd fd fd fd


GDB:

(gdb) i r
rax            0x0	0
rbx            0x7fffd8221180	140736819499392
rcx            0x7fffffff9a50	140737488329296
rdx            0x7fffd84a4dc0	140736822136256
rsi            0x8	8
rdi            0x7fffd8221180	140736819499392
rbp            0x8	0x8
rsp            0x7fffffff9940	0x7fffffff9940
r8             0x7ffff7ec6048	140737352851528
r9             0xc2b	3115
r10            0x7fffd8161bc0	140736818715584
r11            0x0	0
r12            0x7fffd84a4dc0	140736822136256
r13            0x0	0
r14            0x3	3
r15            0x7ffff4290b88	140737289718664
rip            0x7ffff42944b6	0x7ffff42944b6 <nsSMILTimeValueSpec::ConvertBetweenTimeContainers(nsSMILTimeValue const&, nsSMILTimeContainer const*)+14>
eflags         0x10202	[ IF RF ]
cs             0x33	51
ss             0x2b	43
ds             0x0	0
es             0x0	0
fs             0x0	0
gs             0x0	0
(gdb) i s
#0  nsSMILTimeValueSpec::ConvertBetweenTimeContainers (this=0x7fffd8221180, aSrcTime=..., aSrcContainer=0x7fffd84a4dc0) at /build/buildd/firefox-trunk-12.0~a1~hg20120114r84451/build-tree/mozilla/content/smil/nsSMILTimeValueSpec.cpp:546
#1  0x00007ffff42946fa in nsSMILTimeValueSpec::HandleNewInterval (this=0x7fffd8221180, aInterval=..., aSrcContainer=0x7fffd84a4dc0) at /build/buildd/firefox-trunk-12.0~a1~hg20120114r84451/build-tree/mozilla/content/smil/nsSMILTimeValueSpec.cpp:183
#2  0x00007ffff4290ba8 in nsSMILTimedElement::NotifyNewIntervalCallback (aKey=<value optimized out>, aData=<value optimized out>) at /build/buildd/firefox-trunk-12.0~a1~hg20120114r84451/build-tree/mozilla/content/smil/nsSMILTimedElement.cpp:2315
#3  0x00007ffff45c415b in PL_DHashTableEnumerate (table=0x7fffd82119b0, etor=0x7ffff4290b88 <nsTHashtable<nsPtrHashKey<nsSMILTimeValueSpec> >::s_EnumStub(PLDHashTable*, PLDHashEntryHdr*, PRUint32, void*)>, arg=0x7fffffff9a50) at /build/buildd/firefox-trunk-12.0~a1~hg20120114r84451/build-tree/mozilla/obj-x86_64-linux-gnu/xpcom/build/pldhash.cpp:754
#4  0x00007ffff4291c54 in nsTHashtable<nsPtrHashKey<nsSMILTimeValueSpec> >::EnumerateEntries (this=<value optimized out>, enumFunc=<value optimized out>, userArg=<value optimized out>) at ../../dist/include/nsTHashtable.h:241
#5  0x00007ffff4291c9e in nsSMILTimedElement::NotifyNewInterval (this=0x7fffd8211900) at /build/buildd/firefox-trunk-12.0~a1~hg20120114r84451/build-tree/mozilla/content/smil/nsSMILTimedElement.cpp:2203
#6  0x00007ffff42926c5 in nsSMILTimedElement::UpdateCurrentInterval (this=0x7fffd8211900, aForceChangeNotice=<value optimized out>) at /build/buildd/firefox-trunk-12.0~a1~hg20120114r84451/build-tree/mozilla/content/smil/nsSMILTimedElement.cpp:1989
.
.
.

Comment 1

[Atte Kettunen]

Windows 7 x64 Aurora build:

Signature: std::_Tree<std::_Tmap_traits<unsigned __int64, nsRefPtr<nsContentView>, std::less<unsigned __int64>, std::allocator<std::pair<unsigned __int64 const, nsRefPtr<nsContentView> > >, int> >::empty()

https://crash-stats.mozilla.com/report/index/bp-bc6d8314-b262-419d-b793-d158a2120122

Comment 2

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

Confirmed in trunk with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:12.0a1) Gecko/20120123 Firefox/12.0a1 using attached testcase.

Confirmed in Firefox 10 beta build 20120118081945 as well.

Confirmed in Firefox 11 aurora also: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:11.0a2) Gecko/20120120 Firefox/11.0a2.

Comment 3

[Daniel Veditz [:dveditz]]

(In reply to Atte Kettunen from comment #1)
> bp-bc6d8314-b262-419d-b793-d158a2120122

truncated stack, looks like it might be an infinite recursive loop. If so might not be exploitable. On the other hand, if aSrcTime is use-after-free at http://hg.mozilla.org/releases/mozilla-aurora/annotate/24cb0e50df85/content/smil/nsSMILTimeValueSpec.cpp#l546 then if you somehow had a usable value there (not the null in the crash) then calling a method on it might be exploitable a few lines later where you do aSrcTime.GetMillis().

Assigning to bbirtles based on hg-blame for a deeper look.

Comment 4

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 1 - Pass the timed element not interval to the notify new interval callback

The core problem appears to be that nsSMILTimedElement::NotifyNewInterval is running a series of callbacks passing a cached pointer to an nsSMILInterval (which isn't ref-counted). In this test case one of those callbacks is resulting in the nsSMILInterval getting cleared so the pointer dangles.

This first patch fixes that by looking up the interval each time and skipping remaining callbacks if the pointer is null.

I'm currently running it through tryserver (hopefully with a suitably unexciting description: https://tbpl.mozilla.org/?tree=Try&rev=eb76cf4ae78a) because landing this should fix the scary stuff.

However, the test case will still abort fatally in debug when it detects the excessive recursion taking place. So the remaining work for this bug is to:

1) Reconsider if we really want recursive updating like this. I think I tried once to remove all the recursion and it didn't work. I need to revisit this, but perhaps we could simply batch the updates when it comes to new intervals (like we do in many other parts).

2) Address the specific test case and work out what the expected behaviour should be here. The recursion limit is just a fallback. We're supposed to be able to detect such chains and break them in a predictable and sensible manner but clearly we're not doing that here.

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 592620 [details] [diff] [review]
Part 1 - Pass the timed element not interval to the notify new interval callback

>   struct NotifyTimeDependentsParams {
>-    nsSMILInterval*      mCurrentInterval;
>+    nsSMILTimedElement*  mElement;
>     nsSMILTimeContainer* mTimeContainer;

Maybe "mTimedElement", to be sure it's clear this isn't an nsIContent (which is what "mElement" refers to in a lot of other places).

Otherwise, looks good (for fixing the crash, at least).

Comment 6

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Pushed:
https://hg.mozilla.org/mozilla-central/rev/836b5e3bc816

Incorporates review feedback from comment 5.

Leaving open because while this fixes the invalid memory read, it doesn't fix the recursion problems in the attached test case that will cause a fatal assertion in debug builds.

Comment 7

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Marking this fixed in FF12 since the scary part is fixed there, and users of FF12 (i.e. opt builds) won't see any problems.

Comment 8

[Atte Kettunen]

Was there any indication that the invalid memory read could cause any security issues?

Comment 9

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Atte Kettunen from comment #8)
> Was there any indication that the invalid memory read could cause any
> security issues?

I'm not certain, but I think so. In the following line the memory pointed to by mCurrentInterval can be read after free.

http://hg.mozilla.org/releases/mozilla-aurora/annotate/24cb0e50df85/content/smil/nsSMILTimedElement.cpp#l2202

It is read here:

http://hg.mozilla.org/releases/mozilla-aurora/annotate/24cb0e50df85/content/smil/nsSMILTimeValueSpec.cpp#l180

as aInterval where Begin()/End() is executed. The non-const version of these used here are not inlined.

Shall I nominate this for beta after it's had a bit of bake time in aurora?

Comment 10

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 2 - Detect and break create-delete cycles

With regards to comment 4, for the time being the recursive approach stays. SMIL's cycle detection behaviour is defined in recursive terms and switching to an iterative approach is not trivial (i.e. deserves a separate bug if we think it's worthwhile).

For now I've added cycle detection for create-delete-create-delete cycles and done what I think is expected in those cases.

Comment 11

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 3 - Crash test [don't check in until FF12 has shipped]

Minimal test case.

Comment 12

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Currently running part 2 through Try: https://tbpl.mozilla.org/?tree=Try&rev=5fd6efcf4ef5

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 593296 [details] [diff] [review]
Part 2 - Detect and break create-delete cycles

>+    NS_ABORT_IF_FALSE(mElementState == STATE_POSTACTIVE,
>+      "Expected to be in post-active state after performing double delete.");

Nix the period at the end here (since we'll end up printing a ";" at the end of the message anyway)

r=me with that

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 593297 [details] [diff] [review]
Part 3 - Crash test [don't check in until FF12 has shipped]

>+++ b/content/smil/crashtests/720103-1.svg
>@@ -0,0 +1,4 @@
>+<?xml version="1.0"?>
>+<svg xmlns="http://www.w3.org/2000/svg">
>+<animate id="a" begin="-3.1s" end="a.begin+0.2s"/>
>+</svg>
>\ No newline at end of file

Add newline at end, and r=me.

Comment 16

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Thanks Daniel for the reviews!

Part 2 (with review comments addressed) pushed to m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1aeba59aa111

Comment 17

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 3 - Crash test; r=dholbert [don't check in until FF11 has shipped]

Address review feedback for part 3.

Comment 18

[Daniel Holbert [:dholbert]]

setting "in-testsuite?" as a marker that the test needs to eventually be checked in (after FF12 has shipped)

Comment 19

[Ed Morley [:emorley]]

(In reply to Brian Birtles (:birtles) from comment #16)
> Part 2 (with review comments addressed) pushed to m-i:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/1aeba59aa111

https://hg.mozilla.org/mozilla-central/rev/1aeba59aa111

Comment 20

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 592620 [details] [diff] [review]
Part 1 - Pass the timed element not interval to the notify new interval callback

[Approval Request Comment]
Regression caused by (bug #): n/a
User impact if declined: potential execution of arbitrary code
Testing completed (on m-c, etc.): baked on m-c for one week, and aurora for a little less
Risk to taking this patch (and alternatives if risky): no known risks other than the possibility of this code change having unintended side effects
String changes made by this patch: none

Comment 21

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Pushed to mozilla-beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/529a3bebd807

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

[Triage comment]

This bug is being tracked for landing on ESR branch.  Please land patches on http://hg.mozilla.org/releases/mozilla-esr10/by Thursday March 1, 2012 in order to be ready for go-to-build on Friday March 2, 2012.

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

Comment 23

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Pushed to ESR branch:
https://hg.mozilla.org/releases/mozilla-esr10/rev/03cd652cf86d

Comment 24

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

Tested this on XP with 3.6.27. No crash.

Comment 25

[Daniel Holbert [:dholbert]]

Cool -- that makes sense, as our SMIL code (including nsSMILTimeValueSpec) wasn't enabled in builds until Firefox 4.

Comment 26

[u279076]

I was able to crash using the attached testcase with the latest Firefox 10 ESR debug build.

Build:
ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012-03-03-mozilla-esr10-debug

Crash Report:
https://crash-stats.mozilla.com/report/index/d4cb4022-1fa3-49f9-8f7e-010ad2120305

Signature:
nsSMILTimeValueSpec::ConvertBetweenTimeContainers

Comment 27

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #26)
> I was able to crash using the attached testcase with the latest Firefox 10
> ESR debug build.

Yes, the debug builds are not fixed. We've only landed part 1 on the ESR branch. Part 1 fixes the crash in opt builds. In debug builds you'll still get an NS_ABORT_IF_FALSE failing. That's fixed by part 2 which isn't on the ESR branch.

Looking at the crash report I'm not entirely sure why it crashed at that point, but at any rate, I'm not expecting ESR debug builds to work. Is there any requirement they do? If so, we need to land part 2 there as well.

Comment 28

[u279076]

Thanks Brian, verified this is fixed in Firefox 10.0.3esr candidate build.

Comment 29

[Jason Smith [:jsmith]]

Verified in FF 11 Beta 6, FF 12 Aurora, FF 13 Nightly.

Comment 30

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Pushed test (part 3) to m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/29dcc30b7ff5

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/29dcc30b7ff5

Comment 32

[Raymond Forbes[:rforbes]]

rforbes-bugspam-for-setting-that-bounty-flag-20130719