Last Comment Bug 882865 - (CVE-2013-1705) ASAN heap-buffer-overflow (read 1) in cryptojs_interpret_key_gen_type
(CVE-2013-1705)
: ASAN heap-buffer-overflow (read 1) in cryptojs_interpret_key_gen_type
Status: RESOLVED FIXED
[adv-main23+][adv-esr1709+]
: csectype-bounds, sec-critical
Product: Core
Classification: Components
Component: Security: PSM (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla25
Assigned To: David Keeler [:keeler] (use needinfo?)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-06-13 12:39 PDT by Nils
Modified: 2014-11-19 20:03 PST (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified
verified
verified
24+
verified
24+
fixed
fixed


Attachments
fix (1.05 KB, patch)
2013-07-01 14:07 PDT, David Keeler [:keeler] (use needinfo?)
no flags Details | Diff | Review
patch and test (2.81 KB, patch)
2013-07-03 13:36 PDT, David Keeler [:keeler] (use needinfo?)
no flags Details | Diff | Review
patch v2 (8.38 KB, patch)
2013-07-09 11:19 PDT, David Keeler [:keeler] (use needinfo?)
brian: superreview+
Details | Diff | Review
patch v3 (8.36 KB, patch)
2013-07-11 14:36 PDT, David Keeler [:keeler] (use needinfo?)
no flags Details | Diff | Review
patch v4 (8.61 KB, patch)
2013-07-12 10:15 PDT, David Keeler [:keeler] (use needinfo?)
Ms2ger: review+
brian: review+
Details | Diff | Review
patch v4.1 (8.86 KB, patch)
2013-07-19 11:03 PDT, David Keeler [:keeler] (use needinfo?)
dkeeler: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Review
testcase to crash non asan browser (445 bytes, text/html)
2013-07-20 06:30 PDT, Nils
no flags Details
patch for esr (9.38 KB, patch)
2013-08-28 12:13 PDT, David Keeler [:keeler] (use needinfo?)
wmccloskey: review+
lukasblakk+bugs: approval‑mozilla‑esr17+
Details | Diff | Review
patch for b2g18 (8.79 KB, patch)
2013-10-15 15:47 PDT, David Keeler [:keeler] (use needinfo?)
dkeeler: review+
bajaj.bhavana: approval‑mozilla‑b2g18+
Details | Diff | Review

Description Nils 2013-06-13 12:39:24 PDT
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
Comment 1 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-06-19 16:11:31 PDT
I may need some help from our Red Hat friends on this one, as I am quite unfamiliar with the nsCrypto code.
Comment 2 Daniel Veditz [:dveditz] 2013-06-26 10:24:03 PDT
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?
Comment 3 David Keeler [:keeler] (use needinfo?) 2013-07-01 11:51:53 PDT
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.
Comment 4 David Keeler [:keeler] (use needinfo?) 2013-07-01 14:07:56 PDT
Created attachment 769838 [details] [diff] [review]
fix

Do we do automated asan builds? If not, I'm not sure there's a way to automate testing this...
Comment 5 Daniel Veditz [:dveditz] 2013-07-03 10:15:47 PDT
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.
Comment 6 David Keeler [:keeler] (use needinfo?) 2013-07-03 13:36:04 PDT
Created attachment 771002 [details] [diff] [review]
patch and test

Here's the patch with a test.
Comment 7 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-07-07 13:38:00 PDT
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?
Comment 8 David Keeler [:keeler] (use needinfo?) 2013-07-09 11:19:27 PDT
Created attachment 772764 [details] [diff] [review]
patch v2

Good call. Here's the updated patch.
Comment 9 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-07-09 11:31:33 PDT
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.
Comment 10 :Ms2ger 2013-07-09 11:36:17 PDT
(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 11 :Ms2ger 2013-07-09 11:45:15 PDT
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.
Comment 12 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-07-09 14:07:24 PDT
(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.
Comment 13 David Keeler [:keeler] (use needinfo?) 2013-07-11 14:36:03 PDT
Created attachment 774270 [details] [diff] [review]
patch v3

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?
Comment 14 :Ms2ger 2013-07-12 02:13:50 PDT
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?
Comment 15 David Keeler [:keeler] (use needinfo?) 2013-07-12 10:15:27 PDT
Created attachment 774748 [details] [diff] [review]
patch v4

Thanks for the review. All of your comments should be addressed.
Comment 16 :Ms2ger 2013-07-13 03:07:57 PDT
Comment on attachment 774748 [details] [diff] [review]
patch v4

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

JSAPI-wise, this looks okay.
Comment 17 Daniel Veditz [:dveditz] 2013-07-17 10:51:40 PDT
If it's only the one-byte underflow it doesn't sound very severe
Comment 18 Nils 2013-07-18 00:44:23 PDT
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.
Comment 19 David Keeler [:keeler] (use needinfo?) 2013-07-18 13:45:18 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5a5d2c176f7
Comment 20 David Keeler [:keeler] (use needinfo?) 2013-07-18 15:57:45 PDT
Backed out for Android test failures: https://hg.mozilla.org/integration/mozilla-inbound/rev/5955cd778745
Comment 21 David Keeler [:keeler] (use needinfo?) 2013-07-19 10:57:42 PDT
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
Comment 22 David Keeler [:keeler] (use needinfo?) 2013-07-19 11:03:36 PDT
Created attachment 778571 [details] [diff] [review]
patch v4.1

Makefile.in changes to prevent running the test on platforms where crypto.generateCRMFRequest doesn't exist. Carrying over r+.
Comment 23 David Keeler [:keeler] (use needinfo?) 2013-07-19 11:09:53 PDT
For the record, khuey r+'d this Makefile.in change on irc.
Comment 24 David Keeler [:keeler] (use needinfo?) 2013-07-19 14:52:55 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b7d3727478a
Comment 25 Nils 2013-07-20 06:29:40 PDT
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 26 Nils 2013-07-20 06:30:13 PDT
Created attachment 778848 [details]
testcase to crash non asan browser
Comment 27 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-07-20 11:56:53 PDT
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.
Comment 28 Ed Morley [:emorley] 2013-07-22 08:53:17 PDT
https://hg.mozilla.org/mozilla-central/rev/3b7d3727478a
Comment 29 Lukas Blakk [:lsblakk] use ?needinfo 2013-07-22 10:09:20 PDT
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.
Comment 31 Al Billings [:abillings] 2013-07-30 17:11:38 PDT
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.
Comment 32 Al Billings [:abillings] 2013-07-30 17:17:42 PDT
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?
Comment 33 David Keeler [:keeler] (use needinfo?) 2013-07-30 17:18:26 PDT
(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.
Comment 34 Daniel Veditz [:dveditz] 2013-08-05 23:11:04 PDT
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
Comment 37 Lukas Blakk [:lsblakk] use ?needinfo 2013-08-28 09:09:01 PDT
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.
Comment 38 David Keeler [:keeler] (use needinfo?) 2013-08-28 12:13:59 PDT
Created attachment 796826 [details] [diff] [review]
patch for esr

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.
Comment 39 Bill McCloskey (:billm) 2013-08-28 12:26:20 PDT
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.
Comment 40 David Keeler [:keeler] (use needinfo?) 2013-08-28 13:08:57 PDT
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.
Comment 41 Lukas Blakk [:lsblakk] use ?needinfo 2013-09-02 16:41:33 PDT
Comment on attachment 796826 [details] [diff] [review]
patch for esr

Thanks for the updated version!
Comment 42 Ryan VanderMeulen [:RyanVM] 2013-09-03 05:45:21 PDT
https://hg.mozilla.org/releases/mozilla-esr17/rev/388256444758
Comment 43 Matt Wobensmith [:mwobensmith][:matt:] 2013-09-11 19:02:00 PDT
Confirmed crash on ASan build 2013-06-06.

Verified fix on ASan builds of FF24, FF25 from 2013-09-11.
Comment 44 bhavana bajaj [:bajaj] 2013-10-15 14:53:16 PDT
NO :keeler to help with a backport patch for mozilla-b2g18.
Comment 45 David Keeler [:keeler] (use needinfo?) 2013-10-15 15:47:41 PDT
Created attachment 817498 [details] [diff] [review]
patch for b2g18

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
Comment 46 David Keeler [:keeler] (use needinfo?) 2013-10-15 16:09:50 PDT
https://hg.mozilla.org/releases/mozilla-b2g18/rev/051a1de1fb54

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