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>
3 files changed
tree: 515ccc82ffdb24abd326dcbb979e06bbf7160bd0
  1. .agents/
  2. .bcr/
  3. .github/
  4. agents/
  5. bench/
  6. cmake/
  7. crypto/
  8. decrepit/
  9. docs/
  10. fuzz/
  11. gen/
  12. include/
  13. infra/
  14. pki/
  15. rust/
  16. ssl/
  17. third_party/
  18. tool/
  19. util/
  20. .bazelignore
  21. .bazelrc
  22. .bazelversion
  23. .clang-format
  24. .clang-format-ignore
  25. .clangd
  26. .gitattributes
  27. .gitignore
  28. API-CONVENTIONS.md
  29. AUTHORS
  30. BREAKING-CHANGES.md
  31. BUILD.bazel
  32. build.json
  33. BUILDING.md
  34. CMakeLists.txt
  35. codereview.settings
  36. CONTRIBUTING.md
  37. FUZZING.md
  38. go.mod
  39. go.sum
  40. INCORPORATING.md
  41. LICENSE
  42. MODULE.bazel
  43. MODULE.bazel.lock
  44. PORTING.md
  45. PRESUBMIT.py
  46. PrivacyInfo.xcprivacy
  47. README.md
  48. SANDBOXING.md
  49. SECURITY.md
  50. STYLE.md
README.md

BoringSSL

BoringSSL is a fork of OpenSSL that is designed to meet Google's needs.

Although BoringSSL is an open source project, it is not intended for general use, as OpenSSL is. We don't recommend that third parties depend upon it. Doing so is likely to be frustrating because there are no guarantees of API or ABI stability.

Programs ship their own copies of BoringSSL when they use it and we update everything as needed when deciding to make API changes. This allows us to mostly avoid compromises in the name of compatibility. It works for us, but it may not work for you.

BoringSSL arose because Google used OpenSSL for many years in various ways and, over time, built up a large number of patches that were maintained while tracking upstream OpenSSL. As Google's product portfolio became more complex, more copies of OpenSSL sprung up and the effort involved in maintaining all these patches in multiple places was growing steadily.

Currently BoringSSL is the SSL library in Chrome/Chromium, Android (but it's not part of the NDK) and a number of other apps/programs.

Project links:

To file a security issue, use the Chromium process and mention in the report this is for BoringSSL. You can ignore the parts of the process that are specific to Chromium/Chrome.

There are other files in this directory which might be helpful: