Marking as a sec-bug; pls declassify, or not, as you see fit. Test path: dom/crypto/test/test_WebCrypto_RSA_OAEP.html RsaOaepTask::DoCrypto() starting line 967 contains this uint32_t outLen; if (mEncrypt) { // PK11_PubEncrypt() checks the plaintext's length and fails if it is too // long to encrypt, i.e. if it is longer than (k - 2hLen - 2) with 'k' // being the length in octets of the RSA modulus n and 'hLen' being the // output length in octets of the chosen hash function. // <https://tools.ietf.org/html/rfc3447#section-7.1> rv = MapSECStatus(PK11_PubEncrypt( mPubKey.get(), CKM_RSA_PKCS_OAEP, &param, mResult.Elements(), &outLen, mResult.Length(), mData.Elements(), mData.Length(), nullptr)); } else { rv = MapSECStatus(PK11_PrivDecrypt( mPrivKey.get(), CKM_RSA_PKCS_OAEP, &param, mResult.Elements(), &outLen, mResult.Length(), mData.Elements(), mData.Length())); } mResult.SetLength(outLen); If the call (in whichever arm) to MapSECStatus doesn't write anything to outLen, then the call to SetLength has an undefined parameter. Seems like the call to SetLength should depend on the returned rv from the MapSECStatus call?

Valgrind complaint. Note that this isn't really consistent with the analysis in comment #0, since that would imply that the uninitialised value was created in RsaOaepTask::DoCrypto's stack frame, but that's not what the message below says. So I suspect that analysis isn't correct. I'll investigate further. Conditional jump or move depends on uninitialised value(s) at 0x684EBDC: nsTArray_Impl<unsigned char, nsTArrayFallibleAllocator>::SetLength(unsigned long) (xpcom/glue/nsTArray.h:1433) by 0x684EE28: mozilla::dom::RsaOaepTask::DoCrypto() (dom/crypto/WebCryptoTask.cpp:985) by 0x6842C5D: mozilla::dom::WebCryptoTask::CalculateResult() (dom/crypto/WebCryptoTask.cpp:351) by 0x73B6421: mozilla::CryptoTask::Run() (security/manager/ssl/src/CryptoTask.cpp:53) by 0x58593E9: nsThread::ProcessNextEvent(bool, bool*) (xpcom/threads/nsThread.cpp:823) by 0x5878D71: NS_ProcessNextEvent(nsIThread*, bool) (xpcom/glue/nsThreadUtils.cpp:265) by 0x5AD3FA5: mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) (ipc/glue/MessagePump.cpp:326) by 0x5AB43CD: MessageLoop::RunInternal() (ipc/chromium/src/base/message_loop.cc:229) by 0x5AB43D8: MessageLoop::RunHandler() (ipc/chromium/src/base/message_loop.cc:222) by 0x5AB469D: MessageLoop::Run() (ipc/chromium/src/base/message_loop.cc:196) by 0x585C953: nsThread::ThreadFunc(void*) (xpcom/threads/nsThread.cpp:350) by 0x4C666F7: _pt_root (nsprpub/pr/src/pthreads/ptthread.c:212) by 0x349F407D13: start_thread (/usr/src/debug/glibc-2.15-a316c1f/nptl/pthread_create.c:309) by 0x349F0F168C: clone (/usr/src/debug//glibc-2.15-a316c1f/sysdeps/unix/sysv/linux/x86_64/clone.S:115) Uninitialised value was created by a stack allocation at 0x146D138A: NSC_Encrypt (security/nss/lib/softoken/pkcs11c.c:1244)

The analysis in comment #0 is incorrect. Here is a new analysis. I think this is a potential crasher, in that it can lead to realloc() being asked to resize a block to an unknown size. The problem is visible in NSC_Encrypt in security/nss/lib/softoken/pkcs11c.c. The result is that the fifth argument CK_ULONG_PTR pulEncryptedDataLen which is an out-parameter, has an undefined value written to it, which eventually winds up back in this call mResult.SetLength(outLen); in RsaOaepTask::DoCrypto() as discussed in comment 0. How this happens is: pkcs11c.c: 1247 unsigned int outlen; .... 1308 rv = (*context->update)(context->cipherInfo, pEncryptedData, 1309 &outlen, maxoutlen, pText.data, pText.len); 1310 crv = (rv == SECSuccess) ? CKR_OK : sftk_MapCryptError(PORT_GetError()); 1311 *pulEncryptedDataLen = (CK_ULONG) outlen; The call (*context->update)(...) can return SECFailure, in which case it does not write to &outlen. But line 1311 ignores this and writes passes the uninitialised outlen back via *pulEncryptedDataLen. The call (*context->update)(...) in this case leads to RSA_EncryptOAEP in security/nss/lib/freebl/rsapkcs.c. This has a call to eme_oaep_encode which returns SECFailure and RSA_EncryptOAEP exits at that point, without writing its out parameter. I didn't chase this further. Back to NSC_Encrypt. It is easy to show that NSC_Encrypt is returning stack allocated garbage using the patch shown in the next comment, which initialises outlen to 0x31415927 and prints it and |rv| after the call to (*context->update). It produces this: 1 INFO TEST-START | /tests/dom/crypto/test/test_WebCrypto_RSA_OAEP.html XXXXQQQQ rv = SECSuccess, outlen = 0x80 2 INFO TEST-PASS | /tests/dom/crypto/test/test_WebCrypto_RSA_OAEP.html | RSA-OAEP encrypt/decrypt round-trip XXXXQQQQ rv = SECSuccess, outlen = 0x100 XXXXQQQQ rv = SECSuccess, outlen = 0x100 3 INFO TEST-PASS | /tests/dom/crypto/test/test_WebCrypto_RSA_OAEP.html | RSA-OAEP key generation and encrypt/decrypt round-trip (SHA-256) 4 INFO TEST-PASS | /tests/dom/crypto/test/test_WebCrypto_RSA_OAEP.html | RSA-OAEP decryption known answer XXXXQQQQ rv = SECSuccess, outlen = 0x100 XXXXQQQQ rv = SECSuccess, outlen = 0x100 XXXXQQQQ rv = SECFailure, outlen = 0x31415927 5 INFO TEST-PASS | /tests/dom/crypto/test/test_WebCrypto_RSA_OAEP.html | RSA-OAEP input data length checks (2048-bit key) and that bogus value, 0x31415927, is returned in *pulEncryptedDataLen. If we now re-run on Valgrind/Memcheck, it stops complaining that the returned length is undefined (because it now isn't). But it does indicate that 0x31415927 is handed onwards to realloc, because it complains like this: Warning: set address range perms: large range [0x39e2e148, 0x6b244040) (undefined) at 0x4809256: realloc (mozhx/coregrind/m_replacemalloc/vg_replace_malloc.c:694) by 0x4828510: moz_realloc (memory/mozalloc/mozalloc.cpp:94) by 0x5826D13: Realloc (ff-O-linux64/dom/datastore/../../dist/include/nsTArray.h:170) by 0x5826D13: nsTArray_base<nsTArrayFallibleAllocator, nsTArray_CopyWithMemutils> ::EnsureCapacity(unsigned long, unsigned long) (ff-O-linux64/ipc/ipdl/../../dist/include/nsTArray-inl.h:187) by 0x584D5EB: nsTArray_base<nsTArrayFallibleAllocator, nsTArray_CopyWithMemutils> ::InsertSlotsAt(unsigned long, unsigned long, unsigned long, unsigned long) (ff-O-linux64/content/media/webaudio/../../../ dist/include/nsTArray-inl.h:286) by 0x684EBF9: InsertElementsAt (ff-O-linux64/dom/crypto/../../dist/include/ nsTArray.h:1479) by 0x684EBF9: nsTArray_Impl<unsigned char, nsTArrayFallibleAllocator>::SetLength (unsigned long) (ff-O-linux64/dom/crypto/../../dist/include /nsTArray.h:1435) by 0x684EE28: mozilla::dom::RsaOaepTask::DoCrypto() (dom/crypto /WebCryptoTask.cpp:985) by 0x6842C5D: mozilla::dom::WebCryptoTask::CalculateResult() (dom/crypto/WebCryptoTask.cpp:351) by 0x73B6421: mozilla::CryptoTask::Run() (security/manager/ssl/src/CryptoTask.cpp:53) In other words, we asked realloc to produce a block of size 0x6b244040 - 0x39e2e148 = 0x31415ef8. Presumably if the junk value returned was much higher, eg 0x60000000 or above on a 32 bit system, the allocation would have failed and general badness would result.

Patch referred to in comment 2: diff --git a/security/nss/lib/softoken/pkcs11c.c b/security/nss/lib/softoken/pkcs11c.c --- a/security/nss/lib/softoken/pkcs11c.c +++ b/security/nss/lib/softoken/pkcs11c.c @@ -1239,17 +1239,17 @@ finish: /* NSC_Encrypt encrypts single-part data. */ CK_RV NSC_Encrypt (CK_SESSION_HANDLE hSession, CK_BYTE_PTR pData, CK_ULONG ulDataLen, CK_BYTE_PTR pEncryptedData, CK_ULONG_PTR pulEncryptedDataLen) { SFTKSession *session; SFTKSessionContext *context; - unsigned int outlen; + unsigned int outlen = 0x31415927; unsigned int maxoutlen = *pulEncryptedDataLen; CK_RV crv; CK_RV crv2; SECStatus rv = SECSuccess; SECItem pText; pText.type = siBuffer; pText.data = pData; @@ -1303,16 +1303,22 @@ CK_RV NSC_Encrypt (CK_SESSION_HANDLE hSe } } } /* do it: NOTE: this assumes buf size is big enough. */ rv = (*context->update)(context->cipherInfo, pEncryptedData, &outlen, maxoutlen, pText.data, pText.len); crv = (rv == SECSuccess) ? CKR_OK : sftk_MapCryptError(PORT_GetError()); + +fprintf(stderr, "XXXXQQQQ rv = %s, outlen = 0x%x\n", + rv == SECSuccess ? "SECSuccess" + : (rv == SECFailure ? "SECFailure" : "SECSomethingElse"), + outlen); + *pulEncryptedDataLen = (CK_ULONG) outlen; if (pText.data != pData) PORT_ZFree(pText.data, pText.len); fail: sftk_TerminateOp( session, SFTK_ENCRYPT, context ); finish: sftk_FreeSession(session);

I'm unclear what the caller/callee contract here is intended to be. It is true that NSC_Encrypt does return SECFailure after writing garbage to *pulEncryptedDataLen. So is it the fault of the callers that they look at the out-param even though the function has failed? Or is it the fault of NSC_Encrypt, that it returns uninitialised data? Misc note: the test that causes all this is sub-case "RSA-OAEP input data length checks (2048-bit key)" of dom/crypto/test/test_WebCrypto_RSA_OAEP.html.

Who would be a good person to review this?

(In reply to Julian Seward [:jseward] from comment #0) > RsaOaepTask::DoCrypto() starting line 967 contains this > > uint32_t outLen; > if (mEncrypt) { > // PK11_PubEncrypt() checks the plaintext's length and fails if it is too > // long to encrypt, i.e. if it is longer than (k - 2hLen - 2) with 'k' > // being the length in octets of the RSA modulus n and 'hLen' being the > // output length in octets of the chosen hash function. > // <https://tools.ietf.org/html/rfc3447#section-7.1> > rv = MapSECStatus(PK11_PubEncrypt( > mPubKey.get(), CKM_RSA_PKCS_OAEP, &param, > mResult.Elements(), &outLen, mResult.Length(), > mData.Elements(), mData.Length(), nullptr)); > } else { > rv = MapSECStatus(PK11_PrivDecrypt( > mPrivKey.get(), CKM_RSA_PKCS_OAEP, &param, > mResult.Elements(), &outLen, mResult.Length(), > mData.Elements(), mData.Length())); > } > mResult.SetLength(outLen); If PK11_PubEncrypt / PK11_PrivDecrypt returns a failing error code, we should at least treat outLen as uninitialized.

(In reply to David Keeler (:keeler) [use needinfo?] from comment #7) > If PK11_PubEncrypt / PK11_PrivDecrypt returns a failing error code, we > should at least treat outLen as uninitialized. Should I interpret this to mean that you would prefer to fix the problem by checking the error code in RsaOaepTask::DoCrypto() rather than by the patch shown in comment #5?

All I meant was we should at least fix RsaOaepTask::DoCrypto (and any similar cases in webcrypto, if they exist). I don't know if an additional fix is needed for NSC_Encrypt. I think it is generally understood that if an NSS function returns a failing error code, we shouldn't assume any of its other outputs have been properly set, but there's definitely a more defensive way of implementing such functions so that it's not even possible for callers to do the wrong thing.

This patch takes Julian's proposed fix and adds the additional fix of checking that the encrypt/decrypt operation succeeded before using the outLen value. Belt and suspenders. The only other use of encrypt/decrypt in WebCrypto is in AesTask, which already does both of these things: http://dxr.mozilla.org/mozilla-central/source/dom/crypto/WebCryptoTask.cpp#599

Oh, I should add that encrypt/decrypt is also used in RsaesPkcs1Task, and it has the same bug as RsaOaepTask. http://dxr.mozilla.org/mozilla-central/source/dom/crypto/WebCryptoTask.cpp#836 However, that entire class is being eliminated with Bug 1037892, so we don't need to do anything here. At least for Nightly -- the fix to this bug should get uplifted to Aurora / Beta. I'll chat with releng about whether it makes more sense to uplift just this or to also uplift 1037892.

(In reply to Richard Barnes [:rbarnes] from comment #10) > This patch takes Julian's proposed fix [...] I verified that this runs clean with V/Memcheck.

Comment on attachment 8496621 [details] [diff] [review] bug-1064320.patch Review of attachment 8496621 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me, although see the second comment in particular. ::: dom/crypto/WebCryptoTask.cpp @@ +823,5 @@ > param.type = siBuffer; > param.data = (unsigned char*) &oaepParams; > param.len = sizeof(oaepParams); > > + uint32_t outLen = 0; We probably actually don't need to initialize outLen here, since it'll never get used if rv isn't success. @@ +840,5 @@ > mPrivKey.get(), CKM_RSA_PKCS_OAEP, &param, > mResult.Elements(), &outLen, mResult.Length(), > mData.Elements(), mData.Length())); > } > + NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_OPERATION_ERR); hmmm - in the error case, would it be necessary to specifically set mResult.SetLength(0)? I'm thinking not, since if this function returns an error, we should never use mResult at all. I just want someone else to double-check my thinking, though.

Thinking of it more, we probably should actually fix NSS to do the right thing and never return uninitialized memory.

(In reply to David Keeler (:keeler) [use needinfo?] from comment #13) > ::: dom/crypto/WebCryptoTask.cpp > @@ +823,5 @@ > > param.type = siBuffer; > > param.data = (unsigned char*) &oaepParams; > > param.len = sizeof(oaepParams); > > > > + uint32_t outLen = 0; > > We probably actually don't need to initialize outLen here, since it'll never > get used if rv isn't success. Belt and suspenders. > @@ +840,5 @@ > > mPrivKey.get(), CKM_RSA_PKCS_OAEP, &param, > > mResult.Elements(), &outLen, mResult.Length(), > > mData.Elements(), mData.Length())); > > } > > + NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_OPERATION_ERR); > > hmmm - in the error case, would it be necessary to specifically set > mResult.SetLength(0)? I'm thinking not, since if this function returns an > error, we should never use mResult at all. I just want someone else to > double-check my thinking, though. I don't think we need to. If this method returns an error, then the Task will conclude with FailWithError(), not Resolve(), and mResult will just get freed. There would be only a very marginal benefit to freeing mResult a little earlier. (And actually, I'm not sure that FallibleTArray frees memory on SetLength.) (In reply to David Keeler (:keeler) [use needinfo?] from comment #14) > Thinking of it more, we probably should actually fix NSS to do the right > thing and never return uninitialized memory. Filed Bug 1075161. (Which is confusing, since I've already been working on bug 1057161)

Same as bug-1064320.patch, but fixing the additional instance of the bug that is present in Aurora and Beta.

Comment on attachment 8497904 [details] [diff] [review] bug-1064320.aurora-beta.patch Empty patch

Forgot qrefresh. Also, carrying forward r+ from keeler. Approval Request Comment [Feature/regressing bug #]: WebCrypto implementation of RSAES-PKCS1-v1_5 and RSA-OAEP (Bug 998803 and Bug 1026398, respectively) [User impact if declined]: Potential crash (see analysis in comment 2) [Describe test coverage new/current, TBPL]: none [Risks and why]: Low risk. Simply checks that the encrypt call succeeded before using an out parameter from that call. [String/UUID change made/needed]: none

https://hg.mozilla.org/releases/mozilla-aurora/rev/7b9331d4bfb3 https://hg.mozilla.org/releases/mozilla-beta/rev/a4529d1ee29c Switching the components on this since the fix landed here was in Core code, not NSS. Bug 1075161 tracks fixing the underlying NSS bug.

Does this impact ESR?

FWIW, asking the person who fixed the bug is probably more useful than asking Dan.

(In reply to Julian Seward [:jseward] from comment #12) > (In reply to Richard Barnes [:rbarnes] from comment #10) > > This patch takes Julian's proposed fix [...] > > I verified that this runs clean with V/Memcheck. Hi Julian - can you tell me what branch you verified this on? Thanks.

Actually, my apologies, you may not be running this in Fx at all, which makes my question moot.