Commit graph

21 commits

Author SHA1 Message Date
Gilles Peskine
4f01121f6e Fix memory leak on error in pkcs7_get_signers_info_set
mbedtls_x509_name allocates memory, which must be freed if there is a
subsequent error.

Credit to OSS-Fuzz (https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=53811).

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
2022-11-27 22:02:10 +01:00
Gilles Peskine
e7f8c616d0 Fix dangling freed pointer in pkcs7_free_signer_info
This may have been a use-after-free, but I haven't worked out whether it was
a problem or not. Even if it turns out to have been ok, keeping invalid
pointers around is fragile.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
2022-11-27 21:55:29 +01:00
Gilles Peskine
47a732635b Simplify control flow in PKCS7 functions
Remove useless goto in several functions.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
2022-11-27 21:55:29 +01:00
Gilles Peskine
290f01b3f5 Fix dangling freed pointer on error in pkcs7_get_signers_info_set
This fixes a use-after-free in PKCS#7 parsing when the signer data is
malformed.

Credit to OSS-Fuzz (https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=53798).

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
2022-11-27 21:55:29 +01:00
Nick Child
3951a4f3ad pkcs7: Use better error codes
Remove an unnecessary debug print (whoops).
Use new error code for when the x509 is expired.
When there are no signers return invalid certificate.

Signed-off-by: Nick Child <nick.child@ibm.com>

Co-authored-by: Dave Rodgman <dave.rodgman@arm.com>
Signed-off-by: Nick Child <nick.child@ibm.com>
2022-10-31 09:38:42 -05:00
Nick Child
5f39767495 pkcs7: Fix imports
Respond to feedback about duplicate imports[1] and new import style [2].

[1] https://github.com/Mbed-TLS/mbedtls/pull/3431#discussion_r991355485
[2] https://github.com/Mbed-TLS/mbedtls/pull/3431#pullrequestreview-1138745361
Signed-off-by: Nick Child <nick.child@ibm.com>
2022-10-28 12:38:41 -05:00
Nick Child
bb82ab764f pkcs7: Respond to feeback on parsing logic
After recieving review on the pkcs7 parsing functions, attempt
to use better API's, increase consisitency and use better
documentation. The changes are in response to the following
comments:
 - use mbedtls_x509_crt_parse_der instead of mbedtls_x509_crt_parse [1]
 - make lack of support for authenticatedAttributes more clear [2]
 - increment pointer in pkcs7_get_content_info_type rather than after [3]
 - rename `start` to `p` for consistency in mbedtls_pkcs7_parse_der [4]

[1] https://github.com/Mbed-TLS/mbedtls/pull/3431#discussion_r992509630
[2] https://github.com/Mbed-TLS/mbedtls/pull/3431#discussion_r992562450
[3] https://github.com/Mbed-TLS/mbedtls/pull/3431#discussion_r992741877
[4] https://github.com/Mbed-TLS/mbedtls/pull/3431#discussion_r992754103

Signed-off-by: Nick Child <nick.child@ibm.com>
2022-10-28 12:28:54 -05:00
Nick Child
73621ef0f0 pkcs7: Improve verify logic and rebuild test data
Various responses to feedback regarding the
pkcs7_verify_signed_data/hash functions. Mainly, merge these two
functions into one to reduce redudant logic [1]. As a result, an
identified bug about skipping over a signer is patched [2].

Additionally, add a conditional in the verify logic that checks if
the given x509 validity period is expired [3]. During testing of this
conditional, it turned out that all of the testing data was expired.
So, rebuild all of the pkcs7 testing data to refresh timestamps.

[1] https://github.com/Mbed-TLS/mbedtls/pull/3431#discussion_r999652525
[2] https://github.com/Mbed-TLS/mbedtls/pull/3431#discussion_r997090215
[3] https://github.com/Mbed-TLS/mbedtls/pull/3431#discussion_r967238206
Signed-off-by: Nick Child <nick.child@ibm.com>
2022-10-28 11:24:25 -05:00
Nick Child
5f9456f3e3 pkcs7: Fix trailing whitespace
Signed-off-by: Nick Child <nick.child@ibm.com>
2022-09-26 09:18:12 -05:00
Nick Child
8ce1b1afc8 pkcs7: Correct various syntatical mistakes
Resond to feedback from the following comments:
 - use correct spacing [1-7]
 - remove unnecessary parenthesis [8]
 - fixup comments [9-11]
 - remove unnecessary init work [12]
 - use var instead of type for sizeof [13]
[1] https://github.com/Mbed-TLS/mbedtls/pull/3431#discussion_r953655691
[2] https://github.com/Mbed-TLS/mbedtls/pull/3431#discussion_r953661514
[3] https://github.com/Mbed-TLS/mbedtls/pull/3431#discussion_r953689929
[4] https://github.com/Mbed-TLS/mbedtls/pull/3431#discussion_r953696384
[5] https://github.com/Mbed-TLS/mbedtls/pull/3431#discussion_r953697558
[6] https://github.com/Mbed-TLS/mbedtls/pull/3431#discussion_r953697793
[7] https://github.com/Mbed-TLS/mbedtls/pull/3431#discussion_r953697951
[8] https://github.com/Mbed-TLS/mbedtls/pull/3431#discussion_r953699102
[9] https://github.com/Mbed-TLS/mbedtls/pull/3431#discussion_r971223775
[10] https://github.com/Mbed-TLS/mbedtls/pull/3431#discussion_r967133905
[11] https://github.com/Mbed-TLS/mbedtls/pull/3431#discussion_r967135932
[12] https://github.com/Mbed-TLS/mbedtls/pull/3431#discussion_r967151430
[13] https://github.com/Mbed-TLS/mbedtls/pull/3431#discussion_r967154159
Signed-off-by: Nick Child <nick.child@ibm.com>
2022-09-14 15:13:52 -05:00
Nick Child
34d5e931cf pkcs7: Use better return code for unimplemented specifications
In response to feedback [1] [2], use MBEDTLS_ERR_PKCS7_FEATURE_UNAVAILABLE
instead of MBEDTLS_ERR_PKCS7_INVALID_FORMAT for errors due to the
pkcs7 implemntation being incomplete.

[1] https://github.com/Mbed-TLS/mbedtls/pull/3431#discussion_r953649079
[2] https://github.com/Mbed-TLS/mbedtls/pull/3431#discussion_r953658276

Signed-off-by: Nick Child <nick.child@ibm.com>
2022-09-14 14:44:03 -05:00
Nick Child
7089ce8381 pkcs7: Handle md errors in multisigner pkcs7 verification
In resonse to feedback [1], if `mbedtls_md_info_from_type` were to
fail then skip the signer and try the next one.

Additionally, use a for loop instead of a while loop when iterating
over signers because it simplifies the use of `continue`.

[1] https://github.com/Mbed-TLS/mbedtls/pull/3431#discussion_r967198650
Signed-off-by: Nick Child <nick.child@ibm.com>
2022-09-14 14:18:00 -05:00
Nick Child
9f4fb3e63f pkcs7: Unite function return style
In response to feedback[1], standardize return variable
management across all pkcs7 functions.

Additionally, when adding return codes from two error values,
use `MBEDTLS_ERROR_ADD` as recommended [2].

[1] https://github.com/Mbed-TLS/mbedtls/pull/3431#discussion_r953634781
[2] https://github.com/Mbed-TLS/mbedtls/pull/3431#discussion_r953635128

Signed-off-by: Nick Child <nick.child@ibm.com>
2022-09-12 16:32:36 -05:00
Nick Child
62b2d7e7d4 pkcs7: Support verification of hash with multiple signers
Make `mbedtls_pkcs7_signed_hash_verify` loop over all signatures in the
PKCS7 structure and return success if any of them verify successfully.

Signed-off-by: Nick Child <nick.child@ibm.com>
2022-09-01 19:45:41 -05:00
Daniel Axtens
3538479faa pkcs7: support multiple signers
Rather than only parsing/verifying one SignerInfo in the SignerInfos
field of the PKCS7 stucture, allow the ability to parse and verify more
than one signature. Verification will return success if any of the signatures
produce a match.

Signed-off-by: Daniel Axtens <dja@axtens.net>
Signed-off-by: Nick Child <nick.child@ibm.com>
2022-09-01 19:45:41 -05:00
Nick Child
5d881c36ea pkcs7: Change copyright
Signed-off-by: Nick Child <nick.child@ibm.com>
2022-09-01 19:45:41 -05:00
Nick Child
6427b34dec pkcs7.c: Use pkcs7_get_version for signerInfo
The function pkcs7_get_version can be used again
when parsing the version of the signerInfo. Both
require that the version be equal to 1. The
pkcs7_get_version function will return error
if the found value is not the expected version
as opposed to mbedtls_asn1_get_int which does not.

Signed-off-by: Nick Child <nick.child@ibm.com>
2022-09-01 19:45:41 -05:00
Nick Child
6671841d91 pkcs7.c: Do not ignore return value of mbedlts_md
CI was failing due to the return value of mbedtls_md being ignored.
If this function does fail, return early and propogate the md error.

Signed-off-by: Nick Child <nick.child@ibm.com>
2022-09-01 19:45:41 -05:00
Nick Child
c448c94fe3 pkcs7: pkcs7_get_content_info_type should reset *p on error
The function `pkcs7_asn1_get_tag` should return an update pointer only
on success. Currently, the pointer is being updated on a failure case.
This commit resets *p to start if the first call to
mbedtls_asn1_get_tag fails.

Signed-off-by: Daniel Axtens <dja@axtens.net>
Signed-off-by: Nick Child <nick.child@ibm.com>
2022-09-01 19:45:41 -05:00
Nayna Jain
673a226698 pkcs7: add support for signed data
OpenSSL provides APIs to generate only the signted data
format PKCS7 i.e. without content type OID. This patch
adds support to parse the data correctly even if formatted
only as signed data

Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
2022-09-01 19:45:41 -05:00
Nayna Jain
c9deb184b0 mbedtls: add support for pkcs7
PKCS7 signing format is used by OpenPOWER Key Management, which is
using mbedtls as its crypto library.

This patch adds the limited support of pkcs7 parser and verification
to the mbedtls. The limitations are:

* Only signed data is supported.
* CRLs are not currently handled.
* Single signer is supported.

Signed-off-by: Daniel Axtens <dja@axtens.net>
Signed-off-by: Eric Richter <erichte@linux.ibm.com>
Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
2022-09-01 19:45:33 -05:00