Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add NULL check before accessing PKCS7 encrypted algorithm #23632

Closed
wants to merge 1 commit into from
Closed

Add NULL check before accessing PKCS7 encrypted algorithm #23632

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion apps/pkcs12.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -901,7 +901,11 @@ int dump_certs_keys_p12(BIO *out, const PKCS12 *p12, const char *pass,
} else if (bagnid == NID_pkcs7_encrypted) {
if (options & INFO) {
BIO_printf(bio_err, "PKCS7 Encrypted data: ");
alg_print(p7->d.encrypted->enc_data->algorithm);
if (p7->d.encrypted == NULL) {
BIO_printf(bio_err, "<no data>\n");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, reverted the commit to the previous version

} else {
alg_print(p7->d.encrypted->enc_data->algorithm);
}
}
bags = PKCS12_unpack_p7encdata(p7, pass, passlen);
} else {
Expand Down
14 changes: 13 additions & 1 deletion test/recipes/80-test_pkcs12.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ if (eval { require Win32::API; 1; }) {
}
$ENV{OPENSSL_WIN32_UTF8}=1;

plan tests => 28;
plan tests => 31;

# Test different PKCS#12 formats
ok(run(test(["pkcs12_format_test"])), "test pkcs12 formats");
Expand Down Expand Up @@ -184,11 +184,23 @@ with({ exit_checker => sub { return shift == 1; } },
"-nomacver"])),
"test bad pkcs12 file 1 (nomacver)");

ok(run(app(["openssl", "pkcs12", "-in", $bad1, "-password", "pass:",
"-info"])),
"test bad pkcs12 file 1 (info)");

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");

ok(run(app(["openssl", "pkcs12", "-in", $bad3, "-password", "pass:",
"-info"])),
"test bad pkcs12 file 3 (info)");
});

# Test with Oracle Trusted Key Usage specified in openssl.cnf
Expand Down