Unify the error checking of crypto::Encryptor and add WARN_UNUSED_RESULT to prevent misuse.

Unify the error checking of crypto::Encryptor and add WARN_UNUSED_RESULT to prevent misuse.

BUG=102722

[Ryan Sleevi, 2011-11-11 07:36:57]

wtc: PTAL. The motivation is that the Encryptor should be well-behaved in the
face of hostile input - rather than DCHECKing, crashing, or potentially leaking
secrets.

pkasting: FYI. In the interim since our discussion, the SecureP2P stuff was
ripped out, so it turned out just one place remaining was ignoring the results.

Places using Encryptor that may be exposed to 'hostile' input:
* chrome/browser/password_manager/encryptor_mac.mm - loads data from the System
Keychain, shared by other apps
* chrome/browser/password_manager/encryptor_linux - loads data from
KWallet/gnome, shared by other apps

Previously, this also included SecureP2PSocket, which would be exposed via UDP,
but this has since been removed.

[wtc, 2011-11-15 02:33:58]

Patch Set 2 LGTM, but I disagree with converting all CHECKs
and DCHECKs to tests.

It is fine to convert the CHECKs/DCHECKs that validate input
parameters to tests.  But the CHECKs/DCHECKs that assert
library function behavior or that were added because of
paranoia should not be converted to tests.  When they become
tests, the tests are confusing because they seem to suggest
we expect those conditions may happen.

I am giving this CL LGTM because I have limited time for
code review this week.  I am counting on you to fix this
CHECK/DCHECK issue before you commit this CL.

http://codereview.chromium.org/8511050/diff/1001/crypto/encryptor_mac.cc
File crypto/encryptor_mac.cc (right):

http://codereview.chromium.org/8511050/diff/1001/crypto/encryptor_mac.cc#newc...
crypto/encryptor_mac.cc:57: if (output_size == 0 || output_size + 1 <
input.size())

I don't think we need to check output_size + 1 < input.size().
The purpose of the test is very subtle.  I believe you are
detecting size_t overflow of output_size+1 on line 65.
If so, it seems that you should also detect input.size() + iv_.size()
overflowing size_t.

http://codereview.chromium.org/8511050/diff/1001/crypto/encryptor_mac.cc#newc...
crypto/encryptor_mac.cc:86: if (ciphertext.empty())

Why don't you also check mode_ != CBC here as you did above>

http://codereview.chromium.org/8511050/diff/1001/crypto/encryptor_nss.cc
File crypto/encryptor_nss.cc (left):

http://codereview.chromium.org/8511050/diff/1001/crypto/encryptor_nss.cc#oldc...
crypto/encryptor_nss.cc:192: CHECK(!digest_len);

IMPORTANT: Some of these CHECKs should not be converted to tests.
If they are not useful, we can remove them.

http://codereview.chromium.org/8511050/diff/1001/crypto/encryptor_nss.cc
File crypto/encryptor_nss.cc (right):

http://codereview.chromium.org/8511050/diff/1001/crypto/encryptor_nss.cc#newc...
crypto/encryptor_nss.cc:117: return false;  // Size overflow.
Nit: Size => size_t

http://codereview.chromium.org/8511050/diff/1001/crypto/encryptor_nss.cc#newc...
crypto/encryptor_nss.cc:120: result.resize(output_len);

Is there a constructor that takes the size input?

http://codereview.chromium.org/8511050/diff/1001/crypto/encryptor_openssl.cc
File crypto/encryptor_openssl.cc (right):

http://codereview.chromium.org/8511050/diff/1001/crypto/encryptor_openssl.cc#...
crypto/encryptor_openssl.cc:135: return false;

If we get here, we have overrun the buffer in 'result'.
So this check should really be an assertion.

[Ryan Sleevi, 2011-12-14 06:07:52]

wtc: Sorry for letting this idle a bit.

I think I've found a reasonable balance between handling user input errors and
dev error/library failures.

I also went and tweaked the result/output relationship to reduce the number of
allocations (instead of output->clear() + result.resize(), it's
output->swap(result), result.resize()).

Reviewing the most recent patchset against trunk, as opposed to a previous
patchset, is the best way to look at things. Since pkasting's change to add
additional error controls has landed, it provides better insight into what
transitioned from CHECK -> soft error and what remained a (D)CHECK.

http://codereview.chromium.org/8511050/diff/1001/crypto/encryptor_mac.cc
File crypto/encryptor_mac.cc (right):

http://codereview.chromium.org/8511050/diff/1001/crypto/encryptor_mac.cc#newc...
crypto/encryptor_mac.cc:86: if (ciphertext.empty())
On 2011/11/15 02:33:58, wtc wrote:
> 
> Why don't you also check mode_ != CBC here as you did above>

Empty ciphertext should always be treated as an error. While it can be treated
as empty plaintext, there's a subtle difference between a valid, empty CBC block
and a 'no data' block of |ciphertext.tmpy()|

For encryption, encrypting empty CBC blocks is fine (and can be used to confound
some known attacks on IV - eg: BEAST), as they will result in non-empty
ciphertext. Encrypting empty blocks under other types isn't.

[wtc, 2011-12-15 02:03:58]

Review comments on Patch Set 5:

Some of my comments below also apply to the same code patterns
in the other files.  I didn't bother to mark all of them.

http://codereview.chromium.org/8511050/diff/13002/crypto/encryptor.cc
File crypto/encryptor.cc (right):

http://codereview.chromium.org/8511050/diff/13002/crypto/encryptor.cc#newcode98
crypto/encryptor.cc:98: return false;

Should we also mark the GenerateCounterMask method with
WARN_UNUSED_RESULT?

It seems that we should support the encryption of nothing.
Does this failure return prevent us from supporting that?

http://codereview.chromium.org/8511050/diff/13002/crypto/encryptor_mac.cc
File crypto/encryptor_mac.cc (right):

http://codereview.chromium.org/8511050/diff/13002/crypto/encryptor_mac.cc#new...
crypto/encryptor_mac.cc:48: output->swap(result);

IMPORTANT: this swap is not necessary.  Our convention is
that if a function fails, the output arguments should be
ignored.

http://codereview.chromium.org/8511050/diff/13002/crypto/encryptor_mac.cc#new...
crypto/encryptor_mac.cc:58: result_size + 1 < input.size()) {

IMPORTANT: The need to test both result_size < input.size() and
result_size + 1 < input.size() is too subtle.  If it is
intentional, please add a comment.

http://codereview.chromium.org/8511050/diff/13002/crypto/encryptor_mac.cc#new...
crypto/encryptor_mac.cc:82: ciphertext->clear();

This ciphertext->clear() call and the one on line 91 below
can be omitted.

http://codereview.chromium.org/8511050/diff/13002/crypto/encryptor_nss.cc
File crypto/encryptor_nss.cc (right):

http://codereview.chromium.org/8511050/diff/13002/crypto/encryptor_nss.cc#new...
crypto/encryptor_nss.cc:83: ciphertext->clear();

The clear() calls before returning false can all be omitted.

[Peter Kasting, 2011-12-15 02:12:39]

http://codereview.chromium.org/8511050/diff/13002/crypto/encryptor_mac.cc
File crypto/encryptor_mac.cc (right):

http://codereview.chromium.org/8511050/diff/13002/crypto/encryptor_mac.cc#new...
crypto/encryptor_mac.cc:48: output->swap(result);
On 2011/12/15 02:03:58, wtc wrote:
> 
> IMPORTANT: this swap is not necessary.  Our convention is
> that if a function fails, the output arguments should be
> ignored.

FWIW I intentionally eliminated both swap()s in my change and replaced them with
direct modification of |output|, which I still think is clearer and cleaner than
the old way that this patch returns to.

I'm not sure I agree that we have a consistent convention about how to deal with
outparams on failure; I thought the convention (if any) was that if easy, code
should leave them untouched rather than explicitly clearing them as this code
does.  This could be implemented by simply eliminating the first swap() (here)
in this function, which I think would be an improvement if we really don't want
to touch |output| in the rest of the function body.

[wtc, 2011-12-15 02:16:44]

http://codereview.chromium.org/8511050/diff/13002/crypto/encryptor_mac.cc
File crypto/encryptor_mac.cc (right):

http://codereview.chromium.org/8511050/diff/13002/crypto/encryptor_mac.cc#new...
crypto/encryptor_mac.cc:48: output->swap(result);
On 2011/12/15 02:12:39, Peter Kasting wrote:
>
> I'm not sure I agree that we have a consistent convention about how to deal
with
> outparams on failure; I thought the convention (if any) was that if easy, code
> should leave them untouched rather than explicitly clearing them as this code
> does.

You are right.  You stated the convention better than I did.

[Ryan Sleevi, 2011-12-15 02:18:16]

Is there a use-case for wanting to be able to treat an empty string as valid
decrypted data? Or an empty string as valid data to encrypt in a non-CBC mode
(where the result would simply be an empty string)

I can't think of any case where we would want to - and certainly, the APIs we're
backed by are generally not accommodating to zero-length inputs, for the correct
reason that they result in zero-length outputs.

http://codereview.chromium.org/8511050/diff/13002/crypto/encryptor.cc
File crypto/encryptor.cc (right):

http://codereview.chromium.org/8511050/diff/13002/crypto/encryptor.cc#newcode98
crypto/encryptor.cc:98: return false;
On 2011/12/15 02:03:58, wtc wrote:
> 
> Should we also mark the GenerateCounterMask method with
> WARN_UNUSED_RESULT?
> 
> It seems that we should support the encryption of nothing.
> Does this failure return prevent us from supporting that?

Yes. It matches the previous CHECK, but empty plaintext in CTR mode is treated
as a failure.

http://codereview.chromium.org/8511050/diff/13002/crypto/encryptor_mac.cc
File crypto/encryptor_mac.cc (right):

http://codereview.chromium.org/8511050/diff/13002/crypto/encryptor_mac.cc#new...
crypto/encryptor_mac.cc:48: output->swap(result);
On 2011/12/15 02:03:58, wtc wrote:
> 
> IMPORTANT: this swap is not necessary.  Our convention is
> that if a function fails, the output arguments should be
> ignored.

This was based on past misuses of this API, where failure was ignored. Even with
WARN_UNUSED_RESULT, it's easy to miss checking the result:
bool b = encryptor_.Encrypt(plaintext, &ciphertext);
// Do something with ciphertext - except perhaps bad.

The effect this has is that it's equivalent to output->clear(). By using
->swap(result), we can re-use output's pointer for result (which is generally
pre-allocated/sized to the right length).

I'm fine removing this extra layer of misuse guarding, and writing everything
into |output| (and then needing to ->clear() on any subsequent / API failures),
I just wanted to make sure the reasoning for it was known.

http://codereview.chromium.org/8511050/diff/13002/crypto/encryptor_mac.cc#new...
crypto/encryptor_mac.cc:58: result_size + 1 < input.size()) {
On 2011/12/15 02:03:58, wtc wrote:
> 
> IMPORTANT: The need to test both result_size < input.size() and
> result_size + 1 < input.size() is too subtle.  If it is
> intentional, please add a comment.

It is intentional - I will add a comment to clarify.

[Peter Kasting, 2011-12-15 02:20:08]

http://codereview.chromium.org/8511050/diff/13002/crypto/encryptor_mac.cc
File crypto/encryptor_mac.cc (right):

http://codereview.chromium.org/8511050/diff/13002/crypto/encryptor_mac.cc#new...
crypto/encryptor_mac.cc:58: result_size + 1 < input.size()) {
On 2011/12/15 02:18:16, Ryan Sleevi wrote:
> On 2011/12/15 02:03:58, wtc wrote:
> > 
> > IMPORTANT: The need to test both result_size < input.size() and
> > result_size + 1 < input.size() is too subtle.  If it is
> > intentional, please add a comment.
> 
> It is intentional - I will add a comment to clarify.

Uff da.

If you're going to add a comment anyway, then please condense to only checking
the "+ 1" case in code.  You can mention the other case in your comment and talk
about what it means and that the + 1 case will cover it.

[wtc, 2011-12-15 22:04:14]

http://codereview.chromium.org/8511050/diff/13002/crypto/encryptor.cc
File crypto/encryptor.cc (right):

http://codereview.chromium.org/8511050/diff/13002/crypto/encryptor.cc#newcode98
crypto/encryptor.cc:98: return false;
On 2011/12/15 02:18:16, Ryan Sleevi wrote:
> It matches the previous CHECK, but empty plaintext in CTR mode is treated
> as a failure.

Thanks for the explanation.  This is fine.

http://codereview.chromium.org/8511050/diff/13002/crypto/encryptor_mac.cc
File crypto/encryptor_mac.cc (right):

http://codereview.chromium.org/8511050/diff/13002/crypto/encryptor_mac.cc#new...
crypto/encryptor_mac.cc:48: output->swap(result);
On 2011/12/15 02:18:16, Ryan Sleevi wrote:
>
> This was based on past misuses of this API, where failure was ignored. Even
with
> WARN_UNUSED_RESULT, it's easy to miss checking the result:
> bool b = encryptor_.Encrypt(plaintext, &ciphertext);
> // Do something with ciphertext - except perhaps bad.

Thank you for the explanation.  It is hard to figure out
what the extra output->swap(result) call at the beginning
of this function is intended for.  I would just remove it
rather than adding a comment to explain it.

[Peter Kasting, 2012-06-13 19:13:23]

rsleevi: This seems to have died.  Did you want to resurrect this?

[Ryan Sleevi, 2012-06-21 18:51:28]

On 2012/06/13 19:13:23, Peter Kasting wrote:
> rsleevi: This seems to have died.  Did you want to resurrect this?

Closed. We killed off the platform-specific encryptors.