Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(933)

Issue 2544533002: Have all overloads of webcrypto::AlgorithmImplementation::DeserializeKeyForClone check the params t… (Closed)

Created:
4 years ago by jbroman
Modified:
4 years ago
Reviewers:
haraken, eroman
CC:
chromium-reviews
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Have all overloads of webcrypto::AlgorithmImplementation::DeserializeKeyForClone check the params type. Presently neither this nor the calling code checks that the algorithm ID and key params type correspond correctly. This code already knows what the expected value is, so it seems a reasonable place to check. BUG=669649 Committed: https://crrev.com/2e33aac2a71771c2e5780d4e2abb95540760fdcf Cr-Commit-Position: refs/heads/master@{#435528}

Patch Set 1 #

Patch Set 2 : add a test for a case similar to the one the fuzzer found #

Total comments: 4

Patch Set 3 : additional checks, per eroman #

Unified diffs Side-by-side diffs Delta from patch set Stats (+32 lines, -0 lines) Patch
M components/webcrypto/algorithms/aes.cc View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
M components/webcrypto/algorithms/ec.cc View 1 chunk +3 lines, -0 lines 0 comments Download
M components/webcrypto/algorithms/hkdf.cc View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
M components/webcrypto/algorithms/hmac.cc View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
M components/webcrypto/algorithms/pbkdf2.cc View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
M components/webcrypto/algorithms/rsa.cc View 1 chunk +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/bindings/modules/v8/serialization/V8ScriptValueSerializerForModulesTest.cpp View 1 1 chunk +10 lines, -0 lines 0 comments Download

Messages

Total messages: 25 (17 generated)
jbroman
4 years ago (2016-11-30 18:11:10 UTC) #7
eroman
Thanks, looks great! I have made some optional suggestions on another checks to add -- ...
4 years ago (2016-11-30 19:15:50 UTC) #10
eroman
lgtm
4 years ago (2016-11-30 19:16:00 UTC) #11
jbroman
Done. +haraken for the unit test change
4 years ago (2016-11-30 19:30:48 UTC) #15
haraken
LGTM
4 years ago (2016-11-30 23:31:46 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2544533002/40001
4 years ago (2016-12-01 00:44:18 UTC) #21
commit-bot: I haz the power
Committed patchset #3 (id:40001)
4 years ago (2016-12-01 02:12:02 UTC) #23
commit-bot: I haz the power
4 years ago (2016-12-01 02:15:33 UTC) #25
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/2e33aac2a71771c2e5780d4e2abb95540760fdcf
Cr-Commit-Position: refs/heads/master@{#435528}

Powered by Google App Engine
This is Rietveld 408576698