Closed Bug 882865 (CVE-2013-1705) Opened 11 years ago Closed 11 years ago

ASAN heap-buffer-overflow (read 1) in cryptojs_interpret_key_gen_type

Categories

(Core :: Security: PSM, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25
Tracking Status
firefox23 --- verified
firefox24 --- verified
firefox25 --- verified
firefox-esr17 24+ verified
b2g18 24+ fixed
b2g-v1.1hd --- fixed

People

(Reporter: nils, Assigned: keeler)

Details

(Keywords: csectype-bounds, sec-critical, Whiteboard: [adv-main23+][adv-esr1709+])

Attachments

(4 files, 5 obsolete files)

The following JavaScript code crashes Firefox Nighly:

o200 = document.documentElement;
o1 = crypto;
try{o1.generateCRMFRequest(o200.writeln,o200,'X X X X X X X X X X X X X X X X X X X X X X X X X X X X X X X X X X X X X X',null,o1,1404343237,Math.PI,[]);}catch(e){}

Asan:

==26839== ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7f76344d8c3f at pc 0x7f76595e4d0a bp 0x7fffeab94e80 sp 0x7fffeab94e78
READ of size 1 at 0x7f76344d8c3f thread T0
    #0 0x7f76595e4d09 in cryptojs_interpret_key_gen_type(char*) /builds/slave/m-cen-l64-dbg-asan-ntly-000000/build/security/manager/ssl/src/nsCrypto.cpp:360:0
    #1 0x7f76595dabe2 in cryptojs_ReadArgsAndGenerateKey(JSContext*, JS::Value*, nsKeyPairInfoStr*, nsIInterfaceRequestor*, PK11SlotInfoStr**, bool) /builds/slave/m-cen-l64-dbg-asan-ntly-000000/build/security/manager/ssl/src/nsCrypto.cpp:947:0
    #2 0x7f76595da0fc in nsCrypto::GenerateCRMFRequest(nsIDOMCRMFObject**) /builds/slave/m-cen-l64-dbg-asan-ntly-000000/build/security/manager/ssl/src/nsCrypto.cpp:1980:0
    #3 0x7f765a66fb05 in NS_InvokeByIndex /builds/slave/m-cen-l64-dbg-asan-ntly-000000/build/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:162:0
    #4 0x7f76592c4c63 in CallMethodHelper::Call() /builds/slave/m-cen-l64-dbg-asan-ntly-000000/build/js/xpconnect/src/XPCWrappedNative.cpp:2267:0
    #5 0x7f76592c4913 in XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) /builds/slave/m-cen-l64-dbg-asan-ntly-000000/build/js/xpconnect/src/XPCWrappedNative.cpp:2233:0
    #6 0x7f76592d9be7 in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) /builds/slave/m-cen-l64-dbg-asan-ntly-000000/build/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1480:0
    #7 0x7f765b8f4c48 in js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) /builds/slave/m-cen-l64-dbg-asan-ntly-000000/build/js/src/jscntxtinlines.h:349:0
    #8 0x7f765b8f4395 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) /builds/slave/m-cen-l64-dbg-asan-ntly-000000/build/js/src/vm/Interpreter.cpp:381:0
    #9 0x7f765b8ee7fa in
    #10 0x7f765b8e2adc in js::RunScript(JSContext*, js::StackFrame*) /builds/slave/m-cen-l64-dbg-asan-ntly-000000/build/js/src/vm/Interpreter.cpp:345:0
    #11 0x7f765b8f65a4 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::AbstractFramePtr, JS::Value*) /builds/slave/m-cen-l64-dbg-asan-ntly-000000/build/js/src/vm/Interpreter.cpp:530:0
    #12 0x7f765b8f6bf7 in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) /builds/slave/m-cen-l64-dbg-asan-ntly-000000/build/js/src/vm/Interpreter.cpp:569:0
    #13 0x7f765ba75dcc in JS::Evaluate(JSContext*, JS::Handle<JSObject*>, JS::CompileOptions, unsigned short const*, unsigned long, JS::Value*) /builds/slave/m-cen-l64-dbg-asan-ntly-000000/build/js/src/jsapi.cpp:5689:0
    #14 0x7f7658a2d433 in nsJSContext::EvaluateString(nsAString_internal const&, JS::Handle<JSObject*>, JS::CompileOptions&, bool, JS::Value*) /builds/slave/m-cen-l64-dbg-asan-ntly-000000/build/dom/base/nsJSEnvironment.cpp:1278:0
    #15 0x7f76589eecb9 in nsGlobalWindow::RunTimeoutHandler(nsTimeout*, nsIScriptContext*) /builds/slave/m-cen-l64-dbg-asan-ntly-000000/build/dom/base/nsGlobalWindow.cpp:10193:0
    #16 0x7f76589db5cc in nsGlobalWindow::RunTimeout(nsTimeout*) /builds/slave/m-cen-l64-dbg-asan-ntly-000000/build/dom/base/nsGlobalWindow.cpp:10437:0
    #17 0x7f76589ee1f7 in nsGlobalWindow::TimerCallback(nsITimer*, void*) /builds/slave/m-cen-l64-dbg-asan-ntly-000000/build/dom/base/nsGlobalWindow.cpp:10684:0
    #18 0x7f765a64041c in nsTimerImpl::Fire() /builds/slave/m-cen-l64-dbg-asan-ntly-000000/build/xpcom/threads/nsTimerImpl.cpp:543:0
    #19 0x7f765a640c9c in nsTimerEvent::Run() /builds/slave/m-cen-l64-dbg-asan-ntly-000000/build/xpcom/threads/nsTimerImpl.cpp:627:0
    #20 0x7f765a63728b in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/m-cen-l64-dbg-asan-ntly-000000/build/xpcom/threads/nsThread.cpp:626:0
    #21 0x7f765a583931 in NS_ProcessNextEvent(nsIThread*, bool) /builds/slave/m-cen-l64-dbg-asan-ntly-000000/build/obj-firefox/xpcom/build/nsThreadUtils.cpp:238:0
    #22 0x7f7659b3305b in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/slave/m-cen-l64-dbg-asan-ntly-000000/build/ipc/glue/MessagePump.cpp:82:0
    #23 0x7f765a6e4051 in MessageLoop::RunInternal() /builds/slave/m-cen-l64-dbg-asan-ntly-000000/build/ipc/chromium/src/base/message_loop.cc:219:0
    #24 0x7f765a6e3f4e in MessageLoop::Run() /builds/slave/m-cen-l64-dbg-asan-ntly-000000/build/ipc/chromium/src/base/message_loop.cc:186:0
    #25 0x7f7659986851 in nsBaseAppShell::Run() /builds/slave/m-cen-l64-dbg-asan-ntly-000000/build/widget/xpwidgets/nsBaseAppShell.cpp:163:0
    #26 0x7f765952b67f in nsAppStartup::Run() /builds/slave/m-cen-l64-dbg-asan-ntly-000000/build/toolkit/components/startup/nsAppStartup.cpp:269:0
    #27 0x7f7657304a39 in XREMain::XRE_mainRun() /builds/slave/m-cen-l64-dbg-asan-ntly-000000/build/toolkit/xre/nsAppRunner.cpp:3851:0
    #28 0x7f7657305d77 in XREMain::XRE_main(int, char**, nsXREAppData const*) /builds/slave/m-cen-l64-dbg-asan-ntly-000000/build/toolkit/xre/nsAppRunner.cpp:3919:0
    #29 0x7f7657306701 in XRE_main /builds/slave/m-cen-l64-dbg-asan-ntly-000000/build/toolkit/xre/nsAppRunner.cpp:4121:0
    #30 0x40c7e6 in do_main(int, char**, nsIFile*) /builds/slave/m-cen-l64-dbg-asan-ntly-000000/build/browser/app/nsBrowserApp.cpp:272:0
    #31 0x40bd0f in main /builds/slave/m-cen-l64-dbg-asan-ntly-000000/build/browser/app/nsBrowserApp.cpp:632:0
    #32 0x7f766411fea4 in ?? ??:0
0x7f76344d8c3f is located 1 bytes to the left of 1-byte region [0x7f76344d8c40,0x7f76344d8c41)
allocated by thread T0 here:
    #0 0x43b1a0 in malloc ??:?
    #1 0x7f765b89bf59 in js::MallocProvider<JSContext>::malloc_(unsigned long) /builds/slave/m-cen-l64-dbg-asan-ntly-000000/build/js/src/jscntxt.h:558:0
    #2 0x7f765c1811d7 in JS::LossyTwoByteCharsToNewLatin1CharsZ(JSContext*, JS::TwoByteChars) /builds/slave/m-cen-l64-dbg-asan-ntly-000000/build/js/src/vm/CharacterEncoding.cpp:21:0
    #3 0x7f765ba7afeb in JS_EncodeString(JSContext*, JSString*) /builds/slave/m-cen-l64-dbg-asan-ntly-000000/build/js/src/jsapi.cpp:6293:0
    #4 0x7f76592298c8 in JSAutoByteString::encodeLatin1(JSContext*, JSString*) /builds/slave/m-cen-l64-dbg-asan-ntly-000000/build/js/src/jsapi.h:4431:0
    #5 0x7f76595daba5 in cryptojs_ReadArgsAndGenerateKey(JSContext*, JS::Value*, nsKeyPairInfoStr*, nsIInterfaceRequestor*, PK11SlotInfoStr**, bool) /builds/slave/m-cen-l64-dbg-asan-ntly-000000/build/security/manager/ssl/src/nsCrypto.cpp:945:0
    #6 0x7f76595da0fc in nsCrypto::GenerateCRMFRequest(nsIDOMCRMFObject**) /builds/slave/m-cen-l64-dbg-asan-ntly-000000/build/security/manager/ssl/src/nsCrypto.cpp:1980:0
    #7 0x7f765a66fb05 in NS_InvokeByIndex /builds/slave/m-cen-l64-dbg-asan-ntly-000000/build/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:162:0
    #8 0x7f76592c4c63 in CallMethodHelper::Call() /builds/slave/m-cen-l64-dbg-asan-ntly-000000/build/js/xpconnect/src/XPCWrappedNative.cpp:2267:0
Shadow byte and word:
  0x1feec689b187: fa
  0x1feec689b180: fa fa fa fa fa fa fa fa
More shadow bytes:
  0x1feec689b160: fa fa fa fa fa fa fa fa
  0x1feec689b168: 00 00 00 00 00 fb fb fb
  0x1feec689b170: fa fa fa fa fa fa fa fa
  0x1feec689b178: 00 00 02 fb fb fb fb fb
=>0x1feec689b180: fa fa fa fa fa fa fa fa
  0x1feec689b188: 01 fb fb fb fb fb fb fb
  0x1feec689b190: fa fa fa fa fa fa fa fa
  0x1feec689b198: fa fa fa fa fa fa fa fa
  0x1feec689b1a0: fa fa fa fa fa fa fa fa
Stats: 231M malloced (213M for red zones) by 286553 calls
Stats: 34M realloced by 16332 calls
Stats: 202M freed by 148988 calls
Stats: 164M really freed by 111061 calls
Stats: 228M (58432 full pages) mmaped in 430 calls
  mmaps   by size class: 7:110565; 8:47081; 9:15345; 10:8176; 11:7395; 12:1280; 13:960; 14:512; 15:192; 16:656; 17:456; 18:26; 19:36; 20:21; 21:1;
  mallocs by size class: 7:161389; 8:61615; 9:24493; 10:17457; 11:12992; 12:2255; 13:1936; 14:1445; 15:341; 16:1164; 17:1366; 18:35; 19:41; 20:22; 21:2;
  frees   by size class: 7:75283; 8:24634; 9:16504; 10:14756; 11:11180; 12:1355; 13:1293; 14:1281; 15:233; 16:1028; 17:1352; 18:30; 19:37; 20:21; 21:1;
  rfrees  by size class: 7:56504; 8:16923; 9:10444; 10:12103; 11:9578; 12:1020; 13:995; 14:1136; 15:180; 16:804; 17:1323; 18:27; 19:22; 20:1; 21:1;
Stats: malloc large: 2971 small slow: 4402
Flags: needinfo?(bsmith)
Summary: ASAN heap-buffer-overflow in cryptojs_interpret_key_gen_type → ASAN heap-buffer-overflow (read 1) in cryptojs_interpret_key_gen_type
Component: Security → Security: PSM
Flags: needinfo?(bsmith)
I may need some help from our Red Hat friends on this one, as I am quite unfamiliar with the nsCrypto code.
Assignee: nobody → bsmith
Assignee: bsmith → mhamrick
Kai: any idea how severe this might be? without knowing the code it could lead to exploitable memory corruption, bad data corrupting a key (weakening crypto?), or maybe just benign. Although ASAN stopped the process after it read the first bogus byte, how much further would it have kept reading?
Flags: needinfo?(kaie)
This looks like an underflow:

346 static nsKeyGenType
347 cryptojs_interpret_key_gen_type(char *keyAlg)
348 {
349   char *end;
350   if (!keyAlg) {
351     return invalidKeyGen;
352   }
353   /* First let's remove all leading and trailing white space */
354   while (isspace(keyAlg[0])) keyAlg++;
355   end = strchr(keyAlg, '\0');
356   if (!end) {
357     return invalidKeyGen;
358   }
359   end--;
360   while (isspace(*end)) end--;
361   end[1] = '\0';

keyAlg is passed in as an empty string (i.e. a pointer to a null character).
end winds up being equal to keyAlg, whereupon it gets decremented and dereferenced, thus underflowing the allocated space. Then, if the byte before keyAlg is a space (e.g. 32), this will overwrite that with a 0. I'm not familiar with our heap data structures, but if that byte is important, this could be pretty bad.
Attached patch fix (obsolete) — Splinter Review
Do we do automated asan builds? If not, I'm not sure there's a way to automate testing this...
Attachment #769838 - Flags: review?(brian)
We do for Linux64:
https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/

The security team does run our regression suites under asan periodically (with the hope of eventually making this a standard automated test run) so it would be worth while checking in a testcase even if it doesn't show much except under ASAN. Eventually we may want to create an ASAN-only group of tests that we don't bother running except in builds that might find something.
Attached patch patch and test (obsolete) — Splinter Review
Here's the patch with a test.
Assignee: mhamrick → dkeeler
Attachment #769838 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #769838 - Flags: review?(brian)
Attachment #771002 - Flags: review?(brian)
Comment on attachment 771002 [details] [diff] [review]
patch and test

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

::: security/manager/ssl/src/nsCrypto.cpp
@@ +357,5 @@
>      return invalidKeyGen;
>    }
>    end--;
>    while (isspace(*end)) end--;
>    end[1] = '\0';

The fix you made seems correct, technically.

However, the above is a new implementation of ns[AC]String::trim. Why not just use nsString::trim itself? After all, the root cause of this bug is the fact that we're doing pointer arithmetic in the first place.

In particular, it seems like you should be able to do something like this in the caller:

nsDepenentJSString dependentKeyAlg;
NS_ENSURE_SUCCESS(dependentKeyAlg.init(cx, jsString),
                  NS_ERROR_UNEXPECTED);
nsAutoString keyAlg(dependentKeyAlg);
keyAlg.Trim();

and then, given a change in parameter type to "const nsAutoString & keyAlg" in this function:

if (keyAlg == NS_LITERAL_STRING("rsa-ex")) {
   ...
} else if (keyAlg == NS_LITERAL_STRING("rsa-dual-use")) {
   ...
}

Then we would be fully-bounds-checked and memory-error-free code.

Does this sound reasonable to you?
Attachment #771002 - Flags: review?(brian)
Attached patch patch v2 (obsolete) — Splinter Review
Good call. Here's the updated patch.
Attachment #771002 - Attachment is obsolete: true
Attachment #772764 - Flags: review?(brian)
Comment on attachment 772764 [details] [diff] [review]
patch v2

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

Ms2ger: please review the use of the JS API.

::: security/manager/ssl/src/nsCrypto.cpp
@@ +930,5 @@
>    jsString = JS_ValueToString(cx, argv[2]);
>    NS_ENSURE_TRUE(jsString, NS_ERROR_OUT_OF_MEMORY);
>    argv[2] = STRING_TO_JSVAL(jsString);
> +  nsDependentJSString dependentKeyGenAlg;
> +  NS_ENSURE_TRUE(dependentKeyGenAlg.init(cx, jsString), NS_ERROR_UNEXPECTED);

Sorry, my suggestion was sloppy. This should be:
 
  rv = dependentKeyGenAlg.init(cx, jsString)
  NS_ENSURE_SUCCESS(rv, rv);

so that we propogate the correct error.

@@ +933,5 @@
> +  nsDependentJSString dependentKeyGenAlg;
> +  NS_ENSURE_TRUE(dependentKeyGenAlg.init(cx, jsString), NS_ERROR_UNEXPECTED);
> +  nsAutoString keyGenAlg(dependentKeyGenAlg);
> +  // Remove whitespace from the beginning and ending of the string
> +  keyGenAlg.CompressWhitespace(true, true);

Nit: you can omit the "true, true" as those are the defaults.

@@ +939,4 @@
>    if (keyGenType->keyGenType == invalidKeyGen) {
>      JS_ReportError(cx, "%s%s%s", JS_ERROR,
>                     "invalid key generation argument:",
> +                   dependentKeyGenAlg.Data());

dependentKeyGenAlg.Data() will be a 16-bit character sequence, but %s expects 8-bit characters. I would ask on #developers if there is a way to pass a 16-bit-character string to JS_ReportError. Or, perhaps we just shouldn't include the keyGenAlg text at all in the error message.

Also, is dependentKeyGenAlg.Data() guaranteed to be null-terminated?

@@ +953,5 @@
>  
>    if (rv != NS_OK) {
>      JS_ReportError(cx,"%s%s%s", JS_ERROR,
>                     "could not generate the key for algorithm ",
> +                   dependentKeyGenAlg.Data());

ditto.
Attachment #772764 - Flags: superreview+
Attachment #772764 - Flags: review?(brian)
Attachment #772764 - Flags: review?(Ms2ger)
(In reply to Brian Smith (:briansmith), was bsmith@mozilla.com (:bsmith) from comment #9)
> Comment on attachment 772764 [details] [diff] [review]
> patch v2
> 
> Review of attachment 772764 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ms2ger: please review the use of the JS API.
> 
> ::: security/manager/ssl/src/nsCrypto.cpp
> @@ +930,5 @@
> >    jsString = JS_ValueToString(cx, argv[2]);
> >    NS_ENSURE_TRUE(jsString, NS_ERROR_OUT_OF_MEMORY);
> >    argv[2] = STRING_TO_JSVAL(jsString);
> > +  nsDependentJSString dependentKeyGenAlg;
> > +  NS_ENSURE_TRUE(dependentKeyGenAlg.init(cx, jsString), NS_ERROR_UNEXPECTED);
> 
> Sorry, my suggestion was sloppy. This should be:
>  
>   rv = dependentKeyGenAlg.init(cx, jsString)
>   NS_ENSURE_SUCCESS(rv, rv);
> 
> so that we propogate the correct error.

init() returns a boolean.
Comment on attachment 772764 [details] [diff] [review]
patch v2

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

::: security/manager/ssl/src/nsCrypto.cpp
@@ +344,5 @@
>   * and translates it to the internal enumeration representing the
>   * key gen type.
>   */
>  static nsKeyGenType
> +cryptojs_interpret_key_gen_type(const nsAutoString& keyAlg)

I'd just take const nsAString& here

@@ +349,2 @@
>  {
> +  if (keyAlg == NS_LITERAL_STRING("rsa-ex")) {

Why not keyAlg.EqualsLiteral()?

@@ +351,2 @@
>      return rsaEnc;
> +  } else if (keyAlg == NS_LITERAL_STRING("rsa-dual-use")) {

(Pre-existing: we don't like else-after-return)

@@ +929,5 @@
>    }
>    jsString = JS_ValueToString(cx, argv[2]);
>    NS_ENSURE_TRUE(jsString, NS_ERROR_OUT_OF_MEMORY);
>    argv[2] = STRING_TO_JSVAL(jsString);
> +  nsDependentJSString dependentKeyGenAlg;

This changes the behaviour when passing non-ASCII, I think. I can't judge if that's fine.

@@ +939,4 @@
>    if (keyGenType->keyGenType == invalidKeyGen) {
>      JS_ReportError(cx, "%s%s%s", JS_ERROR,
>                     "invalid key generation argument:",
> +                   dependentKeyGenAlg.Data());

I wouldn't rely on the null-termination, no.
Attachment #772764 - Flags: review?(Ms2ger)
(In reply to :Ms2ger from comment #11)
> ::: security/manager/ssl/src/nsCrypto.cpp
> @@ +344,5 @@
> >   * and translates it to the internal enumeration representing the
> >   * key gen type.
> >   */
> >  static nsKeyGenType
> > +cryptojs_interpret_key_gen_type(const nsAutoString& keyAlg)
> 
> I'd just take const nsAString& here

I think "const nsAutoString &" is better because it probably helps (and doesn't hurt) the compiler's ability to optimize the code (for size, particularly).

> 
> @@ +349,2 @@
> >  {
> > +  if (keyAlg == NS_LITERAL_STRING("rsa-ex")) {
> 
> Why not keyAlg.EqualsLiteral()?

I guess EqualsLiteral/EqualsASCII is possibly more efficient. Cool.

> @@ +929,5 @@
> >    }
> >    jsString = JS_ValueToString(cx, argv[2]);
> >    NS_ENSURE_TRUE(jsString, NS_ERROR_OUT_OF_MEMORY);
> >    argv[2] = STRING_TO_JSVAL(jsString);
> > +  nsDependentJSString dependentKeyGenAlg;
> 
> This changes the behaviour when passing non-ASCII, I think. I can't judge if
> that's fine.

I think this is a non-issue we later ensure that the value matches an item in a fixed whitelist of ASCII strings.
Attached patch patch v3 (obsolete) — Splinter Review
Ok - I think I'm properly handling the wide/narrow conversion in the error reporting now.
Brian - how does using nsAutoString over nsAString help the compiler optimize?
Attachment #772764 - Attachment is obsolete: true
Attachment #774270 - Flags: review?(brian)
Flags: needinfo?(kaie)
Comment on attachment 774270 [details] [diff] [review]
patch v3

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

::: security/manager/ssl/src/nsCrypto.cpp
@@ +340,4 @@
>  }
>  
>  /*
>   * This function converts a string read through JavaScript parameters

Update the comment to note that it should have been trimmed

@@ +945,5 @@
>    argv[2] = STRING_TO_JSVAL(jsString);
> +  nsDependentJSString dependentKeyGenAlg;
> +  NS_ENSURE_TRUE(dependentKeyGenAlg.init(cx, jsString), NS_ERROR_UNEXPECTED);
> +  nsAutoString keyGenAlg(dependentKeyGenAlg);
> +  // Remove whitespace from the beginning and ending of the string

This comment doesn't add anything.

@@ +946,5 @@
> +  nsDependentJSString dependentKeyGenAlg;
> +  NS_ENSURE_TRUE(dependentKeyGenAlg.init(cx, jsString), NS_ERROR_UNEXPECTED);
> +  nsAutoString keyGenAlg(dependentKeyGenAlg);
> +  // Remove whitespace from the beginning and ending of the string
> +  keyGenAlg.CompressWhitespace();

Did you mean to Trim()? CompressWhitespace also changes internal whitespace. That doesn't matter, I guess, as any string with internal whitespace will be rejected anyway.

@@ +952,2 @@
>    if (keyGenType->keyGenType == invalidKeyGen) {
> +    char *keyGenAlgNarrow = JS_EncodeString(cx, jsString);

You have to null-check this. However, it looks equivalent to...

NS_LossyConvertUTF16toASCII keyGenAlgNarrow(dependentKeyGenAlg);
JS_ReportError(cx, "%s%s%s", JS_ERROR,
               "invalid key generation argument:",
               keyGenAlgNarrow.get());
goto loser;

::: security/manager/ssl/tests/mochitest/bugs/test_bug882865.html
@@ +27,5 @@
> +    try {
> +      o1.generateCRMFRequest(o200.writeln, o200, 'X X X X X X X X X X X X X X X X X X X X X X X X X X X X X X X X X X X X X X', null, o1, 1404343237, Math.PI, []);
> +      ok(false, "execution should not reach this line");
> +    } catch (e) {
> +      is(e.toString(), "Error: error:invalid key generation argument:", "expected exception");

How does this not have the argument?
Attachment #774270 - Flags: review?(Ms2ger)
Attached patch patch v4 (obsolete) — Splinter Review
Thanks for the review. All of your comments should be addressed.
Attachment #774270 - Attachment is obsolete: true
Attachment #774270 - Flags: review?(brian)
Attachment #774748 - Flags: review?(Ms2ger)
Comment on attachment 774748 [details] [diff] [review]
patch v4

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

JSAPI-wise, this looks okay.
Attachment #774748 - Flags: review?(Ms2ger) → review+
If it's only the one-byte underflow it doesn't sound very severe
Attachment #774748 - Flags: review?(brian) → review+
Daniel: I wouldn't dismiss it as not very severe because it is only one byte, especially with the control over the heap that Javascript allows us to have. I will try to look into the heap structures a bit further to see whether this could be exploited.
Looks like MOZ_DISABLE_CRYPTOLEGACY is defined on Android and b2g, so crypto.generateCRMFRequest isn't available there.

https://tbpl.mozilla.org/?tree=Try&rev=49603ae4f2bd
Attached patch patch v4.1Splinter Review
Makefile.in changes to prevent running the test on platforms where crypto.generateCRMFRequest doesn't exist. Carrying over r+.
Attachment #774748 - Attachment is obsolete: true
Attachment #778571 - Flags: review+
For the record, khuey r+'d this Makefile.in change on irc.
Quick and dirty PoC code in test.html which shows that this shouldn't be sec-moderate.

Firefox uses the jemalloc heap, my understanding is that the single blocks don't have heap metadata prepended and instead blocks of the same size are allocated directly next to each other. That means that this vulnerability can corrupt single bytes from adjacent blocks (these bytes might be pointers). We are able to choose the block size by allocating arbitrary amount of spaces, thus we should be able to corrupt arbitrary blocks allocated in the same thread.

test.html triggers many corruptions repeatedly to determine how likely memory corruptions will be that lead to other crashes. By loading test.html in one tab and trying to browse websites in a second thread, we can trigger a lot of different crashes, some of which look exploitable.
Comment on attachment 778571 [details] [diff] [review]
patch v4.1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): longstanding bug
User impact if declined: potentially exploitable memory corruption
Testing completed (on m-c, etc.): landed on mozilla-inbound already
Risk to taking this patch (and alternatives if risky): low risk because virtual nobody other than people looking to exploit the browser use this API. Also, the changes are minimal.
String or IDL/UUID changes made by this patch: none

Based on Nils latest comment, we may be zero-daying ourselves by having landed this on inbound, so it seems like a good idea to uplift this.
Attachment #778571 - Flags: approval-mozilla-beta?
Attachment #778571 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/3b7d3727478a
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Comment on attachment 778571 [details] [diff] [review]
patch v4.1

Just say no to zero daying ourselves, let's get this into this week's Betas.
Attachment #778571 - Flags: approval-mozilla-beta?
Attachment #778571 - Flags: approval-mozilla-beta+
Attachment #778571 - Flags: approval-mozilla-aurora?
Attachment #778571 - Flags: approval-mozilla-aurora+
Whiteboard: [adv-main23+]
Alias: CVE-2013-1705
Sigh. And we checked in the test, thus showing the world how to cause the UAF.

Note: we normally check in tests for security issues no less than six weeks after a public release with the fix.
Based on comment 25 and others, I wonder if this bug is mis-rated as a sec-moderate. This sounds at least sec-high. Anyone have any opinions on this besides Nils?
(In reply to Al Billings [:abillings] from comment #31)
> Sigh. And we checked in the test, thus showing the world how to cause the
> UAF.
> 
> Note: we normally check in tests for security issues no less than six weeks
> after a public release with the fix.

Looks like I misunderstood the guidelines in https://wiki.mozilla.org/Security/Bug_Approval_Process - maybe it should be a little more explicit about what gets checked in to where when.
Attachment #778848 - Attachment mime type: text/plain → text/html
Clearly seeing critical-type crashes out of attachment 778848 [details]

As Brian said in comment 27 "longstanding bug" -- here's some for ESR 17
bp-1226c52b-7c41-4b3a-ab90-f35642130806
bp-f7c95e3d-64e3-4f27-8500-d8eec2130806
Flags: sec-bounty?
Flags: sec-bounty?
This got flagged the day before the 23 release for ESR17 and should have gone in the last release, tracking for this current release - please nominate the patch for uplift if it applies cleanly or provide an ESR branch patch.
Flags: needinfo?(dkeeler)
Attached patch patch for esrSplinter Review
The original patch didn't apply cleanly to esr.
Additionally, I had to make a few changes that I'd like someone who knows JS to have a look at:
- I had to add nsJSUtils.h to the EXPORTS in dom/base/Makefile.in
- JS_ReportError wasn't doing what it used to - instead of reporting a nice string to the user, I'm seeing a generic NS_ERROR_FAILURE. I changed the test to reflect this, and I think this is probably okay, but I'd like to know if I'm missing something obvious and easy.
Attachment #796826 - Flags: review?(wmccloskey)
Flags: needinfo?(dkeeler)
Comment on attachment 796826 [details] [diff] [review]
patch for esr

I don't know why JS_ReportError would have changed behavior. This seems fine though.
Attachment #796826 - Flags: review?(wmccloskey) → review+
Comment on attachment 796826 [details] [diff] [review]
patch for esr

Thanks for the quick review!

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: is sec-critical
User impact if declined: crashes, maybe even remote code execution
Fix Landed on Version: 25
Risk to taking this patch (and alternatives if risky): could break crypto.generateCRMFRequest, but this patch has been on central for a while with no indication that this has happened
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #796826 - Flags: approval-mozilla-esr17?
Comment on attachment 796826 [details] [diff] [review]
patch for esr

Thanks for the updated version!
Attachment #796826 - Flags: approval-mozilla-esr17? → approval-mozilla-esr17+
Confirmed crash on ASan build 2013-06-06.

Verified fix on ASan builds of FF24, FF25 from 2013-09-11.
Whiteboard: [adv-main23+] → [adv-main23+][adv-esr1709+]
NO :keeler to help with a backport patch for mozilla-b2g18.
Flags: needinfo?(dkeeler)
Attached patch patch for b2g18Splinter Review
NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): longstanding bug
User impact if declined: crashes, potentially exploitable
Testing completed: on m-c, esr, etc.
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none
Attachment #817498 - Flags: review+
Attachment #817498 - Flags: approval-mozilla-b2g18?
Flags: needinfo?(dkeeler)
Attachment #817498 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Group: core-security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: