Cast: Added support for AES-CTR crypto.

Cast: Added support for AES-CTR crypto.

Full audio and video frames can now be encrypted decrypted within cast.
Since AES-CTR mode is used there is no need for padding.

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

[pwestin, 2013-11-06 19:28:35]

pleas review

[Alpha Left Google, 2013-11-07 00:33:52]

Can you also add a BUG=315931 to the CL description. I'm going to review this.

https://codereview.chromium.org/62843002/diff/30001/media/cast/DEPS
File media/cast/DEPS (right):

https://codereview.chromium.org/62843002/diff/30001/media/cast/DEPS#newcode2
media/cast/DEPS:2: "+crypto",
I think you need an owner from crypto dir to add this dependency.

[pwestin, 2013-11-07 00:37:06]

Added bug and added crypto owner.

[wtc, 2013-11-07 01:00:35]

Patch set 2 LGTM. I only reviewed media/cast/DEPS and the first .cc file in the
CL.

I assume how AES CTR mode is used has already been reviewed by someone familiar
with crypto. This is important.

Do you want me to review the entire CL?

https://codereview.chromium.org/62843002/diff/30001/media/cast/audio_receiver...
File media/cast/audio_receiver/audio_receiver.cc (right):

https://codereview.chromium.org/62843002/diff/30001/media/cast/audio_receiver...
media/cast/audio_receiver/audio_receiver.cc:104: audio_config.aes_key.size() ==
kAesBlockSize * 2) {

Block size and key size are different quantities, so I recommend that you add a
new constant kAes128KeySize (= 16), even though it happens to be equal to
kAesBlockSize.

(AES key size can also be 192 bits and 256 bits.)

https://codereview.chromium.org/62843002/diff/30001/media/cast/audio_receiver...
media/cast/audio_receiver/audio_receiver.cc:112: audio_config.aes_iv_mask.size()
!= 0) {

BUG: the second one should test audio_config.aes_key.size() != 0.

[wtc, 2013-11-07 01:07:52]

On 2013/11/07 01:00:35, wtc wrote:
>
> I assume how AES CTR mode is used has already been reviewed by someone
familiar
> with crypto. This is important.

I should elaborate: at least two things need to be checked. 1) for a given AES
key, the "nonce" for the CTR mode is not reused. 2) how HMAC is combined with
AES CTR mode to provide authenticated encryption.

[Alpha Left Google, 2013-11-07 01:10:10]

Let me go over high level / design first. wtc: Please look at the encryption
scheme after I'm done. We'll separate the authentication in a separate change.

https://codereview.chromium.org/62843002/diff/30001/media/cast/audio_receiver...
File media/cast/audio_receiver/audio_receiver.cc (right):

https://codereview.chromium.org/62843002/diff/30001/media/cast/audio_receiver...
media/cast/audio_receiver/audio_receiver.cc:157:
base::StringPiece(reinterpret_cast<const char*>(payload_data),
nit: indent 4 spaces.

https://codereview.chromium.org/62843002/diff/30001/media/cast/audio_receiver...
media/cast/audio_receiver/audio_receiver.cc:159: &plaintext)) {
nit: indent 4 spaces as well.

https://codereview.chromium.org/62843002/diff/30001/media/cast/audio_receiver...
media/cast/audio_receiver/audio_receiver.cc:245: EncodedAudioFrame*
decrypted_audio_frame = new EncodedAudioFrame();
We should use a scoped_ptr<T> here.

https://codereview.chromium.org/62843002/diff/30001/media/cast/audio_receiver...
media/cast/audio_receiver/audio_receiver.cc:278: EncodedAudioFrame*
decrypted_audio_frame = new EncodedAudioFrame();
Use scoped_ptr<T>.

https://codereview.chromium.org/62843002/diff/30001/media/cast/audio_receiver...
media/cast/audio_receiver/audio_receiver.cc:374: DCHECK(false);
Why?

https://codereview.chromium.org/62843002/diff/30001/media/cast/audio_receiver...
File media/cast/audio_receiver/audio_receiver.h (right):

https://codereview.chromium.org/62843002/diff/30001/media/cast/audio_receiver...
media/cast/audio_receiver/audio_receiver.h:85: bool DecryptAudioFrame(const
EncodedAudioFrame* encrypted_frame,
Please add a comment about the parameters. Which one is input, which one is
output?

Why not just return scoped_ptr<EncodedAudioFrame> instead of using an output
pointer?

https://codereview.chromium.org/62843002/diff/30001/media/cast/audio_sender/a...
File media/cast/audio_sender/audio_sender.cc (right):

https://codereview.chromium.org/62843002/diff/30001/media/cast/audio_sender/a...
media/cast/audio_sender/audio_sender.cc:99: iv_mask_ =
ConvertFromBase16String(audio_config.aes_iv_mask);
The IV should be a binary string instead of a base64. It should the
application's join to convert from base64 to binary.

https://codereview.chromium.org/62843002/diff/30001/media/cast/audio_sender/a...
media/cast/audio_sender/audio_sender.cc:102:
ConvertFromBase16String(audio_config.aes_key));
The key should be a binary string instead of a base64. It should the
application's join to convert from base64 to binary.

https://codereview.chromium.org/62843002/diff/30001/media/cast/audio_sender/a...
File media/cast/audio_sender/audio_sender.h (right):

https://codereview.chromium.org/62843002/diff/30001/media/cast/audio_sender/a...
media/cast/audio_sender/audio_sender.h:70: bool EncryptAudioFrame(const
EncodedAudioFrame* audio_frame,
Why not just return scoped_ptr<T>?

https://codereview.chromium.org/62843002/diff/30001/media/cast/cast_config.h
File media/cast/cast_config.h (right):

https://codereview.chromium.org/62843002/diff/30001/media/cast/cast_config.h#...
media/cast/cast_config.h:57: std::string aes_key;
Please add a document saying these keys and mask are binary strings.

https://codereview.chromium.org/62843002/diff/30001/media/cast/cast_defines.h
File media/cast/cast_defines.h (right):

https://codereview.chromium.org/62843002/diff/30001/media/cast/cast_defines.h...
media/cast/cast_defines.h:135: inline char ConvertFromBase16Char(char base_16) {
Don't do this. You should assume the key and IV are already binary strings. It
is really the application (test app / chrome) to convert from base64 to binary.
Also there's already base/base64.h that does the conversion.

https://codereview.chromium.org/62843002/diff/30001/media/cast/cast_sender.h
File media/cast/cast_sender.h (right):

https://codereview.chromium.org/62843002/diff/30001/media/cast/cast_sender.h#...
media/cast/cast_sender.h:40: virtual void
InsertCodedVideoFrame(EncodedVideoFrame* video_frame,
Why remove the const?

https://codereview.chromium.org/62843002/diff/30001/media/cast/framer/framer.cc
File media/cast/framer/framer.cc (right):

https://codereview.chromium.org/62843002/diff/30001/media/cast/framer/framer....
media/cast/framer/framer.cc:38: linked_ptr<FrameBuffer> frame_buffer(new
FrameBuffer()); // TODO wrap count?
What does this mean?

https://codereview.chromium.org/62843002/diff/30001/media/cast/test/end2end_u...
File media/cast/test/end2end_unittest.cc (right):

https://codereview.chromium.org/62843002/diff/30001/media/cast/test/end2end_u...
media/cast/test/end2end_unittest.cc:202: 
nit: please don't fix style in a functional CL in the future.

https://codereview.chromium.org/62843002/diff/30001/media/cast/video_receiver...
File media/cast/video_receiver/video_receiver.cc (right):

https://codereview.chromium.org/62843002/diff/30001/media/cast/video_receiver...
media/cast/video_receiver/video_receiver.cc:135:
ConvertFromBase16String(video_config.aes_key));
Again don't do the conversion in this low level step.

https://codereview.chromium.org/62843002/diff/30001/media/cast/video_receiver...
media/cast/video_receiver/video_receiver.cc:250: EncodedVideoFrame*
decrypted_video_frame = new EncodedVideoFrame();
This should be a scoped_ptr<T>.

https://codereview.chromium.org/62843002/diff/30001/media/cast/video_receiver...
media/cast/video_receiver/video_receiver.cc:341: EncodedVideoFrame*
decrypted_video_frame = new EncodedVideoFrame();
Same here, use scoped_ptr<T>.

https://codereview.chromium.org/62843002/diff/30001/media/cast/video_sender/v...
File media/cast/video_sender/video_sender.cc (right):

https://codereview.chromium.org/62843002/diff/30001/media/cast/video_sender/v...
media/cast/video_sender/video_sender.cc:119:
ConvertFromBase16String(video_config.aes_key));
Again don't do the conversion here.

https://codereview.chromium.org/62843002/diff/30001/media/cast/video_sender/v...
File media/cast/video_sender/video_sender.h (right):

https://codereview.chromium.org/62843002/diff/30001/media/cast/video_sender/v...
media/cast/video_sender/video_sender.h:60: void
InsertCodedVideoFrame(EncodedVideoFrame* video_frame,
Why take away the const? The call goes to EncryptVideoFrame which already takes
const EncodedVideoFrame isn't it?

https://codereview.chromium.org/62843002/diff/30001/media/cast/video_sender/v...
media/cast/video_sender/video_sender.h:105: bool EncryptVideoFrame(const
EncodedVideoFrame* encoded_frame,
I'd return a scoped_ptr<EncodedVideoframe> instead.

[pwestin, 2013-11-07 17:16:04]

PTAL

https://codereview.chromium.org/62843002/diff/30001/media/cast/DEPS
File media/cast/DEPS (right):

https://codereview.chromium.org/62843002/diff/30001/media/cast/DEPS#newcode2
media/cast/DEPS:2: "+crypto",
On 2013/11/07 00:33:52, Alpha wrote:
> I think you need an owner from crypto dir to add this dependency.

Done.

https://codereview.chromium.org/62843002/diff/30001/media/cast/audio_receiver...
File media/cast/audio_receiver/audio_receiver.cc (right):

https://codereview.chromium.org/62843002/diff/30001/media/cast/audio_receiver...
media/cast/audio_receiver/audio_receiver.cc:104: audio_config.aes_key.size() ==
kAesBlockSize * 2) {
On 2013/11/07 01:00:36, wtc wrote:
> 
> Block size and key size are different quantities, so I recommend that you add
a
> new constant kAes128KeySize (= 16), even though it happens to be equal to
> kAesBlockSize.
> 
> (AES key size can also be 192 bits and 256 bits.)

Done.

https://codereview.chromium.org/62843002/diff/30001/media/cast/audio_receiver...
media/cast/audio_receiver/audio_receiver.cc:112: audio_config.aes_iv_mask.size()
!= 0) {
On 2013/11/07 01:00:36, wtc wrote:
> 
> BUG: the second one should test audio_config.aes_key.size() != 0.

Done.

https://codereview.chromium.org/62843002/diff/30001/media/cast/audio_receiver...
media/cast/audio_receiver/audio_receiver.cc:157:
base::StringPiece(reinterpret_cast<const char*>(payload_data),
On 2013/11/07 01:10:11, Alpha wrote:
> nit: indent 4 spaces.

Done.

https://codereview.chromium.org/62843002/diff/30001/media/cast/audio_receiver...
media/cast/audio_receiver/audio_receiver.cc:159: &plaintext)) {
On 2013/11/07 01:10:11, Alpha wrote:
> nit: indent 4 spaces as well.

Done.

https://codereview.chromium.org/62843002/diff/30001/media/cast/audio_receiver...
media/cast/audio_receiver/audio_receiver.cc:245: EncodedAudioFrame*
decrypted_audio_frame = new EncodedAudioFrame();
On 2013/11/07 01:10:11, Alpha wrote:
> We should use a scoped_ptr<T> here.

Done.

https://codereview.chromium.org/62843002/diff/30001/media/cast/audio_receiver...
media/cast/audio_receiver/audio_receiver.cc:278: EncodedAudioFrame*
decrypted_audio_frame = new EncodedAudioFrame();
On 2013/11/07 01:10:11, Alpha wrote:
> Use scoped_ptr<T>.

Done.

https://codereview.chromium.org/62843002/diff/30001/media/cast/audio_receiver...
media/cast/audio_receiver/audio_receiver.cc:374: DCHECK(false);
On 2013/11/07 01:10:11, Alpha wrote:
> Why?

No idea how that ended up there

https://codereview.chromium.org/62843002/diff/30001/media/cast/audio_receiver...
File media/cast/audio_receiver/audio_receiver.h (right):

https://codereview.chromium.org/62843002/diff/30001/media/cast/audio_receiver...
media/cast/audio_receiver/audio_receiver.h:85: bool DecryptAudioFrame(const
EncodedAudioFrame* encrypted_frame,
On 2013/11/07 01:10:11, Alpha wrote:
> Please add a comment about the parameters. Which one is input, which one is
> output?
> 
> Why not just return scoped_ptr<EncodedAudioFrame> instead of using an output
> pointer?

it felt weird allowing the scoped ptr being empty and use that to check for
errors; just my preference

https://codereview.chromium.org/62843002/diff/30001/media/cast/audio_sender/a...
File media/cast/audio_sender/audio_sender.cc (right):

https://codereview.chromium.org/62843002/diff/30001/media/cast/audio_sender/a...
media/cast/audio_sender/audio_sender.cc:99: iv_mask_ =
ConvertFromBase16String(audio_config.aes_iv_mask);
On 2013/11/07 01:10:11, Alpha wrote:
> The IV should be a binary string instead of a base64. It should the
> application's join to convert from base64 to binary.

Done.

https://codereview.chromium.org/62843002/diff/30001/media/cast/audio_sender/a...
media/cast/audio_sender/audio_sender.cc:102:
ConvertFromBase16String(audio_config.aes_key));
On 2013/11/07 01:10:11, Alpha wrote:
> The key should be a binary string instead of a base64. It should the
> application's join to convert from base64 to binary.

Done.

https://codereview.chromium.org/62843002/diff/30001/media/cast/audio_sender/a...
File media/cast/audio_sender/audio_sender.h (right):

https://codereview.chromium.org/62843002/diff/30001/media/cast/audio_sender/a...
media/cast/audio_sender/audio_sender.h:70: bool EncryptAudioFrame(const
EncodedAudioFrame* audio_frame,
On 2013/11/07 01:10:11, Alpha wrote:
> Why not just return scoped_ptr<T>?

Same as above; I think this is cleaner since the ptr can be NULL

https://codereview.chromium.org/62843002/diff/30001/media/cast/cast_config.h
File media/cast/cast_config.h (right):

https://codereview.chromium.org/62843002/diff/30001/media/cast/cast_config.h#...
media/cast/cast_config.h:57: std::string aes_key;
On 2013/11/07 01:10:11, Alpha wrote:
> Please add a document saying these keys and mask are binary strings.

Done.

https://codereview.chromium.org/62843002/diff/30001/media/cast/cast_defines.h
File media/cast/cast_defines.h (right):

https://codereview.chromium.org/62843002/diff/30001/media/cast/cast_defines.h...
media/cast/cast_defines.h:135: inline char ConvertFromBase16Char(char base_16) {
On 2013/11/07 01:10:11, Alpha wrote:
> Don't do this. You should assume the key and IV are already binary strings. It
> is really the application (test app / chrome) to convert from base64 to
binary.
> Also there's already base/base64.h that does the conversion.

Moved to test app. I did find the base 64 but not the base 16 in chromium

https://codereview.chromium.org/62843002/diff/30001/media/cast/cast_sender.h
File media/cast/cast_sender.h (right):

https://codereview.chromium.org/62843002/diff/30001/media/cast/cast_sender.h#...
media/cast/cast_sender.h:40: virtual void
InsertCodedVideoFrame(EncodedVideoFrame* video_frame,
On 2013/11/07 01:10:11, Alpha wrote:
> Why remove the const?

Ohh I did padding in my first version; changed back to const

https://codereview.chromium.org/62843002/diff/30001/media/cast/framer/framer.cc
File media/cast/framer/framer.cc (right):

https://codereview.chromium.org/62843002/diff/30001/media/cast/framer/framer....
media/cast/framer/framer.cc:38: linked_ptr<FrameBuffer> frame_buffer(new
FrameBuffer()); // TODO wrap count?
On 2013/11/07 01:10:11, Alpha wrote:
> What does this mean?

I have a separate cl coming changing frame id to be 32 bits to avoid frame id to
wrap. Which is an issue for the crypto.

Removed this comment

https://codereview.chromium.org/62843002/diff/30001/media/cast/test/end2end_u...
File media/cast/test/end2end_unittest.cc (right):

https://codereview.chromium.org/62843002/diff/30001/media/cast/test/end2end_u...
media/cast/test/end2end_unittest.cc:202: 
On 2013/11/07 01:10:11, Alpha wrote:
> nit: please don't fix style in a functional CL in the future.

Done.

https://codereview.chromium.org/62843002/diff/30001/media/cast/video_receiver...
File media/cast/video_receiver/video_receiver.cc (right):

https://codereview.chromium.org/62843002/diff/30001/media/cast/video_receiver...
media/cast/video_receiver/video_receiver.cc:135:
ConvertFromBase16String(video_config.aes_key));
On 2013/11/07 01:10:11, Alpha wrote:
> Again don't do the conversion in this low level step.

Done.

https://codereview.chromium.org/62843002/diff/30001/media/cast/video_receiver...
media/cast/video_receiver/video_receiver.cc:250: EncodedVideoFrame*
decrypted_video_frame = new EncodedVideoFrame();
On 2013/11/07 01:10:11, Alpha wrote:
> This should be a scoped_ptr<T>.

Done.

https://codereview.chromium.org/62843002/diff/30001/media/cast/video_receiver...
media/cast/video_receiver/video_receiver.cc:341: EncodedVideoFrame*
decrypted_video_frame = new EncodedVideoFrame();
On 2013/11/07 01:10:11, Alpha wrote:
> Same here, use scoped_ptr<T>.

Done.

https://codereview.chromium.org/62843002/diff/30001/media/cast/video_sender/v...
File media/cast/video_sender/video_sender.cc (right):

https://codereview.chromium.org/62843002/diff/30001/media/cast/video_sender/v...
media/cast/video_sender/video_sender.cc:119:
ConvertFromBase16String(video_config.aes_key));
On 2013/11/07 01:10:11, Alpha wrote:
> Again don't do the conversion here.

Done.

https://codereview.chromium.org/62843002/diff/30001/media/cast/video_sender/v...
File media/cast/video_sender/video_sender.h (right):

https://codereview.chromium.org/62843002/diff/30001/media/cast/video_sender/v...
media/cast/video_sender/video_sender.h:60: void
InsertCodedVideoFrame(EncodedVideoFrame* video_frame,
On 2013/11/07 01:10:11, Alpha wrote:
> Why take away the const? The call goes to EncryptVideoFrame which already
takes
> const EncodedVideoFrame isn't it?

Removed; changed due to padding, which we don't need any longer.

https://codereview.chromium.org/62843002/diff/30001/media/cast/video_sender/v...
media/cast/video_sender/video_sender.h:105: bool EncryptVideoFrame(const
EncodedVideoFrame* encoded_frame,
On 2013/11/07 01:10:11, Alpha wrote:
> I'd return a scoped_ptr<EncodedVideoframe> instead.

Same as above; I think this is cleaner since the ptr can be NULL

[Alpha Left Google, 2013-11-08 22:40:01]

https://codereview.chromium.org/62843002/diff/180001/media/cast/audio_receive...
File media/cast/audio_receiver/audio_receiver.cc (right):

https://codereview.chromium.org/62843002/diff/180001/media/cast/audio_receive...
media/cast/audio_receiver/audio_receiver.cc:111: audio_config.aes_iv_mask.size()
!= 0) {
Do you mean audio_config.aes_key.size() here?

https://codereview.chromium.org/62843002/diff/180001/media/cast/audio_receive...
media/cast/audio_receiver/audio_receiver.cc:156:
base::StringPiece(reinterpret_cast<const char*>(payload_data),
nit: this should be indented by 4 spaces.

https://codereview.chromium.org/62843002/diff/180001/media/cast/audio_receive...
media/cast/audio_receiver/audio_receiver.cc:157: payload_size), &plaintext)) {
payload_size should align with reinterpret_cast above.

&plaintext should be in the next line.

https://codereview.chromium.org/62843002/diff/180001/media/cast/audio_receive...
media/cast/audio_receiver/audio_receiver.cc:159: reinterpret_cast<const
uint8*>(plaintext.data()), plaintext.size(),
Can you add a new method to AudioDecoder takes a string in addition to const
uint8*? There's quit a lot of reinterpret_cast here.

https://codereview.chromium.org/62843002/diff/180001/media/cast/audio_sender/...
File media/cast/audio_sender/audio_sender.h (right):

https://codereview.chromium.org/62843002/diff/180001/media/cast/audio_sender/...
media/cast/audio_sender/audio_sender.h:70: bool EncryptAudioFrame(const
EncodedAudioFrame* audio_frame,
Need a comment to explain how |encrypted_video_frame| should be passed in.

https://codereview.chromium.org/62843002/diff/180001/media/cast/test/end2end_...
File media/cast/test/end2end_unittest.cc (right):

https://codereview.chromium.org/62843002/diff/180001/media/cast/test/end2end_...
media/cast/test/end2end_unittest.cc:514: char ConvertFromBase16Char(char
base_16) {
Can you put this in a helper source file?

https://codereview.chromium.org/62843002/diff/180001/media/cast/video_sender/...
File media/cast/video_sender/video_sender.h (right):

https://codereview.chromium.org/62843002/diff/180001/media/cast/video_sender/...
media/cast/video_sender/video_sender.h:106: EncodedVideoFrame*
encrypted_video_frame);
Need a comment to explain how |encrypted_video_frame| should be passed in.

[pwestin, 2013-11-12 22:07:22]

PTAL

https://codereview.chromium.org/62843002/diff/180001/media/cast/audio_receive...
File media/cast/audio_receiver/audio_receiver.cc (right):

https://codereview.chromium.org/62843002/diff/180001/media/cast/audio_receive...
media/cast/audio_receiver/audio_receiver.cc:111: audio_config.aes_iv_mask.size()
!= 0) {
On 2013/11/08 22:40:02, Alpha wrote:
> Do you mean audio_config.aes_key.size() here?

Done.

https://codereview.chromium.org/62843002/diff/180001/media/cast/audio_receive...
media/cast/audio_receiver/audio_receiver.cc:156:
base::StringPiece(reinterpret_cast<const char*>(payload_data),
On 2013/11/08 22:40:02, Alpha wrote:
> nit: this should be indented by 4 spaces.

Done.

https://codereview.chromium.org/62843002/diff/180001/media/cast/audio_receive...
media/cast/audio_receiver/audio_receiver.cc:157: payload_size), &plaintext)) {
On 2013/11/08 22:40:02, Alpha wrote:
> payload_size should align with reinterpret_cast above.
> 
> &plaintext should be in the next line.

Done.

https://codereview.chromium.org/62843002/diff/180001/media/cast/audio_receive...
media/cast/audio_receiver/audio_receiver.cc:159: reinterpret_cast<const
uint8*>(plaintext.data()), plaintext.size(),
On 2013/11/08 22:40:02, Alpha wrote:
> Can you add a new method to AudioDecoder takes a string in addition to const
> uint8*? There's quit a lot of reinterpret_cast here.

Dont think that is a good idea it will only move one reinterpret_cast from one
location to another.

https://codereview.chromium.org/62843002/diff/180001/media/cast/audio_sender/...
File media/cast/audio_sender/audio_sender.h (right):

https://codereview.chromium.org/62843002/diff/180001/media/cast/audio_sender/...
media/cast/audio_sender/audio_sender.h:70: bool EncryptAudioFrame(const
EncodedAudioFrame* audio_frame,
On 2013/11/08 22:40:02, Alpha wrote:
> Need a comment to explain how |encrypted_video_frame| should be passed in.

Done.

https://codereview.chromium.org/62843002/diff/180001/media/cast/test/end2end_...
File media/cast/test/end2end_unittest.cc (right):

https://codereview.chromium.org/62843002/diff/180001/media/cast/test/end2end_...
media/cast/test/end2end_unittest.cc:514: char ConvertFromBase16Char(char
base_16) {
On 2013/11/08 22:40:02, Alpha wrote:
> Can you put this in a helper source file?

Done.

https://codereview.chromium.org/62843002/diff/180001/media/cast/video_sender/...
File media/cast/video_sender/video_sender.h (right):

https://codereview.chromium.org/62843002/diff/180001/media/cast/video_sender/...
media/cast/video_sender/video_sender.h:106: EncodedVideoFrame*
encrypted_video_frame);
On 2013/11/08 22:40:02, Alpha wrote:
> Need a comment to explain how |encrypted_video_frame| should be passed in.

Done.

[Alpha Left Google, 2013-11-13 20:56:54]

I don't have any more comments now. I think wtc@ should look at the usage of
encrypter and decrypter to make sure the scheme is actually secure.

[wtc, 2013-11-13 20:57:17]

Patch set 4 LGTM.

High-level comments:

1. IMPORTANT: the uint8 frame_id member of EncodedAudioFrame and
EncodedAudioFrame is used to construct the counter block. This means after 256
frames the counter block will be repeated. This is a critical problem for CTR
mode. How do you plan to fix this problem?

2. IMPORTANT: I don't see any message authentication code such as HMAC used. I
assume you are implementing a standard. Does that standard use a MAC in
conjunction with AES-CTR?

3. For brevity, I didn't add comments to the video files because they are very
similar to the audio files. Almost all of my comments in the audio files also
apply to the video files.

4. I didn't review the unit tests.

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_receive...
File media/cast/audio_receiver/audio_receiver.cc (right):

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_receive...
media/cast/audio_receiver/audio_receiver.cc:153:
decryptor_->SetCounter(GetAesNounce(rtp_header.frame_id, iv_mask_));

GetAesNounce seems to be a typo. Should it be GetAesNonce?

We should check the return value of decryptor_->SetCounter().

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_receive...
media/cast/audio_receiver/audio_receiver.cc:157: DCHECK(false) << "Decryption
error";

This should be a LOG statement rather than a DCHECK if decryption error may
result from network errors.

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_receive...
media/cast/audio_receiver/audio_receiver.cc:165: rtp_header);

Nit: ideally, this code should look like:

  if (audio_decoder_) {
    DCHECK(!audio_buffer_) << "Invalid internal state";
    ... initialize plaintext to be payload_data ...
    if (decryptor_) {
      ... decrypt payload_data into plaintext ...
    }
    audio_decoder_->IncomingParsedRtpPacket(... plaintext ...,
                                            rtp_header);
  }

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_receive...
media/cast/audio_receiver/audio_receiver.cc:242: DCHECK(false) << "Decryption
error";

This should be a LOG statement rather than a DCHECK if decryption error may
result from network errors.

Similarly for the DCHECK on line 271.

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_receive...
media/cast/audio_receiver/audio_receiver.cc:272: return;

Do we need to do
    queued_encoded_callbacks_.push_back(callback);
before returning?

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_receive...
media/cast/audio_receiver/audio_receiver.cc:363: new EncodedAudioFrame());

I think we just need a std::string for the decrypted data, rather than an
EncodedAudioFrame:
  
  decryptor_->SetCounter(GetAesNounce((*audio_frame)->frame_id, iv_mask_));
  
  std::string decrypted_data;
  if (!decryptor_->Decrypt((*audio_frame)->data, &decrypted_data))
    return false;

  (*audio_frame)->data.swap(decrypted_data);
  return true;

Would this work?

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_receive...
media/cast/audio_receiver/audio_receiver.cc:373: decrypted_audio_frame->frame_id
= (*audio_frame)->frame_id;

Do we need to copy the "samples" field?

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_receive...
File media/cast/audio_receiver/audio_receiver.h (right):

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_receive...
media/cast/audio_receiver/audio_receiver.h:85: bool
DecryptAudioFrame(scoped_ptr<EncodedAudioFrame>* audio_frame);

Nit: add a short comment to document this method.

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_receive...
media/cast/audio_receiver/audio_receiver.h:116: scoped_ptr<crypto::Encryptor>
decryptor_;

Nit: I remember we could get away with just a forward declaration of
crypto::Encryptor when using scoped_ptr. I'll leave this up to you.

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_sender/...
File media/cast/audio_sender/audio_encoder.cc (right):

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_sender/...
media/cast/audio_sender/audio_encoder.cc:43: audio_frame->frame_id =
frame_id_++;

Nit: set audio_frame->frame_id before you set audio_frame->samples, so that you
don't separate line 42 from the related DCHECK on line 44.

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_sender/...
File media/cast/audio_sender/audio_sender.cc (right):

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_sender/...
media/cast/audio_sender/audio_sender.cc:141: DCHECK(false) << "Encryption
error";

Nit: use NOTREACHED() instead of DCHECK(false).

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_sender/...
media/cast/audio_sender/audio_sender.cc:146: }

Nit: it would be better to avoid having two
rtp_sender_.IncomingEncodedAudioFrame() calls. But this is fine, too.

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_sender/...
media/cast/audio_sender/audio_sender.cc:160: return
encryptor_->Encrypt(audio_frame->data, &encrypted_frame->data);

Nit: in another file you seem to take care to not modify the output argument
unless the function returns true. I am not sure if we should also do that here.

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_sender/...
media/cast/audio_sender/audio_sender.cc:176: }

Nit: this method is extremely similar to InsertCodedAudioFrame(). The only
differences are:

1) whether |audio_frame| is a plain pointer or a scoped_ptr.
2) InsertCodedAudioFrame does callback.Run() at the end.

I wonder if it is possible to share code.

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_sender/...
File media/cast/audio_sender/audio_sender.h (right):

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_sender/...
media/cast/audio_sender/audio_sender.h:70: // Caller must allocate the
destination |encrypted_frame| the data member

Nit: the first sentence should end after "the destination |encrypted_frame|".

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_sender/...
media/cast/audio_sender/audio_sender.h:72: bool EncryptAudioFrame(const
EncodedAudioFrame* audio_frame,

Nit: would it be better to make |audio_frame| a const reference instead of a
const pointer?

https://codereview.chromium.org/62843002/diff/260001/media/cast/cast_config.h
File media/cast/cast_config.h (right):

https://codereview.chromium.org/62843002/diff/260001/media/cast/cast_config.h...
media/cast/cast_config.h:57: std::string aes_key;  // Binary strings of size
kAesKeySize.

Nit: in these comments, should we use a singular "string" because each of the
std::string members is just one string?

https://codereview.chromium.org/62843002/diff/260001/media/cast/cast_defines.h
File media/cast/cast_defines.h (right):

https://codereview.chromium.org/62843002/diff/260001/media/cast/cast_defines....
media/cast/cast_defines.h:135: inline std::string GetAesNounce(uint32 frame_id,
const std::string& iv_mask) {

Nit: this function should be documented.

Nounce => Nonce?

https://codereview.chromium.org/62843002/diff/260001/media/cast/cast_defines....
media/cast/cast_defines.h:136: std::string aes_ivec(kAesKeySize, 0);

What does "ivec" stands for?

https://codereview.chromium.org/62843002/diff/260001/media/cast/cast_defines....
media/cast/cast_defines.h:138: aes_ivec[11] = frame_id & 0xff;

Nit: it may be useful to add a comment to point out you are serializing frame_id
in big-endian order (aes_ivec[8] is the most significant byte of frame_id).

https://codereview.chromium.org/62843002/diff/260001/media/cast/cast_defines....
media/cast/cast_defines.h:143: for (size_t i = 0; i < kAesKeySize; ++i) {

OK, after reading this function, I am now sure we should add a new constant,
kAesBlockSize, to be used with iv_mask. kAesBlockSize is also 16, but
conceptually it is not the size of the AES key (which can be 16, 24, or 32
bytes).

https://codereview.chromium.org/62843002/diff/260001/media/cast/test/crypto_u...
File media/cast/test/crypto_utility.cc (right):

https://codereview.chromium.org/62843002/diff/260001/media/cast/test/crypto_u...
media/cast/test/crypto_utility.cc:22: }  // namespace

Nit: add a blank line before this line. (This is probably moot.)

https://codereview.chromium.org/62843002/diff/260001/media/cast/test/crypto_u...
File media/cast/test/crypto_utility.h (right):

https://codereview.chromium.org/62843002/diff/260001/media/cast/test/crypto_u...
media/cast/test/crypto_utility.h:13: std::string ConvertFromBase16String(const
std::string base_16);

Nit: Base16String => HexString?

IMPORTANT: this function may not be necessary. You should be able to use
base::HexStringToBytes.

  #include "base/strings/string_number_conversions.h"

  BASE_EXPORT bool HexStringToBytes(const std::string& input,
                                    std::vector<uint8>* output);

The only problem is that base::HexStringToBytes uses std::vector<uint8> as the
output. If you need to use std::string, ConvertFromBase16String can simply call
base::HexStringToBytes and convert std::vector<uint8> to std::string. You can
find sample code here:

https://codereview.chromium.org/54043003/diff/370001/net/quic/test_tools/quic...

https://codereview.chromium.org/62843002/diff/260001/media/cast/test/end2end_...
File media/cast/test/end2end_unittest.cc (right):

https://codereview.chromium.org/62843002/diff/260001/media/cast/test/end2end_...
media/cast/test/end2end_unittest.cc:546: 20);

Is this an unrelated change? Just curious.

https://codereview.chromium.org/62843002/diff/260001/media/cast/test/end2end_...
media/cast/test/end2end_unittest.cc:955: 

Nit: use only one blank line here and on line 900 between the tests.

[pwestin, 2013-11-15 19:38:16]

PTAL

Frame id is changed to 32 bit is another cl (already landed).

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_receive...
File media/cast/audio_receiver/audio_receiver.cc (right):

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_receive...
media/cast/audio_receiver/audio_receiver.cc:153:
decryptor_->SetCounter(GetAesNounce(rtp_header.frame_id, iv_mask_));
On 2013/11/13 20:57:17, wtc wrote:
> 
> GetAesNounce seems to be a typo. Should it be GetAesNonce?
> 
> We should check the return value of decryptor_->SetCounter().

Done.

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_receive...
media/cast/audio_receiver/audio_receiver.cc:157: DCHECK(false) << "Decryption
error";
On 2013/11/13 20:57:17, wtc wrote:
> 
> This should be a LOG statement rather than a DCHECK if decryption error may
> result from network errors.

Done.

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_receive...
media/cast/audio_receiver/audio_receiver.cc:165: rtp_header);
On 2013/11/13 20:57:17, wtc wrote:
> 
> Nit: ideally, this code should look like:
> 
>   if (audio_decoder_) {
>     DCHECK(!audio_buffer_) << "Invalid internal state";
>     ... initialize plaintext to be payload_data ...
>     if (decryptor_) {
>       ... decrypt payload_data into plaintext ...
>     }
>     audio_decoder_->IncomingParsedRtpPacket(... plaintext ...,
>                                             rtp_header);
>   }

Done.

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_receive...
media/cast/audio_receiver/audio_receiver.cc:242: DCHECK(false) << "Decryption
error";
On 2013/11/13 20:57:17, wtc wrote:
> 
> This should be a LOG statement rather than a DCHECK if decryption error may
> result from network errors.
> 
> Similarly for the DCHECK on line 271.

Done.

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_receive...
media/cast/audio_receiver/audio_receiver.cc:272: return;
On 2013/11/13 20:57:17, wtc wrote:
> 
> Do we need to do
>     queued_encoded_callbacks_.push_back(callback);
> before returning?

Done.

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_receive...
media/cast/audio_receiver/audio_receiver.cc:363: new EncodedAudioFrame());
On 2013/11/13 20:57:17, wtc wrote:
> 
> I think we just need a std::string for the decrypted data, rather than an
> EncodedAudioFrame:
>   
>   decryptor_->SetCounter(GetAesNounce((*audio_frame)->frame_id, iv_mask_));
>   
>   std::string decrypted_data;
>   if (!decryptor_->Decrypt((*audio_frame)->data, &decrypted_data))
>     return false;
> 
>   (*audio_frame)->data.swap(decrypted_data);
>   return true;
> 
> Would this work?

Done.

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_receive...
media/cast/audio_receiver/audio_receiver.cc:373: decrypted_audio_frame->frame_id
= (*audio_frame)->frame_id;
On 2013/11/13 20:57:17, wtc wrote:
> 
> Do we need to copy the "samples" field?

Done.

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_receive...
File media/cast/audio_receiver/audio_receiver.h (right):

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_receive...
media/cast/audio_receiver/audio_receiver.h:85: bool
DecryptAudioFrame(scoped_ptr<EncodedAudioFrame>* audio_frame);
On 2013/11/13 20:57:17, wtc wrote:
> 
> Nit: add a short comment to document this method.

Done.

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_receive...
media/cast/audio_receiver/audio_receiver.h:116: scoped_ptr<crypto::Encryptor>
decryptor_;
On 2013/11/13 20:57:17, wtc wrote:
> 
> Nit: I remember we could get away with just a forward declaration of
> crypto::Encryptor when using scoped_ptr. I'll leave this up to you.

Done.

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_sender/...
File media/cast/audio_sender/audio_encoder.cc (right):

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_sender/...
media/cast/audio_sender/audio_encoder.cc:43: audio_frame->frame_id =
frame_id_++;
On 2013/11/13 20:57:17, wtc wrote:
> 
> Nit: set audio_frame->frame_id before you set audio_frame->samples, so that
you
> don't separate line 42 from the related DCHECK on line 44.

Done.

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_sender/...
File media/cast/audio_sender/audio_sender.cc (right):

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_sender/...
media/cast/audio_sender/audio_sender.cc:141: DCHECK(false) << "Encryption
error";
On 2013/11/13 20:57:17, wtc wrote:
> 
> Nit: use NOTREACHED() instead of DCHECK(false).

Done.

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_sender/...
media/cast/audio_sender/audio_sender.cc:146: }
On 2013/11/13 20:57:17, wtc wrote:
> 
> Nit: it would be better to avoid having two
> rtp_sender_.IncomingEncodedAudioFrame() calls. But this is fine, too.

ok

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_sender/...
media/cast/audio_sender/audio_sender.cc:160: return
encryptor_->Encrypt(audio_frame->data, &encrypted_frame->data);
On 2013/11/13 20:57:17, wtc wrote:
> 
> Nit: in another file you seem to take care to not modify the output argument
> unless the function returns true. I am not sure if we should also do that
here.

Done.

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_sender/...
media/cast/audio_sender/audio_sender.cc:176: }
On 2013/11/13 20:57:17, wtc wrote:
> 
> Nit: this method is extremely similar to InsertCodedAudioFrame(). The only
> differences are:
> 
> 1) whether |audio_frame| is a plain pointer or a scoped_ptr.
> 2) InsertCodedAudioFrame does callback.Run() at the end.
> 
> I wonder if it is possible to share code.

It's not a trivial change so I will leave that to another cl

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_sender/...
File media/cast/audio_sender/audio_sender.h (right):

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_sender/...
media/cast/audio_sender/audio_sender.h:70: // Caller must allocate the
destination |encrypted_frame| the data member
On 2013/11/13 20:57:17, wtc wrote:
> 
> Nit: the first sentence should end after "the destination |encrypted_frame|".

Done.

https://codereview.chromium.org/62843002/diff/260001/media/cast/audio_sender/...
media/cast/audio_sender/audio_sender.h:72: bool EncryptAudioFrame(const
EncodedAudioFrame* audio_frame,
On 2013/11/13 20:57:17, wtc wrote:
> 
> Nit: would it be better to make |audio_frame| a const reference instead of a
> const pointer?

Done.

https://codereview.chromium.org/62843002/diff/260001/media/cast/cast_config.h
File media/cast/cast_config.h (right):

https://codereview.chromium.org/62843002/diff/260001/media/cast/cast_config.h...
media/cast/cast_config.h:57: std::string aes_key;  // Binary strings of size
kAesKeySize.
On 2013/11/13 20:57:17, wtc wrote:
> 
> Nit: in these comments, should we use a singular "string" because each of the
> std::string members is just one string?

Done.

https://codereview.chromium.org/62843002/diff/260001/media/cast/cast_defines.h
File media/cast/cast_defines.h (right):

https://codereview.chromium.org/62843002/diff/260001/media/cast/cast_defines....
media/cast/cast_defines.h:135: inline std::string GetAesNounce(uint32 frame_id,
const std::string& iv_mask) {
On 2013/11/13 20:57:17, wtc wrote:
> 
> Nit: this function should be documented.
> 
> Nounce => Nonce?

Done.

https://codereview.chromium.org/62843002/diff/260001/media/cast/cast_defines....
media/cast/cast_defines.h:136: std::string aes_ivec(kAesKeySize, 0);
On 2013/11/13 20:57:17, wtc wrote:
> 
> What does "ivec" stands for?

Done.

https://codereview.chromium.org/62843002/diff/260001/media/cast/cast_defines....
media/cast/cast_defines.h:138: aes_ivec[11] = frame_id & 0xff;
On 2013/11/13 20:57:17, wtc wrote:
> 
> Nit: it may be useful to add a comment to point out you are serializing
frame_id
> in big-endian order (aes_ivec[8] is the most significant byte of frame_id).

Done.

https://codereview.chromium.org/62843002/diff/260001/media/cast/cast_defines....
media/cast/cast_defines.h:143: for (size_t i = 0; i < kAesKeySize; ++i) {
On 2013/11/13 20:57:17, wtc wrote:
> 
> OK, after reading this function, I am now sure we should add a new constant,
> kAesBlockSize, to be used with iv_mask. kAesBlockSize is also 16, but
> conceptually it is not the size of the AES key (which can be 16, 24, or 32
> bytes).

Done.

https://codereview.chromium.org/62843002/diff/260001/media/cast/test/crypto_u...
File media/cast/test/crypto_utility.cc (right):

https://codereview.chromium.org/62843002/diff/260001/media/cast/test/crypto_u...
media/cast/test/crypto_utility.cc:22: }  // namespace
On 2013/11/13 20:57:17, wtc wrote:
> 
> Nit: add a blank line before this line. (This is probably moot.)

removed this code

https://codereview.chromium.org/62843002/diff/260001/media/cast/test/crypto_u...
File media/cast/test/crypto_utility.h (right):

https://codereview.chromium.org/62843002/diff/260001/media/cast/test/crypto_u...
media/cast/test/crypto_utility.h:13: std::string ConvertFromBase16String(const
std::string base_16);
On 2013/11/13 20:57:17, wtc wrote:
> 
> Nit: Base16String => HexString?
> 
> IMPORTANT: this function may not be necessary. You should be able to use
> base::HexStringToBytes.
> 
>   #include "base/strings/string_number_conversions.h"
> 
>   BASE_EXPORT bool HexStringToBytes(const std::string& input,
>                                     std::vector<uint8>* output);
> 
> The only problem is that base::HexStringToBytes uses std::vector<uint8> as the
> output. If you need to use std::string, ConvertFromBase16String can simply
call
> base::HexStringToBytes and convert std::vector<uint8> to std::string. You can
> find sample code here:
> 
>
https://codereview.chromium.org/54043003/diff/370001/net/quic/test_tools/quic...

Done.

https://codereview.chromium.org/62843002/diff/260001/media/cast/test/end2end_...
File media/cast/test/end2end_unittest.cc (right):

https://codereview.chromium.org/62843002/diff/260001/media/cast/test/end2end_...
media/cast/test/end2end_unittest.cc:546: 20);
On 2013/11/13 20:57:17, wtc wrote:
> 
> Is this an unrelated change? Just curious.

Removed should not changed; at least not in this cl

https://codereview.chromium.org/62843002/diff/260001/media/cast/test/end2end_...
media/cast/test/end2end_unittest.cc:955: 
On 2013/11/13 20:57:17, wtc wrote:
> 
> Nit: use only one blank line here and on line 900 between the tests.

Done.

[pwestin, 2013-11-15 19:42:47]

WTC@

It was deiced that we did not need authentication in this first release. Hence
no HMAC

[pwestin, 2013-11-15 19:43:54]

s/deiced/decided

[wtc, 2013-11-15 20:13:36]

Patch set 5 LGTM:

Re: "It was decided that we did not need authentication in this first release"

Please make sure the decision was made by someone who understands why the crypto
people recommend HMAC should be used in conjunction with AES-CTR. Perhaps I
should ask to see the specification of the encryption.

https://codereview.chromium.org/62843002/diff/430001/media/cast/audio_receive...
File media/cast/audio_receiver/audio_receiver.cc (right):

https://codereview.chromium.org/62843002/diff/430001/media/cast/audio_receive...
media/cast/audio_receiver/audio_receiver.cc:153: payload_size);

The changes you made to these audio_xxx files should also be made to the
video_xxx files because the code in the video_xxx files is very similar.

https://codereview.chromium.org/62843002/diff/430001/media/cast/audio_receive...
media/cast/audio_receiver/audio_receiver.cc:157: NOTREACHED();

Should we add a return statement?

https://codereview.chromium.org/62843002/diff/430001/media/cast/cast_config.h
File media/cast/cast_config.h (right):

https://codereview.chromium.org/62843002/diff/430001/media/cast/cast_config.h...
media/cast/cast_config.h:58: std::string aes_iv_mask;  // Binary strings of size
kAesKeySize.

Nit: also change "strings" to "string" in these two comments.

https://codereview.chromium.org/62843002/diff/430001/media/cast/cast_defines.h
File media/cast/cast_defines.h (right):

https://codereview.chromium.org/62843002/diff/430001/media/cast/cast_defines....
media/cast/cast_defines.h:137: std::string aes_iv_mask(kAesBlockSize, 0);

This variable should be named just "aes_iv" or "aes_nonce", because there is
already an "iv_mask".

https://codereview.chromium.org/62843002/diff/430001/media/cast/cast_defines....
media/cast/cast_defines.h:139: // Serializing frame_id in big-endian order
(aes_ivec[8] is the most

aes_ivec => aes_iv or aes_nonce

[pwestin, 2013-11-15 22:11:10]

PTAL

Wtc@ shared a doc with you offline

https://codereview.chromium.org/62843002/diff/430001/media/cast/audio_receive...
File media/cast/audio_receiver/audio_receiver.cc (right):

https://codereview.chromium.org/62843002/diff/430001/media/cast/audio_receive...
media/cast/audio_receiver/audio_receiver.cc:153: payload_size);
On 2013/11/15 20:13:37, wtc wrote:
> 
> The changes you made to these audio_xxx files should also be made to the
> video_xxx files because the code in the video_xxx files is very similar.

Done.

https://codereview.chromium.org/62843002/diff/430001/media/cast/audio_receive...
media/cast/audio_receiver/audio_receiver.cc:157: NOTREACHED();
On 2013/11/15 20:13:37, wtc wrote:
> 
> Should we add a return statement?

Done.

https://codereview.chromium.org/62843002/diff/430001/media/cast/cast_config.h
File media/cast/cast_config.h (right):

https://codereview.chromium.org/62843002/diff/430001/media/cast/cast_config.h...
media/cast/cast_config.h:58: std::string aes_iv_mask;  // Binary strings of size
kAesKeySize.
On 2013/11/15 20:13:37, wtc wrote:
> 
> Nit: also change "strings" to "string" in these two comments.

Done.

https://codereview.chromium.org/62843002/diff/430001/media/cast/cast_defines.h
File media/cast/cast_defines.h (right):

https://codereview.chromium.org/62843002/diff/430001/media/cast/cast_defines....
media/cast/cast_defines.h:137: std::string aes_iv_mask(kAesBlockSize, 0);
On 2013/11/15 20:13:37, wtc wrote:
> 
> This variable should be named just "aes_iv" or "aes_nonce", because there is
> already an "iv_mask".

Done.

https://codereview.chromium.org/62843002/diff/430001/media/cast/cast_defines....
media/cast/cast_defines.h:139: // Serializing frame_id in big-endian order
(aes_ivec[8] is the most
On 2013/11/15 20:13:37, wtc wrote:
> 
> aes_ivec => aes_iv or aes_nonce

Done.

[wtc, 2013-11-16 00:34:36]

Patch set 7 LGTM.

[Alpha Left Google, 2013-11-16 00:36:37]

LGTM now wtc has approved.

[commit-bot: I haz the power, 2013-11-18 21:39:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pwestin@google.com/62843002/1020001

[commit-bot: I haz the power, 2013-11-19 01:02:59]

Change committed as 235864