Add NULL check before accessing PKCS7 encrypted algorithm

[lejcik]

Printing content of an invalid test certificate causes application crash, because of NULL dereference:

user@user:~/openssl$ openssl pkcs12 -in test/recipes/80-test_pkcs12_data/bad2.p12 -passin pass: -info
MAC: sha256, Iteration 2048
MAC length: 32, salt length: 8
PKCS7 Encrypted data: Segmentation fault (core dumped)

[lejcik]

not sure how to fix this, I've added a check for NULL dereference and the error is then handled in PKCS12_unpack_p7encdata(), output then looks like:

user@user:~/openssl$ openssl pkcs12 -in test/recipes/80-test_pkcs12_data/bad2.p12 -passin pass: -info
MAC: sha256, Iteration 2048
MAC length: 32, salt length: 8
PKCS7 Encrypted data: <no data>
Error outputting keys and certificates
80CB79DB737F0000:error:11800065:PKCS12 routines:PKCS12_unpack_p7encdata:decode error:crypto/pkcs12/p12_add.c:163:

[InfoHunter]

Since PKCS12_unpack_p7encdata is going to be called anyway, how about call it first and then print the algorithm, like:

...
bags = PKCS12_unpack_p7encdata(p7, pass, passlen);
if (bags == NULL)
    goto err;
alg_print(p7->d.encrypted->enc_data->algorithm);
....

[lejcik]

fixed as suggested, now the output looks like:

user@user:~/openssl$ openssl pkcs12 -in test/recipes/80-test_pkcs12_data/bad2.p12 -passin pass: -info
MAC: sha256, Iteration 2048
MAC length: 32, salt length: 8
Error outputting keys and certificates
800B080A977F0000:error:11800065:PKCS12 routines:PKCS12_unpack_p7encdata:decode error:crypto/pkcs12/p12_add.c:163:

does this look better now?

[bernd-edlinger]

Well, actually I'd agree with @t8m that the previous version was already okay.

[lejcik]

ok, reverted the commit to the previous version

[t8m]

I actually like the original patch more than the new one.

[bernd-edlinger]

Could you please add a test case?

[bernd-edlinger]

Could you please add a test case?

Ping...
Note: that is not difficult at all:

diff --git a/test/recipes/80-test_pkcs12.t b/test/recipes/80-test_pkcs12.t
index 817b0bf064..c524e57fef 100644
--- a/test/recipes/80-test_pkcs12.t
+++ b/test/recipes/80-test_pkcs12.t
@@ -54,7 +54,7 @@ if (eval { require Win32::API; 1; }) {
 }
 $ENV{OPENSSL_WIN32_UTF8}=1;
 
-plan tests => 28;
+plan tests => 29;
 
 # Test different PKCS#12 formats
 ok(run(test(["pkcs12_format_test"])), "test pkcs12 formats");
@@ -187,6 +187,10 @@ with({ exit_checker => sub { return shift == 1; } },
         ok(run(app(["openssl", "pkcs12", "-in", $bad2, "-password", "pass:"])),
            "test bad pkcs12 file 2");
 
+        ok(run(app(["openssl", "pkcs12", "-in", $bad2, "-password", "pass:",
+                    "-info"])),
+           "test bad pkcs12 file 2 (info)");
+
         ok(run(app(["openssl", "pkcs12", "-in", $bad3, "-password", "pass:"])),
            "test bad pkcs12 file 3");
      });

[lejcik]

@bernd-edlinger thanks for hint how to add a test case,

I added test cases for all 3 bad certificates, as I tested them on my ubuntu (OpenSSL 3.0.3 3), all the bad certificates caused openssl app crash, so rather let have these cases covered:

# openssl pkcs12 -in bad1.p12 -passin pass: -info
MAC: sha256, Iteration 2048
MAC length: 32, salt length: 8
Segmentation fault
#
# openssl pkcs12 -in bad2.p12 -passin pass: -info
MAC: sha256, Iteration 2048
MAC length: 32, salt length: 8
PKCS7 Encrypted data: Segmentation fault
#
# openssl pkcs12 -in bad3.p12 -passin pass: -info
MAC: sha256, Iteration 2048
MAC length: 32, salt length: 8
Segmentation fault

does the test look ok now?

[bernd-edlinger]

does the test look ok now?

Excellent. Thanks.

Two things would be still to do.

1. It would be good to remove the reverted commit
   completely.

2. More importantly the CLA-check is failing with the last commit,
   that is funny because the first two commits had a CLA on file.
   The difference is how the Author's e-mail address is spelled.

So could you please squash all the commits (git rebase -i HEAD^^^^)
remove the second and third commits from the list list
and change the last commit from pick to squash.
Then do a force-push (git push -f)

[lejcik]

ok, I've updated the git history, the commits are now squashed, is this what you wanted?

I never did such "black magic" with git commit history, so at least I've learned something new 🙂

[openssl-machine]

This pull request is ready to merge

[t8m]

@lejcik Could you please submit a CLA? https://www.openssl.org/policies/cla.html According to our CLA policy we cannot accept your contribution without that.

[lejcik]

ok, how to do that? is it enough to add CLA: trivial line into the commit description, and do push -f ?

I think this is a trivial change category...

[t8m]

Unfortunately this is not eligible for CLA: trivial.

May I ask you to fill in and sign a regular CLA document and e-mail it to us as described on the above linked CLA page?

[t8m]

Merged to all the active branches. Thank you for your contribution.