Add streaming support for PKCS7_VerifySignedData. by TakayukiMatsuo · Pull Request #6961 · wolfSSL/wolfssl

TakayukiMatsuo · 2023-11-10T05:37:27Z

Description

Extend PKCS7_VerifySignedData streaming to support PKCS7 bundle which has multiple part octet.

Fixes zd#16606

Testing

Run an application and data provided in zd#16606.

Checklist

added tests
updated/added doxygen
updated appropriate READMEs
Updated manual and documentation

TakayukiMatsuo · 2023-12-11T09:51:31Z

Hi　@JacobBarthelmeh ,

In this PR, I made a small change to wc_PKCS7_AddDataToStream. This change resets pksc7->stream>idx to 0 after reading the input data that satisfies the argument expected. Without this modification, If pksc7->stream>idx is not reset to 0, the input data will be skipped in the next wc_PKCS7_AddDataToStream call.

I thought this was a bug, so made a change in this PR. However, this change caused errors in existing streaming test cases.
Am wondering if this change is wrong, or if I'm calling wc_PKCS7_AddDataToStream incorrectly. I would be happy to hear your opinions on the changes I have made.

Thanks,
Tak

douzzer

Hi Tak. Most of the change requests from my review are for reports from automated analysis.

The one bigger problem is regarding the change to wc_PKCS7_AddDataToStream(). In order to get the existing unit tests to pass, I had to comment that out.

I don't yet understand the problem you were addressing with that clause, but whatever it is, we need to include new test(s) in wolfcrypt/test/test.c and/or tests/api.c that test for it and fail without the fix.

douzzer · 2023-12-20T19:43:37Z

This needs to move up to avoid -Wdeclaration-after-statement.

Moved that variable declaration up.

douzzer · 2023-12-20T19:46:00Z

line length

fixed the length.

douzzer · 2023-12-20T19:46:26Z

line length

fixed the length.

douzzer · 2023-12-20T19:46:43Z

line length

fixed the length.

douzzer · 2023-12-20T19:46:57Z

line length

fixed the length.

douzzer · 2023-12-20T19:49:51Z

With this clause added, there are numerous test failures, including testwolfcrypt:

error L=43158 code=-140 (ASN parsing error, invalid input)

All tests pass with this commented out, but I assume that's creating other problems this was intended to address.

removed the portion.

douzzer · 2023-12-20T20:02:06Z

These assignments are producing

wolfssl/wolfcrypt/src/pkcs7.c:5403:13: warning: Value stored to 'multiPart' is never read [clang-analyzer-deadcode.DeadStores] wolfssl/wolfcrypt/src/pkcs7.c:5404:13: warning: Value stored to 'detached' is never read [clang-analyzer-deadcode.DeadStores]

removed those two lines.

dgarske · 2024-01-17T18:17:40Z

@TakayukiMatsuo , I see PR review feedback that is not yet fixed. Can you provide an update on the status of this PR? Are you planning to push further fixes? Thanks, David Garske

JacobBarthelmeh · 2024-01-17T18:24:17Z

FYI for the current failing PRB master it looks to be a build when PKCS7 streaming mode is disabled (defining the macro NO_PKCS7_STREAM):

wolfcrypt/src/pkcs7.c:5273:18: error: ‘PKCS7’ has no member named ‘stream’
 5273 |         if (pkcs7->stream->content != NULL) {

      |                  ^~

In file included from ./wolfssl/wolfcrypt/pkcs7.h:29,

                 from wolfcrypt/src/pkcs7.c:31:

wolfcrypt/src/pkcs7.c:5274:24: error: ‘PKCS7’ has no member named ‘stream’
 5274 |             XFREE(pkcs7->stream->content, pkcs7->heap, DYNAMIC_TYPE_PKCS7);
      |                        ^~
./wolfssl/wolfcrypt/types.h:573:63: note: in definition of macro ‘XFREE’

Not as sure on the krb5 actions test, it would need farther investigation.

TakayukiMatsuo · 2024-01-19T06:51:43Z

@douzzer , @JacobBarthelmeh and @dgarske , thank you for your help. It's been a while, but I will adjust my time to complete this PR.

bandi13 · 2024-02-05T20:12:09Z

retest this please

cconlon · 2024-02-20T00:35:55Z

@TakayukiMatsuo Is this ready for re-review?

Thanks,
Chris

TakayukiMatsuo · 2024-02-20T00:39:21Z

@cconlon , yes, it is ready.

cconlon · 2024-02-20T00:42:51Z

Thanks @TakayukiMatsuo, I'm assigning over to @JacobBarthelmeh for re-review. @JacobBarthelmeh, feel free to have @douzzer re-review his comments if you would like.

JacobBarthelmeh

There appears to be some issues when trying this out. Could you let me know if these tests are invalid?

Steps to recreate:

Create a test SMIME bundle

cd wolfssl-examples/pkcs7
openssl smime -sign -in ~/Documents/wolfssl/configure.ac -out test-signed -signer ../certs/ca-cert.pem -nodetach -nocerts -binary -outform DER -stream -inkey ../certs/ca-key.pem

Alter the pkcs7-verify.c example to take in an argument and verify in chunks:

--- a/pkcs7/pkcs7-verify.c
+++ b/pkcs7/pkcs7-verify.c
@@ -42,7 +42,7 @@ int main(int argc, char** argv)
 #endif
 
     /* load DER PKCS7 */
-    derFile = fopen("signed.p7s", "rb");
+    derFile = fopen(argv[1], "rb");
     if (derFile) {
         fseek(derFile, 0, SEEK_END);
         derSz = (int)ftell(derFile);
@@ -70,9 +70,14 @@ int main(int argc, char** argv)
     if (rc != 0) goto exit;
     rc = wc_PKCS7_InitWithCert(&pkcs7, NULL, 0);
     if (rc != 0) goto exit;
-    rc = wc_PKCS7_VerifySignedData(&pkcs7, derBuf, derSz);
-    if (rc != 0) goto exit;
 
+    int z, chunkSz = 1;
+    for (z = 0; z < derSz;) {
+        int sz = (z + chunkSz > derSz)? derSz - z : chunkSz;
+        rc = wc_PKCS7_VerifySignedData(&pkcs7, derBuf+z, sz);
+        z+=sz;
+    }
+    if (rc != 0) goto exit;
     printf("PKCS7 Verify Success\n");
 
 exit:

With chunkSz set to 1 it fails out with -270 (want more data), but set to 10 it succeeds with -272 (needs sig verify) which is expected here since -nocerts was used when creating the test bundle.

Moving on to a bundle that has certificates in it:

openssl smime -sign -in ~/Documents/wolfssl/configure.ac -out test-signed -signer ../certs/ca-cert.pem -nodetach -binary -outform DER -stream -inkey ../certs/ca-key.pem

The test application is getting -270 with chunkSz set to 10:

bash-3.2$ ./pkcs7-verify test-signed 
Der 318126
RC=-270

Upping chunkSz to 100 then saw it verify successfully.

douzzer

looks mostly good -- super-quick-check found one trivial defect.

douzzer · 2024-02-21T20:27:02Z

This is warning on C++:

[all-g++] [7 of 31] [2bce5fef9f] configure... real 0m21.405s user 0m8.403s sys 0m14.967s build...In file included from ./wolfssl/wolfcrypt/pkcs7.h:29, from wolfcrypt/src/pkcs7.c:31: wolfcrypt/src/pkcs7.c: In function ‘int PKCS7_VerifySignedData(PKCS7*, const byte*, word32, byte*, word32, byte*, word32)’: 9386a882b9 (<juliusz@wolfssl.com> 2021-10-19 15:51:29 +0200 571) #define XMALLOC(s, h, t) ((void)(h), (void)(t), wolfSSL_Malloc((s))) ./wolfssl/wolfcrypt/types.h:571:67: error: invalid conversion from ‘void*’ to ‘byte*’ {aka ‘unsigned char*’} [-fpermissive] 571 | #define XMALLOC(s, h, t) ((void)(h), (void)(t), wolfSSL_Malloc((s))) | ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~ | | | void* 2bce5fef9f (<tak@wolfssl.com> 2023-11-07 13:09:38 +0900 5363) pkcs7->contentDynamic = XMALLOC(pkcs7->stream->contentSz, wolfcrypt/src/pkcs7.c:5363:43: note: in expansion of macro ‘XMALLOC’ 5363 | pkcs7->contentDynamic = XMALLOC(pkcs7->stream->contentSz, | ^~~~~~~ make[2]: *** [Makefile:7251: wolfcrypt/src/src_libwolfssl_la-pkcs7.lo] Error 1 make[2]: *** Waiting for unfinished jobs.... make[1]: *** [Makefile:8064: all-recursive] Error 1 make: *** [Makefile:4782: all] Error 2 real 0m14.125s user 1m20.695s sys 0m8.076s all-g++ fail_build failed config: '--enable-all' '--enable-testcert' '--enable-srtp' '--enable-sp-math-all' 'CC=g++' 'CPPFLAGS=-DNO_WOLFSSL_CIPHER_SUITE_TEST -DWOLFSSL_OLD_PRIME_CHECK'

Added a cast to XMALLOC.

TakayukiMatsuo · 2024-02-24T03:15:19Z

Hi Jacob,

Probably need to modify the code fragment you use a bit to be able to handle the return code from wc_PKCS7_VerifySignedData, like this:

 for (z = 0; z < derSz;) {
        int sz = (z + chunkSz > derSz)? derSz - z : chunkSz;
        rc = wc_PKCS7_VerifySignedData(&pkcs7, derBuf + z, sz);
        if (rc < 0) {
            if (rc == WC_PKCS7_WANT_READ_E) {
                z += sz;
                continue;
            }
            printf("Error z:%d %d: %s\n", z, rc, wc_GetErrorString(rc));
            break;
        }
        else {
            break;
        }
    }

With this code, could get expected result with the test bundle file.
Tak

JacobBarthelmeh · 2024-02-26T08:24:47Z

Hi Jacob,

Probably need to modify the code fragment you use a bit to be able to handle the return code from wc_PKCS7_VerifySignedData, like this:
 for (z = 0; z < derSz;) {
        int sz = (z + chunkSz > derSz)? derSz - z : chunkSz;
        rc = wc_PKCS7_VerifySignedData(&pkcs7, derBuf + z, sz);
        if (rc < 0) {
            if (rc == WC_PKCS7_WANT_READ_E) {
                z += sz;
                continue;
            }
            printf("Error z:%d %d: %s\n", z, rc, wc_GetErrorString(rc));
            break;
        }
        else {
            break;
        }
    }
With this code, could get expected result with the test bundle file. Tak

Tak, adding an early bail on error case does succeed. This likely means that the last bytes of the bundle are not being parsed though and that it is reaching the (-272) error in this case early. With indef it should only be 0's but I think we should have it confirm that they are. Adding derBuf[derSz - 2] = derBuf[derSz - 2] + 1; before verifying the bundle seems to still return success.

bandi13 · 2024-02-27T14:26:29Z

retest this please

TakayukiMatsuo · 2024-02-27T18:06:07Z

Hi Jacob, added trailing zero's check.

JacobBarthelmeh · 2024-02-28T14:40:40Z

Hi @TakayukiMatsuo , Please double check the case of altering the last bytes in the bundle before verification.

TakayukiMatsuo · 2024-02-29T02:04:20Z

Hi @JacobBarthelmeh , Confirmed ASN_PARSE_E(-140) is returned when the last byte of the bundle is altered by adding derBuf[derSz - 2] = derBuf[derSz - 2] + 1; to the sample code.

JacobBarthelmeh

Thanks @TakayukiMatsuo for all your hard work on this!

cast of XMALLOC return was addressed

TakayukiMatsuo requested a review from wolfSSL-Bot November 10, 2023 06:33

TakayukiMatsuo force-pushed the pkcs7 branch from aa8855e to 8236af1 Compare November 12, 2023 10:59

JacobBarthelmeh assigned TakayukiMatsuo Nov 15, 2023

TakayukiMatsuo force-pushed the pkcs7 branch 3 times, most recently from 0cc9a0a to ee14511 Compare December 8, 2023 12:01

cconlon assigned JacobBarthelmeh and douzzer Dec 11, 2023

douzzer requested changes Dec 20, 2023

View reviewed changes

douzzer unassigned douzzer and JacobBarthelmeh Dec 20, 2023

dgarske removed the request for review from wolfSSL-Bot January 17, 2024 18:17

TakayukiMatsuo force-pushed the pkcs7 branch from b30cb28 to 109faea Compare January 26, 2024 05:32

TakayukiMatsuo force-pushed the pkcs7 branch from 109faea to 2315750 Compare February 5, 2024 06:40

TakayukiMatsuo force-pushed the pkcs7 branch 2 times, most recently from b30c7a4 to ac3e7a6 Compare February 9, 2024 01:18

TakayukiMatsuo force-pushed the pkcs7 branch 5 times, most recently from 595c59f to 746f302 Compare February 19, 2024 11:11

cconlon assigned JacobBarthelmeh and unassigned TakayukiMatsuo Feb 20, 2024

JacobBarthelmeh requested changes Feb 21, 2024

View reviewed changes

JacobBarthelmeh assigned TakayukiMatsuo and unassigned JacobBarthelmeh Feb 21, 2024

douzzer previously requested changes Feb 21, 2024

View reviewed changes

TakayukiMatsuo force-pushed the pkcs7 branch from 746f302 to 6b128b7 Compare February 23, 2024 22:15

TakayukiMatsuo assigned JacobBarthelmeh and unassigned TakayukiMatsuo Feb 24, 2024

TakayukiMatsuo requested review from JacobBarthelmeh and douzzer February 24, 2024 03:16

JacobBarthelmeh assigned TakayukiMatsuo and unassigned JacobBarthelmeh Feb 26, 2024

Add streaming support for PKCS7_VerifySignedData.

eeda0ca

TakayukiMatsuo force-pushed the pkcs7 branch from 6b128b7 to eeda0ca Compare February 27, 2024 06:05

TakayukiMatsuo assigned JacobBarthelmeh and unassigned TakayukiMatsuo Feb 27, 2024

JacobBarthelmeh assigned TakayukiMatsuo and unassigned JacobBarthelmeh Feb 28, 2024

TakayukiMatsuo assigned JacobBarthelmeh and unassigned TakayukiMatsuo Feb 29, 2024

JacobBarthelmeh approved these changes Mar 1, 2024

View reviewed changes

JacobBarthelmeh merged commit 95eb179 into wolfSSL:master Mar 1, 2024

TakayukiMatsuo deleted the pkcs7 branch March 1, 2024 20:13

Conversation

TakayukiMatsuo commented Nov 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Checklist

Uh oh!

TakayukiMatsuo commented Dec 11, 2023

Uh oh!

douzzer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgarske commented Jan 17, 2024

Uh oh!

JacobBarthelmeh commented Jan 17, 2024

Uh oh!

TakayukiMatsuo commented Jan 19, 2024

Uh oh!

bandi13 commented Feb 5, 2024

Uh oh!

cconlon commented Feb 20, 2024

Uh oh!

TakayukiMatsuo commented Feb 20, 2024

Uh oh!

cconlon commented Feb 20, 2024

Uh oh!

JacobBarthelmeh left a comment

Choose a reason for hiding this comment

Uh oh!

douzzer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TakayukiMatsuo commented Feb 24, 2024

Uh oh!

JacobBarthelmeh commented Feb 26, 2024

Uh oh!

bandi13 commented Feb 27, 2024

Uh oh!

TakayukiMatsuo commented Feb 27, 2024

Uh oh!

JacobBarthelmeh commented Feb 28, 2024

Uh oh!

TakayukiMatsuo commented Feb 29, 2024

Uh oh!

JacobBarthelmeh left a comment

TakayukiMatsuo commented Nov 10, 2023 •

edited

Loading