Add Aes128GcmEncrypter and Aes128GcmDecrypter

Add Aes128GcmEncrypter and Aes128GcmDecrypter for the
AEAD_AES_128_GCM authenticated encryption algorithm.

An OpenSSL-based implementation is included. The NSS-based
implementation will be added later.

Merge internal change: 42641062

Add the GetKey() and GetNoncePrefix() methods to Aes128GcmEncrypter
and Aes128GcmDecrypter.

Merge internal change: 43570937

R=rch@chromium.org,rtenneti@chromium.org
BUG=none
TEST=none

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

[Ryan Hamilton, 2013-03-13 03:28:17]

As I started looking into this, I realized I'm confused.  Is this code shared
between Chromium and internal, or is this a port?

https://codereview.chromium.org/12623017/diff/8002/net/quic/crypto/aes_128_gc...
File net/quic/crypto/aes_128_gcm_decrypter_openssl.cc (right):

https://codereview.chromium.org/12623017/diff/8002/net/quic/crypto/aes_128_gc...
net/quic/crypto/aes_128_gcm_decrypter_openssl.cc:25: if (key.size() !=
sizeof(key_)) {
Should this be a DCHECK?

https://codereview.chromium.org/12623017/diff/8002/net/quic/crypto/aes_128_gc...
net/quic/crypto/aes_128_gcm_decrypter_openssl.cc:33: if (nonce_prefix.size() !=
kNoncePrefixSize) {
ditto.

[Ryan Hamilton, 2013-03-13 03:29:34]

One more question.  Do we need an NSS implementation of this?

[wtc, 2013-03-13 19:08:07]

https://codereview.chromium.org/12623017/diff/8002/net/quic/crypto/aes_128_gc...
File net/quic/crypto/aes_128_gcm_decrypter_openssl.cc (right):

https://codereview.chromium.org/12623017/diff/8002/net/quic/crypto/aes_128_gc...
net/quic/crypto/aes_128_gcm_decrypter_openssl.cc:25: if (key.size() !=
sizeof(key_)) {

On 2013/03/13 03:28:17, Ryan Hamilton wrote:
> Should this be a DCHECK?

This function returns bool, so we can do real error checking,
which I believe is better than DCHECK.

Or are you suggesting we do both?

  DCHECK_EQ(key.size(), sizeof(key_));
  if (key.size() != sizeof(key_)) {
    return false;
  }

https://codereview.chromium.org/12623017/diff/25001/net/quic/crypto/aes_128_g...
File net/quic/crypto/aes_128_gcm_decrypter_nss.cc (right):

https://codereview.chromium.org/12623017/diff/25001/net/quic/crypto/aes_128_g...
net/quic/crypto/aes_128_gcm_decrypter_nss.cc:62: // TODO(wtc): implement this
function using NSS.

On 2013/03/13 03:29:34, Ryan Hamilton wrote:
> One more question.  Do we need an NSS implementation of this?

Yes, we do. I added a stub implementation in this CL.
I will work on a real implementation later.

https://codereview.chromium.org/12623017/diff/25001/net/quic/crypto/aes_128_g...
File net/quic/crypto/aes_128_gcm_decrypter_openssl.cc (right):

https://codereview.chromium.org/12623017/diff/25001/net/quic/crypto/aes_128_g...
net/quic/crypto/aes_128_gcm_decrypter_openssl.cc:3: // found in the LICENSE
file.

On 2013/03/13 03:28:17, Ryan Hamilton wrote:
> As I started looking into this, I realized I'm confused.  Is this code shared
> between Chromium and internal, or is this a port?

This code is shared between Chromium and internal, so
the changes I made in this CL will need to be carried
back to internal.

I did need to rename the .cc files to _openssl.cc files
in the Chromium source tree, because Chromium needs two
.cc files: _openssl.cc and _nss.cc.

[Ryan Hamilton, 2013-03-13 22:06:10]

lgtm

https://codereview.chromium.org/12623017/diff/8002/net/quic/crypto/aes_128_gc...
File net/quic/crypto/aes_128_gcm_decrypter_openssl.cc (right):

https://codereview.chromium.org/12623017/diff/8002/net/quic/crypto/aes_128_gc...
net/quic/crypto/aes_128_gcm_decrypter_openssl.cc:25: if (key.size() !=
sizeof(key_)) {
On 2013/03/13 19:08:07, wtc wrote:
> 
> On 2013/03/13 03:28:17, Ryan Hamilton wrote:
> > Should this be a DCHECK?
> 
> This function returns bool, so we can do real error checking,
> which I believe is better than DCHECK.
> 
> Or are you suggesting we do both?
> 
>   DCHECK_EQ(key.size(), sizeof(key_));
>   if (key.size() != sizeof(key_)) {
>     return false;
>   }

I guess my real question is "under what circumstances will these sizes not be
equal".  If it is a programmer error for them to not match, then I would be
inclined to add a DCHECK (and the if, to be defensive).  But it's up to you.

https://codereview.chromium.org/12623017/diff/25001/net/quic/crypto/aes_128_g...
File net/quic/crypto/aes_128_gcm_decrypter_openssl.cc (right):

https://codereview.chromium.org/12623017/diff/25001/net/quic/crypto/aes_128_g...
net/quic/crypto/aes_128_gcm_decrypter_openssl.cc:3: // found in the LICENSE
file.
On 2013/03/13 19:08:07, wtc wrote:
> 
> On 2013/03/13 03:28:17, Ryan Hamilton wrote:
> > As I started looking into this, I realized I'm confused.  Is this code
shared
> > between Chromium and internal, or is this a port?
> 
> This code is shared between Chromium and internal, so
> the changes I made in this CL will need to be carried
> back to internal.
> 
> I did need to rename the .cc files to _openssl.cc files
> in the Chromium source tree, because Chromium needs two
> .cc files: _openssl.cc and _nss.cc.

Ah, ok.  In that case we'll need to update the scripts so that, for example, 
changes to aes_128_gcm_encrypter.cc are applied to
aes_128_gcm_encrypter_openssl.cc.

I can take care of this.

[wtc, 2013-03-13 22:59:16]

https://codereview.chromium.org/12623017/diff/8002/net/quic/crypto/aes_128_gc...
File net/quic/crypto/aes_128_gcm_decrypter_openssl.cc (right):

https://codereview.chromium.org/12623017/diff/8002/net/quic/crypto/aes_128_gc...
net/quic/crypto/aes_128_gcm_decrypter_openssl.cc:25: if (key.size() !=
sizeof(key_)) {

On 2013/03/13 22:06:10, Ryan Hamilton wrote:
>
> I guess my real question is "under what circumstances will these sizes not be
> equal".

It can be a programmer error. But you can also imagine we store keys on the
disk, the file is corrupted, and we read back a corrupted key of the wrong
length.

> If it is a programmer error for them to not match, then I would be
> inclined to add a DCHECK (and the if, to be defensive).  But it's up to you.

Done.

https://codereview.chromium.org/12623017/diff/25001/net/quic/crypto/aes_128_g...
File net/quic/crypto/aes_128_gcm_decrypter_openssl.cc (right):

https://codereview.chromium.org/12623017/diff/25001/net/quic/crypto/aes_128_g...
net/quic/crypto/aes_128_gcm_decrypter_openssl.cc:3: // found in the LICENSE
file.

On 2013/03/13 22:06:10, Ryan Hamilton wrote:
>
> Ah, ok.  In that case we'll need to update the scripts so that, for example, 
> changes to aes_128_gcm_encrypter.cc are applied to
> aes_128_gcm_encrypter_openssl.cc.
> 
> I can take care of this.

We may rename these files the samw way in the internal source tree, so that we
can use aes_128_gcm_decrypter.cc for the methods that are common between the
OpenSSL and NSS based implementations.

[commit-bot: I haz the power, 2013-03-13 23:04:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/12623017/40001

[commit-bot: I haz the power, 2013-03-14 22:38:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/12623017/40001

[commit-bot: I haz the power, 2013-03-14 22:38:21]

Failed to apply patch for net/net.gyp:
While running patch -p0 --forward --force --no-backup-if-mismatch;
  patching file net/net.gyp
  Hunk #1 succeeded at 672 (offset -24 lines).
  Hunk #2 succeeded at 701 (offset -25 lines).
  Hunk #3 FAILED at 1103.
  Hunk #4 succeeded at 1131 (offset 3 lines).
  Hunk #5 succeeded at 1542 (offset -3 lines).
  1 out of 5 hunks FAILED -- saving rejects to file net/net.gyp.rej

Patch:       net/net.gyp
Index: net/net.gyp
===================================================================
--- net/net.gyp	(revision 187740)
+++ net/net.gyp	(working copy)
@@ -696,6 +696,12 @@
         'quic/congestion_control/tcp_cubic_sender.h',
         'quic/congestion_control/tcp_receiver.cc',
         'quic/congestion_control/tcp_receiver.h',
+        'quic/crypto/aes_128_gcm_decrypter.h',
+        'quic/crypto/aes_128_gcm_decrypter_nss.cc',
+        'quic/crypto/aes_128_gcm_decrypter_openssl.cc',
+        'quic/crypto/aes_128_gcm_encrypter.h',
+        'quic/crypto/aes_128_gcm_encrypter_nss.cc',
+        'quic/crypto/aes_128_gcm_encrypter_openssl.cc',
         'quic/crypto/crypto_framer.cc',
         'quic/crypto/crypto_framer.h',
         'quic/crypto/crypto_handshake.cc',
@@ -720,6 +726,7 @@
         'quic/crypto/quic_encrypter.h',
         'quic/crypto/quic_random.cc',
         'quic/crypto/quic_random.h',
+        'quic/crypto/scoped_evp_cipher_ctx.h',
         'quic/quic_bandwidth.cc',
         'quic/quic_bandwidth.h',
         'quic/quic_blocked_writer_interface.h',
@@ -1096,6 +1103,8 @@
               'socket/ssl_client_socket_nss.h',
               'socket/ssl_server_socket_nss.cc',
               'socket/ssl_server_socket_nss.h',
+              'quic/crypto/aes_128_gcm_decrypter_nss.cc',
+              'quic/crypto/aes_128_gcm_encrypter_nss.cc',
               'quic/crypto/p256_key_exchange_nss.cc',
               'third_party/mozilla_security_manager/nsKeygenHandler.cpp',
               'third_party/mozilla_security_manager/nsKeygenHandler.h',
@@ -1121,7 +1130,10 @@
               'base/x509_certificate_openssl.cc',
               'base/x509_util_openssl.cc',
               'base/x509_util_openssl.h',
+              'quic/crypto/aes_128_gcm_decrypter_openssl.cc',
+              'quic/crypto/aes_128_gcm_encrypter_openssl.cc',
               'quic/crypto/p256_key_exchange_openssl.cc',
+              'quic/crypto/scoped_evp_cipher_ctx.h',
               'socket/ssl_client_socket_openssl.cc',
               'socket/ssl_client_socket_openssl.h',
               'socket/ssl_server_socket_openssl.cc',
@@ -1535,6 +1547,8 @@
         'quic/congestion_control/paced_sender_test.cc',
         'quic/congestion_control/tcp_cubic_sender_test.cc',
         'quic/congestion_control/tcp_receiver_test.cc',
+        'quic/crypto/aes_128_gcm_decrypter_test.cc',
+        'quic/crypto/aes_128_gcm_encrypter_test.cc',
         'quic/crypto/crypto_framer_test.cc',
         'quic/crypto/crypto_handshake_test.cc',
         'quic/crypto/curve25519_key_exchange_test.cc',

[commit-bot: I haz the power, 2013-03-15 00:00:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/12623017/87001

[commit-bot: I haz the power, 2013-03-15 07:49:39]

Change committed as 188292

[eroman, 2013-11-01 01:58:12]

https://chromiumcodereview.appspot.com/12623017/diff/87001/net/quic/crypto/ae...
File net/quic/crypto/aes_128_gcm_decrypter_test.cc (right):

https://chromiumcodereview.appspot.com/12623017/diff/87001/net/quic/crypto/ae...
net/quic/crypto/aes_128_gcm_decrypter_test.cc:266: bool DecodeHexString(const
char* in,
Random drive-by: This seems like a duplication of base::HexStringToBytes()

If this is something that interest you, I can replace it.

[wtc, 2013-11-01 18:42:02]

https://chromiumcodereview.appspot.com/12623017/diff/87001/net/quic/crypto/ae...
File net/quic/crypto/aes_128_gcm_decrypter_test.cc (right):

https://chromiumcodereview.appspot.com/12623017/diff/87001/net/quic/crypto/ae...
net/quic/crypto/aes_128_gcm_decrypter_test.cc:266: bool DecodeHexString(const
char* in,

On 2013/11/01 01:58:18, eroman wrote:
> Random drive-by: This seems like a duplication of base::HexStringToBytes()
> 
> If this is something that interest you, I can replace it.

Replacing this with base::HexStringToBytes() would be good. Thanks.

[eroman, 2013-11-01 22:29:30]

FYI: Done in https://codereview.chromium.org/54043003/


On Fri, Nov 1, 2013 at 11:42 AM, <wtc@chromium.org> wrote:

>
> https://chromiumcodereview.appspot.com/12623017/diff/
> 87001/net/quic/crypto/aes_128_gcm_decrypter_test.cc
> File net/quic/crypto/aes_128_gcm_decrypter_test.cc (right):
>
> https://chromiumcodereview.appspot.com/12623017/diff/
> 87001/net/quic/crypto/aes_128_gcm_decrypter_test.cc#newcode266
> net/quic/crypto/aes_128_gcm_decrypter_test.cc:266: bool
> DecodeHexString(const char* in,
>
> On 2013/11/01 01:58:18, eroman wrote:
>
>> Random drive-by: This seems like a duplication of
>>
> base::HexStringToBytes()
>
>  If this is something that interest you, I can replace it.
>>
>
> Replacing this with base::HexStringToBytes() would be good. Thanks.
>
> https://chromiumcodereview.appspot.com/12623017/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.