Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
141853006:
[webcrypto] Validate JWK import of AES keys: key length must match algorithm. (Closed)
|
Created:
6 years, 10 months ago by eroman Modified:
6 years, 10 months ago Reviewers:
Ryan Sleevi CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[webcrypto] Validate JWK import of AES keys: key length must match algorithm.
BUG=245025
R=rsleevi@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=248834
Patch Set 1 #
Total comments: 6
Patch Set 2 : Rebase #Patch Set 3 : Change ordering of key length #
Total comments: 4
Patch Set 4 : fix ctor initializer list #Patch Set 5 : Hopefully fix android compile error (can't infer bool type) #Patch Set 6 : #
Messages
Total messages: 30 (0 generated)
https://codereview.chromium.org/141853006/diff/1/content/renderer/webcrypto/w... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/141853006/diff/1/content/renderer/webcrypto/w... content/renderer/webcrypto/webcrypto_impl.cc:43: class JwkAlgorithmInfo { Can we not avoid having to create this class by making use of templated indirection? You're allowed to specify integer constants as part of template parameters, so you should be able to bind "required_key_size_length"
https://codereview.chromium.org/141853006/diff/1/content/renderer/webcrypto/w... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/141853006/diff/1/content/renderer/webcrypto/w... content/renderer/webcrypto/webcrypto_impl.cc:43: class JwkAlgorithmInfo { On 2014/01/31 20:55:59, Ryan Sleevi wrote: > Can we not avoid having to create this class by making use of templated > indirection? > > You're allowed to specify integer constants as part of template parameters, so > you should be able to bind "required_key_size_length" Not sure I follow, can u explain further?
https://codereview.chromium.org/141853006/diff/1/content/renderer/webcrypto/w... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/141853006/diff/1/content/renderer/webcrypto/w... content/renderer/webcrypto/webcrypto_impl.cc:43: class JwkAlgorithmInfo { On 2014/01/31 21:02:50, eroman wrote: > On 2014/01/31 20:55:59, Ryan Sleevi wrote: > > Can we not avoid having to create this class by making use of templated > > indirection? > > > > You're allowed to specify integer constants as part of template parameters, so > > you should be able to bind "required_key_size_length" > > Not sure I follow, can u explain further? Well, I was thinking you could smuggle required_key_length_bytes_ as a template parameter, and instead of returning a blink::WebCryptoAlgorithm, you return a webcrypto::Status and fill in a blink::WebCryptoAlgorithm* iff the key length matches the required key length. That said, it's really unfortunate that the JWK requirements have to live in the embedder side - it seems like the whole mapping of JWK algs to Blink algs is something better done on the blink side? IDK https://codereview.chromium.org/141853006/diff/1/content/renderer/webcrypto/w... content/renderer/webcrypto/webcrypto_impl.cc:70: return required_key_length_bytes_ != byte_length; nit: delete extra " " https://codereview.chromium.org/141853006/diff/1/content/renderer/webcrypto/w... content/renderer/webcrypto/webcrypto_impl.cc:79: AlgorithmCreationFunc creation_func_; pedantic nit: It seems a little structurally cleaner to have creation_func_ first (since that's common for all) and then any *optional* parameters (such as required_key_length_bytes_) follow afterwards.
https://codereview.chromium.org/141853006/diff/1/content/renderer/webcrypto/w... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/141853006/diff/1/content/renderer/webcrypto/w... content/renderer/webcrypto/webcrypto_impl.cc:43: class JwkAlgorithmInfo { On 2014/01/31 21:15:09, Ryan Sleevi wrote: > On 2014/01/31 21:02:50, eroman wrote: > > On 2014/01/31 20:55:59, Ryan Sleevi wrote: > > > Can we not avoid having to create this class by making use of templated > > > indirection? > > > > > > You're allowed to specify integer constants as part of template parameters, > so > > > you should be able to bind "required_key_size_length" > > > > Not sure I follow, can u explain further? > > Well, I was thinking you could smuggle required_key_length_bytes_ as a template > parameter, and instead of returning a blink::WebCryptoAlgorithm, you return a > webcrypto::Status and fill in a blink::WebCryptoAlgorithm* iff the key length > matches the required key length. > > That said, it's really unfortunate that the JWK requirements have to live in the > embedder side - it seems like the whole mapping of JWK algs to Blink algs is > something better done on the blink side? IDK I am open to moving more JWK to the blink side, some of the issues are: - Export to JWK would likely mainly have to be on the chromium side, otherwise would need to teach blink how to convert some other key representation to JWK. If exports lives in Chromium, would be asymetric to have import on the blink side since a different set of options might be understood. - Key wrap/unwrap for JWK formats would have to jump back over to blink assuming that the core unwrap/wrap was happening on chromium side. Not sure I see a blink-only JWK solution that would be cleaner, but will think about some more.
ping
On 2014/02/04 01:41:04, eroman wrote: > ping ping on pedantiry of ordering required -> optional
Done.
lgtm https://codereview.chromium.org/141853006/diff/190001/content/renderer/webcry... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/141853006/diff/190001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl.cc:45: JwkAlgorithmInfo() : style: : creation_func_(NULL), required_key_length_bytes(...) https://codereview.chromium.org/141853006/diff/190001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl.cc:75: enum {NO_KEY_SIZE_REQUIREMENT = UINT_MAX}; style: spaces after { / before }
https://codereview.chromium.org/141853006/diff/190001/content/renderer/webcry... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/141853006/diff/190001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl.cc:45: JwkAlgorithmInfo() : On 2014/02/04 02:36:35, Ryan Sleevi wrote: > style: > : creation_func_(NULL), > required_key_length_bytes(...) Done. https://codereview.chromium.org/141853006/diff/190001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl.cc:75: enum {NO_KEY_SIZE_REQUIREMENT = UINT_MAX}; On 2014/02/04 02:36:35, Ryan Sleevi wrote: > style: spaces after { / before } Done.
The CQ bit was checked by eroman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eroman@chromium.org/141853006/250001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
The CQ bit was checked by eroman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eroman@chromium.org/141853006/250001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on android_dbg for step(s) slave_steps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
The CQ bit was checked by eroman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eroman@chromium.org/141853006/650001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on linux_rel for step(s) base_unittests, browser_tests, interactive_ui_tests, net_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
The CQ bit was checked by eroman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eroman@chromium.org/141853006/650001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on ios_dbg_simulator for step(s) url_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
The CQ bit was checked by eroman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eroman@chromium.org/141853006/650001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eroman@chromium.org/141853006/650001
Message was sent while issue was closed.
Committed patchset #6 manually as r248834 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
