682727 - (CVE-2011-3652) Crash during datamove (ACCESS VIOLATION READ) [@ memcpy | nsAString_internal::Assign(unsigned short const*, unsigned int)]

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Rh0]

Attachment: testcase to crash Firefox 6

When running the attached testcase on a native WinXP SP3 installation (not in VirtualBox)with Firefox 6, it crashes during a datamove at memcpy in method nsAString_internal::Assign(unsigned short const*, unsigned int).

Crash IDs:
bp-61c8c0ae-0b67-4063-bd2b-906392110828
bp-5534e024-7733-42bd-a565-464ce2110828

WinDBG states this as Probably Exploitable. This memcpy crash seems similar to the one in bug 505305 ([sg:critical?]). The testcase is based on the testcase of bug 651990.

Steps to reproduce:
1. Open testcase in Fx6
2. Click the button

Greetings, 
Rh0

Comment 1

[Bob Clary [:bc] (inactive)]

WinXP w/1G ram

Debug Nightly/Aurora/Beta crashed but MSVC was unable to capture the crash.

Beta/7
bp-8541ebe3-25e1-4b5e-896c-59ea42110828 (no signature)

Aurora/8
bp-731b301b-ada4-45ff-9bd8-2af542110828
[@ nsAString_internal::Assign(nsAString_internal const&) ]

Nightly/9
bp-66d68216-8e2e-4b84-8a47-cdcce2110828
[@ memcpy | nsAString_internal::Assign(wchar_t const*, unsigned int) ]

Comment 3

[Daniel Veditz [:dveditz]]

Not realistic to squeeze into Firefox 7, targeting the next release.

Comment 4

[Michal Novotny [:michal]]

Attachment: fix

The problem is that the allocation of the buffer fails while flattening the string at http://hg.mozilla.org/mozilla-central/annotate/7f4867717226/js/src/vm/String.cpp#l207 but we don't set mValid=false in xpc_qsDOMString/xpc_qsAUTF8String.

Comment 5

[Michal Novotny [:michal]]

Pushed to tryserver https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=d78227785476

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 563014 [details] [diff] [review]
fix

Good find, Michal! r=jst

Comment 7

[Josh Aas]

Comment on attachment 563014 [details] [diff] [review]
fix

>     JSString *s = InitOrStringify<traits>(cx, v, pval, nullBehavior,
>                                           undefinedBehavior);
>     if (!s)
>         return;
...
>     JSString *s = InitOrStringify<traits>(cx, v, pval, eNull, eNull);
>     if (!s)
>         return;

I don't know this code at all but I want to make sure we don't also need to set mValid to JS_FALSE in the early returns prior to the ones you patched.

Comment 8

[Johnny Stenback (:jst)]

InitOrStringify() already set mValid to false in all error cases, so don't need to set it again in those cases.

Comment 9

[Michal Novotny [:michal]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/dd0cc50e9fe6

Comment 10

[Jesse Ruderman]

http://hg.mozilla.org/mozilla-central/rev/dd0cc50e9fe6

Comment 11

[Marcia Knous [:marcia]]

I tried verifying this in a Win XP VM and on a Win 7 machine using Mozilla/5.0 (Windows NT 6.1; rv:10.0a1) Gecko/20111006 Firefox/10.0a1, but in both cases Firefox stops responding but no crash reporter comes up.

Comment 12

[Christopher Blizzard (:blizzard)]

Comment on attachment 563014 [details] [diff] [review]
fix

Approved for Aurora (Update 9) and Beta (Update 8).  Please land as soon as appropriate.

Comment 13

[Rh0]

To comment 11 :
Using XP and Firefox 6 in my testings, the crash appeared only on native installations of XP and not in VMs running in VirtualBox.

Comment 14

[Marcia Knous [:marcia]]

I used both a VM and real hardware to test, and got the same result - the nightly stopped responding. So it does not seem that the patch really fixes the issue unless I am misunderstanding something.

(In reply to Rh0 from comment #13)
> To comment 11 :
> Using XP and Firefox 6 in my testings, the crash appeared only on native
> installations of XP and not in VMs running in VirtualBox.

Comment 15

[Marcia Knous [:marcia]]

I don't have access to Win hardware at the moment, so I would prefer if we could hold off checking this in until QA can do some more testing on Windows hardware in the lab. Thanks.

Comment 16

[juan becerra [:juanb]]

I tried reproducing this problem on Fx5, 7.0.1, and the most recent Nightly on a Win7 machine. In all three the testcase crashes the application. It takes a while, but they all crash.

When I try to look at the crash reports, they can't be found: bp-1dbea091-1abf-448b-8fbb-313ad2111006

It could be that this is now crashing in a different place, but without seeing the crash signature I can't tell. I'll re-open as soon as the crash report shows up, or after consulting with someone in the security team.

Comment 17

[Marcia Knous [:marcia]]

I was able to crash again, but I get a different signature: 
https://crash-stats.mozilla.com/report/index/bp-29f53417-90cb-44c6-9459-c185f2111007

Should I file new bug for this crash and should it be security sensitive?

Comment 18

[Michal Novotny [:michal]]

(In reply to Marcia Knous [:marcia] from comment #17)
> I was able to crash again, but I get a different signature: 
> https://crash-stats.mozilla.com/report/index/bp-29f53417-90cb-44c6-9459-
> c185f2111007
> 
> Should I file new bug for this crash and should it be security sensitive?

No, this is an intentional crash in OOM situation.

Comment 19

[Rh0]

(In reply to juan becerra [:juanb] from comment #16)
> I tried reproducing this problem on Fx5, 7.0.1, and the most recent Nightly
> on a Win7 machine. In all three the testcase crashes the application. It
> takes a while, but they all crash.
> 
> When I try to look at the crash reports, they can't be found:
> bp-1dbea091-1abf-448b-8fbb-313ad2111006
> 
> It could be that this is now crashing in a different place, but without
> seeing the crash signature I can't tell. I'll re-open as soon as the crash
> report shows up, or after consulting with someone in the security team.

With Fx7.0.1 in WinXP/2GB RAM running in the latest VirtualBox (4.1.4), I get the crash at memcpy:
bp-4ec14c4f-b56e-4e44-b194-520422111007

but also another one ( [@ js_NextActiveContext(JSRuntime*, JSContext*)] ):
bp-6015acf5-0c04-467a-88ad-365f82111007

Fx10.0a1 crashes as well, but I cannot get a signature, too:
bp-3f3d522c-282d-4f15-a872-f07152111007

In this case Firefox can be started in WinDBG (Windows Debugger) to check the crash. In my case I get for Fx10.0a1 this exception:

(f60.e58): Break instruction exception - code 80000003 (first chance)
eax=00000000 ebx=7d929740 ecx=7817a4c8 edx=781c3c70 esi=7813e457 edi=7817a392
eip=00a8193d esp=001275b8 ebp=7f9f9470 iopl=0         nv up ei pl nz na pe nc
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             efl=00000206
mozalloc!mozalloc_abort+0x2b:
00a8193d cc              int     3

And this call stack (obtained with command "kc 10" in WinDBG):

mozalloc!mozalloc_abort
mozalloc!mozalloc_handle_oom
xul!nsTArray_base<nsTArrayDefaultAllocator>::EnsureCapacity
xul!gfxTextRun::CopyGlyphDataFrom
xul!TextRunWordCache::LookupWord
xul!TextRunWordCache::MakeTextRun
xul!MakeTextRun
xul!BuildTextRunsScanner::BuildTextRunForFrames
xul!BuildTextRunsScanner::FlushFrames
xul!BuildTextRuns
xul!nsIFrame::InvalidateInternal
xul!nsIFrame::InvalidateInternal
xul!nsIFrame::InvalidateInternal
xul!nsBlockFrame::InvalidateInternal
xul!nsLayoutUtils::IntrinsicForContainer
xul!nsTableCellFrame::GetMinWidth


I think this a crash/break on purpose due to OOM.

Comment 20

[Johnny Stenback (:jst)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/433458808fbe
https://hg.mozilla.org/releases/mozilla-beta/rev/890a66b91702

Comment 21

[u279076]

Is the attached testcase only usable in a debug build? Can QA verify this fix on a "normal" build?

Comment 22

[Michal Novotny [:michal]]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #21)
> Is the attached testcase only usable in a debug build? Can QA verify this
> fix on a "normal" build?

Yes, and even with debug build the crash is not 100% reproducible. It depends on what is in the uninitialized buffer and if GC runs during the test.

Comment 23

[u279076]

I assume "yes" refers to whether or not this needs a debug build. Based on comment 22, I don't see how QA will be able to reliable verify this fix.

Comment 24

[Michal Novotny [:michal]]

Branch 1.9.2 isn't affected and doesn't need this patch. Member variable mValid seems to be set correctly in all 3 classes (xpc_qsDOMString, xpc_qsAString, xpc_qsACString).