Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10800091:
Replace memcmp() with HMAC.VerifyTruncated() in aes_decryptor.cc (Closed)
|
Created:
8 years, 5 months ago by fgalligan1 Modified:
8 years, 5 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionReplace memcmp() with HMAC.VerifyTruncated() in aes_decryptor.cc
BUG=138682
TEST=media_unittests --gtest_filter=AesDecryptor*
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148240
Patch Set 1 #
Total comments: 5
Patch Set 2 : Addressing comments from Patch Set 2. #Patch Set 3 : Add comment about checking checksum_size. #
Total comments: 2
Patch Set 4 : Addressing comments from Patch Set 3. #Messages
Total messages: 13 (0 generated)
https://chromiumcodereview.appspot.com/10800091/diff/1/media/crypto/aes_decry... File media/crypto/aes_decryptor.cc (right): https://chromiumcodereview.appspot.com/10800091/diff/1/media/crypto/aes_decry... media/crypto/aes_decryptor.cc:69: DCHECK(input.GetDecryptConfig()->checksum_size() <= nit: DCHECK_LE https://chromiumcodereview.appspot.com/10800091/diff/1/media/crypto/aes_decry... media/crypto/aes_decryptor.cc:76: reinterpret_cast<const char*>(input.GetDecryptConfig()->checksum()), Are there checks elsewhere that make sure that input.GetDecryptConfig()->checksum_size() is some sane value (eg: the 96-bits proposed in the current draft)? Just wondering if an attacker could, say, substitute just 1 byte instead. If there are, it might be worth adding as a comment.
https://chromiumcodereview.appspot.com/10800091/diff/1/media/crypto/aes_decry... File media/crypto/aes_decryptor.cc (right): https://chromiumcodereview.appspot.com/10800091/diff/1/media/crypto/aes_decry... media/crypto/aes_decryptor.cc:69: DCHECK(input.GetDecryptConfig()->checksum_size() <= On 2012/07/24 05:49:56, Ryan Sleevi wrote: > nit: DCHECK_LE Done. https://chromiumcodereview.appspot.com/10800091/diff/1/media/crypto/aes_decry... media/crypto/aes_decryptor.cc:76: reinterpret_cast<const char*>(input.GetDecryptConfig()->checksum()), On 2012/07/24 05:49:56, Ryan Sleevi wrote: > Are there checks elsewhere that make sure that > input.GetDecryptConfig()->checksum_size() is some sane value (eg: the 96-bits > proposed in the current draft)? > > Just wondering if an attacker could, say, substitute just 1 byte instead. > > If there are, it might be worth adding as a comment. In webm_cluster_parser we explicitly set the size to 96 bits. But there is not a check after that until the check to make sure that the size <= DigestLength. Are you worried that an attacker could append data to the digest? Wouldn't an attacker changing any data with the digest cause an integrity check failure?
https://chromiumcodereview.appspot.com/10800091/diff/1/media/crypto/aes_decry... File media/crypto/aes_decryptor.cc (right): https://chromiumcodereview.appspot.com/10800091/diff/1/media/crypto/aes_decry... media/crypto/aes_decryptor.cc:76: reinterpret_cast<const char*>(input.GetDecryptConfig()->checksum()), On 2012/07/24 15:27:25, fgalligan1 wrote: > On 2012/07/24 05:49:56, Ryan Sleevi wrote: > > Are there checks elsewhere that make sure that > > input.GetDecryptConfig()->checksum_size() is some sane value (eg: the 96-bits > > proposed in the current draft)? > > > > Just wondering if an attacker could, say, substitute just 1 byte instead. > > > > If there are, it might be worth adding as a comment. > In webm_cluster_parser we explicitly set the size to 96 bits. But there is not a > check after that until the check to make sure that the size <= DigestLength. > > Are you worried that an attacker could append data to the digest? Wouldn't an > attacker changing any data with the digest cause an integrity check failure? No, quite the opposite. I was wondering if the attacker could craft a malicious packet, such that the checksum was lifted from a legitimate packet, but then truncated to a single byte (or single bit, if you're dealing in bits). Because VerifyTruncated() will accept any truncated inputs, it's up to the caller to ensure that there is not "over" truncation. Legitimate packet: [IV][Data][HMAC-12bytes] Malicious Packet: [IV][Malicious Data][HMAC-1byte]
Note: Implementation itself LGTM, and I'll be on the road for the next two weeks and don't want to block this CL. Just wanted to highlight the risk of the truncation attack and ensure it was addressed somehow (eg: ensuring that the HMAC size is exactly what is expected for that media/packet type)
On 2012/07/24 15:46:27, Ryan Sleevi wrote: > Note: Implementation itself LGTM, and I'll be on the road for the next two weeks > and don't want to block this CL. > > Just wanted to highlight the risk of the truncation attack and ensure it was > addressed somehow (eg: ensuring that the HMAC size is exactly what is expected > for that media/packet type) I think it is OK. In the WebM parser we always copy the first 12 bytes of the block. So if an attacker removed some bytes we would copy the first 12 which would end up copying data after the HMAC. Then the IC check would fail for that frame and the frame would not be decrypted and thrown out.
On 2012/07/24 16:11:10, fgalligan1 wrote: > On 2012/07/24 15:46:27, Ryan Sleevi wrote: > > Note: Implementation itself LGTM, and I'll be on the road for the next two > weeks > > and don't want to block this CL. > > > > Just wanted to highlight the risk of the truncation attack and ensure it was > > addressed somehow (eg: ensuring that the HMAC size is exactly what is expected > > for that media/packet type) > > I think it is OK. In the WebM parser we always copy the first 12 bytes of the > block. So if an attacker removed some bytes we would copy the first 12 which > would end up copying data after the HMAC. Then the IC check would fail for that > frame and the frame would not be decrypted and thrown out. Perfect. That was the sort of check I was hoping for (eg: that some other layer does a fixed length check)
LGTM for media/. On 2012/07/24 16:26:35, Ryan Sleevi wrote: > Perfect. That was the sort of check I was hoping for (eg: that some other layer > does a fixed length check) fgalligan, should we add a comment about this here since it's not obvious from the code? It would be nice to just DCHECK or something, but then we'd have to pull in the WebM constant.
On 2012/07/24 17:43:20, ddorwin wrote: > LGTM for media/. > > On 2012/07/24 16:26:35, Ryan Sleevi wrote: > > Perfect. That was the sort of check I was hoping for (eg: that some other > layer > > does a fixed length check) > > fgalligan, should we add a comment about this here since it's not obvious from > the code? It would be nice to just DCHECK or something, but then we'd have to > pull in the WebM constant. Sure. I added a generic comment in PS3. PTAL.
LGTM % nit https://chromiumcodereview.appspot.com/10800091/diff/8003/media/crypto/aes_de... File media/crypto/aes_decryptor.cc (right): https://chromiumcodereview.appspot.com/10800091/diff/8003/media/crypto/aes_de... media/crypto/aes_decryptor.cc:71: // what is defined by the format. Thanks. One might assume this is related to the next line. To avoid that, you might say something like, "Here, just confirm that the size is not bigger than the cypher's digest length."
https://chromiumcodereview.appspot.com/10800091/diff/8003/media/crypto/aes_de... File media/crypto/aes_decryptor.cc (right): https://chromiumcodereview.appspot.com/10800091/diff/8003/media/crypto/aes_de... media/crypto/aes_decryptor.cc:71: // what is defined by the format. On 2012/07/24 18:09:40, ddorwin wrote: > Thanks. One might assume this is related to the next line. To avoid that, you > might say something like, "Here, just confirm that the size is not bigger than > the cypher's digest length." Done. PTAL.
lgtm Thanks.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fgalligan@chromium.org/10800091/3002
Change committed as 148240 |
