Last Comment Bug 746855 - (CVE-2012-3972) [ASan] READ heap-buffer-overflow in format-number()
(CVE-2012-3972)
: [ASan] READ heap-buffer-overflow in format-number()
Status: VERIFIED FIXED
[sg:moderate][asan][advisory-tracking...
: privacy, sec-moderate
Product: Core
Classification: Components
Component: XSLT (show other bugs)
: 12 Branch
: x86_64 Linux
: -- major (vote)
: mozilla17
Assigned To: William Chen [:wchen] (Vacation until 8/25)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-18 19:05 PDT by Nicolas Grégoire
Modified: 2012-10-10 18:40 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
fixed
verified
verified
15+
fixed


Attachments
ASan error log (8.21 KB, text/plain)
2012-04-18 19:05 PDT, Nicolas Grégoire
no flags Details
Minimized XSLT test case (412 bytes, text/xml)
2012-04-18 19:06 PDT, Nicolas Grégoire
no flags Details
HTML page demonstrating the bug (JS + XSLT loop) (1.11 KB, text/html)
2012-04-25 14:40 PDT, Nicolas Grégoire
no flags Details
Fix to correct buffer overflow (2.87 KB, patch)
2012-06-11 10:33 PDT, William Chen [:wchen] (Vacation until 8/25)
jonas: review+
Details | Diff | Splinter Review
Fix to correct out of bounds read (2.87 KB, patch)
2012-07-17 07:49 PDT, William Chen [:wchen] (Vacation until 8/25)
jonas: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑esr10+
Details | Diff | Splinter Review

Description Nicolas Grégoire 2012-04-18 19:05:36 PDT
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.
Comment 1 Nicolas Grégoire 2012-04-18 19:06:16 PDT
Created attachment 616421 [details]
Minimized XSLT test case
Comment 2 Nicolas Grégoire 2012-04-25 14:40:00 PDT
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.
Comment 3 Al Billings [:abillings] 2012-05-01 15:56:15 PDT
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
Comment 4 Al Billings [:abillings] 2012-05-16 10:55:20 PDT
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.
Comment 5 Johnny Stenback (:jst, jst@mozilla.com) 2012-06-06 09:33:51 PDT
William, can you look into this one?
Comment 6 William Chen [:wchen] (Vacation until 8/25) 2012-06-11 10:33:56 PDT
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.
Comment 7 Peter Van der Beken [:peterv] 2012-06-15 02:49:17 PDT
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.
Comment 8 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-06-16 18:26:25 PDT
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.
Comment 9 Nicolas Grégoire 2012-07-17 02:36:30 PDT
If it's really a buffer overflow, perhaps that the criticity (currently sec-moderate) should be reviewed, no ?
Comment 10 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-07-17 02:39:54 PDT
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.
Comment 11 William Chen [:wchen] (Vacation until 8/25) 2012-07-17 07:44:14 PDT
(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.
Comment 12 William Chen [:wchen] (Vacation until 8/25) 2012-07-17 07:49:14 PDT
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.
Comment 13 William Chen [:wchen] (Vacation until 8/25) 2012-07-24 13:17:07 PDT
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
Comment 14 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-07-25 17:02:51 PDT
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.
Comment 15 Andrew McCreight [:mccr8] 2012-07-26 14:20:08 PDT
Do we want to land this with the test case, given that it affects 14?
Comment 16 Al Billings [:abillings] 2012-07-26 14:22:24 PDT
No, please hold off on that until we ship a release with this.
Comment 17 Andrew McCreight [:mccr8] 2012-07-26 15:01:49 PDT
Landed on inbound without the test, and with a slightly more obscure checkin message.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d7cf67c125e
Comment 18 Alex Keybl [:akeybl] 2012-07-26 16:13:14 PDT
(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!
Comment 19 Andrew McCreight [:mccr8] 2012-07-26 16:14:45 PDT
Per comment 14, it is "the format-number feature for XSLT, implemented since the dawn of Firefox.".
Comment 21 Alex Keybl [:akeybl] 2012-07-27 16:43:23 PDT
(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 22 Alex Keybl [:akeybl] 2012-07-27 16:44:07 PDT
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.
Comment 23 Andrew McCreight [:mccr8] 2012-07-27 16:53:25 PDT
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. :)
Comment 24 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-07-30 07:43:13 PDT
If this really is sg:moderate, why bother porting it to ESR?
Comment 25 William Chen [:wchen] (Vacation until 8/25) 2012-07-30 11:15:39 PDT
(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.
Comment 26 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-07-30 15:40:20 PDT
Thanks William! Could you attach a patch which applies cleanly to ESR and ask for esr approval
Comment 27 Andrew McCreight [:mccr8] 2012-07-30 15:49:27 PDT
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.
Comment 28 William Chen [:wchen] (Vacation until 8/25) 2012-07-30 20:06:53 PDT
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.
Comment 29 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-01 17:44:57 PDT
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.
Comment 30 Andrew McCreight [:mccr8] 2012-08-01 18:09:19 PDT
https://hg.mozilla.org/releases/mozilla-esr10/rev/87d51b64d78a
Comment 31 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-14 14:59:29 PDT
Is this something we can put in-testsuite? or does it need manual verification?
Comment 32 Nicolas Grégoire 2012-08-18 08:53:05 PDT
Attachment #618441 [details] can be used to easily trigger the out-of-bounds read. An ASan-enabled build can then detect this behavior.
Comment 33 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-21 16:24:13 PDT
Thanks Nicolas, I'll flag this for QA verification using your testcase.
Comment 34 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-23 15:27:04 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.