Skip to content

Detail observed behavior of AllowUnknownCertificateAuthority #6660

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Apr 27, 2021
Merged

Detail observed behavior of AllowUnknownCertificateAuthority #6660

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e2da3d7
Detail observed behavior of AllowUnknownCertificateAuthority
stewartadam Apr 21, 2021
a99cb15
Update docstrings for clarity
stewartadam Apr 23, 2021
bf7d538
Remove %2A from links per GitHub CI feedback
stewartadam Apr 23, 2021
64ddee1
Use see instead of xref
stewartadam Apr 23, 2021
439e88d
Remove note about wincrypt.h
stewartadam Apr 26, 2021
d6f8fbc
Adjust wording to be more concise
stewartadam Apr 26, 2021
da1c1f9
Remove elboration about self-signed roots
stewartadam Apr 27, 2021
2d279d3
Update xml/System.Security.Cryptography.X509Certificates/X509Verifica…
bartonjs Apr 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions xml/System.Security.Cryptography.X509Certificates/X509VerificationFlags.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,10 @@
<format type="text/markdown"><![CDATA[

## Remarks
These flags indicate the conditions under which chain verification should occur. For example, if an application does not require certificates time values in a chain to be valid, the IgnoreNotTimeValid flag can be used.
These flags indicate the conditions under which chain verification should occur. For example, if an application does not require certificates time values in a chain to be valid, the IgnoreNotTimeValid flag can be used.

Note that the behavior of certificate validation in .NET Core aims to standardize its behavior according to the underlying win32 implementation APIs (i.e. CertVerifyCertificateChainPolicy in wincrypt.h), even on other platforms. Therefore, AllowUnknownCertificateAuthority not only allows for an untrusted roots, but also permits partial chains on all platforms.

## Examples
The following example opens the current user's personal certificate store, allows the user to select a certificate, then writes certificate and certificate chain information to the console. The output depends on the certificate you select.

Expand Down Expand Up @@ -131,7 +131,7 @@
</ReturnValue>
<MemberValue>16</MemberValue>
<Docs>
<summary>Ignore that the chain cannot be verified due to an unknown certificate authority (CA).</summary>
<summary>Ignore that the chain cannot be verified due to an unknown certificate authority (CA), or a partial chain. Note that ignoring partial chain skips verification that the signing authority of the certificate under validation was actually signed by one of the known CAs, so this must be performed separately if desired.</summary>
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what the second sentence is trying to say. If a candidate is found but the signature doesn't work out, that produces a different error.

Suggested change
<summary>Ignore that the chain cannot be verified due to an unknown certificate authority (CA), or a partial chain. Note that ignoring partial chain skips verification that the signing authority of the certificate under validation was actually signed by one of the known CAs, so this must be performed separately if desired.</summary>
<summary>Ignore that the chain cannot be verified due to an unknown certificate authority (CA), or a partial chain.</summary>

Copy link
Contributor Author

@stewartadam stewartadam Apr 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Updated the docstring.

I'm trying to elaborate on the consequences of ignoring partial chain in the return value of chain.Build() - in particular, that because partial chain status gets suppressed, the certificate under validation is essentially always considered valid:

If unknown CAs are allowed, the certificate under validation doesn't actually have to be signed by any of the certificates added to the trust store for validation to still succeed. PartialChain is returned in the X509ChainStatus in this scenario, but the bool result of X509Chain.Build() is still true so unless you know that implementation detail and to go look at the statuses, you'd incorrectly assume the certificate was valid.

Given the flag name and its current docstring, the user probably only intended to trust unknown CAs, not partial chains.

IMO this is a key piece of information the docs need to disclose; that users should expect to have to do their own validation of that the fingerprint on the end-certificate's issuer and their expected signing root match, even if chain.Build() returned success (true).

Copy link
Member

Choose a reason for hiding this comment

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

If unknown CAs are allowed, the certificate under validation doesn't actually have to be signed by any of the certificates added to the trust store for validation to still succeed.

Yes, that's true by definition: By specifying this flag you've said it's OK for the chain to end in a CA you don't know about. Whether last thing in the chain is a root authority or an intermediate (or the end-entity itself) you've said "trust doesn't matter" and you need to do something about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true by definition: By specifying this flag you've said it's OK for the chain to end in a CA you don't know about.

The docs don't describe this definition. They say it only ignores UntrustedRoot flag, which is the behavior most users are actually after; verifying a complete chain but allowing for the UntrustedRoot chain status flag is very common for validating certificates against a self-signed root CA. Build() returning verification success for any chain at all in this scenario has not been documented nor is expected.

Most other software that interacts with X.509 has similar flags or options for validating a certificate with an unknown/self-signed certificate authority, but they otherwise keeps all rules the same - i.e. requiring completed chain, and hence the user confusion around this.

The .NET implementation's unexpected behavior regarding partial chain and verification success has come up in the past (dotnet/runtime/issues/26449) and has turned users away.

What do you suggest?

Copy link
Member

Choose a reason for hiding this comment

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

verifying a complete chain but allowing for the UntrustedRoot chain status flag is very common for validating certificates against a self-signed root CA

The correct piece of action for that scenario is that after calling Build you check that chain.ChainElements[^1].Certificate.RawData.SequenceEquals(s_pinnedCertBytes). If you're doing that, it doesn't matter if it ended in UnknownRoot or PartialChain (or a known/trusted root). If you're not doing that, you're not actually testing it came from your own root, and is a security hole.

The only way that the two interact is if you did a check that if the last one ended in UntrustedRoot then you check pinning (which probably isn't what's desired, but maybe it is during "I plan to use a trusted CA cert soon" rollout phases), in which case merely the "or a partial chain" addition in docs covers the scenario.

Copy link
Contributor Author

@stewartadam stewartadam Apr 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I don't disagree with that, and it's been described well in the above linked GitHub issue discussions which is why I'm trying to move it into the docs. If you feel the examples section is a better place for it, I'm happy to add one there.

But let's put specific root CA pinning aside, and consider a more general case of a user wanting to use chain.Build() to verify a given certificate against the OS trust store --

in which case merely the "or a partial chain" addition in docs covers the scenario

IMO "or a partial chain" is a far cry from its actual implications, namely that the user would always need to perform manual issuer verification, because ignoring partial chain implies X509Chain.Build() now returns true no matter what the contents of the trusted certificate store is; the certificate could be entirely unrelated to the trusted root CAs. That behavior is unintuitive and these classes are in a security sensitive area, so I think it warrants being explicit for the sake of our users.

I've updated the wording again for conciseness. If you feel that the docs only warrant a shorter "or a partial chain" then let me know and I'll adjust my PR.

Copy link
Member

Choose a reason for hiding this comment

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

But let's put specific root CA pinning aside, and consider a more general case of a user wanting to use chain.Build() to verify a given certificate against the OS trust store

Then they don't set the X509VerificationFlags.AllowUnknownCertificateAuthority flag, and now PartialChain (or an unknown root) make Build() return false, and everything's happy.

If you feel that the docs only warrant a shorter "or a partial chain" then let me know

I do feel that way. Especially because of the technical errors in your statement (e.g. it'll return false if it's expired (unless the "ignore validity times" flag is also set)).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Especially because of the technical errors in your statement (e.g. it'll return false if it's expired (unless the "ignore validity times" flag is also set)).

Fair enough, the first sentence of the elaboration was ambiguous. Either way, I've removed all of it at your request.

</Docs>
</Member>
<Member MemberName="IgnoreCertificateAuthorityRevocationUnknown">
Expand Down