Add PKCS7-internal BIO_f_cipher

[WillChilds-Klein]

Issues:

Addresses CryptoAlg-2494

Description of changes:

This change introduces a new filter BIO, BIO_f_cipher for use in PR 1816.

The cipher BIO sits in front of a backing BIO, encrypting incoming writes before writing to the backing BIO and decrypting data read from the backing BIO. This implementation is almost an exact copy of OpenSSL's with some functionality removed. We try to change as little of the underlying logic as possible, but rename variables and add comments for clarity.

Call-outs:

• n/a

Testing:

• new unit tests. test considerations and flow are documented in-line.

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 93.15589% with 18 lines in your changes missing coverage. Please review.

Project coverage is 78.54%. Comparing base (ca8180c) to head (1a2d1dc).

Files with missing lines	Patch %	Lines
crypto/pkcs7/bio/cipher.c	91.30%	16 Missing ⚠️
crypto/pkcs7/bio/bio_deprecated_test.cc	97.46%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1836      +/-   ##
==========================================
+ Coverage   78.49%   78.54%   +0.04%     
==========================================
  Files         585      587       +2     
  Lines       99630    99893     +263     
  Branches    14254    14289      +35     
==========================================
+ Hits        78206    78459     +253     
- Misses      20789    20797       +8     
- Partials      635      637       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[samuel40791765]

Just some preliminary comments, haven't gone through everything yet.

[justsmth]

To prevent making a second call to EVP_CipherFinal_ex on a subsequent iteration, should it be setting ctx->done = 1 prior to the continue? Then, ctx->done would need to be checked prior to the call to BIO_read(next, ctx->buf, to_read).

[justsmth]

The bytes_read <= 0 condition here seems to imply an error in next for which we should also set: ctx->ok = 0?

Our documentation says BIO_read:

returns the number of bytes read, zero on EOF, or a negative number on error

[justsmth]

Should we also set ctx->ok = 0 when bytes_written < 0?

[justsmth]

We should return an error if this is called when ctx->done == 1 (or ctx->ok == 0).

[justsmth]

For BIO_write, I think we should distinguish bytes_written == 0 (no bytes written) from bytes_written < 0 (an error). On an error, we might need to set ctx->ok = 0 and ctx->done = 1?

[justsmth]

I think this loop instead be conditioned on ctx->buf_len > 0 || ctx->done == 0. It seems possible/likely that ctx->buf_len == 0 holds when BIO_flush is called, in which case this loop never executes (EVP_CipherFinal_ex never gets called), and it returns 1 as-if it succeeded.

  while (ctx->buf_len > 0 || ctx->done == 0) {
    if (ctx->buf_len > 0) {
...
    } else if(ctx->done == 0) {
...
    }
  }

[justsmth]

In this case, I think the return value should be 0 b/c the buffer was not fully flushed. (If we return a positive value, the caller may assume EVP_CipherFinal_ex was called and all data was written.