746855 - (CVE-2012-3972) [ASan] READ heap-buffer-overflow in format-number()

Status: VERIFIED FIXED

(Core :: XSLT, defect)

Description

[Nicolas Grégoire]

Attachment: 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]

Attachment: Minimized XSLT test case

Comment 2

[Nicolas Grégoire]

Attachment: 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 - ex-MoCo]]

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 - ex-MoCo]]

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)]

William, can you look into this one?

Comment 6

[William Chen [:wchen]]

Attachment: 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]]

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]

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]

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]

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]]

(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]]

Attachment: 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]]

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]

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]]

Do we want to land this with the test case, given that it affects 14?

Comment 16

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

No, please hold off on that until we ship a release with this.

Comment 17

[Andrew McCreight [:mccr8]]

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]]

(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]]

Per comment 14, it is "the format-number feature for XSLT, implemented since the dawn of Firefox.".

Comment 20

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/9d7cf67c125e

Comment 21

[Alex Keybl [:akeybl]]

(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]]

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]]

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 (Exited; not receiving bugmail, old account, do not use)]

If this really is sg:moderate, why bother porting it to ESR?

Comment 25

[William Chen [:wchen]]

(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]

Thanks William! Could you attach a patch which applies cleanly to ESR and ask for esr approval

Comment 27

[Andrew McCreight [:mccr8]]

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]]

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]

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]]

https://hg.mozilla.org/releases/mozilla-esr10/rev/87d51b64d78a

Comment 31

[u279076]

Is this something we can put in-testsuite? or does it need manual verification?

Comment 32

[Nicolas Grégoire]

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

[u279076]

Thanks Nicolas, I'll flag this for QA verification using your testcase.

Comment 34

[u279076]

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.