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

Fixed BearSSL bug: error on CURLOPT_SSL_VERIFYPEER 0 and expired cert #8475

Closed
wants to merge 1 commit into from

Conversation

jan2000
Copy link
Contributor

@jan2000 jan2000 commented Feb 19, 2022

Separated from: #8106

The x509_minimal engine of BearSSL is used to parse the certificate chain. When verifypeer is set to 0 the result of the end_chain method (xm_end_chain) is overridden to return BR_ERR_OK so that the chain is deemed valid even if it is not. The engine is also used to extract the public key from the cert. However, because the x509_minimal engine stops parsing if any validity check fails the parsing can stop before the public key is extracted. Checking the BearSSL code in x509_minimal.t0:decode-certificate we can see that this can happen for example for ERR_X509_EXPIRED. This will mean that x509_get_pkey will return no key if the cert is expired and verifypeer is set to 0, what will result in bearssl_run_until returning CURLE_SSL_CONNECT_ERROR.

This is fixed by using the x509_decode engine instead of the x509_minimal engine to parse and extract the public key from the first cert of the chain.

/* ignore any X.509 errors */
err = BR_ERR_OK;
if(!x509->verifypeer) {
return br_x509_decoder_last_error(&x509->decoder);
Copy link
Member

@jay jay Feb 19, 2022

Choose a reason for hiding this comment

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

Why did you change this to return the error in the decoder when !verifypeer?

Copy link
Contributor

@michaelforney michaelforney Feb 20, 2022

Choose a reason for hiding this comment

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

BearSSL has two APIs that deal with X.509 certificates. One is br_x509_minimal, which decodes a chain of certificates, verifying that they are valid, each one certifies the preceding one, and that the root certificate is in the trust anchor list. At the end of the chain, if there is no other error, it returns 0 if trusted, and BR_ERR_X509_NOT_TRUSTED if not. In either case, you can access the public key of the end-entity using the get_pkey function.

However, if there was some other error (for example, a DN mismatch, where a certificate doesn't certify the preceding one, or if one of the certificates is expired), then br_x509_minimal just stops processing the chain, and returns an error from end_chain. In this case, get_pkey will return NULL, even if it has decoded a key.

So, if we wish to ignore errors with expired or out-of-order certificates, then br_x509_minimal is not useful. Instead, we can use the second API, br_x509_decoder, which decodes a single certificate. This commit switches the curl x509 engine to using br_x509_decoder if !verifypeer, and br_x509_minimal otherwise.

The overall effect of this change is the following:
If verifypeer is false, curl will now only consider the first certificate in the chain sent by the server. If it can decode the first certificate and extract a key (regardless of the contents of subsequent certificates), then it accepts the chain. Previously, curl would accept the chain only if it was completely valid apart from none of the certificates being trusted.

Copy link
Member

@jay jay Feb 20, 2022

Choose a reason for hiding this comment

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

Ok thanks for the thorough explanation

@jay jay added the TLS label Feb 19, 2022
@jay
Copy link
Member

@jay jay commented Feb 20, 2022

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants