crypto: add OpenPGP symmetric encryption for ChromeOS

crypto: add OpenPGP symmetric encryption for ChromeOS

This contains a basic implementation of OpenPGP (RFC 4880) symmetric
encryption and decryption. It's written specifically on top of OpenSSL,
which should suffice for the intended use case.

BUG=none
TEST=crypto_unittest

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=90633

[Greg Spencer (Chromium), 2011-06-23 20:55:00]

Most of my comments are around naming, use of unsigned, or inclusion of some
links to the outside RFCs you reference.  [Sorry, I feel like I'm coming across
as a hard-ass here: I'm just trying to review it properly]

Please take some time to come up with good variable names: they should be
optimized for the reader, not for typing convenience, since they'll be read far
more often than they'll be typed, and it helps self-document the code.

Also, you use "unsigned" an awful lot in here, and we generally discourage that
unless it's really a bit field (which some of yours are), but even then it
should at least be a well defined size (e.g. uint32_t instead) to avoid
architecture-dependent bugs.

http://codereview.chromium.org/7247005/diff/1/crypto/openpgp_symmetric_encryp...
File crypto/openpgp_symmetric_encryption_openssl.cc (right):

http://codereview.chromium.org/7247005/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_openssl.cc:92: base::StringPiece s_;
Please name this something better: "string_" or "data_" or something more
descriptive.

http://codereview.chromium.org/7247005/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_openssl.cc:98: const EVP_MD *md,
Please change "md" to "digest" or "message_digest" or something more
descriptive.

http://codereview.chromium.org/7247005/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_openssl.cc:109: EVP_MD_CTX ctx;
It'd be nice to call this "context" as well.  I know these are probably fairly
well known abbreviations, but it makes it a lot easier to read the code.

http://codereview.chromium.org/7247005/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_openssl.cc:118: unsigned written = 0;
Please use uint32_t here (or uint32) and elsewhere, to be more explicit about
the size of the int.  In fact, use of unsigned types is discouraged generally,
except for specific uses.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Intege...

http://codereview.chromium.org/7247005/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_openssl.cc:161:
OpenPGPSymmetricEncrytion::Result Decrypt(base::StringPiece in,
This should be a const reference: const base::StringPiece& in, as should the
passphrase argument.

http://codereview.chromium.org/7247005/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_openssl.cc:164: Reader r(in);
Please call this "reader" or something more descriptive.  In general, names
which are readable and not abbreviated are preferred: optimize for the reader,
not the typist.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Genera...

http://codereview.chromium.org/7247005/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_openssl.cc:208: uint8 b;
Please use a better name here.

http://codereview.chromium.org/7247005/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_openssl.cc:251: // r. See RFC 4880, section
4.2.2.4.
Is there a url you could provide here to this spec (preferably to the right
section)?

http://codereview.chromium.org/7247005/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_openssl.cc:262: for (;;) {
nit: I think "while (true)" is more readable, but it's up to you.

http://codereview.chromium.org/7247005/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_openssl.cc:316: if (length_spec < 192) {
It might be good to document these seemingly-arbitrary numbers either with
comments, or by turning them into self documenting constants.  There are a bunch
of these in this code, and it starts to look like magic after a while.

http://codereview.chromium.org/7247005/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_openssl.cc:369: uint8 key[32], c;
Don't declare multiple variables on one line.  Separate "c" out into its own
declaration, and changing its name to something more descriptive.

You also might consider initializing the key array with {0}.

http://codereview.chromium.org/7247005/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_openssl.cc:460: uint8 fre[AES_BLOCK_SIZE];
Please rename fre to something more descriptive.

http://codereview.chromium.org/7247005/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_openssl.cc:502: if (plaintext[plaintext_size
- SHA_DIGEST_LENGTH - 2] != 0xd3 ||
You might add a comment about why you're looking for arbitrary values at the end
of the text.

http://codereview.chromium.org/7247005/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_openssl.cc:504: // Not an MDC packet.
What does MDC stand for?

http://codereview.chromium.org/7247005/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_openssl.cc:553: ByteString ske =
SerializeSymmetricKeyEncrypted(passphrase, &key);
Better name needed for "ske".

http://codereview.chromium.org/7247005/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_openssl.cc:556: ByteString se =
SerializeSymmetricallyEncrypted(literal_data, key);
Better name needed for "se"

http://codereview.chromium.org/7247005/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_openssl.cc:608: // It's a random value, so
endianness doesn't matter.
What?  You're not going to byte-flip a completely random number? Haha! :-)

http://codereview.chromium.org/7247005/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_openssl.cc:623: ByteString se;
Better name needed for "se".

http://codereview.chromium.org/7247005/diff/1/crypto/openpgp_symmetric_encryp...
File crypto/openpgp_symmetric_encryption_test_openssl.cc (right):

http://codereview.chromium.org/7247005/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_test_openssl.cc:11: // AES-128, GPG
Are these "official" test vectors?  If so, it might be good to link to where you
obtained them.

[agl, 2011-06-27 20:42:53]

http://codereview.chromium.org/7247005/diff/1/crypto/openpgp_symmetric_encryp...
File crypto/openpgp_symmetric_encryption_openssl.cc (right):

http://codereview.chromium.org/7247005/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_openssl.cc:92: base::StringPiece s_;
On 2011/06/23 20:55:00, Greg Spencer (Chromium) wrote:
> Please name this something better: "string_" or "data_" or something more
> descriptive.

Done.

http://codereview.chromium.org/7247005/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_openssl.cc:98: const EVP_MD *md,
On 2011/06/23 20:55:00, Greg Spencer (Chromium) wrote:
> Please change "md" to "digest" or "message_digest" or something more
> descriptive.

Done.

http://codereview.chromium.org/7247005/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_openssl.cc:109: EVP_MD_CTX ctx;
On 2011/06/23 20:55:00, Greg Spencer (Chromium) wrote:
> It'd be nice to call this "context" as well.  I know these are probably fairly
> well known abbreviations, but it makes it a lot easier to read the code.

Done.

http://codereview.chromium.org/7247005/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_openssl.cc:118: unsigned written = 0;
On 2011/06/23 20:55:00, Greg Spencer (Chromium) wrote:
> Please use uint32_t here (or uint32) and elsewhere, to be more explicit about
> the size of the int.  In fact, use of unsigned types is discouraged generally,
> except for specific uses.

Although I don't mind changing unsigned for uint32 everywhere, I'm certainly not
going to use signed numbers here. Inappropriate use of signed numbers causes
horrible bugs, even in the most careful hands. (See the bcrypt bug in OpenBSD
from last week for example.)

http://codereview.chromium.org/7247005/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_openssl.cc:161:
OpenPGPSymmetricEncrytion::Result Decrypt(base::StringPiece in,
On 2011/06/23 20:55:00, Greg Spencer (Chromium) wrote:
> This should be a const reference: const base::StringPiece& in, as should the
> passphrase argument.

That's a misguided optimization in non-performance critical code. Using a const
reference is even slower on 64-bit platforms.

http://codereview.chromium.org/7247005/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_openssl.cc:164: Reader r(in);
On 2011/06/23 20:55:00, Greg Spencer (Chromium) wrote:
> Please call this "reader" or something more descriptive.  In general, names
> which are readable and not abbreviated are preferred: optimize for the reader,
> not the typist.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Genera...

Done.

http://codereview.chromium.org/7247005/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_openssl.cc:208: uint8 b;
On 2011/06/23 20:55:00, Greg Spencer (Chromium) wrote:
> Please use a better name here.

Done.

http://codereview.chromium.org/7247005/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_openssl.cc:251: // r. See RFC 4880, section
4.2.2.4.
On 2011/06/23 20:55:00, Greg Spencer (Chromium) wrote:
> Is there a url you could provide here to this spec (preferably to the right
> section)?

Done.

http://codereview.chromium.org/7247005/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_openssl.cc:369: uint8 key[32], c;
On 2011/06/23 20:55:00, Greg Spencer (Chromium) wrote:
> Don't declare multiple variables on one line.  Separate "c" out into its own
> declaration, and changing its name to something more descriptive.
> 
> You also might consider initializing the key array with {0}.

Done.

http://codereview.chromium.org/7247005/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_openssl.cc:460: uint8 fre[AES_BLOCK_SIZE];
On 2011/06/23 20:55:00, Greg Spencer (Chromium) wrote:
> Please rename fre to something more descriptive.

No. It matches the spec.

http://codereview.chromium.org/7247005/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_openssl.cc:502: if (plaintext[plaintext_size
- SHA_DIGEST_LENGTH - 2] != 0xd3 ||
On 2011/06/23 20:55:00, Greg Spencer (Chromium) wrote:
> You might add a comment about why you're looking for arbitrary values at the
end
> of the text.

Done.

http://codereview.chromium.org/7247005/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_openssl.cc:623: ByteString se;
On 2011/06/23 20:55:00, Greg Spencer (Chromium) wrote:
> Better name needed for "se".

Done.

http://codereview.chromium.org/7247005/diff/1/crypto/openpgp_symmetric_encryp...
File crypto/openpgp_symmetric_encryption_test_openssl.cc (right):

http://codereview.chromium.org/7247005/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_test_openssl.cc:11: // AES-128, GPG
On 2011/06/23 20:55:00, Greg Spencer (Chromium) wrote:
> Are these "official" test vectors?  If so, it might be good to link to where
you
> obtained them.

Done.

[Greg Spencer (Chromium), 2011-06-27 21:00:07]

Can you upload a new patch set?  I don't see your changes yet.

http://codereview.chromium.org/7247005/diff/1/crypto/openpgp_symmetric_encryp...
File crypto/openpgp_symmetric_encryption_openssl.cc (right):

http://codereview.chromium.org/7247005/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_openssl.cc:118: unsigned written = 0;
On 2011/06/27 20:42:53, agl wrote:
> On 2011/06/23 20:55:00, Greg Spencer (Chromium) wrote:
> > Please use uint32_t here (or uint32) and elsewhere, to be more explicit
about
> > the size of the int.  In fact, use of unsigned types is discouraged
generally,
> > except for specific uses.
> 
> Although I don't mind changing unsigned for uint32 everywhere, I'm certainly
not
> going to use signed numbers here. Inappropriate use of signed numbers causes
> horrible bugs, even in the most careful hands. (See the bcrypt bug in OpenBSD
> from last week for example.) 

Well, while I'm not entirely in this camp myself, the style guide is pretty
clear on this point, so you probably will have to use signed types here:

"Decision: You should not use the unsigned integer types such as uint32_t,
unless the quantity you are representing is really a bit pattern rather than a
number, or unless you need defined twos-complement overflow. In particular, do
not use unsigned types to say a number will never be negative. Instead, use
assertions for this."

It's also true that inappropriate use of unsigned types can cause horrible bugs
as well, so given that they are both potentially dangerous, I'm inclined to
follow the style guide.

http://codereview.chromium.org/7247005/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_openssl.cc:161:
OpenPGPSymmetricEncrytion::Result Decrypt(base::StringPiece in,
On 2011/06/27 20:42:53, agl wrote:
> On 2011/06/23 20:55:00, Greg Spencer (Chromium) wrote:
> > This should be a const reference: const base::StringPiece& in, as should the
> > passphrase argument.
> 
> That's a misguided optimization in non-performance critical code. Using a
const
> reference is even slower on 64-bit platforms.

It's not meant to just be an optimization.  It is also widely used convention in
most Google code to pass read-only parameters this way, and indicates to the
reader which parts are inputs and not modified.