Implement verify() for HMAC using NSS

Implement verify() for HMAC using NSS

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

[eroman, 2013-09-25 23:18:51]

https://codereview.chromium.org/24616003/diff/1/content/renderer/webcrypto_im...
File content/renderer/webcrypto_impl_nss.cc (right):

https://codereview.chromium.org/24616003/diff/1/content/renderer/webcrypto_im...
content/renderer/webcrypto_impl_nss.cc:13: #include "crypto/secure_util.h"
this should be sorted above crypto/scroped_nss

https://codereview.chromium.org/24616003/diff/1/content/renderer/webcrypto_im...
content/renderer/webcrypto_impl_nss.cc:270:
DCHECK_EQ(PK11_GetMechanism(sym_key->key()),
Isn't this a duplication of DigestInternal? Please share the code instead. I.e.
call a helper function, or even SignInternal directly for now.

https://codereview.chromium.org/24616003/diff/1/content/renderer/webcrypto_im...
content/renderer/webcrypto_impl_nss.cc:308: // To ensure optimal security usage,
do not support truncated signatures.
This comment should be in terms of the webcrypto spec instead. i.e. "webcrypto
spec doesn't allow this" or "webcrypto spec is ambiguous on this but we chose to
XXX". I am fairly sure the intent of webcrypto spec is to not verify truncated
signatures.

[Bryan Eyler, 2013-09-25 23:57:05]

https://codereview.chromium.org/24616003/diff/1/content/renderer/webcrypto_im...
File content/renderer/webcrypto_impl_nss.cc (right):

https://codereview.chromium.org/24616003/diff/1/content/renderer/webcrypto_im...
content/renderer/webcrypto_impl_nss.cc:13: #include "crypto/secure_util.h"
On 2013/09/25 23:18:51, eroman wrote:
> this should be sorted above crypto/scroped_nss

For alphabetical ordering?  I think this is correct...

https://codereview.chromium.org/24616003/diff/1/content/renderer/webcrypto_im...
content/renderer/webcrypto_impl_nss.cc:270:
DCHECK_EQ(PK11_GetMechanism(sym_key->key()),
On 2013/09/25 23:18:51, eroman wrote:
> Isn't this a duplication of DigestInternal? Please share the code instead.
I.e.
> call a helper function, or even SignInternal directly for now.

Yeah, I was trying to avoid creating a WebArrayBuffer, but I guess this is such
a minimal call that it's worth all this extra duped code.  Fixed and it's a lot
cleaner.

https://codereview.chromium.org/24616003/diff/1/content/renderer/webcrypto_im...
content/renderer/webcrypto_impl_nss.cc:308: // To ensure optimal security usage,
do not support truncated signatures.
On 2013/09/25 23:18:51, eroman wrote:
> This comment should be in terms of the webcrypto spec instead. i.e. "webcrypto
> spec doesn't allow this" or "webcrypto spec is ambiguous on this but we chose
to
> XXX". I am fairly sure the intent of webcrypto spec is to not verify truncated
> signatures.

Done.

[Bryan Eyler, 2013-09-26 00:00:03]

On 2013/09/25 23:57:05, Bryan Eyler wrote:
>
https://codereview.chromium.org/24616003/diff/1/content/renderer/webcrypto_im...
> File content/renderer/webcrypto_impl_nss.cc (right):
> 
>
https://codereview.chromium.org/24616003/diff/1/content/renderer/webcrypto_im...
> content/renderer/webcrypto_impl_nss.cc:13: #include "crypto/secure_util.h"
> On 2013/09/25 23:18:51, eroman wrote:
> > this should be sorted above crypto/scroped_nss
> 
> For alphabetical ordering?  I think this is correct...
> 
>
https://codereview.chromium.org/24616003/diff/1/content/renderer/webcrypto_im...
> content/renderer/webcrypto_impl_nss.cc:270:
> DCHECK_EQ(PK11_GetMechanism(sym_key->key()),
> On 2013/09/25 23:18:51, eroman wrote:
> > Isn't this a duplication of DigestInternal? Please share the code instead.
> I.e.
> > call a helper function, or even SignInternal directly for now.
> 
> Yeah, I was trying to avoid creating a WebArrayBuffer, but I guess this is
such
> a minimal call that it's worth all this extra duped code.  Fixed and it's a
lot
> cleaner.

* not worth all this extra duped code

> 
>
https://codereview.chromium.org/24616003/diff/1/content/renderer/webcrypto_im...
> content/renderer/webcrypto_impl_nss.cc:308: // To ensure optimal security
usage,
> do not support truncated signatures.
> On 2013/09/25 23:18:51, eroman wrote:
> > This comment should be in terms of the webcrypto spec instead. i.e.
"webcrypto
> > spec doesn't allow this" or "webcrypto spec is ambiguous on this but we
chose
> to
> > XXX". I am fairly sure the intent of webcrypto spec is to not verify
truncated
> > signatures.
> 
> Done.

[eroman, 2013-09-26 00:10:42]

https://codereview.chromium.org/24616003/diff/1/content/renderer/webcrypto_im...
File content/renderer/webcrypto_impl_nss.cc (right):

https://codereview.chromium.org/24616003/diff/1/content/renderer/webcrypto_im...
content/renderer/webcrypto_impl_nss.cc:13: #include "crypto/secure_util.h"
On 2013/09/25 23:57:05, Bryan Eyler wrote:
> On 2013/09/25 23:18:51, eroman wrote:
> > this should be sorted above crypto/scroped_nss
> 
> For alphabetical ordering?  I think this is correct...

Oops, my bad :)

https://codereview.chromium.org/24616003/diff/7001/content/renderer/webcrypto...
File content/renderer/webcrypto_impl_nss.cc (right):

https://codereview.chromium.org/24616003/diff/7001/content/renderer/webcrypto...
content/renderer/webcrypto_impl_nss.cc:274:
crypto::SecureMemEqual(result.data(), signature, signature_size) &&
This is incorrect: the size check needs to be done BEFORE the comparison,
otherwise it may out of bounds.

Please add a regression test where signature_size is > result.byteLength()

[Bryan Eyler, 2013-09-26 00:18:07]

https://codereview.chromium.org/24616003/diff/7001/content/renderer/webcrypto...
File content/renderer/webcrypto_impl_nss.cc (right):

https://codereview.chromium.org/24616003/diff/7001/content/renderer/webcrypto...
content/renderer/webcrypto_impl_nss.cc:274:
crypto::SecureMemEqual(result.data(), signature, signature_size) &&
On 2013/09/26 00:10:43, eroman wrote:
> This is incorrect: the size check needs to be done BEFORE the comparison,
> otherwise it may out of bounds.
> 
> Please add a regression test where signature_size is > result.byteLength()

Done.

[eroman, 2013-09-26 00:19:34]

lgtm

[Bryan Eyler, 2013-09-26 00:22:47]

Reviewers:
+jamesr for owners approval
+sleevi in case you want to take a look?

[jamesr, 2013-09-26 00:26:57]

lgtm

[Ryan Sleevi, 2013-09-26 00:44:54]

lgtm

https://codereview.chromium.org/24616003/diff/14001/content/renderer/webcrypt...
File content/renderer/webcrypto_impl_nss.cc (right):

https://codereview.chromium.org/24616003/diff/14001/content/renderer/webcrypt...
content/renderer/webcrypto_impl_nss.cc:9: #include <vector>
there should be a linebreak here between C and C++ system headers.

https://codereview.chromium.org/24616003/diff/14001/content/renderer/webcrypt...
content/renderer/webcrypto_impl_nss.cc:283: return true;
Shouldn't the default be return false as well, and instead specifically return
true on 276?

[Bryan Eyler, 2013-09-26 00:57:39]

https://codereview.chromium.org/24616003/diff/14001/content/renderer/webcrypt...
File content/renderer/webcrypto_impl_nss.cc (right):

https://codereview.chromium.org/24616003/diff/14001/content/renderer/webcrypt...
content/renderer/webcrypto_impl_nss.cc:283: return true;
On 2013/09/26 00:44:55, Ryan Sleevi wrote:
> Shouldn't the default be return false as well, and instead specifically return
> true on 276?

In line with the style of the rest of this file, I've been early exiting on
failure and making getting to the end a success.  Should I make this change
anyway?

[Ryan Sleevi, 2013-09-26 01:02:23]

On 2013/09/26 00:57:39, Bryan Eyler wrote:
>
https://codereview.chromium.org/24616003/diff/14001/content/renderer/webcrypt...
> File content/renderer/webcrypto_impl_nss.cc (right):
> 
>
https://codereview.chromium.org/24616003/diff/14001/content/renderer/webcrypt...
> content/renderer/webcrypto_impl_nss.cc:283: return true;
> On 2013/09/26 00:44:55, Ryan Sleevi wrote:
> > Shouldn't the default be return false as well, and instead specifically
return
> > true on 276?
> 
> In line with the style of the rest of this file, I've been early exiting on
> failure and making getting to the end a success.  Should I make this change
> anyway?

My behaviour for crypto is
early exits for failure
early returns for success
default always fails


The failure mode for "forgetting" something is that it doesn't work, as opposed
to it lies.

This can be addressed in a follow-up CL that makes the change file-wide.

[Bryan Eyler, 2013-09-26 02:17:43]

https://codereview.chromium.org/24616003/diff/14001/content/renderer/webcrypt...
File content/renderer/webcrypto_impl_nss.cc (right):

https://codereview.chromium.org/24616003/diff/14001/content/renderer/webcrypt...
content/renderer/webcrypto_impl_nss.cc:9: #include <vector>
On 2013/09/26 00:44:55, Ryan Sleevi wrote:
> there should be a linebreak here between C and C++ system headers.

Done.

[commit-bot: I haz the power, 2013-09-26 02:30:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bryaneyler@google.com/24616003/21001

[eroman, 2013-09-26 02:45:50]

hopefully the commit queue does the right thing. since if my changelist
gets committed first then this one will hit a merge conflict :)


On Wed, Sep 25, 2013 at 7:30 PM, <commit-bot@chromium.org> wrote:

> CQ is trying da patch. Follow status at
> https://chromium-status.**appspot.com/cq/bryaneyler@**
>
google.com/24616003/21001<https://chromium-status.appspot.com/cq/bryaneyler@google.com/24616003/21001>
>
>
>
https://codereview.chromium.**org/24616003/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Bryan Eyler, 2013-09-26 18:51:14]

Hi Eric,
PTAL: Rebase was non-trivial in the unittests.

[eroman, 2013-09-26 18:54:16]

lgtm

[commit-bot: I haz the power, 2013-09-26 18:58:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bryaneyler@google.com/24616003/50001

[commit-bot: I haz the power, 2013-09-26 22:28:12]

Change committed as 225589