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

[ASan] READ heap-buffer-overflow in format-number()

VERIFIED FIXED in Firefox 15

Status

()

Core
XSLT
--
major
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Nicolas Grégoire, Assigned: wchen)

Tracking

({privacy, sec-moderate})

12 Branch
mozilla17
x86_64
Linux
privacy, sec-moderate
Points:
---

Firefox Tracking Flags

(firefox14 wontfix, firefox15 fixed, firefox16 verified, firefox17 verified, firefox-esr1015+ fixed)

Details

(Whiteboard: [sg:moderate][asan][advisory-tracking+][qa-])

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 616420 [details]
ASan error log

ASan trace :

==3077== ERROR: AddressSanitizer heap-buffer-overflow on address 0x7fafa86aa2dd at pc 0x7fafd761d317 bp 0x7ffff9c0aeb0 sp 0x7ffff9c0aea8
READ of size 1 at 0x7fafa86aa2dd thread T0
    #0 0x7fafd761d317 (/home/nicob/Asan/firefox/firefox+0x7fafd761d317)
    #1 0x7fafd74b0100 (/home/nicob/Asan/firefox/firefox+0x7fafd74b0100)
    #2 0x7fafd75f8989 (/home/nicob/Asan/firefox/firefox+0x7fafd75f8989)
    #3 0x7fafd769ed84 (/home/nicob/Asan/firefox/firefox+0x7fafd769ed84)
    #4 0x7fafdeccc060 (/home/nicob/Asan/firefox/firefox+0x7fafdeccc060)
    #5 0x7fafdaab5d32 (/home/nicob/Asan/firefox/firefox+0x7fafdaab5d32)
    [...]
0x7fafa86aa2dd is located 163 bytes to the left of 31-byte region [0x7fafa86aa380,0x7fafa86aa39f)
allocated by thread T0 here:
    #0 0x42cb94 (/home/nicob/Asan/firefox/firefox+0x42cb94)
    #1 0x7fafeced862e (/home/nicob/Asan/firefox/firefox+0x7fafeced862e)
    #2 0x7fafd76187f8 (/home/nicob/Asan/firefox/firefox+0x7fafd76187f8)
    #3 0x7fafd74b0100 (/home/nicob/Asan/firefox/firefox+0x7fafd74b0100)
    #4 0x7fafd75f8989 (/home/nicob/Asan/firefox/firefox+0x7fafd75f8989)
    #5 0x7fafd769ed84 (/home/nicob/Asan/firefox/firefox+0x7fafd769ed84)
    [...]

Tested versions :

- Firefox 11.0 / Windows XP 3 / x86
- Firefox 12.0a1 / Linux / x64 (public ASan build from https://people.mozilla.com/~choller/firefox/asan/)

Note : the offset of the invalid read can be manipulated by modifying the divisor.
(Reporter)

Comment 1

5 years ago
Created attachment 616421 [details]
Minimized XSLT test case
Attachment #616421 - Attachment mime type: text/plain → text/xml
(Reporter)

Comment 2

5 years ago
Created attachment 618441 [details]
HTML page demonstrating the bug (JS + XSLT loop)

It's possible to partially influence the read location and to retrieve some data. This should probably be considered as an information leak.
Attachment #618441 - Attachment mime type: text/plain → text/html
I couldn't reproduce with the first testcase. With the second one using my ASAN OS X build, I got the following:

==9288== ERROR: AddressSanitizer heap-buffer-overflow on address 0x00011774045b at pc 0x105939f9d bp 0x7fff5fbf37f0 sp 0x7fff5fbf37e8
READ of size 1 at 0x00011774045b thread T0
    #0 0x105939f9c in txFormatNumberFunctionCall::evaluate txFormatNumberFunctionCall.cpp:332
    #1 0x10590cc3f in txValueOf::execute txInstructions.cpp:963
    #2 0x105933c1c in txXSLTProcessor::execute txXSLTProcessor.cpp:82
    #3 0x106cb8027 in NS_InvokeByIndex_P xptcinvoke_x86_64_unix.cpp:196
    #4 0x10617137d in XPCWrappedNative::CallMethod XPCWrappedNative.cpp:3117
    #5 0x10617ffd4 in XPC_WN_CallMethod XPCWrappedNativeJSOps.cpp:1548
    #6 0x10749a76f in js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (in XUL) + 479
    #7 0x10748be8c in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (in XUL) + 57516
    #8 0x10747dd60 in js::RunScript(JSContext*, JSScript*, js::StackFrame*) (in XUL) + 336
    #9 0x10749a802 in js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (in XUL) + 626
    #10 0x10740c101 in js::Invoke(JSContext*, js::InvokeArgsGuard&, js::MaybeConstruct) (in XUL) + 81
0x00011774045b is located 37 bytes to the left of 31-byte region [0x000117740480,0x00011774049f)
allocated by thread T0 here:
got symbolicator for /Users/albill/hg/mozilla-central-asan/objdir-ff-asan/dist/bin/firefox, base address 100000000
    #0 0x10000bbb3 in (anonymous namespace)::mz_malloc(_malloc_zone_t*, unsigned long) (in firefox) + 67
    #1 0x7fff8b7303c8 in malloc_zone_malloc (in libsystem_c.dylib) + 77
    #2 0x7fff8b7311a4 in malloc (in libsystem_c.dylib) + 44
    #3 0x1020da530 in moz_xmalloc mozalloc.cpp:87
    #4 0x10590cc3f in txValueOf::execute txInstructions.cpp:963
    #5 0x105933c1c in txXSLTProcessor::execute txXSLTProcessor.cpp:82
    #6 0x106cb8027 in NS_InvokeByIndex_P xptcinvoke_x86_64_unix.cpp:196
    #7 0x10617137d in XPCWrappedNative::CallMethod XPCWrappedNative.cpp:3117
    #8 0x10617ffd4 in XPC_WN_CallMethod XPCWrappedNativeJSOps.cpp:1548
    #9 0x10749a76f in js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (in XUL) + 479
    #10 0x10748be8c in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (in XUL) + 57516
    #11 0x10747dd60 in js::RunScript(JSContext*, JSScript*, js::StackFrame*) (in XUL) + 336
    #12 0x10749a802 in js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (in XUL) + 626
    #13 0x10740c101 in js::Invoke(JSContext*, js::InvokeArgsGuard&, js::MaybeConstruct) (in XUL) + 81
==9288== ABORTING
Stats: 465M malloced (407M for red zones) by 844856 calls
Stats: 54M realloced by 24843 calls
Stats: 429M freed by 672179 calls
Stats: 277M really freed by 485876 calls
Stats: 548M (140371 full pages) mmaped in 137 calls
  mmaps   by size class: 8:393192; 9:73719; 10:45045; 11:20470; 12:4096; 13:3072; 14:2304; 15:640; 16:512; 17:832; 18:128; 19:64; 20:16; 22:5;
  mallocs by size class: 8:595185; 9:119935; 10:75660; 11:36145; 12:5922; 13:5002; 14:3200; 15:1177; 16:909; 17:1396; 18:209; 19:85; 20:26; 22:6;
  frees   by size class: 8:449305; 9:102415; 10:71417; 11:33164; 12:4811; 13:4593; 14:2943; 15:1112; 16:741; 17:1379; 18:187; 19:82; 20:24; 22:6;
  rfrees  by size class: 8:335956; 9:68317; 10:46176; 11:23471; 12:3593; 13:3619; 14:2527; 15:794; 16:554; 17:636; 18:140; 19:76; 20:15; 22:2;
Stats: malloc large: 1811 small slow: 4625
Shadow byte and word:
  0x100022ee808b: fa
  0x100022ee8088: fa fa fa fa fa fa fa fa
More shadow bytes:
  0x100022ee8068: fa fa fa fa fa fa fa fa
  0x100022ee8070: fd fd fd fd fd fd fd fd
  0x100022ee8078: fd fd fd fd fd fd fd fd
  0x100022ee8080: fa fa fa fa fa fa fa fa
=>0x100022ee8088: fa fa fa fa fa fa fa fa
  0x100022ee8090: 00 00 00 07 fb fb fb fb
  0x100022ee8098: fb fb fb fb fb fb fb fb
  0x100022ee80a0: fa fa fa fa fa fa fa fa
  0x100022ee80a8: fa fa fa fa fa fa fa fa
Status: UNCONFIRMED → NEW
Ever confirmed: true
Critsmash triage: Calling this a sec-moderate because this appears to be a read only issue with information disclosure based on comment 2. If this is incorrect, please state so.
Keywords: sec-moderate
Whiteboard: [sg:moderate]
William, can you look into this one?
Assignee: nobody → wchen
(Assignee)

Comment 6

5 years ago
Created attachment 631937 [details] [diff] [review]
Fix to correct buffer overflow

The buffer overflow is caused by PR_dtoa returning a negative offset for the decimal position during conversion of very small numbers (which is strange but makes sense).

The buffer overflow occurs during a check for a carry when formatting numbers at content/xslt/src/xslt/txFormatNumberFunctionCall.cpp:299.

bool carry = (i+1 < buflen) && (buf[i+1] >= '5');

This currently causes non-deterministic and inaccurate formatting of numbers. You can see this in the testcases where a number very close to zero gets rounded to .1 sometimes and .0 other times.

Luckily the fix is simple and the check for carry seems to be the only line that incorrectly handles the negative decimal offset.
Attachment #631937 - Flags: review?(peterv)
Comment on attachment 631937 [details] [diff] [review]
Fix to correct buffer overflow

Review of attachment 631937 [details] [diff] [review]:
-----------------------------------------------------------------

This code has sicking's name all over it.
Attachment #631937 - Flags: review?(peterv) → review?(jonas)
Comment on attachment 631937 [details] [diff] [review]
Fix to correct buffer overflow

Review of attachment 631937 [details] [diff] [review]:
-----------------------------------------------------------------

I'm pretty sure we don't want to check in the testcase until this bug has been opened up. Please check with dveditz what the proper procedure is.

::: content/xslt/src/xslt/txFormatNumberFunctionCall.cpp
@@ +296,4 @@
>                    (intDigits-1)/groupSize); // group separators
>  
>      PRInt32 i = bufIntDigits + maxFractionSize - 1;
> +    bool carry = (0 <= i+1) && (i+1 < buflen) && (buf[i+1] >= '5');

I'd prefer (i+1 >= 0) as I prefer having the tested expression to the left of the operator.

OTOH the current setup makes sense since you're testing that (0 <= i+1 < buflen)

Up to you.
Attachment #631937 - Flags: review?(jonas) → review+
(Reporter)

Comment 9

5 years ago
If it's really a buffer overflow, perhaps that the criticity (currently sec-moderate) should be reviewed, no ?
Either way, the patch here should land. I believe we just missed the train to get this into Firefox 14, so let's not slip another release.
(Assignee)

Comment 11

5 years ago
(In reply to Nicolas Grégoire from comment #9)
> If it's really a buffer overflow, perhaps that the criticity (currently
> sec-moderate) should be reviewed, no ?

I talked with dveditz about this bug and technically it is an out of bounds read. He said that we need to take a look at how bad this bug is for other branches.

(In reply to Jonas Sicking (:sicking) from comment #10)
> Either way, the patch here should land. I believe we just missed the train
> to get this into Firefox 14, so let's not slip another release.

I'll work with someone on the security team to get this landed soon.
(Assignee)

Comment 12

5 years ago
Created attachment 642951 [details] [diff] [review]
Fix to correct out of bounds read

I updated the patch description to change buffer overflow to out of bounds read and updated the reviewer to sicking.
Attachment #631937 - Attachment is obsolete: true
Attachment #642951 - Flags: review?(jonas)
Attachment #642951 - Flags: review?(jonas) → review+
Whiteboard: [sg:moderate] → [sg:moderate][asan]
status-firefox-esr10: --- → affected
status-firefox14: --- → wontfix
status-firefox15: --- → affected
status-firefox16: --- → affected
status-firefox17: --- → affected
Keywords: privacy
(Assignee)

Comment 13

5 years ago
Comment on attachment 642951 [details] [diff] [review]
Fix to correct out of bounds read

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 746855
User impact if declined: Users will be vulnerable to random memory reads.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): none
String or UUID changes made by this patch: none
Attachment #642951 - Flags: approval-mozilla-beta?
Attachment #642951 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
For the record, a correction to the above

Bug caused by (feature/regressing bug #): the format-number feature for XSLT, implemented since the dawn of Firefox.

Updated

5 years ago
tracking-firefox-esr10: --- → ?
Do we want to land this with the test case, given that it affects 14?
No, please hold off on that until we ship a release with this.
Landed on inbound without the test, and with a slightly more obscure checkin message.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d7cf67c125e
Keywords: checkin-needed
(In reply to William Chen [:wchen] from comment #13)
> Bug caused by (feature/regressing bug #): 746855

This is bug 746855, but I don't see earlier check-ins. What's the regressing bug here? Thanks!
Per comment 14, it is "the format-number feature for XSLT, implemented since the dawn of Firefox.".
https://hg.mozilla.org/mozilla-central/rev/9d7cf67c125e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
(In reply to Andrew McCreight [:mccr8] from comment #19)
> Per comment 14, it is "the format-number feature for XSLT, implemented since
> the dawn of Firefox.".

Doh missed that.
Comment on attachment 642951 [details] [diff] [review]
Fix to correct out of bounds read

[Triage Comment]
No-risk sg:moderate fix. Let's land on branches. Please prepare an ESR10 fix as well, as it appears to be affected.
Attachment #642951 - Flags: approval-mozilla-beta?
Attachment #642951 - Flags: approval-mozilla-beta+
Attachment #642951 - Flags: approval-mozilla-aurora?
Attachment #642951 - Flags: approval-mozilla-aurora+

Updated

5 years ago
tracking-firefox-esr10: ? → 15+
https://hg.mozilla.org/releases/mozilla-aurora/rev/92d17113de26
https://hg.mozilla.org/releases/mozilla-beta/rev/5a85437b01b9

The patch doesn't apply cleanly to ESR-10.  The actual carry line appears the same, but I guess the context is different somehow, so wchen should probably look at it to make sure it is okay to land. :)
status-firefox15: affected → fixed
status-firefox16: affected → fixed
status-firefox17: affected → fixed
If this really is sg:moderate, why bother porting it to ESR?
(Assignee)

Comment 25

5 years ago
(In reply to Andrew McCreight [:mccr8] from comment #23)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/92d17113de26
> https://hg.mozilla.org/releases/mozilla-beta/rev/5a85437b01b9
> 
> The patch doesn't apply cleanly to ESR-10.  The actual carry line appears
> the same, but I guess the context is different somehow, so wchen should
> probably look at it to make sure it is okay to land. :)

The bug is in ESR-10 and the fix is the same. The reason the patch doesn't apply cleanly is due to the MBool to bool replacement a while back.
Thanks William! Could you attach a patch which applies cleanly to ESR and ask for esr approval
I've just fixed it myself locally, so you don't need to upload a patch.  Just fill out the ESR10 thing and I can land it if it is approved.  For some reason I thought it was approved before.
(Assignee)

Comment 28

5 years ago
Comment on attachment 642951 [details] [diff] [review]
Fix to correct out of bounds read

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Security bug with no risk fix.
User impact if declined: Users will be vulnerable to random memory reads.
Fix Landed on Version: 15, 16, 17
Risk to taking this patch (and alternatives if risky): None
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #642951 - Flags: approval-mozilla-esr10?
Comment on attachment 642951 [details] [diff] [review]
Fix to correct out of bounds read

Approving to take for the upcoming ESR, please land as soon as possible see https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for details if needed.
Attachment #642951 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
https://hg.mozilla.org/releases/mozilla-esr10/rev/87d51b64d78a
status-firefox-esr10: affected → fixed
Whiteboard: [sg:moderate][asan] → [sg:moderate][asan][advisory-tracking+]
Is this something we can put in-testsuite? or does it need manual verification?
Whiteboard: [sg:moderate][asan][advisory-tracking+] → [sg:moderate][asan][advisory-tracking+][qa?]
(Reporter)

Comment 32

5 years ago
Attachment #618441 [details] can be used to easily trigger the out-of-bounds read. An ASan-enabled build can then detect this behavior.
Thanks Nicolas, I'll flag this for QA verification using your testcase.
Keywords: verifyme
Whiteboard: [sg:moderate][asan][advisory-tracking+][qa?] → [sg:moderate][asan][advisory-tracking+]
Alias: CVE-2012-3972
Confirmed testcase reproducible with try-server ASan build from decoder with changeset 9f3cc040e41a.

Verified testcase not reproducible with: 
 * 17.0a1: 198ca6edd0ae (debug) built on 20120823 by decoder
 * 16.0a2: 805e936380ab (debug) built on 20120823 by decoder

qa- for Firefox 15 and ESR15 as builds are not available.
Status: RESOLVED → VERIFIED
status-firefox16: fixed → verified
status-firefox17: fixed → verified
Keywords: verifyme
Whiteboard: [sg:moderate][asan][advisory-tracking+] → [sg:moderate][asan][advisory-tracking+][qa-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.