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.

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

Fix for setting wrong version in CSRs. #8136

Merged
merged 4 commits into from
Nov 15, 2024
Merged

Conversation

anhu
Copy link
Member

@anhu anhu commented Oct 31, 2024

Fixes ZD 18873

Testing

$ cd wolfssl
$ ./configure --enable-wolfclu
$ make all check
$ sudo make install
$cd ../wolfCLU
$ ./configure (all tried with CFLAG=-DNO_WOLFSSL_REQ_PRINT) 
$ make all
$ ./wolfssl -genkey rsa -size 2048 -out temp.key -outform pem  -output KEYPAIR
$ ./wolfssl req -new -days 1095 -config csr.conf -key temp.key.priv -out service.csr  -outform PEM -sha256
$ openssl req -in service.csr -text
        Version: 1 (0x0)
$ ./wolfssl req -in service.csr  -noout -text
        Version: 1 (0x0)

@anhu anhu assigned wolfSSL-Bot and unassigned anhu Nov 6, 2024
Copy link
Contributor

@dgarske dgarske left a comment

Choose a reason for hiding this comment

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

Looks good otherwise

src/x509.c Outdated
@@ -14840,6 +14842,23 @@ void wolfSSL_X509_REQ_free(WOLFSSL_X509* req)
wolfSSL_X509_free(req);
}

int wolfSSL_X509_REQ_set_version(WOLFSSL_X509 *x, long version) {
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: Functions use brace on new line.

src/x509.c Outdated
long wolfSSL_X509_REQ_get_version(const WOLFSSL_X509 *req) {
WOLFSSL_ENTER("wolfSSL_X509_REQ_get_version");
if (req == NULL) {
return WOLFSSL_FAILURE;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would prefer return return 0; /* invalid arg */ here instead of WOLFSSL_FAILURE = 0.

@@ -14840,6 +14842,25 @@ void wolfSSL_X509_REQ_free(WOLFSSL_X509* req)
wolfSSL_X509_free(req);
}

int wolfSSL_X509_REQ_set_version(WOLFSSL_X509 *x, long version)
Copy link
Contributor

Choose a reason for hiding this comment

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

See wolfSSL_X509_set_version -> x509->version = (int) v + 1;

/* print version of cert. Note that we increment by 1 because for REQs,
* the value stored in x509->version is the actual value of the field; not
* the version. */
if (X509PrintVersion(bio, (int)wolfSSL_X509_REQ_get_version(x509) + 1, 8)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really the right way/place to adjust the version? The wolfSSL_X509_get_version and wolfSSL_X509_set_version do it differently.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think the reason this is correct is because this is specific for REQs; not certificates.

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn’t the +1 be inside the get function? Or is this the same way openssl does it?

Copy link
Member Author

Choose a reason for hiding this comment

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

If the +1 was in the wolfSSL_X509_REQ_get_version() then it would return the wrong value. This behaviour matches OpenSSL.

@bandi13
Copy link
Contributor

bandi13 commented Nov 12, 2024

retest this please

@anhu anhu requested a review from dgarske November 12, 2024 22:55
@dgarske dgarske removed their request for review November 13, 2024 17:57
@dgarske dgarske merged commit 21bfcaf into wolfSSL:master Nov 15, 2024
143 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants