Add Version Check for CSR Verification #24677
Conversation
openssl-machine
added
the
hold: cla required
openssl-machine
removed
the
hold: cla required
|
@erbsland-dev would you please sign an icla as documented here so that we can review, and potentially accept this PR? I don't think we can circumvent the agreement, as it wouldn't be considered trivial. |
t8m
added
hold: cla required
crypto/x509/x_all.c
Outdated
| @@ -43,6 +43,11 @@ int X509_verify(X509 *a, EVP_PKEY *r) | |||
| int X509_REQ_verify_ex(X509_REQ *a, EVP_PKEY *r, OSSL_LIB_CTX *libctx, | |||
| const char *propq) | |||
| { | |||
| if (X509_REQ_get_version(a) != X509_REQ_VERSION_1) { | |||
| ERR_raise_data(ERR_LIB_X509, ERR_R_UNSUPPORTED, "version != 1"); | |||
There was a problem hiding this comment.
IMO ERR_raise(ERR_LIB_X509, X509_R_UNSUPPORTED_VERSION); would be more appropriate. Of course that requires running make update to add the new error reason.
There was a problem hiding this comment.
@t8m I fully agree. I avoided adding a new error reason to keep this pull request minimal and its impact easily observable.
However, since the library lacks version checks in most of the X509 code, I would recommend introducing this new error code along with stricter version checking for all X509 functions.
|
@nhorman I also wrote a test for this, but I found no guidelines if the test shall go along with the original PR, or be committed as independent PR. |
It should be included in this PR. |
openssl-machine
removed
the
hold: cla required
As suggested in PR openssl#24677, introduce a new error code `X509_R_UNSUPPORTED_VERSION` to report malformed X509 requests with a version other than 1. Changing the error reporting in `X509_REQ_verify_ex`, using the new error code.
|
@t8m Based on your recommendation, I added the new error code X509_R_UNSUPPORTED_VERSION after assessing its impact. The addition of this error code for X509 has minimal impact but significantly improves the readability of the returned error message. |
t8m
added
tests: present
Fixes openssl#5738: This change introduces a check for the version number of a CSR document before its signature is verified. If the version number is not 1 (encoded as zero), the verification function fails with an `X509_R_UNSUPPORTED_VERSION` error. To minimize impact, this check is only applied when verifying a certificate signing request using the `-verify` argument, resulting in a `X509_REQ_verify` call. This ensures that malformed certificate requests are rejected by a certification authority, enhancing security and preventing potential issues.
Tests openssl#5738: Introduce a new test to verify that a malformed X509 request with the version field set to version 6 fails either early when reading from data or later when `X509_REQ_verify` is called. Adding a new test recipe `60-test_x509_req.t`
|
@t8m I apologize for the oversights; I’m not accustomed to writing C code anymore. I have also squashed the commits into two: one for the changes and one for the tests. Additionally, I removed the “CLA: trivial” note and adjusted the commit messages accordingly. |
t8m
removed
the
approval: otc review pending
Update the `x509_req_test` to ensure ANSI compatibility. The integrated certificate string was too long, so the PEM certificate has been moved to `certs/x509-req-detect-invalid-version.pem`. The test have been updated to load this certificate from the file on disk.
openssl-machine
removed
the
hold: cla required
|
@nhorman please rereview |
|
@t8m @nhorman If there are any smaller fixes or enhancements I can help with, please let me know. Since I don’t have a comprehensive overview of the project, it’s difficult for me to identify the most important issues. However, as a lifelong user of the library, I’m eager to contribute. If you can point me to specific tasks that need implementation, I’d be happy to work on them. |
|
Review holds, approved |
|
@erbsland-dev thank you for the offer. To keep you informed, typically we mark issues that we feel should be addressed, but are not within our capacity to handle, with the "help wanted" tag. You are more than welcome to scan that list at your discretion and create PR's for them at will. If you like, leave a comment and I'll happily assign them to you. Further, we have a "good first issue" tag that further refines out issues that have been in the past considered to be a good starting point for people just getting involved, if you're so inclined. A word of warning, as you have likely noticed, I've been cleaning up our issue list lately, and the help wanted issues are not well groomed yet, so its a bit of a "wild west" situtation, and you may find many issues that are old, and not clearly defined. I apologize for that, but do you're best, cleaning that up is on my list :) |
t8m
added
approval: done
openssl-machine
added
approval: ready to merge
|
This pull request is ready to merge |
|
merged to master, thank you for your contribution! |
Fixes #5738: This change introduces a check for the version number of a CSR document before its signature is verified. If the version number is not 1 (encoded as zero), the verification function fails with an `X509_R_UNSUPPORTED_VERSION` error. To minimize impact, this check is only applied when verifying a certificate signing request using the `-verify` argument, resulting in a `X509_REQ_verify` call. This ensures that malformed certificate requests are rejected by a certification authority, enhancing security and preventing potential issues. Reviewed-by: Neil Horman <nhorman@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #24677)
Tests #5738: Introduce a new test to verify that a malformed X509 request with the version field set to version 6 fails either early when reading from data or later when `X509_REQ_verify` is called. Adding a new test recipe `60-test_x509_req.t` Reviewed-by: Neil Horman <nhorman@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #24677)
Update the `x509_req_test` to ensure ANSI compatibility. The integrated certificate string was too long, so the PEM certificate has been moved to `certs/x509-req-detect-invalid-version.pem`. The test have been updated to load this certificate from the file on disk. Reviewed-by: Neil Horman <nhorman@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #24677)
Fixes #5738: This change introduces a check for the version number of a CSR document before its signature is verified. If the version number is not 1 (encoded as zero), the verification function fails with an
X509_R_UNSUPPORTED_VERSIONerror.To minimise impact, this check is only applied when verifying a certificate signing request using the
-verifyargument limiting it to theX509_REQ_verify_exandX509_REQ_verifyfunctions. This ensures that malformed certificate requests are rejected by a certification authority, enhancing security and preventing potential issues.