Commit graph

40 commits

Author SHA1 Message Date
Dave Rodgman
a1b2bfff46 Add clarifying comments
Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>
2023-02-20 14:45:09 +00:00
Dave Rodgman
fc64352253 Adjust position of empty line
Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>
2023-02-16 16:23:09 +00:00
Dave Rodgman
f691268ee9 Add missing initialisers
Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>
2023-02-10 12:56:10 +00:00
Demi Marie Obenour
35598adb78 pkcs7: Check that hash algs are in digestAlgorithms
Since only a single hash algorithm is currenlty supported, this avoids
having to perform hashing more than once.

Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
2023-02-10 12:56:10 +00:00
Demi Marie Obenour
6cfc469296 pkcs7: reject signatures with internal data
A CMS signature can have internal data, but mbedTLS does not support
verifying such signatures.  Reject them during parsing.

Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>
2023-02-10 12:56:10 +00:00
Demi Marie Obenour
e373a254c4 pkcs7: do not store content type OIDs
They will always be constant.

Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
2023-02-10 12:56:10 +00:00
Demi Marie Obenour
55d9df25ef Simple cleanup
No change in behavior.

Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
2023-02-10 12:56:10 +00:00
Demi Marie Obenour
4ec8355795 Check for junk after SignedData
There must not be any.

Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
2023-02-10 12:56:10 +00:00
Demi Marie Obenour
aaf3c0028d pkcs7: do not store content type OID
Since only one content type (signed data) is supported, storing the
content type just wastes memory.

Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
2023-02-10 12:56:10 +00:00
Demi Marie Obenour
512818b1d2 pkcs7: check that content lengths fill whole buffer
Otherwise invalid data could be accepted.

Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>
2023-02-10 12:56:10 +00:00
Dave Rodgman
78c6f40736
Fix code-style
Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>
2023-02-09 09:21:14 +00:00
Nick Child
14f255f332 pkcs7: Remove unnecessary dependencies
stdio, stdlib and string header files are not
used. Remove them.

Signed-off-by: Nick Child <nick.child@ibm.com>
2023-02-08 15:38:48 +00:00
Nick Child
3dafc6c3b3 pkcs7: Drop support for signature in contentInfo of signed data
The contentInfo field of PKCS7 Signed Data structures can
optionally contain the content of the signature. Per RFC 2315
it can also contain any of the PKCS7 data types. Add test and
comments making it clear that the current implementation
only supports the DATA content type and the data must be empty.

Return codes should be clear whether content was invalid or
unsupported.
Identification and fix provided by:
 - Demi Marie Obenour <demiobenour@gmail.com>
 - Dave Rodgman <dave.rodgman@arm.com>

Signed-off-by: Nick Child <nick.child@ibm.com>
2023-02-07 20:04:52 +00:00
Nick Child
282d50493a pkcs7: Remove duplicate oid condition
MBEDTLS_OID_PKCS7_ENCRYPTED_DATA was listed twice in
the oid conditional. Remove one of them.

Signed-off-by: Nick Child <nick.child@ibm.com>
2023-02-01 18:32:55 +00:00
Nick Child
3bd17f2f58 pkcs7: Use end_issuer_and_sn where appropriate
There were some areas where `end_signer` were being
used when it makes more sense to use `end_issuer_and_sn`,
as pointed out by demiobenour@gmail.com.

Signed-off-by: Nick Child <nick.child@ibm.com>
2023-01-31 20:42:26 +00:00
Nick Child
ec81709516 pkcs7: Ensure all data in asn1 structure is accounted for
Several PKCS7 invalid ASN1 Tests were failing due to extra
data bytes or incorrect content lengths going unnoticed. Make
the parser aware of possible malformed ASN1 data.

Signed-off-by: Nick Child <nick.child@ibm.com>
2023-01-30 16:44:58 +00:00
Gilles Peskine
449bd8303e Switch to the new code style
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
2023-01-11 14:50:10 +01:00
Bence Szépkúti
f7641544ea Correct the fix for the PKCS 7 memory leak
This corrects an issue in the origina fix in
4f01121f6e.

Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
2022-12-12 21:59:03 +01:00
Tom Cosgrove
1797b05602 Fix typos prior to release
Signed-off-by: Tom Cosgrove <tom.cosgrove@arm.com>
2022-12-04 17:19:59 +00:00
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