tree 515ccc82ffdb24abd326dcbb979e06bbf7160bd0
parent bdc509820b70d8cb81f04f3d91b334b4e2933708
author David Benjamin <davidben@google.com> 1785781055 -0400
committer boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com <boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com> 1785951011 -0700

Unwind EVP_CIPHER_CTX poisoning

The original motivation here was that CBC caused the wrapper layer to
call into the EVP_CIPHER twice. An error in the second call would put
the EVP_CIPHER_CTX in an unpredictable state. And so
https://boringssl-review.googlesource.com/c/boringssl/+/54185 added some
basic "poison" tracking to try to catch egregious caller mistakes.

This then got extended significantly in
https://boringssl-review.googlesource.com/c/boringssl/+/83447 to catch
all failures. Extending it caused some problems, both in Conscrypt:

1. This changed public API at the Conscrypt level if a Java Cipher
   object was called after an error. See b/504728350. While somewhat
   questionable caller behavior, it was an unexpected behavior change.

2. The Java Cipher APIs promise that you can retry after a
   ShortBufferException. See [0]. Conscrypt currently does all the
   bounds checks itself, but this is itself problematic, so we'd rather
   shift that into BoringSSL, which is better suited to calculate this.

https://boringssl-review.googlesource.com/c/boringssl/+/99648 patched
this with further complexity to the 'poisoned' bit, but it turns out
this was all unnecessary. If we look at where cipher_update is
*actually* fallible, it turns out:

- Almost every cipher_update is infallible, notably all the CBC ones.

- AES-GCM and AES-XTS check if their keys have been initialized. (The
  others should but don't. We should probably have a better notion of
  when the EVP_CIPHER_CTX has been initialized. Either way, this is not
  a pattern of failure that can trigger our problem.)

- AES-XTS checks if its input is long enough.

- AES-GCM checks if the input is too long.

Our XTS implementation doesn't support streaming anyway, and GCM is not
padded or block-buffered, so the motivation for poisoning doesn't apply.
THat means this was all moot. Indeed it's telling that the only positive
test for poisoning had to make a custom EVP_CIPHER, and supporting
custom EVP_CIPHERs is explicitly out of scope.

Unwind the whole poisoning machinery entirely. Also fix some of the
tests added in 99648 to actually test what [0] needs: that retrying the
operation gives the same thing. Also fix a bug, caught by asserts, in
the EVP_EncryptUpdate_ex length check: EVP_CIPHER_CTX_max_next_update
checks ctx->encrypt, but decrypt calls into encrypt.

[0] https://docs.oracle.com/en/java/javase/21/docs/api/java.base/javax/crypto/Cipher.html#:~:text=at%20outputOffset%20inclusive.-,If%20the%20output%20buffer%20is%20too%20small%20to%20hold%20the%20result%2C%20a%20ShortBufferException%20is%20thrown.%20In%20this%20case%2C%20repeat%20this%20call%20with%20a%20larger%20output%20buffer.%20Use%20getOutputSize%20to%20determine%20how%20big%20the%20output%20buffer%20should%20be.,-If%20inputLen%20is

Fixed: 520121641
Change-Id: I7c1c4138d2553fbd9ea72013962f60fa9a3e628c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/99990
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Rudolf Polzer <rpolzer@google.com>
Presubmit-BoringSSL-Verified: boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com <boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com>