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

Issue 2873373003: Change ClearKey session IDs to be random strings (Closed)

Created:
3 years, 7 months ago by jrummell
Modified:
3 years, 6 months ago
Reviewers:
hubbe, xhwang, DaleCurtis
CC:
chromium-reviews, eme-reviews_chromium.org, feature-media-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Change ClearKey session IDs to be random strings Currently AesDecryptor session IDs are numbered sequentially (e.g. "1", "2", etc.). To support persistent sessions using AesDecryptor we need random session IDs so that there is less chance that a saved ID matches an existing session ID. The new session IDs are a 8 digit random hexadecimal number followed by an 8 digit sequence number to ensure they are unique in the process. Session IDs are a printable string to make logging them easier. As AesDecryptor may be run in a sandbox (External Clear Key, used for testing), we are limited in what functions can be called. In particular, the functions in base/rand_util.h and crypto::GenerateRandomKey() can't be used. So use a trivial rand function for the random number. BUG=478960 TEST=media unit tests pass Review-Url: https://codereview.chromium.org/2873373003 Cr-Commit-Position: refs/heads/master@{#476857} Committed: https://chromium.googlesource.com/chromium/src/+/1862926fcdeb5bd96594c55e674947b9e04e111e

Patch Set 1 #

Total comments: 5

Patch Set 2 : GenerateRandomKey #

Total comments: 2

Patch Set 3 : add count (+rebase) #

Total comments: 8

Patch Set 4 : changes #

Total comments: 8

Patch Set 5 : changes #

Patch Set 6 : add length test #

Patch Set 7 : No GenerateRandomKey #

Total comments: 2

Patch Set 8 : comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+54 lines, -7 lines) Patch
M media/cdm/aes_decryptor.h View 1 chunk +0 lines, -4 lines 0 comments Download
M media/cdm/aes_decryptor.cc View 1 2 3 4 5 6 7 3 chunks +38 lines, -3 lines 0 comments Download
M media/cdm/aes_decryptor_unittest.cc View 1 2 3 4 5 6 2 chunks +16 lines, -0 lines 0 comments Download

Messages

Total messages: 52 (30 generated)
jrummell
PTAL
3 years, 7 months ago (2017-05-10 18:24:39 UTC) #2
xhwang
+hubbe, who added test_random.h. Please see my question in the comment. https://codereview.chromium.org/2873373003/diff/1/media/cdm/aes_decryptor.cc File media/cdm/aes_decryptor.cc (right): ...
3 years, 7 months ago (2017-05-10 22:25:36 UTC) #8
hubbe
https://codereview.chromium.org/2873373003/diff/1/media/cdm/aes_decryptor.cc File media/cdm/aes_decryptor.cc (right): https://codereview.chromium.org/2873373003/diff/1/media/cdm/aes_decryptor.cc#newcode62 media/cdm/aes_decryptor.cc:62: for (int i = seed % 10; i > ...
3 years, 7 months ago (2017-05-10 22:40:00 UTC) #9
DaleCurtis
Isn't this what base::UnguessableToken is for?
3 years, 7 months ago (2017-05-15 19:33:05 UTC) #11
jrummell
On 2017/05/15 19:33:05, DaleCurtis_OOO_May_5_To_May23 wrote: > Isn't this what base::UnguessableToken is for? base::UnguessableToken calls base::randbytes(), ...
3 years, 7 months ago (2017-05-15 20:20:28 UTC) #12
DaleCurtis
Is the sandbox configured correctly? I.e., are we sure we can't enable randbytes in this ...
3 years, 7 months ago (2017-05-15 21:55:11 UTC) #13
jrummell
Looks like crypto::SymmetricKey::GenerateRandomKey() works in the sandbox, so using it to create a random session ...
3 years, 7 months ago (2017-05-15 22:07:00 UTC) #14
xhwang
Thanks! Please update the CL description. Also, I have a question on using crypto::SymmetricKey::GenerateRandomKey(). https://codereview.chromium.org/2873373003/diff/20001/media/cdm/aes_decryptor.cc ...
3 years, 7 months ago (2017-05-15 23:12:40 UTC) #15
jrummell
Updated description, and comment below. https://codereview.chromium.org/2873373003/diff/20001/media/cdm/aes_decryptor.cc File media/cdm/aes_decryptor.cc (right): https://codereview.chromium.org/2873373003/diff/20001/media/cdm/aes_decryptor.cc#newcode35 media/cdm/aes_decryptor.cc:35: std::string GenerateSessionId() { On ...
3 years, 7 months ago (2017-05-16 21:18:20 UTC) #17
xhwang
On 2017/05/16 21:18:20, jrummell wrote: > Updated description, and comment below. > > https://codereview.chromium.org/2873373003/diff/20001/media/cdm/aes_decryptor.cc > ...
3 years, 7 months ago (2017-05-23 21:16:08 UTC) #18
jrummell
Updated to add |next_session_id++| to the ID.
3 years, 7 months ago (2017-05-23 23:22:49 UTC) #19
xhwang
https://codereview.chromium.org/2873373003/diff/40001/media/cdm/aes_decryptor.cc File media/cdm/aes_decryptor.cc (right): https://codereview.chromium.org/2873373003/diff/40001/media/cdm/aes_decryptor.cc#newcode37 media/cdm/aes_decryptor.cc:37: // Create a random session ID. The generated 128 ...
3 years, 7 months ago (2017-05-24 18:53:26 UTC) #20
jrummell
Updated. https://codereview.chromium.org/2873373003/diff/40001/media/cdm/aes_decryptor.cc File media/cdm/aes_decryptor.cc (right): https://codereview.chromium.org/2873373003/diff/40001/media/cdm/aes_decryptor.cc#newcode37 media/cdm/aes_decryptor.cc:37: // Create a random session ID. The generated ...
3 years, 7 months ago (2017-05-26 18:13:10 UTC) #21
xhwang
Please update the CL description, which is outdated now. https://codereview.chromium.org/2873373003/diff/60001/media/cdm/aes_decryptor.cc File media/cdm/aes_decryptor.cc (right): https://codereview.chromium.org/2873373003/diff/60001/media/cdm/aes_decryptor.cc#newcode41 media/cdm/aes_decryptor.cc:41: ...
3 years, 7 months ago (2017-05-26 23:25:48 UTC) #26
jrummell
Updated. https://codereview.chromium.org/2873373003/diff/60001/media/cdm/aes_decryptor.cc File media/cdm/aes_decryptor.cc (right): https://codereview.chromium.org/2873373003/diff/60001/media/cdm/aes_decryptor.cc#newcode41 media/cdm/aes_decryptor.cc:41: // value to ensure that the session ID ...
3 years, 7 months ago (2017-05-27 00:20:33 UTC) #28
xhwang
lgtm % nit, thanks! https://codereview.chromium.org/2873373003/diff/60001/media/cdm/aes_decryptor_unittest.cc File media/cdm/aes_decryptor_unittest.cc (right): https://codereview.chromium.org/2873373003/diff/60001/media/cdm/aes_decryptor_unittest.cc#newcode1059 media/cdm/aes_decryptor_unittest.cc:1059: EXPECT_EQ(kNumIterations, seen_sessions.size()); On 2017/05/27 00:20:33, ...
3 years, 7 months ago (2017-05-27 00:30:05 UTC) #33
jrummell
Updated test. Thanks. https://codereview.chromium.org/2873373003/diff/60001/media/cdm/aes_decryptor_unittest.cc File media/cdm/aes_decryptor_unittest.cc (right): https://codereview.chromium.org/2873373003/diff/60001/media/cdm/aes_decryptor_unittest.cc#newcode1059 media/cdm/aes_decryptor_unittest.cc:1059: EXPECT_EQ(kNumIterations, seen_sessions.size()); On 2017/05/27 00:30:05, xhwang ...
3 years, 6 months ago (2017-05-30 21:34:09 UTC) #35
jrummell
Looks like GenerateRandomKey() doesn't work in the linux sandbox (no problems locally, but the trybots ...
3 years, 6 months ago (2017-06-02 00:46:49 UTC) #43
xhwang
Thanks! Yes, please update the CL description :) LGTM % tiny nit https://codereview.chromium.org/2873373003/diff/120001/media/cdm/aes_decryptor.cc File media/cdm/aes_decryptor.cc ...
3 years, 6 months ago (2017-06-02 16:54:08 UTC) #44
jrummell
Thanks for the reviews. https://codereview.chromium.org/2873373003/diff/120001/media/cdm/aes_decryptor.cc File media/cdm/aes_decryptor.cc (right): https://codereview.chromium.org/2873373003/diff/120001/media/cdm/aes_decryptor.cc#newcode52 media/cdm/aes_decryptor.cc:52: // ever saved by External ...
3 years, 6 months ago (2017-06-02 23:24:53 UTC) #46
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/2873373003/140001
3 years, 6 months ago (2017-06-02 23:25:28 UTC) #49
commit-bot: I haz the power
3 years, 6 months ago (2017-06-03 02:12:40 UTC) #52
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/1862926fcdeb5bd96594c55e6749...

Powered by Google App Engine
This is Rietveld 408576698