The default bug view has changed. See this FAQ.
Bug 720103 (CVE-2012-0457)

ASAN: heap-use-after-free READ of size 8 at nsSMILTimeValueSpec::ConvertBetweenTimeContainers

VERIFIED FIXED in Firefox 11

Status

()

Core
SVG
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: Atte Kettunen, Assigned: birtles)

Tracking

Trunk
mozilla13
x86_64
Linux
Points:
---
Bug Flags:
sec-bounty +
in-testsuite +

Firefox Tracking Flags

(firefox10 affected, firefox11 verified, firefox12 verified, firefox-esr1011+ verified, status1.9.2 unaffected)

Details

(Whiteboard: [sg:critical][asan][qa!])

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 590449 [details]
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
.
.
.
Component: General → SVG
Product: Firefox → Core
QA Contact: general → general
(Reporter)

Comment 1

5 years ago
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
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.
Status: UNCONFIRMED → NEW
status-firefox10: --- → affected
status-firefox11: --- → affected
status-firefox12: --- → affected
Ever confirmed: true
(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.
Assignee: nobody → birtles
Whiteboard: [asan]
(Assignee)

Comment 4

5 years ago
Created attachment 592620 [details] [diff] [review]
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.
Attachment #592620 - Flags: review?(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).
Attachment #592620 - Flags: review?(dholbert) → review+
(Assignee)

Comment 6

5 years ago
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.
(Assignee)

Comment 7

5 years ago
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.
status-firefox12: affected → fixed
(Reporter)

Comment 8

5 years ago
Was there any indication that the invalid memory read could cause any security issues?
(Assignee)

Comment 9

5 years ago
(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?
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Attachment #592620 - Flags: checkin+
(Assignee)

Comment 10

5 years ago
Created attachment 593296 [details] [diff] [review]
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.
Attachment #593296 - Flags: review?(dholbert)
(Assignee)

Comment 11

5 years ago
Created attachment 593297 [details] [diff] [review]
Part 3 - Crash test [don't check in until FF12 has shipped]

Minimal test case.
Attachment #593297 - Flags: review?(dholbert)
(Assignee)

Comment 12

5 years ago
Currently running part 2 through Try: https://tbpl.mozilla.org/?tree=Try&rev=5fd6efcf4ef5
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
Attachment #593296 - Flags: review?(dholbert) → review+
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.
Attachment #593297 - Flags: review?(dholbert) → review+
Whiteboard: [asan] → [sg:critical][asan]
(Assignee)

Comment 16

5 years ago
Thanks Daniel for the reviews!

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

Updated

5 years ago
Attachment #593296 - Flags: checkin+
(Assignee)

Comment 17

5 years ago
Created attachment 593651 [details] [diff] [review]
Part 3 - Crash test; r=dholbert [don't check in until FF11 has shipped]

Address review feedback for part 3.
Attachment #593297 - Attachment is obsolete: true
Attachment #593651 - Flags: review+
setting "in-testsuite?" as a marker that the test needs to eventually be checked in (after FF12 has shipped)
Flags: in-testsuite?
(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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
(Assignee)

Comment 20

5 years ago
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
Attachment #592620 - Flags: approval-mozilla-beta?
Attachment #592620 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Comment 21

5 years ago
Pushed to mozilla-beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/529a3bebd807
status-firefox11: affected → fixed
(Assignee)

Updated

5 years ago
Attachment #593651 - Attachment description: Part 3 - Crash test; r=dholbert [don't check in until FF12 has shipped] → Part 3 - Crash test; r=dholbert [don't check in until FF11 has shipped]
Whiteboard: [sg:critical][asan] → [sg:critical][asan][qa+]
[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.
tracking-firefox-esr10: --- → 11+
(Assignee)

Comment 23

5 years ago
Pushed to ESR branch:
https://hg.mozilla.org/releases/mozilla-esr10/rev/03cd652cf86d
status-firefox-esr10: --- → fixed
Tested this on XP with 3.6.27. No crash.
status1.9.2: --- → unaffected
Cool -- that makes sense, as our SMIL code (including nsSMILTimeValueSpec) wasn't enabled in builds until Firefox 4.
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
(Assignee)

Comment 27

5 years ago
(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.
Thanks Brian, verified this is fixed in Firefox 10.0.3esr candidate build.
status-firefox-esr10: fixed → verified
Verified in FF 11 Beta 6, FF 12 Aurora, FF 13 Nightly.
Status: RESOLVED → VERIFIED
status-firefox11: fixed → verified
status-firefox12: fixed → verified
Whiteboard: [sg:critical][asan][qa+] → [sg:critical][asan]
Whiteboard: [sg:critical][asan] → [sg:critical][asan][qa!]
Alias: CVE-2012-0457
Group: core-security
(Assignee)

Comment 30

5 years ago
Pushed test (part 3) to m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/29dcc30b7ff5
https://hg.mozilla.org/mozilla-central/rev/29dcc30b7ff5
Flags: in-testsuite? → in-testsuite+
rforbes-bugspam-for-setting-that-bounty-flag-20130719
Flags: sec-bounty+
You need to log in before you can comment on or make changes to this bug.