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

Reject certificates with explicit ec parameters in strict mode [1.1.1] #12909

Closed
wants to merge 3 commits into from

Conversation

@t8m
Copy link
Member

@t8m t8m commented Sep 18, 2020

This is backport of #12683 to 1.1.1 branch. The difference here is that we enable this check only with strict checking.

Checklist
  • documentation is added or updated
  • tests are added or updated
t8m added 2 commits Aug 21, 2020
The function returns 1 when the encoding of a decoded EC key used
explicit encoding of the curve parameters.
The check is applied only with X509_V_FLAG_X509_STRICT.

Fixes #12139
@t8m t8m requested review from romen and DDvO Sep 18, 2020
@romen
romen approved these changes Sep 18, 2020
Copy link
Member

@romen romen left a comment

Partial approval:

  • conditioned to CI passing
  • needs confirmation by someone else to say if we need a more specific opt-in mechanism or if it is fine to make the X509_V_FLAG_X509_STRICT flag stricter
crypto/x509/x509_vfy.c Show resolved Hide resolved
@DDvO
DDvO approved these changes Sep 18, 2020
Copy link
Contributor

@DDvO DDvO left a comment

LGTM

test/certs/setup.sh Outdated Show resolved Hide resolved
@DDvO
DDvO approved these changes Sep 18, 2020
Copy link
Contributor

@DDvO DDvO left a comment

Still LGTM

@openssl-machine
Copy link

@openssl-machine openssl-machine commented Sep 19, 2020

This pull request is ready to merge

@DDvO
Copy link
Contributor

@DDvO DDvO commented Sep 21, 2020

@t8m, looks like you can merge this.

openssl-machine pushed a commit that referenced this pull request Sep 21, 2020
The function returns 1 when the encoding of a decoded EC key used
explicit encoding of the curve parameters.

Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
Reviewed-by: Nicola Tuveri <nic.tuv@gmail.com>
(Merged from #12909)
openssl-machine pushed a commit that referenced this pull request Sep 21, 2020
The check is applied only with X509_V_FLAG_X509_STRICT.

Fixes #12139

Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
Reviewed-by: Nicola Tuveri <nic.tuv@gmail.com>
(Merged from #12909)
@t8m
Copy link
Member Author

@t8m t8m commented Sep 21, 2020

Merged to 1.1.1 branch. Thank you for the reviews.

@@ -525,6 +526,14 @@ static int check_chain_extensions(X509_STORE_CTX *ctx)
ret = 1;
break;
}
if ((ctx->param->flags & X509_V_FLAG_X509_STRICT) && num > 1) {
/* Check for presence of explicit elliptic curve parameters */
ret = check_curve(x);

This comment has been minimized.

@kaduk

kaduk Mar 18, 2021
Contributor

This assignment seems highly problematic, in that it allows an existing ret value of zero (e.g., corresponding to X509_V_ERR_INVALID_CA from the previous stanza) to be overwritten with a value of one (success), thus masking the "invalid CA" error.

In master we have avoided this problem by introducing CB_FAIL_IF() (originally CHECK_CB()) in #13312, that invokes the callback for each potential error and returns immediately, so in the aforementioned case of an invalid CA we don't even get this far.

AFAIK, it is not appropriate to backport #13312 to 1.1.1, so we should probably just skip this check entirely if ret is already zero. I will make a PR.

This comment has been minimized.

@t8m

t8m Mar 18, 2021
Author Member

I think this is even CVE worthy.

This comment has been minimized.

@DDvO

DDvO Mar 22, 2021
Contributor

@kaduk, very good that you noticed this!
The problem was not immediate to see when reviewing the insertion of

ret = check_curve(x);

because the criticality of masking any ret == 0 is due to the wider context of this change.

I suggest fixing this simply by replacing

        if ((ctx->param->flags & X509_V_FLAG_X509_STRICT) && num > 1) {

by

        if (err == 0 && num > 1
            && (ctx->param->flags & X509_V_FLAG_X509_STRICT) != 0) {

This comment has been minimized.

@DDvO

DDvO Mar 25, 2021
Contributor

For conspiracy theorists & practitioners:
the bug was committed on 2020-09-11 09:09:29 😉

This comment has been minimized.

@t8m

t8m Mar 25, 2021
Author Member

I certainly had to be under some alien mind control when committing that buggy change. 🤣

This comment has been minimized.

@DDvO

DDvO Mar 25, 2021
Contributor

Ah, now I see that the bug fix added a regression test elsewhere, in verify_extra_test.c,
which does catch the issue.

Yet still wondering why
openssl verify -x509_strict -trusted test/certs/root-cert.pem -untrusted test/certs/ca-nonca.pem test/certs/ee-cert.pem
does not catch it - there must be a further hidden condition.

This comment has been minimized.

@DDvO

DDvO Mar 25, 2021
Contributor

Meanwhile found why this is so: although

ret = 0

had been masked, the subsequent

ctx->error = X509_V_ERR_INVALID_CA;

was/is not affected, and apps/verify.c not only checks the return code but also
X509_STORE_CTX_get_error(csc), which still indicates the error!
Consequently, the verity app still says

error test/certs/ee-cert.pem: verification failed

and returns 2 (a shell error code).

In other words, only library uses of X509_verify_cert()
that rely on the returned (zero vs. nonzero) code alone were affected.

Given that X509_verify_cert() already contains at its end:

    /*
     * Safety-net.  If we are returning an error, we must also set ctx->error,
     * so that the chain is not considered verified should the error be ignored
     * (e.g. TLS with SSL_VERIFY_NONE).
     */
    if (ret <= 0 && ctx->error == X509_V_OK)
        ctx->error = X509_V_ERR_UNSPECIFIED;
    return ret;

this brings up a idea to make the safety net more tight:
add

    if (ret > 0 && ctx->error != X509_V_OK)
        ret = 0;

This comment has been minimized.

@DDvO

DDvO Mar 25, 2021
Contributor

Adding this check, it turns out that in current master 4 test cases start failing,
and for 1.1.1 it's even 51 cases!
This seems to suggest that there are several more such situations than just the one bug discussed and fixed here.
Which is even more worrying.

This comment has been minimized.

@DDvO

DDvO Mar 25, 2021
Contributor

This stuff is hard to debug because the situations occur only in SSL-related tests,
which are very involved (if not to say: incomprehensible),
and because the error codes are in part misleading.

In particular, internal_verify() can throw X509_V_ERR_UNABLE_TO_VERIFY_LEAF_SIGNATURE - guess what this means?
It means X509_V_FLAG_PARTIAL_CHAIN is not set, there is only one cert in the chain, and this cert is not self-issued :-(
statem_lib.c translates this to SSL_AD_UNKNOWN_CA, which increases confusion.

This comment has been minimized.

@DDvO

DDvO Mar 25, 2021
Contributor

It turns out that most of these cases are harmless and simply due to the fact that verify_cb_cert() sets ctx->error but in case the error is waived by ctx->verify_cb() then the original state of ctx->error is not restored.

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

Successfully merging this pull request may close these issues.

None yet

7 participants