[webcrypto] Simplify the AES-KW workaround for NSS and remove valgrind suppression

[webcrypto] Simplify the AES-KW workaround for NSS and remove valgrind suppression

BUG=349939, 245025
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259153

[eroman, 2014-03-24 23:09:18]

padolph: FYI

[wtc, 2014-03-24 23:49:41]

Review comments on patch set 1:

https://codereview.chromium.org/210463003/diff/1/content/child/webcrypto/plat...
File content/child/webcrypto/platform_crypto_nss.cc (right):

https://codereview.chromium.org/210463003/diff/1/content/child/webcrypto/plat...
content/child/webcrypto/platform_crypto_nss.cc:523: if
(!NSS_VersionCheck("3.16.0")) {

Nit: you can use "3.16". See
http://mxr.mozilla.org/mozilla-central/source/security/nss/lib/nss/nss.h#36. The
".0" is not used in the NSS version string.

https://codereview.chromium.org/210463003/diff/1/content/child/webcrypto/plat...
content/child/webcrypto/platform_crypto_nss.cc:524: // Workaround for
https://code.google.com/p/chromium/issues/detail?id=349939

I suggest citing the upstream NSS bug report instead:
https://bugzilla.mozilla.org/show_bug.cgi?id=981170

[eroman, 2014-03-24 23:56:04]

https://codereview.chromium.org/210463003/diff/1/content/child/webcrypto/plat...
File content/child/webcrypto/platform_crypto_nss.cc (right):

https://codereview.chromium.org/210463003/diff/1/content/child/webcrypto/plat...
content/child/webcrypto/platform_crypto_nss.cc:523: if
(!NSS_VersionCheck("3.16.0")) {
On 2014/03/24 23:49:41, wtc wrote:
> 
> Nit: you can use "3.16". See
> http://mxr.mozilla.org/mozilla-central/source/security/nss/lib/nss/nss.h#36.
The
> ".0" is not used in the NSS version string.

Done.

https://codereview.chromium.org/210463003/diff/1/content/child/webcrypto/plat...
content/child/webcrypto/platform_crypto_nss.cc:524: // Workaround for
https://code.google.com/p/chromium/issues/detail?id=349939
On 2014/03/24 23:49:41, wtc wrote:
> 
> I suggest citing the upstream NSS bug report instead:
> https://bugzilla.mozilla.org/show_bug.cgi?id=981170

Done.

[wtc, 2014-03-25 00:03:25]

Patch set 2 LGTM.

https://codereview.chromium.org/210463003/diff/20002/content/child/webcrypto/...
File content/child/webcrypto/platform_crypto_nss.cc (right):

https://codereview.chromium.org/210463003/diff/20002/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:529: // This workaround re-wraps
the key and compares the result with the

This workaround is quite heavyweight.

Since we know NSS calls PORT_SetError(SEC_ERROR_BAD_DATA) before returning
successfully, would it work to call PORT_SetError(0) before calling
PK11_WrapSymKey, and checks PORT_GetError() == SEC_ERROR_BAD_DATA if
PK11_WrapSymKey returns SECSuccess?

[eroman, 2014-03-25 00:18:25]

https://codereview.chromium.org/210463003/diff/20002/content/child/webcrypto/...
File content/child/webcrypto/platform_crypto_nss.cc (right):

https://codereview.chromium.org/210463003/diff/20002/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:529: // This workaround re-wraps
the key and compares the result with the
On 2014/03/25 00:03:25, wtc wrote:
> 
> This workaround is quite heavyweight.
> 
> Since we know NSS calls PORT_SetError(SEC_ERROR_BAD_DATA) before returning
> successfully, would it work to call PORT_SetError(0) before calling
> PK11_WrapSymKey, and checks PORT_GetError() == SEC_ERROR_BAD_DATA if
> PK11_WrapSymKey returns SECSuccess?

Excellent idea! Done.

Indeed it looks from https://hg.mozilla.org/projects/nss/rev/753bb69e543c that
should be sufficient.

Using your workaround I can now also remove the valgrind suppression.

[Ryan Sleevi, 2014-03-25 01:45:59]

lgtm

[wtc, 2014-03-25 02:02:00]

Patch set 3 LGTM.

https://codereview.chromium.org/210463003/diff/70001/content/child/webcrypto/...
File content/child/webcrypto/platform_crypto_nss.cc (right):

https://codereview.chromium.org/210463003/diff/70001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:511: PORT_SetError(0);

Nit: add a comment to note this is part of the workaround.

https://codereview.chromium.org/210463003/diff/70001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:532: #endif

Nit: it may be better to put the workaround after the if (!new_key) check, on
line 539.

[eroman, 2014-03-25 02:07:05]

https://codereview.chromium.org/210463003/diff/70001/content/child/webcrypto/...
File content/child/webcrypto/platform_crypto_nss.cc (right):

https://codereview.chromium.org/210463003/diff/70001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:511: PORT_SetError(0);
On 2014/03/25 02:02:01, wtc wrote:
> 
> Nit: add a comment to note this is part of the workaround.

Done.

https://codereview.chromium.org/210463003/diff/70001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:532: #endif
On 2014/03/25 02:02:01, wtc wrote:
> 
> Nit: it may be better to put the workaround after the if (!new_key) check, on
> line 539.

Done.

[eroman, 2014-03-25 02:08:01]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-03-25 02:10:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eroman@chromium.org/210463003/90001

[commit-bot: I haz the power, 2014-03-25 04:54:05]

Change committed as 259153