x509_vfy.c: Make chain building more intuitive, flexible, and complete#13748
x509_vfy.c: Make chain building more intuitive, flexible, and complete#13748DDvO wants to merge 1 commit intoopenssl:masterfrom
Conversation
DDvO
changed the title
|
I am worried about changing behavior. Please make the new behavior off by default. |
@richsalz can you make more concrete what you fear could go wrong here? |
|
The provided change happens to fix an issue that recently was reported on the openssl-users mailing list: Someone wants to use a directly trusted self-signed SSL server cert
As the example just given shows, this would be counter-productive. |
|
With this change, what is the need for the |
|
The fact that someone reported an issue does not outweigh the current users who are depending on the present semantics. |
|
Have you seen the discussion in #7871?
|
|
@kroeckx yes I have read the discussion in #7871. (My comment there raised the same issue as I did here: this is a behavior change happening very late in the 3.0 release process and I am opposed.) I wryly note @vdukhovni's comment, #7871 (comment), that @DDvO gave a thumbs-up to. :) @DDvO, my question "what does this to the requiring PARTIAL_CHAINS" was directed to you. This is a behavior change. There are no tests added that show the behavior change before and after. And when I pushed on that, the response was "one should hope that the multitude of existing tests on chain building and verification covers all critical cases, and none of them fails after this change." HOPE IS NOT GOOD ENOUGH. Here is a use-case that is not covered, for example: all sorts of certs are thrown into a trust store, including "trusted roots" and intermediaries. A few users, that want a particular path through a new intermediary, turn on PARTIAL_CHAINS. Consider when a CA adds a new digest mechanism (or crypto algorithm) that isn't supported on some devices. Chain building and trust validation is hard, full of dark and mysterious corner cases, and changing it should be done upfront EARLY in the release plan, to socialize it (as Viktor said). It should not be done LATE while we are trying to count the days to get the BETA out. |
|
@openssl/otc, I think this PR deserves a special OTC discussion. |
|
I became aware of the discussion in #7871 just a couple of hours back, and meanwhile I've added to two major contributions to it. |
DDvO
removed
approval: otc review pending
triaged: bug
DDvO
changed the title
DDvO
force-pushed
the
chain_building_completeness
branch
2 times, most recently
from
296668a to
2511fb9
Compare
This PR does not aim at changing the semantics of |
Sure, but I still doubt that someone is depending on the incompleteness of the hitherto implementation. |
I've meanwhile added the example discussed recently on the openssl-users list that I mentioned above: It demonstrates the effectiveness of the enhancements done here for an extremely simple case. |
|
It will take me a couple of days of elapsed time to run some experiments. |
|
I haven't looked at this PR in a little while. There are some good bits here IIRC and some things here and some more controversial. Perhaps you could stage just the parts that are cleaning up code and not changing policy into a smaller separate PR, and we can then think about just the policy changes separately, if you're still keen on those too? |
|
Yeah I'll do that (as soon as I find the time for it). |
DDvO
force-pushed
the
chain_building_completeness
branch
from
8c48e5f to
65b90fe
Compare
DDvO
force-pushed
the
chain_building_completeness
branch
from
1a1bfe3 to
e6c093c
Compare
@vdukhovni, I've meanwhlle carved out all unrelated improvements, In a nutshell: |
|
@DDvO, does your latest comment mean you want this in 3.0? |
Not really - I did not change the milestone, which still is Post 3.0.0. |
crypto/x509/x509_vfy.c
Outdated
There was a problem hiding this comment.
Previously exact match of untrusted certificates was only for the EE certificate, allowing one to specify the EE cert as a trust anchor (typically a non-default CAfile with just that cert) for the connection in question.
Now we appear to be doing something similar for intermediate CAs, but why???
There was a problem hiding this comment.
As I wrote in the description of this PR:
it makes little sense to try finding an issuer of the current end of the chain if this end can be found immediately in the trust store (and is actually trusted).
There was a problem hiding this comment.
Other than the EE cert, we always know the provenance of each subsequent certificate as we're building the chain, because we added it, either by getting it from the trust store or from the untrusted store (often peer-provided chain). This is tracked by virtue of the "num_untrusted" variable. Therefore, I still fail to see the point (which you're failing to explain precisely in a non hand waving way) of applying the EE-special trade-in for a trust-store equivalent at any other depth in the chain.
You just have to explain this much better.
There was a problem hiding this comment.
Good point that for each cert in the chain being constructed, except for the target cert (which BTW does not need to be an EE cert) we already know whether its is from the trust store, and in fact we already call check_trust() on the current cert if it is from the trust store, and stop the loop if its is either trusted or rejected!
This was not clear to me because the whole chain building loop is so spread-out and complicated with detail (which partly could and better should be abstracted away).
So thanks to your comment I meanwhile see that for the certs after the target cert the new check is actually redundant!
There was a problem hiding this comment.
Good point that for each cert in the chain being constructed, except for the target cert (which BTW does not need to be an EE cert) we already know whether its is from the trust store, and in fact we already call
check_trust()on the current cert if it is from the trust store, and stop the loop if its is either trusted or rejected!
This was not clear to me because the whole chain building loop is so spread-out and complicated with detail (which partly could and better should be abstracted away).
So thanks to your comment I meanwhile see that for the certs after the target cert the new check is actually redundant!
We're making progress! If you limit the new check for just depth 0, with a suitable comment, my objections may be resolved. You're then just handling EE direct match for partial chain early, skipping the chain construction. That's probably OK, if that's what you're after. I can support that change, building a more complete chain seems redundant (the caller asked for partial chains, and has the EE cert in the store, could they possible indicate intent any more clearly???)
There was a problem hiding this comment.
Done this, as soon as possible, as follows:
/*
* Stop immediately if target cert is directly trusted/rejected.
* This improves completeness and prevents inefficiency and spurious
* errors, which can occur when it makes no sense to look for an issuer.
*/
if (num == 1 && (trust = check_trust(ctx, 1)) != X509_TRUST_UNTRUSTED)
break;
crypto/x509/x509_vfy.c
Outdated
There was a problem hiding this comment.
This used to make the EE cert "trusted", is that now done elsewhere?
There was a problem hiding this comment.
Yes, this is now done within check_trust() -
as I wrote in the new preliminary comment above:
It is now covered by the generalized check_trust() called above, which checks direct trust for current cert (at index num_trusted - 1)
crypto/x509/x509_vfy.c
Outdated
There was a problem hiding this comment.
Please explain exactly which cases this changes from prior behavior...
Is this just intended to handle the EE case of partial chain sooner?
Previous code tried to only do exact EE match as a last resort, building a chain to some trusted issuer if at all possible. I think just in case applications prefer that outcome. This was Stephen Henson's decision, and he's no longer with the project, so there's no certainty as to why, but I think that's plausible.
What's the new rationale? And why did the code have to change?
There was a problem hiding this comment.
A pity that Stephen Henson did not document the rationale for that (IMHO strange) behavior.
As stated several times before, the very idea of this PR is to stop chain building as soon as possible, making the resulting chain as short as possible, which has several advantages:
- completeness - the new test case that would not have succeeded before:
"accept ee-cert2 although deceptively matching also first cert in ee-certs" - IMO more security because only a minimal number of certs are involved in the chain, which less chances of errors/bug/attacks in between.
- efficiency
- sometimes it simply does not make sense to try and find an issuer first, and doing so can cause needless spurious errors
crypto/x509/x509_vfy.c
Outdated
There was a problem hiding this comment.
Does this mean we can't as easily go back to not trusted-first, just by changing the flags at the top of the function?
I guess going back to "untrusted first" is not likely to happen...
There was a problem hiding this comment.
I also think that going back to "untrusted first" is not likely to happen,
but if we wanna keep this option we can remove the tentative #if 0 and #endif,
which I used to verify/demonstrate that it is not necessary (anymore).
DDvO
force-pushed
the
chain_building_completeness
branch
from
e6c093c to
2c5bc52
Compare
|
Rebased to fix a merge conflict. |
crypto/x509/x509_vfy.c
Outdated
There was a problem hiding this comment.
Didn't we finally agree that this is not quite right? Was that in a separate PR that overlaps with this one?
There was a problem hiding this comment.
Right. It was in this PR, namely in #13748 (comment) - please place any further comments on that specific topic there.
I just haven't found the time yet to adapt this PR accordingly.
What you could do in the meantime is to answer to my latest reactions to your comments in #13735. This has been waiting for your further input since end of January,
and would be great if we can finalize that soon.
There was a problem hiding this comment.
Update: @vdukhovni, I've just done that as discussed above.
DDvO
force-pushed
the
chain_building_completeness
branch
from
2c5bc52 to
9b44459
Compare
DDvO
changed the title
|
I think we need to set up a voice call and go through all the outstanding open questions in the multiple X509 PRs. There are just too many places to find all the unresolved questions. I've lost track of where to look. My timezone is GMT-0400. I can do 9AM to 10AM some morning, pick a day and send me a conference link, along with a list of URLs for the relevant PRs and specific unresolved comments inside those PRs. |
Stop chain building immediately if target cert is directly trusted/rejected. This is required because it not always makes sense to try and find an issuer cert. This allows test case "accept ee-cert although deceptively matching also first cert in ee-certs" succeed and prevents spurious errors for instance in test case "accept last-resort direct leaf match Ed25519-signed self-issued cert". This also allows getting rid of exceptional cases in find_issuer() and build_chain(). Also simplify internal_verify() and refactor the invocation of check_dane_issuer().
DDvO
force-pushed
the
chain_building_completeness
branch
from
9b44459 to
cf695c7
Compare
t8m
added
triaged: feature
|
Could you please rebase the PR to resolve conflicts? |
|
@vdukhovni, @t8m, unfortunately, I won't have the time to continue working on this. |
|
Closing for the time being. |
8 participants
UPDATE: this may need further improvements and is put on hold until v3.0.0 is released.
This PR includes some general improvements of the chain building algorithm and fixes a design flaw of
build_chain():it makes little sense to try finding an issuer of the
current end of the chaintarget cert if this cert can be found immediately in the trust store (and is actually trusted).So better stop chain building as early as possible, i.e, as soon as reaching a trust anchor.
This is more in line with RFC 4158 advising for an efficient construction of a chain with minimal length
and should also minimize the risk of picking a non-suitable path,
which is of particular importance in the given implementation because it does not backtrack.
Note that the changes do not aim at changing the implicit trust anchor behavior:
If a certificate is in the trust store and does not have trust attributes,
accept it as implicit trust anchor if it is self-signed or
X509_V_FLAG_PARTIAL_CHAINis set.Also improveUpdate: this side-topic moved to #14128.ossl_cmp_build_cert_chain()using the improve implementationand make it public under the name
OSSL_build_cert_chain().Moreover, generalize the internal
check_trust()to work not with leaf but with current cert.This allows getting rid of strange exceptional cases in
find_issuer()(and maybe also in other parts of the overall chain building algorithm).
Among others, this provides a more general fix (compared to #13762)
of the regression introduced reported for v1.1.1 in #13739.
Checklist