Add PKCS7-internal BIO_f_md #1886

WillChilds-Klein · 2024-09-27T21:57:55Z

Issues:

Addresses CryptoAlg-2494

Description of changes:

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

Call-outs:

n/a

Testing:

new unit tests

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 · 2024-09-27T22:20:14Z

Codecov Report

Attention: Patch coverage is 96.25668% with 7 lines in your changes missing coverage. Please review.

Project coverage is 78.71%. Comparing base (460a9dd) to head (81637c1).

Files with missing lines	Patch %	Lines
crypto/pkcs7/bio/md.c	91.86%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1886      +/-   ##
==========================================
+ Coverage   78.67%   78.71%   +0.03%     
==========================================
  Files         585      588       +3     
  Lines      100849   101037     +188     
  Branches    14299    14314      +15     
==========================================
+ Hits        79347    79535     +188     
- Misses      20868    20869       +1     
+ Partials      634      633       -1

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

samuel40791765 · 2024-10-02T21:03:09Z

crypto/pkcs7/bio/md.c

+#include "../crypto/bio/internal.h"
+#include "../internal.h"
+
+// BIO_put and BIO_get both add to the digest, BIO_gets returns the digest


Was this a leaked note? It doesn't seem like BIO_f_md has a puts method or am I overlooking something

hm, should be "md_write and md_read both contribute to the digest, md_gets returns digest contents"

crypto/pkcs7/bio/md.c

justsmth · 2024-10-07T16:02:06Z

crypto/pkcs7/bio/md.c

+static const BIO_METHOD methods_md = {
+    BIO_TYPE_MD, "message digest", md_write, md_read, /*puts*/ NULL,
+    md_gets,     md_ctrl,          md_new,   md_free, /*callback_ctrl*/ NULL,
+};


NP: This would be easier to read if each field was on a separate line.

I agree, but this was clang-format's doing. I've been using it for formatting stuff in the pkcs7/ module. If there's some set of flags/options that we prefer to use I'm happy to specify them.

NP: In other places, we have a comment next to each value indicating the field name it corresponds to:

static const BIO_METHOD methods_md = { BIO_TYPE_MD, // type "message digest", // name md_write, // bwrite md_read, // bread NULL, // bputs md_gets, // bgets md_ctrl, // ctrl md_new, // create md_free, // destroy NULL, // callback_ctrl };

justsmth · 2024-10-07T16:15:18Z

crypto/pkcs7/bio/md.c

+  ret = BIO_read(next, out, outl);
+  if (BIO_get_init(b)) {


This seems to support the use-case of leaving this BIO uninitialized while still reading data from next. In this case, data is returned that has not been digested. Is this use-case tested as well?

hm, good point. this use-case does appear to be supported as far back as OpenSSL 1.1.1. I didn't consider this, I can add a test case.

I do wonder -- should we support this? For compatibility, I'm leaning towards "yes" but it seems like a sharp edge that could cause subtle issues with digest calculation (see also response below)...

OpenSSL's own documentation indicates that the caller should re-initialize after DigestFinal (although it doesn't say anything about initial initialization):

After the digest has been retrieved from a digest BIO it must be reinitialized by calling BIO_reset(), or BIO_set_md() before any more data is passed through it.

Per discussion here, we now require the BIO to be initialized before it can do any IO.

hm, at any rate, it looks like IO on uninitialized BIO's is prevented higher in the call stack in bio.c.

justsmth · 2024-10-07T16:16:44Z

crypto/pkcs7/bio/md.c

+  BIO_clear_retry_flags(b);
+  BIO_copy_next_retry(b);


If b was not initialized, should we still be calling BIO_clear_retry_flags and BIO_copy_next_retry?

I think the answer to this is "yes". It would be needed to support its use prior to (or without) initialization.

this is addressed by requiring b to be initialized before it can do any IO.

justsmth · 2024-10-07T16:19:04Z

crypto/pkcs7/bio/md.c

+      if (EVP_DigestUpdate(ctx, (unsigned char *)out, ret) <= 0) {
+        return -1;


If this ever returns an error, should we somehow mark this BIO and being not OK for subsequent use? In this case, next has provided new data that can not be reprocessed on any subsequent call.

OpenSSL doesn't do this, but i agree philosophically that we should fail loudly and not silently corrupt the digest.

EVP_DigestUpdate 's documentation stipulates an interface contract of "It returns one". Looking at the implementation, the only case where it can break that contract is if an update function pointer isn't configured on ctx. I believe we can trust that to always be populated as long as ctx has been initialized by EVP_DigestInit_ex.

So, in other words, it looks like EVP_DigestUpdate can only fail if ctx hasn't been initialized. This makes me consider... should we fail b's reads/writes before next's IO if b hasn't been initialized? this would address your concern here and make it harder to misuse the BIO_f_md.

crypto/pkcs7/bio/md.c

justsmth · 2024-10-24T18:54:24Z

crypto/pkcs7/bio/md.c

+// |md_write| and |md_read| both contribute to the digest, |md_gets| returns
+// digest contents
+static int md_write(BIO *h, char const *buf, int num);
+static int md_read(BIO *h, char *buf, int size);
+static int md_gets(BIO *h, char *str, int size);
+static long md_ctrl(BIO *h, int cmd, long arg1, void *arg2);
+static int md_new(BIO *h);
+static int md_free(BIO *data);


NP: I would prefer having methods_md and BIO_f_md at the bottom of this source file. With that change, these static function declarations can be deleted.

justsmth · 2024-10-24T18:58:07Z

crypto/pkcs7/bio/md.c

+static const BIO_METHOD methods_md = {
+    BIO_TYPE_MD, "message digest", md_write, md_read, /*puts*/ NULL,
+    md_gets,     md_ctrl,          md_new,   md_free, /*callback_ctrl*/ NULL,
+};


NP: In other places, we have a comment next to each value indicating the field name it corresponds to:

static const BIO_METHOD methods_md = { BIO_TYPE_MD, // type "message digest", // name md_write, // bwrite md_read, // bread NULL, // bputs md_gets, // bgets md_ctrl, // ctrl md_new, // create md_free, // destroy NULL, // callback_ctrl };

justsmth · 2024-10-24T19:09:32Z

crypto/pkcs7/bio/md.c

+    if (!EVP_DigestUpdate(ctx, (const unsigned char *)in, ret)) {
+      BIO_clear_retry_flags(b);
+      return 0;
+    }


NP: Since this is a non-recoverable error, shouldn't the return value be negative? The BIO_write documentation seems to say that.

// returns the value from calling the |callback_ex|, otherwise |BIO_write|
// returns the number of bytes written, or a negative number on error.

I see OpenSSL returns 0 for this, so compatibility might dictate we do the same. (I'm also bothered that the we're unable to inform the caller about the actual number of bytes written to next b/c we can't return a value indicating success. But that's just the nature of this API.)

WillChilds-Klein force-pushed the pkcs7-bio-md branch from 36c0ed7 to 9b7deca Compare September 29, 2024 20:44

WillChilds-Klein changed the title ~~Initial impl, basic test~~ [DRAFT] Add PKCS7-internal BIO_f_md Sep 29, 2024

WillChilds-Klein force-pushed the pkcs7-bio-md branch from 80f21e6 to df434b2 Compare September 30, 2024 15:42

WillChilds-Klein changed the title ~~[DRAFT] Add PKCS7-internal BIO_f_md~~ Add PKCS7-internal BIO_f_md Sep 30, 2024

WillChilds-Klein marked this pull request as ready for review September 30, 2024 16:57

WillChilds-Klein requested a review from a team as a code owner September 30, 2024 16:57

WillChilds-Klein requested review from samuel40791765 and justsmth September 30, 2024 16:58

WillChilds-Klein force-pushed the pkcs7-bio-md branch from bc6f15f to 1b1e2f6 Compare October 1, 2024 13:31

samuel40791765 reviewed Oct 2, 2024

View reviewed changes

WillChilds-Klein force-pushed the pkcs7-bio-md branch from 1b1e2f6 to 8828fbd Compare October 4, 2024 15:10

justsmth reviewed Oct 7, 2024

View reviewed changes

WillChilds-Klein added 4 commits October 24, 2024 15:11

Add PKCS7-internal BIO_f_md

de78167

Fix comment

c2a933d

Rename md bio test

773e668

Address PR feedback

7a3f7bc

WillChilds-Klein force-pushed the pkcs7-bio-md branch from 8828fbd to 7a3f7bc Compare October 24, 2024 15:11

WillChilds-Klein requested review from samuel40791765 and justsmth October 24, 2024 15:16

Fix strict compiler error

81637c1

justsmth approved these changes Oct 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PKCS7-internal BIO_f_md #1886

Add PKCS7-internal BIO_f_md #1886

WillChilds-Klein commented Sep 27, 2024

codecov-commenter commented Sep 27, 2024 •

edited

Loading

samuel40791765 Oct 2, 2024

WillChilds-Klein Oct 3, 2024

justsmth Oct 7, 2024

WillChilds-Klein Oct 14, 2024

justsmth Oct 24, 2024 •

edited

Loading

justsmth Oct 7, 2024 •

edited

Loading

WillChilds-Klein Oct 14, 2024

WillChilds-Klein Oct 24, 2024

WillChilds-Klein Oct 24, 2024

justsmth Oct 7, 2024

justsmth Oct 7, 2024

WillChilds-Klein Oct 24, 2024

justsmth Oct 7, 2024

WillChilds-Klein Oct 14, 2024

justsmth Oct 24, 2024

justsmth Oct 24, 2024 •

edited

Loading

justsmth Oct 24, 2024

		if (EVP_DigestUpdate(ctx, (unsigned char *)out, ret) <= 0) {
		return -1;

Add PKCS7-internal BIO_f_md #1886

Are you sure you want to change the base?

Add PKCS7-internal BIO_f_md #1886

Conversation

WillChilds-Klein commented Sep 27, 2024

Issues:

Description of changes:

Call-outs:

Testing:

codecov-commenter commented Sep 27, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justsmth Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

justsmth Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justsmth Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 27, 2024 •

edited

Loading

justsmth Oct 24, 2024 •

edited

Loading

justsmth Oct 7, 2024 •

edited

Loading

justsmth Oct 24, 2024 •

edited

Loading