Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
118623002:
[webcrypto] Add raw symmetric key AES-KW wrap/unwrap for NSS. (Closed)
|
Created:
7 years ago by padolph Modified:
6 years, 9 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Description[webcrypto] Add raw symmetric key AES-KW wrap/unwrap for NSS.
BUG=245025
TEST=content_unittests --gtest_filter="WebCryptoImpl*"
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255270
Patch Set 1 #Patch Set 2 : minor refactoring #
Total comments: 10
Patch Set 3 : fixes for bryaneyler #
Total comments: 16
Patch Set 4 : rebase #Patch Set 5 : fixes for eroman #
Total comments: 3
Patch Set 6 : rebase and add large data test #
Total comments: 9
Patch Set 7 : fixes for rsleevi #Patch Set 8 : rebase #
Total comments: 27
Patch Set 9 : fixes for eroman #Patch Set 10 : rebase #Patch Set 11 : rebase and refactor #
Total comments: 8
Patch Set 12 : fixes for eroman #Patch Set 13 : rebase #
Total comments: 14
Patch Set 14 : fixes for rsleevi #Patch Set 15 : minor comment change: added a TODO #
Total comments: 6
Patch Set 16 : fixes for eroman #Patch Set 17 : changed ASSERTS from last change to EXPECTS, to match original code intent #Messages
Total messages: 43 (0 generated)
https://codereview.chromium.org/118623002/diff/20001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/118623002/diff/20001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:202: DCHECK_EQ(algorithm.id(), key.algorithm().id()); Is this right? Isn't algorithm the key wrap algorithm, but the key algorithm can be anything? Instead, I think we should return an error if the algorithm.id() != AESKW, since that's the only wrap algorithm we currently support. https://codereview.chromium.org/118623002/diff/20001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:254: DCHECK_EQ(algorithm.id(), key.algorithm().id()); Same here. https://codereview.chromium.org/118623002/diff/20001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:650: case blink::WebCryptoAlgorithmIdAesKw: { According to the spec, the encrypt() function is not the one to call to do the unwrap, but rather the unwrap() function. https://codereview.chromium.org/118623002/diff/20001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:710: case blink::WebCryptoAlgorithmIdAesKw: { Same here. https://codereview.chromium.org/118623002/diff/20001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/118623002/diff/20001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1769: algorithm, Might be good to try different algorithms too, to exercise the issue I mentioned about the algorithms matching.
I don't believe we want to expose encrypt/decrypt explicitly. According to the spec AES-KW has wrapKey/unwrapKey: https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#aes-kw There is a wrapKey/unwrapKey method that can be overridden in WebCryptoImpl: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
On 2013/12/18 23:30:37, eroman wrote: > I don't believe we want to expose encrypt/decrypt explicitly. > > According to the spec AES-KW has wrapKey/unwrapKey: > https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#aes-kw > > There is a wrapKey/unwrapKey method that can be overridden in WebCryptoImpl: > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... I struggled with this a bit, and indeed want to discuss it. Here is my reasoning for coding it this way: Encrypt/DecryptInternal are internal implementation details, called only by WebCryptoImpl::encrypt/decrypt. We can add suitable checks to make sure the internal versions can never be called by encrypt/decrypt. The spec says wrap = export + encrypt, unwrap = decrypt + import. Coding AES-KW this way allows an implementation exactly to this spec, and symmetric with all the other types of key wrapping.
On 2013/12/18 23:42:55, padolph wrote: > On 2013/12/18 23:30:37, eroman wrote: > > I don't believe we want to expose encrypt/decrypt explicitly. > > > > According to the spec AES-KW has wrapKey/unwrapKey: > > https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#aes-kw > > > > There is a wrapKey/unwrapKey method that can be overridden in WebCryptoImpl: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > I struggled with this a bit, and indeed want to discuss it. Here is my reasoning > for coding it this way: > Encrypt/DecryptInternal are internal implementation details, called only by > WebCryptoImpl::encrypt/decrypt. We can add suitable checks to make sure the > internal versions can never be called by encrypt/decrypt. > The spec says wrap = export + encrypt, unwrap = decrypt + import. Coding AES-KW > this way allows an implementation exactly to this spec, and symmetric with all > the other types of key wrapping. The spec only says this because the *external* effects must be the same. The internal implementation is allowed to be implemented using any means, provided that from the caller's perspective, it has the same behaviour. Using only wrap/unwrap only meets this requirement on the Blink side. If not, there's a spec bug. We definitely should not be exposing encryptInternal/decryptInternal
https://codereview.chromium.org/118623002/diff/20001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/118623002/diff/20001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:202: DCHECK_EQ(algorithm.id(), key.algorithm().id()); On 2013/12/18 23:30:32, Bryan Eyler wrote: > Is this right? Isn't algorithm the key wrap algorithm, but the key algorithm > can be anything? In key wrapping terms that is correct. But this function only does the second half of the wrapping algorithm: Wrap = export + encrypt. The encrypt step is just regular encryption and has the same requirements, so I copied that block of DCHECKS from other encryption code. > Instead, I think we should return an error if the algorithm.id() != AESKW, since > that's the only wrap algorithm we currently support. In that regard, I could remove the algorithm parameter altogether, and DCHECK only the key.algorithm.id(). But then again I will also be doing the DCHECK on algorithm in the calling code so it is a wash. https://codereview.chromium.org/118623002/diff/20001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:254: DCHECK_EQ(algorithm.id(), key.algorithm().id()); On 2013/12/18 23:30:32, Bryan Eyler wrote: > Same here. See above. https://codereview.chromium.org/118623002/diff/20001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:650: case blink::WebCryptoAlgorithmIdAesKw: { On 2013/12/18 23:30:32, Bryan Eyler wrote: > According to the spec, the encrypt() function is not the one to call to do the > unwrap, but rather the unwrap() function. The spec indeed says that you cannot do AES-KW from encrypt. But that requirement should not necessarily influence the internal implementation. Since wrap = export + encrypt and unwrap = decrypt + import, coding it this way lets us do the wrap/unwrap internal code in an identical way for all the supported wrap algorithms. What's missing is suitable checks to prevent the embedder (not the API client) from doing the wrong thing. I add those in. https://codereview.chromium.org/118623002/diff/20001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:710: case blink::WebCryptoAlgorithmIdAesKw: { On 2013/12/18 23:30:32, Bryan Eyler wrote: > Same here. See above. https://codereview.chromium.org/118623002/diff/20001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/118623002/diff/20001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1769: algorithm, On 2013/12/18 23:30:32, Bryan Eyler wrote: > Might be good to try different algorithms too, to exercise the issue I mentioned > about the algorithms matching. Done.
(manual email) me 0 minutes ago #6 https://codereview.chromium.org/118623002/diff/20001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/118623002/diff/20001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:202: DCHECK_EQ(algorithm.id(), key.algorithm().id()); On 2013/12/18 23:30:32, Bryan Eyler wrote: > Is this right? Isn't algorithm the key wrap algorithm, but the key algorithm > can be anything? In key wrapping terms that is correct. But this function only does the second half of the wrapping algorithm: Wrap = export + encrypt. The encrypt step is just regular encryption and has the same requirements, so I copied that block of DCHECKS from other encryption code. > Instead, I think we should return an error if the algorithm.id() != AESKW, since > that's the only wrap algorithm we currently support. In that regard, I could remove the algorithm parameter altogether, and DCHECK only the key.algorithm.id(). But then again I will also be doing the DCHECK on algorithm in the calling code so it is a wash. https://codereview.chromium.org/118623002/diff/20001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:254: DCHECK_EQ(algorithm.id(), key.algorithm().id()); On 2013/12/18 23:30:32, Bryan Eyler wrote: > Same here. See above. https://codereview.chromium.org/118623002/diff/20001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:650: case blink::WebCryptoAlgorithmIdAesKw: { On 2013/12/18 23:30:32, Bryan Eyler wrote: > According to the spec, the encrypt() function is not the one to call to do the > unwrap, but rather the unwrap() function. The spec indeed says that you cannot do AES-KW from encrypt. But that requirement should not necessarily influence the internal implementation. Since wrap = export + encrypt and unwrap = decrypt + import, coding it this way lets us do the wrap/unwrap internal code in an identical way for all the supported wrap algorithms. What's missing is suitable checks to prevent the embedder (not the API client) from doing the wrong thing. I add those in. https://codereview.chromium.org/118623002/diff/20001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:710: case blink::WebCryptoAlgorithmIdAesKw: { On 2013/12/18 23:30:32, Bryan Eyler wrote: > Same here. See above. https://codereview.chromium.org/118623002/diff/20001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/118623002/diff/20001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:1769: algorithm, On 2013/12/18 23:30:32, Bryan Eyler wrote: > Might be good to try different algorithms too, to exercise the issue I mentioned > about the algorithms matching. Done. On Wed, Dec 18, 2013 at 3:48 PM, <rsleevi@chromium.org> wrote: > On 2013/12/18 23:42:55, padolph wrote: >> >> On 2013/12/18 23:30:37, eroman wrote: >> > I don't believe we want to expose encrypt/decrypt explicitly. >> > >> > According to the spec AES-KW has wrapKey/unwrapKey: >> > >> > https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#aes-kw >> > >> > There is a wrapKey/unwrapKey method that can be overridden in >> > WebCryptoImpl: >> > >> > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > >> I struggled with this a bit, and indeed want to discuss it. Here is my > > reasoning >> >> for coding it this way: >> Encrypt/DecryptInternal are internal implementation details, called only >> by >> WebCryptoImpl::encrypt/decrypt. We can add suitable checks to make sure >> the >> internal versions can never be called by encrypt/decrypt. >> The spec says wrap = export + encrypt, unwrap = decrypt + import. Coding > > AES-KW >> >> this way allows an implementation exactly to this spec, and symmetric with >> all >> the other types of key wrapping. > > > The spec only says this because the *external* effects must be the same. > > The internal implementation is allowed to be implemented using any means, > provided that from the caller's perspective, it has the same behaviour. > > Using only wrap/unwrap only meets this requirement on the Blink side. If > not, > there's a spec bug. > > We definitely should not be exposing encryptInternal/decryptInternal > > https://codereview.chromium.org/118623002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/12/18 23:48:01, Ryan Sleevi wrote: > On 2013/12/18 23:42:55, padolph wrote: > > On 2013/12/18 23:30:37, eroman wrote: > > > I don't believe we want to expose encrypt/decrypt explicitly. > > > > > > According to the spec AES-KW has wrapKey/unwrapKey: > > > https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#aes-kw > > > > > > There is a wrapKey/unwrapKey method that can be overridden in WebCryptoImpl: > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > I struggled with this a bit, and indeed want to discuss it. Here is my > reasoning > > for coding it this way: > > Encrypt/DecryptInternal are internal implementation details, called only by > > WebCryptoImpl::encrypt/decrypt. We can add suitable checks to make sure the > > internal versions can never be called by encrypt/decrypt. > > The spec says wrap = export + encrypt, unwrap = decrypt + import. Coding > AES-KW > > this way allows an implementation exactly to this spec, and symmetric with all > > the other types of key wrapping. > > The spec only says this because the *external* effects must be the same. > > The internal implementation is allowed to be implemented using any means, > provided that from the caller's perspective, it has the same behaviour. > > Using only wrap/unwrap only meets this requirement on the Blink side. If not, > there's a spec bug. > > We definitely should not be exposing encryptInternal/decryptInternal Ryan, I'm not sure what your direction here is. We are not exposing encryptInternal/decryptInternal. If "the internal implementation is allowed to be implemented using any means", then you are OK with my current design? Basically, I think that INTERNALLY AES-KW is an encryption cipher, just like AES-CBC or RSA. It seems better to treat it as such instead of special casing it. That will be done at a higher level, for which I will upload code shortly.
(manual email) On 2013/12/18 23:48:01, Ryan Sleevi wrote: > On 2013/12/18 23:42:55, padolph wrote: > > On 2013/12/18 23:30:37, eroman wrote: > > > I don't believe we want to expose encrypt/decrypt explicitly. > > > > > > According to the spec AES-KW has wrapKey/unwrapKey: > > > https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#aes-kw > > > > > > There is a wrapKey/unwrapKey method that can be overridden in WebCryptoImpl: > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > I struggled with this a bit, and indeed want to discuss it. Here is my > reasoning > > for coding it this way: > > Encrypt/DecryptInternal are internal implementation details, called only by > > WebCryptoImpl::encrypt/decrypt. We can add suitable checks to make sure the > > internal versions can never be called by encrypt/decrypt. > > The spec says wrap = export + encrypt, unwrap = decrypt + import. Coding > AES-KW > > this way allows an implementation exactly to this spec, and symmetric with all > > the other types of key wrapping. > > The spec only says this because the *external* effects must be the same. > > The internal implementation is allowed to be implemented using any means, > provided that from the caller's perspective, it has the same behaviour. > > Using only wrap/unwrap only meets this requirement on the Blink side. If not, > there's a spec bug. > > We definitely should not be exposing encryptInternal/decryptInternal Ryan, I'm not sure what your direction here is. We are not exposing encryptInternal/decryptInternal. If "the internal implementation is allowed to be implemented using any means", then you are OK with my current design? Basically, I think that INTERNALLY AES-KW is an encryption cipher, just like AES-CBC or RSA. It seems better to treat it as such instead of special casing it. That will be done at a higher level, for which I will upload code shortly. On Wed, Dec 18, 2013 at 4:10 PM, Paul Adolph <padolph@netflix.com> wrote: > (manual email) > > > me > 0 minutes ago #6 > https://codereview.chromium.org/118623002/diff/20001/content/renderer/webcryp... > File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): > > https://codereview.chromium.org/118623002/diff/20001/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_impl_nss.cc:202: DCHECK_EQ(algorithm.id(), > key.algorithm().id()); > On 2013/12/18 23:30:32, Bryan Eyler wrote: >> Is this right? Isn't algorithm the key wrap algorithm, but the key algorithm >> can be anything? > > In key wrapping terms that is correct. But this function only does the second > half of the wrapping algorithm: Wrap = export + encrypt. The encrypt step is > just regular encryption and has the same requirements, so I copied that block of > DCHECKS from other encryption code. > >> Instead, I think we should return an error if the algorithm.id() != AESKW, > since >> that's the only wrap algorithm we currently support. > > In that regard, I could remove the algorithm parameter altogether, and DCHECK > only the key.algorithm.id(). But then again I will also be doing the DCHECK on > algorithm in the calling code so it is a wash. > > https://codereview.chromium.org/118623002/diff/20001/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_impl_nss.cc:254: DCHECK_EQ(algorithm.id(), > key.algorithm().id()); > On 2013/12/18 23:30:32, Bryan Eyler wrote: >> Same here. > > See above. > > https://codereview.chromium.org/118623002/diff/20001/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_impl_nss.cc:650: case > blink::WebCryptoAlgorithmIdAesKw: { > On 2013/12/18 23:30:32, Bryan Eyler wrote: >> According to the spec, the encrypt() function is not the one to call to do the >> unwrap, but rather the unwrap() function. > > The spec indeed says that you cannot do AES-KW from encrypt. But that > requirement should not necessarily influence the internal implementation. Since > wrap = export + encrypt and unwrap = decrypt + import, coding it this way lets > us do the wrap/unwrap internal code in an identical way for all the supported > wrap algorithms. > > What's missing is suitable checks to prevent the embedder (not the API client) > from doing the wrong thing. I add those in. > > https://codereview.chromium.org/118623002/diff/20001/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_impl_nss.cc:710: case > blink::WebCryptoAlgorithmIdAesKw: { > On 2013/12/18 23:30:32, Bryan Eyler wrote: >> Same here. > > See above. > > https://codereview.chromium.org/118623002/diff/20001/content/renderer/webcryp... > File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): > > https://codereview.chromium.org/118623002/diff/20001/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_impl_unittest.cc:1769: algorithm, > On 2013/12/18 23:30:32, Bryan Eyler wrote: >> Might be good to try different algorithms too, to exercise the issue I > mentioned >> about the algorithms matching. > > Done. > > > On Wed, Dec 18, 2013 at 3:48 PM, <rsleevi@chromium.org> wrote: >> On 2013/12/18 23:42:55, padolph wrote: >>> >>> On 2013/12/18 23:30:37, eroman wrote: >>> > I don't believe we want to expose encrypt/decrypt explicitly. >>> > >>> > According to the spec AES-KW has wrapKey/unwrapKey: >>> > >>> > https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#aes-kw >>> > >>> > There is a wrapKey/unwrapKey method that can be overridden in >>> > WebCryptoImpl: >>> > >>> > >> >> >> https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... >> >> >>> I struggled with this a bit, and indeed want to discuss it. Here is my >> >> reasoning >>> >>> for coding it this way: >>> Encrypt/DecryptInternal are internal implementation details, called only >>> by >>> WebCryptoImpl::encrypt/decrypt. We can add suitable checks to make sure >>> the >>> internal versions can never be called by encrypt/decrypt. >>> The spec says wrap = export + encrypt, unwrap = decrypt + import. Coding >> >> AES-KW >>> >>> this way allows an implementation exactly to this spec, and symmetric with >>> all >>> the other types of key wrapping. >> >> >> The spec only says this because the *external* effects must be the same. >> >> The internal implementation is allowed to be implemented using any means, >> provided that from the caller's perspective, it has the same behaviour. >> >> Using only wrap/unwrap only meets this requirement on the Blink side. If >> not, >> there's a spec bug. >> >> We definitely should not be exposing encryptInternal/decryptInternal >> >> https://codereview.chromium.org/118623002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I see what the discrepancies here are now, and why it's confusing us. If this tied into the wrap/unwrap functions, that would make this much more clear, although it would increase the size of the CL a lot. Not sure how Eric and Ryan feel? On 2013/12/19 00:07:15, padolph wrote: > https://codereview.chromium.org/118623002/diff/20001/content/renderer/webcryp... > File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): > > https://codereview.chromium.org/118623002/diff/20001/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_impl_nss.cc:202: DCHECK_EQ(algorithm.id(), > key.algorithm().id()); > On 2013/12/18 23:30:32, Bryan Eyler wrote: > > Is this right? Isn't algorithm the key wrap algorithm, but the key algorithm > > can be anything? > > In key wrapping terms that is correct. But this function only does the second > half of the wrapping algorithm: Wrap = export + encrypt. The encrypt step is > just regular encryption and has the same requirements, so I copied that block of > DCHECKS from other encryption code. > > > Instead, I think we should return an error if the algorithm.id() != AESKW, > since > > that's the only wrap algorithm we currently support. > > In that regard, I could remove the algorithm parameter altogether, and DCHECK > only the key.algorithm.id(). But then again I will also be doing the DCHECK on > algorithm in the calling code so it is a wash. > > https://codereview.chromium.org/118623002/diff/20001/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_impl_nss.cc:254: DCHECK_EQ(algorithm.id(), > key.algorithm().id()); > On 2013/12/18 23:30:32, Bryan Eyler wrote: > > Same here. > > See above. > > https://codereview.chromium.org/118623002/diff/20001/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_impl_nss.cc:650: case > blink::WebCryptoAlgorithmIdAesKw: { > On 2013/12/18 23:30:32, Bryan Eyler wrote: > > According to the spec, the encrypt() function is not the one to call to do the > > unwrap, but rather the unwrap() function. > > The spec indeed says that you cannot do AES-KW from encrypt. But that > requirement should not necessarily influence the internal implementation. Since > wrap = export + encrypt and unwrap = decrypt + import, coding it this way lets > us do the wrap/unwrap internal code in an identical way for all the supported > wrap algorithms. > > What's missing is suitable checks to prevent the embedder (not the API client) > from doing the wrong thing. I add those in. > > https://codereview.chromium.org/118623002/diff/20001/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_impl_nss.cc:710: case > blink::WebCryptoAlgorithmIdAesKw: { > On 2013/12/18 23:30:32, Bryan Eyler wrote: > > Same here. > > See above. > > https://codereview.chromium.org/118623002/diff/20001/content/renderer/webcryp... > File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): > > https://codereview.chromium.org/118623002/diff/20001/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_impl_unittest.cc:1769: algorithm, > On 2013/12/18 23:30:32, Bryan Eyler wrote: > > Might be good to try different algorithms too, to exercise the issue I > mentioned > > about the algorithms matching. > > Done.
On 2013/12/19 00:25:42, Bryan Eyler wrote:
> I see what the discrepancies here are now, and why it's confusing us. If this
> tied into the wrap/unwrap functions, that would make this much more clear,
> although it would increase the size of the CL a lot. Not sure how Eric and
Ryan
> feel?
I am fine either way. Let me distill it down to this. Which wrapKey function
would you rather see?
void WebCryptoImpl::wrapKey(
blink::WebCryptoKeyFormat format,
const blink::WebCryptoKey& key,
const blink::WebCryptoKey& wrappingKey,
const blink::WebCryptoAlgorithm& algorithm,
blink::WebCryptoResult result) {
blink::WebArrayBuffer buffer;
if (!WrapKeyInternal(format, key, wrappingKey, algorithm, &buffer)) {
result.completeWithError();
} else {
result.completeWithBuffer(buffer);
}
}
void WebCryptoImpl::wrapKey(
blink::WebCryptoKeyFormat format,
const blink::WebCryptoKey& key,
const blink::WebCryptoKey& wrappingKey,
const blink::WebCryptoAlgorithm& algorithm,
blink::WebCryptoResult result) {
blink::WebArrayBuffer key_buffer;
if (!ExportKeyInternal(format, key, &key_buffer)) {
result.completeWithError();
return;
}
blink::WebArrayBuffer buffer;
if (!EncryptInternal(algorithm, wrappingKey, key_buffer.data(),
key_buffer.byteLength(), &buffer)) {
result.completeWithError();
} else {
result.completeWithBuffer(buffer);
}
}
The first function is what I think you expected to see. This means that there
will be NSS and OpenSSL versions of WrapKeyInternal() that are mostly the same.
This seemed cumbersome to me.
The second function depends on symmetry across encryption algorithms and usage
enforcement inside Blink. But is fully complete and portable; no extra
WrapKeyInternal() implementations are required. And as a bonus it matches the
spec literally.
Thanks Paul. I agree that putting it in Encrypt/Decrypt and having the general unwrap/wrap implementation sounds good. I was initially concerned that it might be possible to call crypt.subtle.encrypt() from blink, and have it then get serviced for algorithms which only support wrapKey(). However that should not be the case, we do verify the operation at the blink layer before passing off to the blink API.
https://codereview.chromium.org/118623002/diff/40001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/118623002/diff/40001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:184: const unsigned int kAesKwIvLength = 8; Can you instead define this in terms of: arraysize(kAesIv) And let the compiler infer the size below. https://codereview.chromium.org/118623002/diff/40001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:189: SECItem* param_item = PK11_ParamFromIV(kAesKwMechanism, &iv_item); I will have to verify this, but my expectation is that NSS will NOT copy the parameters. In which case returning this from a function is incorrect and will lead to a user-after-free. If that is the case, the lifetime of kAesIv / iv_item will need to outlive that of param_item https://codereview.chromium.org/118623002/diff/40001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:205: // TODO(padolph): Should this be a DCHECK() instead? This must be a runtime check since blink layer does not enforce it. https://codereview.chromium.org/118623002/diff/40001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:225: const unsigned int output_length = data_size + 8; Paranoia: possibility of integer overflow? I should hook up some safe integer helpers for use in this file. I guess in this case the defined behavior is wraparound and it will just cause the operation to fail from insufficient buffer. https://codereview.chromium.org/118623002/diff/40001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:240: DCHECK_EQ(output_length, wrapped_key_item.len); Please make this a runtime check and fail if they don't match. Want to be as defensive as possible when calling into libraries. https://codereview.chromium.org/118623002/diff/40001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:267: const unsigned int plaintext_length = data_size - 8; Isn't it possible to have 0-length keys? In fact we have HMAC unittests with 0-byte keys. In that case, subtracting 8 would be bad. What you have as a DCHECK above should be a runtime check instead. https://codereview.chromium.org/118623002/diff/40001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:643: switch (algorithm.id()) { switching to a switch statement would best be left to a separate refactor CL. It makes the differences hard to read here. https://codereview.chromium.org/118623002/diff/40001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:703: switch (algorithm.id()) { Same comment.
https://codereview.chromium.org/118623002/diff/40001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/118623002/diff/40001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:184: const unsigned int kAesKwIvLength = 8; On 2013/12/20 01:58:14, eroman wrote: > Can you instead define this in terms of: > arraysize(kAesIv) > > And let the compiler infer the size below. Done. https://codereview.chromium.org/118623002/diff/40001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:189: SECItem* param_item = PK11_ParamFromIV(kAesKwMechanism, &iv_item); On 2013/12/20 01:58:14, eroman wrote: > I will have to verify this, but my expectation is that NSS will NOT copy the > parameters. > > In which case returning this from a function is incorrect and will lead to a > user-after-free. > > If that is the case, the lifetime of kAesIv / iv_item will need to outlive that > of param_item This function was not saving too much code duplication anyway. Moved back to in line. https://codereview.chromium.org/118623002/diff/40001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:205: // TODO(padolph): Should this be a DCHECK() instead? On 2013/12/20 01:58:14, eroman wrote: > This must be a runtime check since blink layer does not enforce it. Done. https://codereview.chromium.org/118623002/diff/40001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:225: const unsigned int output_length = data_size + 8; On 2013/12/20 01:58:14, eroman wrote: > Paranoia: possibility of integer overflow? I should hook up some safe integer > helpers for use in this file. I guess in this case the defined behavior is > wraparound and it will just cause the operation to fail from insufficient > buffer. RFC 3394 does not specify the max allowed input length, so I added code to fail if the size exceeeds whatever fits into an unsigned. This is reasonable since we are only ever wrapping keys, which are generally small. https://codereview.chromium.org/118623002/diff/40001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:240: DCHECK_EQ(output_length, wrapped_key_item.len); On 2013/12/20 01:58:14, eroman wrote: > Please make this a runtime check and fail if they don't match. Want to be as > defensive as possible when calling into libraries. Done. https://codereview.chromium.org/118623002/diff/40001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:267: const unsigned int plaintext_length = data_size - 8; On 2013/12/20 01:58:14, eroman wrote: > Isn't it possible to have 0-length keys? In fact we have HMAC unittests with > 0-byte keys. > > In that case, subtracting 8 would be bad. > > What you have as a DCHECK above should be a runtime check instead. Done. https://codereview.chromium.org/118623002/diff/40001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:643: switch (algorithm.id()) { On 2013/12/20 01:58:14, eroman wrote: > switching to a switch statement would best be left to a separate refactor CL. It > makes the differences hard to read here. Reverted and added a TODO. https://codereview.chromium.org/118623002/diff/40001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:703: switch (algorithm.id()) { On 2013/12/20 01:58:14, eroman wrote: > Same comment. Reverted and added a TODO.
It looks good to me, but I do have one concern about the assumption of a key being symmetric - see comment. https://codereview.chromium.org/118623002/diff/60002/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/118623002/diff/60002/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:211: // generic secret blob, since we can't be certain what it is at this point. Will we be hitting any issues with unwrapping a non-symmetric key? What I'm thinking is that they ImportSymKey might fail, at some point, to support such a large symmetric key. Currently, the unit test assumes the only keys wrapped/unwrapped are symmetric, so it might be worth checking to add a 2048-bit RSA key to this test. What do you think?
https://codereview.chromium.org/118623002/diff/60002/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/118623002/diff/60002/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:211: // generic secret blob, since we can't be certain what it is at this point. On 2014/01/15 19:30:17, Bryan Eyler wrote: > Will we be hitting any issues with unwrapping a non-symmetric key? What I'm > thinking is that they ImportSymKey might fail, at some point, to support such a > large symmetric key. Currently, the unit test assumes the only keys > wrapped/unwrapped are symmetric, so it might be worth checking to add a 2048-bit > RSA key to this test. What do you think? Good point. I don't think using an asym key as an input to PK11_ImportSymKey should be a problem since CKK_GENERIC_SECRET says the input is raw data. But you are right, NSS might have some max allowed size PK11_ImportSymKey. I'll add a big RSA key (or some other large data blob) as you suggest to the unit tests to find out.
https://codereview.chromium.org/118623002/diff/60002/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/118623002/diff/60002/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:211: // generic secret blob, since we can't be certain what it is at this point. On 2014/01/15 22:58:52, padolph wrote: > On 2014/01/15 19:30:17, Bryan Eyler wrote: > > Will we be hitting any issues with unwrapping a non-symmetric key? What I'm > > thinking is that they ImportSymKey might fail, at some point, to support such > a > > large symmetric key. Currently, the unit test assumes the only keys > > wrapped/unwrapped are symmetric, so it might be worth checking to add a > 2048-bit > > RSA key to this test. What do you think? > > Good point. I don't think using an asym key as an input to PK11_ImportSymKey > should be a problem since CKK_GENERIC_SECRET says the input is raw data. But you > are right, NSS might have some max allowed size PK11_ImportSymKey. I'll add a > big RSA key (or some other large data blob) as you suggest to the unit tests to > find out. I have asked rsleevi to comment on this, hopefully he will be able to tomorrow. I agree that layering the data through a temporary SymKey doesn't seem like the right abstraction. My naive thinking is would it work to use the same encryption code as AES-CBC, but pass the AES-KW iv, and set mechanism to CKM_NSS_AES_KEY_WRAP?
Not LGTM from a layering/design perspective. We shouldn't be forcing keys within NSS to be converted. https://codereview.chromium.org/118623002/diff/160001/content/renderer/webcry... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/118623002/diff/160001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_nss.cc:376: const CK_MECHANISM_TYPE kAesKwMechanism = CKM_NSS_AES_KEY_WRAP; I don't see there being any benefit from adding this constant. Just use the CKM value https://codereview.chromium.org/118623002/diff/160001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_nss.cc:378: const unsigned char kAesIv[] = {0xA6, 0xA6, 0xA6, 0xA6, 0xA6, 0xA6, 0xA6, 0xA6}; You can make this a function-local const. https://codereview.chromium.org/118623002/diff/160001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_nss.cc:385: blink::WebArrayBuffer* buffer) { From an API design perspective, having the AES-KW take a blanket set of data to use is not ideal, because it defeats one of the core designs of the WebCrypto API - that is, to support keeping keys within a cryptographic boundary. The internal AesKw function should take a WebCryptoKey. If the WebCryptoKey is an NSS key handle, it should use the NSS key functions directly. If it's not (for example, a JWK), then it should convert into the appropriate format. https://codereview.chromium.org/118623002/diff/160001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_nss.cc:393: DCHECK(data); Why not DCHECK on line 389? https://codereview.chromium.org/118623002/diff/160001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_nss.cc:402: // generic secret blob, since we can't be certain what it is at this point. Pronouns considered harmful: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/NH-S6KCkr2M/yv0z3...
On 2014/01/16 23:41:14, Ryan Sleevi (OOO until 2-21) wrote: > Not LGTM from a layering/design perspective. We shouldn't be forcing keys within > NSS to be converted. > > https://codereview.chromium.org/118623002/diff/160001/content/renderer/webcry... > content/renderer/webcrypto/webcrypto_impl_nss.cc:385: blink::WebArrayBuffer* > buffer) { > From an API design perspective, having the AES-KW take a blanket set of data to > use is not ideal, because it defeats one of the core designs of the WebCrypto > API - that is, to support keeping keys within a cryptographic boundary. > > The internal AesKw function should take a WebCryptoKey. If the WebCryptoKey is > an NSS key handle, it should use the NSS key functions directly. If it's not > (for example, a JWK), then it should convert into the appropriate format. I see your point about layering, but I think it only applies to one special case: Wrapping a symmetric key in raw format. For all other formats (jwk, spki, pkcs8), the WebCryptoKey is indeed inside NSS, but needs to be pulled out and transformed to a string of bytes according to the desired format, and then sent back into NSS for encryption. AFAICT NSS does not provide any algorithms besides AES-KW that operate on internal NSS key data. I think that is what you mean by your statement: "If it's not (for example, a JWK), then it should convert into the appropriate format." So the code is acting how you want except for that special case of AES-KW + Symmetric key + raw format, correct? So a fix would be to add a new internal function that takes a WebCryptoKey and applies AES-KW on it directly. This new function would be called from wrap() iff the wrapping alg is AES-KW, the key to be wrapped is a symmetric key, and the format is raw. When this special case does not apply, wrap will do a more generic export + encrypt operation. Do you agree?
https://codereview.chromium.org/118623002/diff/160001/content/renderer/webcry... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/118623002/diff/160001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_nss.cc:376: const CK_MECHANISM_TYPE kAesKwMechanism = CKM_NSS_AES_KEY_WRAP; On 2014/01/16 23:41:15, Ryan Sleevi (OOO until 2-21) wrote: > I don't see there being any benefit from adding this constant. Just use the CKM > value Done. https://codereview.chromium.org/118623002/diff/160001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_nss.cc:378: const unsigned char kAesIv[] = {0xA6, 0xA6, 0xA6, 0xA6, 0xA6, 0xA6, 0xA6, 0xA6}; On 2014/01/16 23:41:15, Ryan Sleevi (OOO until 2-21) wrote: > You can make this a function-local const. This is used in two separate functions. I don't want to have duplicate function-local consts. https://codereview.chromium.org/118623002/diff/160001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_nss.cc:393: DCHECK(data); On 2014/01/16 23:41:15, Ryan Sleevi (OOO until 2-21) wrote: > Why not DCHECK on line 389? Eric prefers this pattern. You won't hit the check if you for some reason want to call the function with zero data size and null for data. https://codereview.chromium.org/118623002/diff/160001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_nss.cc:402: // generic secret blob, since we can't be certain what it is at this point. On 2014/01/16 23:41:15, Ryan Sleevi (OOO until 2-21) wrote: > Pronouns considered harmful: > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/NH-S6KCkr2M/yv0z3... Done.
https://codereview.chromium.org/118623002/diff/250001/content/renderer/webcry... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/118623002/diff/250001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_nss.cc:399: SECItem iv_item = {siBuffer, const_cast<unsigned char*>(kAesIv), there is a helper for this once you rebase https://codereview.chromium.org/118623002/diff/250001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_nss.cc:404: DCHECK(param_item); Not sure if this can be reached, perhaps make it a return Status::ErrorUnexpected() if in doubt https://codereview.chromium.org/118623002/diff/250001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_nss.cc:406: // Turn the data to be wrapped into a PK11SymKey in order to use the NSS I chatted with Sleevi and he suggested just starting with sym keys. I am fine with this changelist if we restrict it to just wrap/unwrap of symkey using PK11_WrapSymKey(). That said I don't quite understand the consequences of using the symkey as a delivery for arbitrary key data. https://codereview.chromium.org/118623002/diff/250001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_nss.cc:410: SECItem data_item = {siBuffer, const_cast<unsigned char*>(data), data_size}; there is a helper for this once you rebase https://codereview.chromium.org/118623002/diff/250001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_nss.cc:425: SECItem wrapped_key_item = {siBuffer, buffer_data, output_length}; there is a helper for this once you rebase https://codereview.chromium.org/118623002/diff/250001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_nss.cc:427: SymKeyHandle* wrapping_key = reinterpret_cast<SymKeyHandle*>(key.handle()); This has changed from my recent refactors. Instead in shared_crypto.cc you will want to call ToPlatformSymKey() to safely cast https://codereview.chromium.org/118623002/diff/250001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_nss.cc:437: buffer->reset(); I wouldn't worry about calling reset in the error cases (the memory is auto-freed) https://codereview.chromium.org/118623002/diff/250001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_nss.cc:458: else if (data_size % 8 != 0) nit: no "else" after a return. Also above you omit the "!= 0" (be consistent; i don't remember what we do elsewhere in this file) https://codereview.chromium.org/118623002/diff/250001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_nss.cc:462: SymKeyHandle* wrapping_key = reinterpret_cast<SymKeyHandle*>(key.handle()); ToPlatformSymKey() after you rebase https://codereview.chromium.org/118623002/diff/250001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_nss.cc:464: SECItem iv_item = {siBuffer, const_cast<unsigned char*>(kAesIv), there is a helper for this once you rebase https://codereview.chromium.org/118623002/diff/250001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_nss.cc:487: if (PK11_ExtractKeyValue(unwrapped_key.get()) != SECSuccess) Per earlier comment, let's restrict this to just symkeys, so extracting the value is not required https://codereview.chromium.org/118623002/diff/250001/content/renderer/webcry... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/118623002/diff/250001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_unittest.cc:2084: "000102030405060708090A0B0C0D0E0F", With recent refactors I have been moving the test data into: content/test/data/webcrypto as .json files After re-basing please follow that same model for this new data. (simplifies re-using it with the layouttests too). https://codereview.chromium.org/118623002/diff/250001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_unittest.cc:2122: for (size_t index = 0; index < ARRAYSIZE_UNSAFE(kTests); index++) { please rename test_index to match other places in this file. https://codereview.chromium.org/118623002/diff/250001/content/renderer/webcry... File content/renderer/webcrypto/webcrypto_util.cc (right): https://codereview.chromium.org/118623002/diff/250001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_util.cc:136: return Status("The data length is invalid: not a multiple of 8 bytes"); I suggest adding AES-KW somewhere in there (The way these errors are displayed is less than ideal: they show up in the devtools console. So the more specific the better)
https://codereview.chromium.org/118623002/diff/250001/content/renderer/webcry... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/118623002/diff/250001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_nss.cc:399: SECItem iv_item = {siBuffer, const_cast<unsigned char*>(kAesIv), On 2014/02/26 23:40:22, eroman wrote: > there is a helper for this once you rebase Done. https://codereview.chromium.org/118623002/diff/250001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_nss.cc:404: DCHECK(param_item); On 2014/02/26 23:40:22, eroman wrote: > Not sure if this can be reached, perhaps make it a return > Status::ErrorUnexpected() if in doubt PK11_ParamFromIV can return NULL in the case of an allocation error. Changed to return Status::ErrorUnexpected(). https://codereview.chromium.org/118623002/diff/250001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_nss.cc:406: // Turn the data to be wrapped into a PK11SymKey in order to use the NSS On 2014/02/26 23:40:22, eroman wrote: > I chatted with Sleevi and he suggested just starting with sym keys. I am fine > with this changelist if we restrict it to just wrap/unwrap of symkey using > PK11_WrapSymKey(). That said I don't quite understand the consequences of using > the symkey as a delivery for arbitrary key data. Ok, this is in line with my earlier question to Ryan. But that changes this CL significantly, so I don't quite get your statement about being fine with the current CL. I will remove the AesKwEncrypt / AesKwDecrypt functions, and replace them with something like AesKwWrapSymKey / AesKwUnwrapSymkey, and change the calling code to invoke these only in the specific case of wrap/unwrap with raw format + symmkey. I wonder if it might be better to just start a new CL. Note that we cannot get to the finish line though until we figure out how to at least unwrap formats other than raw (specifically JWK), so please keep thinking about this. https://codereview.chromium.org/118623002/diff/250001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_nss.cc:410: SECItem data_item = {siBuffer, const_cast<unsigned char*>(data), data_size}; On 2014/02/26 23:40:22, eroman wrote: > there is a helper for this once you rebase Done. https://codereview.chromium.org/118623002/diff/250001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_nss.cc:425: SECItem wrapped_key_item = {siBuffer, buffer_data, output_length}; On 2014/02/26 23:40:22, eroman wrote: > there is a helper for this once you rebase Done. https://codereview.chromium.org/118623002/diff/250001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_nss.cc:427: SymKeyHandle* wrapping_key = reinterpret_cast<SymKeyHandle*>(key.handle()); On 2014/02/26 23:40:22, eroman wrote: > This has changed from my recent refactors. Instead in shared_crypto.cc you will > want to call ToPlatformSymKey() to safely cast Done. https://codereview.chromium.org/118623002/diff/250001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_nss.cc:437: buffer->reset(); On 2014/02/26 23:40:22, eroman wrote: > I wouldn't worry about calling reset in the error cases (the memory is > auto-freed) Done. https://codereview.chromium.org/118623002/diff/250001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_nss.cc:458: else if (data_size % 8 != 0) On 2014/02/26 23:40:22, eroman wrote: > nit: no "else" after a return. Also above you omit the "!= 0" (be consistent; i > don't remember what we do elsewhere in this file) Done. https://codereview.chromium.org/118623002/diff/250001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_nss.cc:462: SymKeyHandle* wrapping_key = reinterpret_cast<SymKeyHandle*>(key.handle()); On 2014/02/26 23:40:22, eroman wrote: > ToPlatformSymKey() after you rebase Done. https://codereview.chromium.org/118623002/diff/250001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_nss.cc:464: SECItem iv_item = {siBuffer, const_cast<unsigned char*>(kAesIv), On 2014/02/26 23:40:22, eroman wrote: > there is a helper for this once you rebase Done. https://codereview.chromium.org/118623002/diff/250001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_nss.cc:487: if (PK11_ExtractKeyValue(unwrapped_key.get()) != SECSuccess) On 2014/02/26 23:40:22, eroman wrote: > Per earlier comment, let's restrict this to just symkeys, so extracting the > value is not required This call is required even on a symkey. Otherwise the subsequent call to PK11_GetKeyData() does not work. https://codereview.chromium.org/118623002/diff/250001/content/renderer/webcry... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/118623002/diff/250001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_unittest.cc:2122: for (size_t index = 0; index < ARRAYSIZE_UNSAFE(kTests); index++) { On 2014/02/26 23:40:22, eroman wrote: > please rename test_index to match other places in this file. Done. https://codereview.chromium.org/118623002/diff/250001/content/renderer/webcry... File content/renderer/webcrypto/webcrypto_util.cc (right): https://codereview.chromium.org/118623002/diff/250001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_util.cc:136: return Status("The data length is invalid: not a multiple of 8 bytes"); On 2014/02/26 23:40:22, eroman wrote: > I suggest adding AES-KW somewhere in there > > (The way these errors are displayed is less than ideal: they show up in the > devtools console. So the more specific the better) Done.
L G T M. Ryan, can you review this? https://codereview.chromium.org/118623002/diff/320001/content/renderer/webcry... File content/renderer/webcrypto/platform_crypto.h (right): https://codereview.chromium.org/118623002/diff/320001/content/renderer/webcry... content/renderer/webcrypto/platform_crypto.h:190: // * |wrapped_key_data| is at least 24 bytes and a multiple of 8 bytes This is not currently guranteed as the checks are done in platform_crypto_nss.cc rather than shared_crypto.cc. Please move the checks over to shared_crypto.cc so this comment holds. https://codereview.chromium.org/118623002/diff/320001/content/renderer/webcry... File content/renderer/webcrypto/platform_crypto_nss.cc (right): https://codereview.chromium.org/118623002/diff/320001/content/renderer/webcry... content/renderer/webcrypto/platform_crypto_nss.cc:1229: #ifdef WEBCRYPTO_HAS_KEY_ALGORITHM No need for the ifdef anymore https://codereview.chromium.org/118623002/diff/320001/content/renderer/webcry... File content/renderer/webcrypto/shared_crypto.cc (right): https://codereview.chromium.org/118623002/diff/320001/content/renderer/webcry... content/renderer/webcrypto/shared_crypto.cc:546: Status Unwrapkey(blink::WebCryptoKeyFormat format, nit: Can you capitalize Key https://codereview.chromium.org/118623002/diff/320001/content/renderer/webcry... content/renderer/webcrypto/shared_crypto.cc:559: // TODO (padolph): Handle formats other than raw nit: remove space after TODO
https://codereview.chromium.org/118623002/diff/320001/content/renderer/webcry... File content/renderer/webcrypto/platform_crypto.h (right): https://codereview.chromium.org/118623002/diff/320001/content/renderer/webcry... content/renderer/webcrypto/platform_crypto.h:190: // * |wrapped_key_data| is at least 24 bytes and a multiple of 8 bytes On 2014/03/01 01:13:54, eroman wrote: > This is not currently guranteed as the checks are done in platform_crypto_nss.cc > rather than shared_crypto.cc. Please move the checks over to shared_crypto.cc so > this comment holds. Done. https://codereview.chromium.org/118623002/diff/320001/content/renderer/webcry... File content/renderer/webcrypto/platform_crypto_nss.cc (right): https://codereview.chromium.org/118623002/diff/320001/content/renderer/webcry... content/renderer/webcrypto/platform_crypto_nss.cc:1229: #ifdef WEBCRYPTO_HAS_KEY_ALGORITHM On 2014/03/01 01:13:54, eroman wrote: > No need for the ifdef anymore Done. https://codereview.chromium.org/118623002/diff/320001/content/renderer/webcry... File content/renderer/webcrypto/shared_crypto.cc (right): https://codereview.chromium.org/118623002/diff/320001/content/renderer/webcry... content/renderer/webcrypto/shared_crypto.cc:546: Status Unwrapkey(blink::WebCryptoKeyFormat format, On 2014/03/01 01:13:54, eroman wrote: > nit: Can you capitalize Key Done. https://codereview.chromium.org/118623002/diff/320001/content/renderer/webcry... content/renderer/webcrypto/shared_crypto.cc:559: // TODO (padolph): Handle formats other than raw On 2014/03/01 01:13:54, eroman wrote: > nit: remove space after TODO Done.
lgtm https://codereview.chromium.org/118623002/diff/360001/content/renderer/webcry... File content/renderer/webcrypto/platform_crypto_nss.cc (right): https://codereview.chromium.org/118623002/diff/360001/content/renderer/webcry... content/renderer/webcrypto/platform_crypto_nss.cc:503: // possible operations for this key type. This comment needs to be moved. |flags| is no longer configured here, but up around line 454. https://codereview.chromium.org/118623002/diff/360001/content/renderer/webcry... content/renderer/webcrypto/platform_crypto_nss.cc:1130: MakeSECItemForBuffer(CryptoData(kAesIv, ARRAYSIZE_UNSAFE(kAesIv))); sizeof, not ARRAYSIZE - you want the actual byte size of kAesIv https://codereview.chromium.org/118623002/diff/360001/content/renderer/webcry... content/renderer/webcrypto/platform_crypto_nss.cc:1160: DCHECK(wrapped_key_data.byte_length() >= 24); DCHECK_GE https://codereview.chromium.org/118623002/diff/360001/content/renderer/webcry... content/renderer/webcrypto/platform_crypto_nss.cc:1161: DCHECK(wrapped_key_data.byte_length() % 8 == 0); DCHECK_EQ https://codereview.chromium.org/118623002/diff/360001/content/renderer/webcry... content/renderer/webcrypto/platform_crypto_nss.cc:1164: MakeSECItemForBuffer(CryptoData(kAesIv, ARRAYSIZE_UNSAFE(kAesIv))); sizeof https://codereview.chromium.org/118623002/diff/360001/content/renderer/webcry... content/renderer/webcrypto/platform_crypto_nss.cc:1191: return Status::Error(); Future CL idea: Support logging PORT_GetError() and friends to figure out _why_ it failed. https://codereview.chromium.org/118623002/diff/360001/content/renderer/webcry... File content/renderer/webcrypto/shared_crypto_unittest.cc (right): https://codereview.chromium.org/118623002/diff/360001/content/renderer/webcry... content/renderer/webcrypto/shared_crypto_unittest.cc:2149: } Add a test for mutating the data, and ensuring that decryption of some known answer fails / the key is different. AES-KW is supposed to prevent data from being tweaked.
https://codereview.chromium.org/118623002/diff/360001/content/renderer/webcry... File content/renderer/webcrypto/platform_crypto_nss.cc (right): https://codereview.chromium.org/118623002/diff/360001/content/renderer/webcry... content/renderer/webcrypto/platform_crypto_nss.cc:503: // possible operations for this key type. On 2014/03/01 02:40:13, Ryan Sleevi wrote: > This comment needs to be moved. |flags| is no longer configured here, but up > around line 454. Done. https://codereview.chromium.org/118623002/diff/360001/content/renderer/webcry... content/renderer/webcrypto/platform_crypto_nss.cc:1130: MakeSECItemForBuffer(CryptoData(kAesIv, ARRAYSIZE_UNSAFE(kAesIv))); On 2014/03/01 02:40:13, Ryan Sleevi wrote: > sizeof, not ARRAYSIZE - you want the actual byte size of kAesIv Done. https://codereview.chromium.org/118623002/diff/360001/content/renderer/webcry... content/renderer/webcrypto/platform_crypto_nss.cc:1160: DCHECK(wrapped_key_data.byte_length() >= 24); On 2014/03/01 02:40:13, Ryan Sleevi wrote: > DCHECK_GE Done. https://codereview.chromium.org/118623002/diff/360001/content/renderer/webcry... content/renderer/webcrypto/platform_crypto_nss.cc:1161: DCHECK(wrapped_key_data.byte_length() % 8 == 0); On 2014/03/01 02:40:13, Ryan Sleevi wrote: > DCHECK_EQ Done. https://codereview.chromium.org/118623002/diff/360001/content/renderer/webcry... content/renderer/webcrypto/platform_crypto_nss.cc:1164: MakeSECItemForBuffer(CryptoData(kAesIv, ARRAYSIZE_UNSAFE(kAesIv))); On 2014/03/01 02:40:13, Ryan Sleevi wrote: > sizeof Done. https://codereview.chromium.org/118623002/diff/360001/content/renderer/webcry... content/renderer/webcrypto/platform_crypto_nss.cc:1191: return Status::Error(); On 2014/03/01 02:40:13, Ryan Sleevi wrote: > Future CL idea: Support logging PORT_GetError() and friends to figure out _why_ > it failed. Good idea. https://codereview.chromium.org/118623002/diff/360001/content/renderer/webcry... File content/renderer/webcrypto/shared_crypto_unittest.cc (right): https://codereview.chromium.org/118623002/diff/360001/content/renderer/webcry... content/renderer/webcrypto/shared_crypto_unittest.cc:2149: } On 2014/03/01 02:40:13, Ryan Sleevi wrote: > Add a test for mutating the data, and ensuring that decryption of some known > answer fails / the key is different. AES-KW is supposed to prevent data from > being tweaked. Done.
The CQ bit was checked by padolph@netflix.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/padolph@netflix.com/118623002/390001
lgtm https://codereview.chromium.org/118623002/diff/390001/content/renderer/webcry... File content/renderer/webcrypto/platform_crypto_nss.cc (right): https://codereview.chromium.org/118623002/diff/390001/content/renderer/webcry... content/renderer/webcrypto/platform_crypto_nss.cc:463: const blink::WebCryptoAlgorithm& hash = GetInnerHashAlgorithm(algorithm); This shouldn't be a reference (pre-exists this CL which is just a move). I guess leave as is, we will correct in a separate changelist. https://codereview.chromium.org/118623002/diff/390001/content/renderer/webcry... content/renderer/webcrypto/platform_crypto_nss.cc:1190: // TODO(padolph): Use NSS PORT_GetError() and friends to report more accurate Sounds good in principle. However I would need a better understanding of what sort of error messages NSS gives before we surface them to the web platform (it shouldn't leak any information to web pages about other web crypto users, key details, etc.) https://codereview.chromium.org/118623002/diff/390001/content/renderer/webcry... File content/renderer/webcrypto/shared_crypto_unittest.cc (right): https://codereview.chromium.org/118623002/diff/390001/content/renderer/webcry... content/renderer/webcrypto/shared_crypto_unittest.cc:148: void ExpectArrayBufferNotMatch(const std::vector<uint8>& expected, For a future change, the right fix is to change this to return an ::Assertion... and just have the one flavor. It also helps with the line numbers then (since failure line isn't reported as this function).
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_x64_rel for step(s) base_unittests, chrome_elf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
https://codereview.chromium.org/118623002/diff/390001/content/renderer/webcry... File content/renderer/webcrypto/platform_crypto_nss.cc (right): https://codereview.chromium.org/118623002/diff/390001/content/renderer/webcry... content/renderer/webcrypto/platform_crypto_nss.cc:463: const blink::WebCryptoAlgorithm& hash = GetInnerHashAlgorithm(algorithm); On 2014/03/04 03:16:26, eroman wrote: > This shouldn't be a reference (pre-exists this CL which is just a move). I guess > leave as is, we will correct in a separate changelist. Done. https://codereview.chromium.org/118623002/diff/390001/content/renderer/webcry... content/renderer/webcrypto/platform_crypto_nss.cc:1190: // TODO(padolph): Use NSS PORT_GetError() and friends to report more accurate On 2014/03/04 03:16:26, eroman wrote: > Sounds good in principle. However I would need a better understanding of what > sort of error messages NSS gives before we surface them to the web platform (it > shouldn't leak any information to web pages about other web crypto users, key > details, etc.) Updated TODO with this concern. https://codereview.chromium.org/118623002/diff/390001/content/renderer/webcry... File content/renderer/webcrypto/shared_crypto_unittest.cc (right): https://codereview.chromium.org/118623002/diff/390001/content/renderer/webcry... content/renderer/webcrypto/shared_crypto_unittest.cc:148: void ExpectArrayBufferNotMatch(const std::vector<uint8>& expected, On 2014/03/04 03:16:26, eroman wrote: > For a future change, the right fix is to change this to return an ::Assertion... > and just have the one flavor. It also helps with the line numbers then (since > failure line isn't reported as this function). Done.
The CQ bit was checked by padolph@netflix.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/padolph@netflix.com/118623002/420001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by eroman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/padolph@netflix.com/118623002/420001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/padolph@netflix.com/118623002/420001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/padolph@netflix.com/118623002/420001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/padolph@netflix.com/118623002/420001
Message was sent while issue was closed.
Change committed as 255270 |
