Last Comment Bug 682727 - (CVE-2011-3652) Crash during datamove (ACCESS VIOLATION READ) [@ memcpy | nsAString_internal::Assign(unsigned short const*, unsigned int)]
(CVE-2011-3652)
: Crash during datamove (ACCESS VIOLATION READ) [@ memcpy | nsAString_internal:...
Status: RESOLVED FIXED
[sg:critical][oom][qa?]
:
Product: Core
Classification: Components
Component: Networking: Cache (show other bugs)
: Trunk
: x86 Windows XP
: -- critical (vote)
: mozilla10
Assigned To: Michal Novotny (:michal)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-28 11:09 PDT by Rh0
Modified: 2014-06-26 10:01 PDT (History)
8 users (show)
rforbes: sec‑bounty+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
wontfix
-
wontfix
+
fixed
+
fixed
+
fixed
unaffected


Attachments
testcase to crash Firefox 6 (372 bytes, text/html)
2011-08-28 11:09 PDT, Rh0
no flags Details
fix (1.65 KB, patch)
2011-09-28 03:01 PDT, Michal Novotny (:michal)
jst: review+
jaas: superreview+
blizzard: approval‑mozilla‑aurora+
blizzard: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Rh0 2011-08-28 11:09:57 PDT
Created attachment 556407 [details]
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:] 2011-08-28 12:41:51 PDT
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] 2011-09-01 13:23:17 PDT
Not realistic to squeeze into Firefox 7, targeting the next release.
Comment 4 Michal Novotny (:michal) 2011-09-28 03:01:36 PDT
Created attachment 563014 [details] [diff] [review]
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) 2011-09-28 03:02:50 PDT
Pushed to tryserver https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=d78227785476
Comment 6 Johnny Stenback (:jst, jst@mozilla.com) 2011-09-28 21:51:49 PDT
Comment on attachment 563014 [details] [diff] [review]
fix

Good find, Michal! r=jst
Comment 7 Josh Aas 2011-09-29 08:03:37 PDT
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, jst@mozilla.com) 2011-09-29 10:24:43 PDT
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) 2011-09-30 12:19:28 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/dd0cc50e9fe6
Comment 11 Marcia Knous [:marcia - use ni] 2011-10-06 14:02:31 PDT
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) 2011-10-06 14:28:25 PDT
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 2011-10-06 16:21:48 PDT
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 - use ni] 2011-10-06 16:42:41 PDT
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 - use ni] 2011-10-06 17:47:51 PDT
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] 2011-10-06 18:15:08 PDT
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 - use ni] 2011-10-07 11:53:08 PDT
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) 2011-10-07 13:00:01 PDT
(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 2011-10-07 14:59:08 PDT
(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 21 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-10-13 11:03:50 PDT
Is the attached testcase only usable in a debug build? Can QA verify this fix on a "normal" build?
Comment 22 Michal Novotny (:michal) 2011-10-17 03:03:57 PDT
(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 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-10-17 07:50:51 PDT
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) 2011-10-27 17:47:20 PDT
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).

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