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 various issues in x509_vfy.c and v3_purp.c related to #1418 and the respective documentation #10587

Closed
wants to merge 9 commits into from

Conversation

DDvO
Copy link
Contributor

@DDvO DDvO commented Dec 7, 2019

Fixes #1418. See discussion there.

  • documentation is added or updated
  • tests are added or updated

@DDvO DDvO force-pushed the fix_x509_vfy_issue_1418 branch from ba0d852 to 34f5f35 Compare Dec 8, 2019
@DDvO DDvO changed the title fix confusions and obscure bugs in x509_vfy.c and v3_purp.c WIP: fix confusions and obscure bugs in x509_vfy.c and v3_purp.c Dec 8, 2019
crypto/x509/v3_purp.c Outdated Show resolved Hide resolved
@bernd-edlinger
Copy link
Member

bernd-edlinger commented Dec 8, 2019

The similarity to #7943 is remarkable 😃

@DDvO DDvO force-pushed the fix_x509_vfy_issue_1418 branch from 63988a6 to e41dd9e Compare Dec 8, 2019
@DDvO
Copy link
Contributor Author

DDvO commented Dec 8, 2019

The similarity to #7943 is remarkable smiley

@bernd-edlinger, indeed the improvement to x509v3_cache_extensions() is essentially the same, except that your code is slightly nicer, so I've adopted it.
If I had been aware of that PR or if it had been finished and merged meanwhile I would not have had to re-discover that fix. On the bright side, you can take my PR as an approval of that portion of your PR ;-)

BTW, I'm not convinced of the other half of PR #7943 (the change you proposed on check_ca()), to which I've commented there.

@DDvO DDvO changed the title WIP: fix confusions and obscure bugs in x509_vfy.c and v3_purp.c fix confusions and obscure bugs in x509_vfy.c and v3_purp.c Dec 8, 2019
@DDvO
Copy link
Contributor Author

DDvO commented Dec 8, 2019

This PR should obsolete #7918.

@mattcaswell
Copy link
Member

mattcaswell commented Dec 9, 2019

@vdukhovni

@t8m
Copy link
Member

t8m commented Dec 9, 2019

Would you please amend the commit message so it is more descriptive? Also it would make sense to squash at this point and/or split the patches if there are multiple unrelated issues fixed.
I suppose we would want a cherry-pick to 1.1.1 branch as well.

@t8m t8m added branch: 1.1.1 branch: master approval: review pending labels Dec 9, 2019
@DDvO
Copy link
Contributor Author

DDvO commented Dec 10, 2019

@t8m, yep, I'll do a cleanup pass, squashing commits and putting more text in the commit(s) that remain after (at least partly) squashing. Hope you saw that there are already descriptions at issue discussion level, in particular #1418 (comment).

I'll also try to separate concerns, which I fear is only partially possible, so likely the new commits each representing a specific (sub-)issue will need to be hierarchical, i.e., not split-able into independent PRs.

Cherry-picking to 1.1.1 l presume should be no problem, but likely best done after this PR has been reviewed and is stable/approved.

BTW, @bernd-edlinger, your change request is already fulfilled.

crypto/x509/v3_purp.c Show resolved Hide resolved
crypto/x509/v3_purp.c Outdated Show resolved Hide resolved
crypto/x509/x509_vfy.c Outdated Show resolved Hide resolved
crypto/x509/x509_vfy.c Show resolved Hide resolved
crypto/x509/x509_vfy.c Outdated Show resolved Hide resolved
crypto/x509/x509_vfy.c Outdated Show resolved Hide resolved
doc/man3/X509_STORE_CTX_get_error.pod Outdated Show resolved Hide resolved
doc/man3/X509_STORE_CTX_get_error.pod Outdated Show resolved Hide resolved
doc/man3/X509_VERIFY_PARAM_set_flags.pod Outdated Show resolved Hide resolved
@kroeckx
Copy link
Member

kroeckx commented Dec 12, 2019

Maybe it's useful to have some self-issued certs that are not self-signed? And one that has a keyUsage without the keyCertSig. And maybe some other variants.

@kroeckx
Copy link
Member

kroeckx commented Dec 12, 2019

I don't understand the logic behind -partial_chain. If a certificate is trusted, why does it matter that it's self-signed or not? But I guess that's not something this PR should change.

@vdukhovni
Copy link

vdukhovni commented Dec 12, 2019

Maybe it's useful to have some self-issued certs that are not self-signed? And one that has a keyUsage without the keyCertSig. And maybe some other variants.

Only with partial_chain. Otherwise, self-issued certs are not trust-anchors. Trust-anchors need to be self-signed.

@vdukhovni
Copy link

vdukhovni commented Dec 12, 2019

I don't understand the logic behind -partial_chain. If a certificate is trusted, why does it matter that it's self-signed or not? But I guess that's not something this PR should change.

The logic allows intermediate certificates to be added to the trust store, without them being trusted, they just represent better versions of the issuer for chain construction.

Windows has an "intermediate certificate store" for this, but OpenSSL does not. We could in principle require all trust-anchors to be stored as "TRUSTED CERTIFICATE" objects with suitable auxiliary trust EKUs, but that's hard to do without major compatibility breakage.

@vdukhovni
Copy link

vdukhovni commented Dec 12, 2019

Maybe it's useful to have some self-issued certs that are not self-signed? And one that has a keyUsage without the keyCertSig. And maybe some other variants.

CAs should generally have keyCertSig, whether they're self-issued, cross (intermediate) or self-signed.

@DDvO
Copy link
Contributor Author

DDvO commented Dec 14, 2019

Maybe it's useful to have some self-issued certs that are not self-signed? And one that has a keyUsage without the keyCertSig.

We already have an example of such an interesting cert among the test certs: test/certs/ee-ed25519.pem. It is just self-issued (but not self-signed) and has just keyAgreement as its keyUsage.

You may have noticed that in this PR I've extended the so far single test case for it in test/recipes/25-test_verify.t:

    # ED25519 certificate from draft-ietf-curdle-pkix-04
    ok(verify("ee-ed25519", "sslserver", ["root-ed25519"], []),
       "accept ED25519 ee cert signature w trusted ED25519 self-signed cert");

by four further ones:

    ok(!verify("root-ed25519", "sslserver", ["ee-ed25519"], []),
       "fail trusted ED25519-signed self-issued cert non-match");

    ok(verify("root-ed25519", "sslserver", ["root-ed25519"], []),
       "accept trusted ED25519 self-signed cert");

    ok(!verify("ee-ed25519", "sslserver", ["ee-ed25519"], []),
       "fail trusted ED25519-signed self-issued cert");

    ok(verify("ee-ed25519", "sslserver", ["ee-ed25519"], [], "-partial_chain"),
       "accept last-resort direct leaf match ED25519-signed self-issued cert");

where in the last one provides a concrete example of the usefulness of -partial_chain (or X509_V_FLAG_PARTIAL_CHAIN, respectively) and for the hint I added to doc/man3/X509_STORE_CTX_get_error.pod and doc/man1/openssl-verify.pod discussed above:

To enable using trusted self-issued but not self-signed certificates
the B<-partial_chain> option may be set.

@DDvO
Copy link
Contributor Author

DDvO commented Dec 24, 2019

Thanks to all how have taken part so far in discussing this PR!

I've just finished all action items (including two change requests) that came up this way:

  • refactored the code to avoid redundancies and increase readability and maintainability
  • retained the original (partly strange) semantics of the API function X509_check_issued()
  • improved the wording of the new documentation texts and comments
  • reverted that part of the changes to build_chain() that did not really make sense and
  • re-structured the PR into a series of pretty self-contained commits with better descriptions

Moreover, I've

@DDvO DDvO force-pushed the fix_x509_vfy_issue_1418 branch from 313035f to c85c05e Compare Dec 25, 2019
@DDvO DDvO changed the title fix confusions and obscure bugs in x509_vfy.c and v3_purp.c fix various issues in x509_vfy.c and v3_purp.c related to #1418 and the respective documentation Dec 25, 2019
@DDvO DDvO force-pushed the fix_x509_vfy_issue_1418 branch 2 times, most recently from a89aa3c to 7862695 Compare Jun 30, 2020
@DDvO DDvO force-pushed the fix_x509_vfy_issue_1418 branch from 7862695 to ab88738 Compare Jun 30, 2020
Copy link

@vdukhovni vdukhovni left a comment

Looks OK to me now.

@vdukhovni
Copy link

vdukhovni commented Jun 30, 2020

Looks OK to me now.

However, my review is for master only. There should be a separate PR for 1.1.1 (if desired), and ideally much smaller. This PR looks much too expansive for a stable release. If there's a minimal change that's compelling for 1.1.1, I'll consider that.

@DDvO
Copy link
Contributor Author

DDvO commented Jun 30, 2020

Looks OK to me now.

Thanks a lot @vdukhovni!
Very glad that this is through finally.

However, my review is for master only. There should be a separate PR for 1.1.1 (if desired), and ideally much smaller. This PR looks much too expansive for a stable release. If there's a minimal change that's compelling for 1.1.1, I'll consider that.

I agree. I will come up with a somewhat minimal backport.
According to users' comments not only in #1418 it looks like there is high demand also for a backport to the LTS version.

@DDvO DDvO added approval: done and removed approval: review pending labels Jul 1, 2020
openssl-machine pushed a commit that referenced this issue Jul 1, 2020
…d certs etc.

Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
(Merged from #10587)
openssl-machine pushed a commit that referenced this issue Jul 1, 2020
This prepares some corrections and improves readability (coding style).
Among others, it adds the static function check_sig_alg_match() and
the internal functions x509_likely_issued() and x509_signing_allowed().

Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
(Merged from #10587)
openssl-machine pushed a commit that referenced this issue Jul 1, 2020
by adding CA basic constraints, CA key usage, and key IDs to the cert
and by add -partial_chain to the verify call that trusts this cert

Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
(Merged from #10587)
openssl-machine pushed a commit that referenced this issue Jul 1, 2020
…ssed X25519 certs

Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
(Merged from #10587)
openssl-machine pushed a commit that referenced this issue Jul 1, 2020
candidate issuer cert cannot be the same as the subject cert 'x'

Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
(Merged from #10587)
openssl-machine pushed a commit that referenced this issue Jul 1, 2020
…k_issued()

Move check that cert signing is allowed from x509v3_cache_extensions() to
where it belongs: internal_verify(), generalize it for proxy cert signing.
Correct and simplify check_issued(), now checking self-issued (not: self-signed).
Add test case to 25-test_verify.t that demonstrates successful fix

Fixes #1418

Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
(Merged from #10587)
openssl-machine pushed a commit that referenced this issue Jul 1, 2020
… X509_verify.pod

Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
(Merged from #10587)
openssl-machine pushed a commit that referenced this issue Jul 1, 2020
Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
(Merged from #10587)
openssl-machine pushed a commit that referenced this issue Jul 1, 2020
…a1 == NULL'

Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
(Merged from #10587)
@DDvO
Copy link
Contributor Author

DDvO commented Jul 1, 2020

Pushed - thanks again @vdukhovni and all others who took part discussing this, also on #1418.

@openssl-machine
Copy link
Collaborator

openssl-machine commented Jul 2, 2020

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@t8m
Copy link
Member

t8m commented Jul 2, 2020

This is already merged.

@t8m t8m closed this Jul 2, 2020
DDvO added a commit to siemens/openssl that referenced this issue Jul 13, 2020
…k_issued()

Move check that cert signing is allowed from x509v3_cache_extensions() to
where it belongs: internal_verify(), generalize it for proxy cert signing.
Correct and simplify check_issued(), now checking self-issued (not: self-signed).
Add test case to 25-test_verify.t that demonstrates successful fix.

As prerequisites, this adds the static function check_sig_alg_match()
and the internal functions x509_likely_issued() and x509_signing_allowed().

This is a backport of the core of PR openssl#10587.
Fixes openssl#1418
openssl-machine pushed a commit that referenced this issue Jul 16, 2020
…k_issued()

Move check that cert signing is allowed from x509v3_cache_extensions() to
where it belongs: internal_verify(), generalize it for proxy cert signing.
Correct and simplify check_issued(), now checking self-issued (not: self-signed).
Add test case to 25-test_verify.t that demonstrates successful fix.

As prerequisites, this adds the static function check_sig_alg_match()
and the internal functions x509_likely_issued() and x509_signing_allowed().

This is a backport of the core of PR #10587.
Fixes #1418

Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from #12357)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: done branch: master branch: 1.1.1
Projects
None yet