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

Detail observed behavior of AllowUnknownCertificateAuthority #6660

Merged
merged 8 commits into from
Apr 27, 2021
Merged

Detail observed behavior of AllowUnknownCertificateAuthority #6660

merged 8 commits into from
Apr 27, 2021

Conversation

stewartadam
Copy link
Contributor

AllowUnknownCertificateAuthority not only ignores untrusted roots, but also partial chains. This updates the documentation to reflect this behavior.

Summary

See dotnet/runtime#49615 for more details. AllowUnknownCertificateAuthority has the (presently) undocumented side effect of also ignoring the PartialChain verification status, which means that there is no validation that any of the root CAs added to the trust store were actually signing the certificate under verification.

Users unaware of this behavior could therefore be lulled into a false sense of safety when verification succeeds.

Fixes #49615.

AllowUnknownCertificateAuthority not only ignores untrusted roots, but also partial chains. This updates the documentation to reflect this behavior.
@opbld34
Copy link

opbld34 commented Apr 22, 2021

Docs Build status updates of commit e2da3d7:

✅ Validation status: passed

File Status Preview URL Details
xml/System.Security.Cryptography.X509Certificates/X509VerificationFlags.xml ✅Succeeded View

For more details, please refer to the build report.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

For any questions, please:

@@ -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

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

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.

@opbld30
Copy link

opbld30 commented Apr 23, 2021

Docs Build status updates of commit ed75623:

❌ Validation status: errors

Please follow instructions here which may help to resolve issue.

File Status Preview URL Details
xml/System.Security.Cryptography.X509Certificates/X509VerificationFlags.xml ❌Error Details
❌Error Details

xml/System.Security.Cryptography.X509Certificates/X509VerificationFlags.xml

  • Line 0, Column 0: [Error-ECMA2Yaml_InternalError]
Intenal Several Error: System.Xml.XmlException: The '%' character, hexadecimal value 0x25, cannot be included in a name. Line 136, position 127.
   at System.Xml.XmlTextReaderImpl.Throw(Exception e)
   at System.Xml.XmlTextReaderImpl.Throw(String res, String[] args)
   at System.Xml.XmlTextReaderImpl.ParseElement()
   at System.Xml.XmlTextReaderImpl.ParseElementContent()
   at System.Xml.Linq.XContainer.ReadContentFrom(XmlReader r)
   at System.Xml.Linq.XDocument.Load(XmlReader reader, LoadOptions options)
   at System.Xml.Linq.XDocument.Parse(String text, LoadOptions options)
   at ECMA2Yaml.ECMALoader.LoadType(FileItem typeFile)
   at ECMA2Yaml.ECMALoader.LoadTypes(String basePath, Namespace ns)

  • Line 0, Column 0: [Error-ECMA2Yaml_File_LoadFailed] Failed to load 1 files, aborting...

For more details, please refer to the build report.

If you see build warnings/errors with permission issues, it might be due to single sign-on (SSO) enabled on Microsoft's GitHub organizations. Please follow instructions here to re-authorize your GitHub account to Docs Build.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

Note: Your PR may contain errors or warnings unrelated to the files you changed. This happens when external dependencies like GitHub alias, Microsoft alias, cross repo links are updated. Please use these instructions to resolve them.

For any questions, please:

Maintain a more concise warning about wincrypt basis and elaborates on the consequences of ignoring PartialChain.
@opbld32
Copy link

opbld32 commented Apr 23, 2021

Docs Build status updates of commit 33c254a:

❌ Validation status: errors

Please follow instructions here which may help to resolve issue.

File Status Preview URL Details
❌Error Details

  • [Error-GitCommitDoesNotExist] Cannot sync git repo to specified commit because commit 33c254a1504d539bcc15c87f19a72bfec86472d3 doesn't exist. It might be caused your branch is deleted or force pushed. If it is not this case, please open a ticket on https://aka.ms/SiteHelp

For more details, please refer to the build report.

If you see build warnings/errors with permission issues, it might be due to single sign-on (SSO) enabled on Microsoft's GitHub organizations. Please follow instructions here to re-authorize your GitHub account to Docs Build.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

Note: Your PR may contain errors or warnings unrelated to the files you changed. This happens when external dependencies like GitHub alias, Microsoft alias, cross repo links are updated. Please use these instructions to resolve them.

For any questions, please:

@opbld30
Copy link

opbld30 commented Apr 23, 2021

Docs Build status updates of commit a99cb15:

❌ Validation status: errors

Please follow instructions here which may help to resolve issue.

File Status Preview URL Details
xml/System.Security.Cryptography.X509Certificates/X509VerificationFlags.xml ❌Error Details
❌Error Details

xml/System.Security.Cryptography.X509Certificates/X509VerificationFlags.xml

  • Line 0, Column 0: [Error-ECMA2Yaml_InternalError]
Intenal Several Error: System.Xml.XmlException: The '%' character, hexadecimal value 0x25, cannot be included in a name. Line 136, position 127.
   at System.Xml.XmlTextReaderImpl.Throw(Exception e)
   at System.Xml.XmlTextReaderImpl.Throw(String res, String[] args)
   at System.Xml.XmlTextReaderImpl.ParseElement()
   at System.Xml.XmlTextReaderImpl.ParseElementContent()
   at System.Xml.Linq.XContainer.ReadContentFrom(XmlReader r)
   at System.Xml.Linq.XDocument.Load(XmlReader reader, LoadOptions options)
   at System.Xml.Linq.XDocument.Parse(String text, LoadOptions options)
   at ECMA2Yaml.ECMALoader.LoadType(FileItem typeFile)
   at ECMA2Yaml.ECMALoader.LoadTypes(String basePath, Namespace ns)

  • Line 0, Column 0: [Error-ECMA2Yaml_File_LoadFailed] Failed to load 1 files, aborting...

For more details, please refer to the build report.

If you see build warnings/errors with permission issues, it might be due to single sign-on (SSO) enabled on Microsoft's GitHub organizations. Please follow instructions here to re-authorize your GitHub account to Docs Build.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

Note: Your PR may contain errors or warnings unrelated to the files you changed. This happens when external dependencies like GitHub alias, Microsoft alias, cross repo links are updated. Please use these instructions to resolve them.

For any questions, please:

@opbld34
Copy link

opbld34 commented Apr 23, 2021

Docs Build status updates of commit bf7d538:

❌ Validation status: errors

Please follow instructions here which may help to resolve issue.

File Status Preview URL Details
xml/System.Security.Cryptography.X509Certificates/X509VerificationFlags.xml ❌Error Details
❌Error Details

xml/System.Security.Cryptography.X509Certificates/X509VerificationFlags.xml

  • Line 0, Column 0: [Error-ECMA2Yaml_InternalError]
Intenal Several Error: System.Xml.XmlException: 'xref' is an undeclared prefix. Line 136, position 61.
   at System.Xml.XmlTextReaderImpl.Throw(Exception e)
   at System.Xml.XmlTextReaderImpl.LookupNamespace(NodeData node)
   at System.Xml.XmlTextReaderImpl.ElementNamespaceLookup()
   at System.Xml.XmlTextReaderImpl.ParseElement()
   at System.Xml.XmlTextReaderImpl.ParseElementContent()
   at System.Xml.Linq.XContainer.ReadContentFrom(XmlReader r)
   at System.Xml.Linq.XDocument.Load(XmlReader reader, LoadOptions options)
   at System.Xml.Linq.XDocument.Parse(String text, LoadOptions options)
   at ECMA2Yaml.ECMALoader.LoadType(FileItem typeFile)
   at ECMA2Yaml.ECMALoader.LoadTypes(String basePath, Namespace ns)

  • Line 0, Column 0: [Error-ECMA2Yaml_File_LoadFailed] Failed to load 1 files, aborting...

For more details, please refer to the build report.

If you see build warnings/errors with permission issues, it might be due to single sign-on (SSO) enabled on Microsoft's GitHub organizations. Please follow instructions here to re-authorize your GitHub account to Docs Build.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

Note: Your PR may contain errors or warnings unrelated to the files you changed. This happens when external dependencies like GitHub alias, Microsoft alias, cross repo links are updated. Please use these instructions to resolve them.

For any questions, please:

@opbld33
Copy link

opbld33 commented Apr 23, 2021

Docs Build status updates of commit 64ddee1:

✅ Validation status: passed

File Status Preview URL Details
xml/System.Security.Cryptography.X509Certificates/X509VerificationFlags.xml ✅Succeeded View

For more details, please refer to the build report.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

For any questions, please:

@opbld32
Copy link

opbld32 commented Apr 26, 2021

Docs Build status updates of commit 5db5d5c:

✅ Validation status: passed

File Status Preview URL Details
xml/System.Security.Cryptography.X509Certificates/X509VerificationFlags.xml ✅Succeeded View

For more details, please refer to the build report.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

For any questions, please:

@opbld32
Copy link

opbld32 commented Apr 26, 2021

Docs Build status updates of commit d6f8fbc:

✅ Validation status: passed

File Status Preview URL Details
xml/System.Security.Cryptography.X509Certificates/X509VerificationFlags.xml ✅Succeeded View

For more details, please refer to the build report.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

For any questions, please:

@opbld32
Copy link

opbld32 commented Apr 27, 2021

Docs Build status updates of commit da1c1f9:

❌ Validation status: errors

Please follow instructions here which may help to resolve issue.

File Status Preview URL Details
xml/System.Security.Cryptography.X509Certificates/X509VerificationFlags.xml ❌Error Details
❌Error Details

xml/System.Security.Cryptography.X509Certificates/X509VerificationFlags.xml

  • Line 0, Column 0: [Error-ECMA2Yaml_InternalError]
Intenal Several Error: System.Xml.XmlException: The 'summary' start tag on line 134 position 10 does not match the end tag of 'Docs'. Line 135, position 9.
   at System.Xml.XmlTextReaderImpl.Throw(Exception e)
   at System.Xml.XmlTextReaderImpl.Throw(String res, String[] args)
   at System.Xml.XmlTextReaderImpl.ThrowTagMismatch(NodeData startTag)
   at System.Xml.XmlTextReaderImpl.ParseEndElement()
   at System.Xml.XmlTextReaderImpl.ParseElementContent()
   at System.Xml.Linq.XContainer.ReadContentFrom(XmlReader r)
   at System.Xml.Linq.XDocument.Load(XmlReader reader, LoadOptions options)
   at System.Xml.Linq.XDocument.Parse(String text, LoadOptions options)
   at ECMA2Yaml.ECMALoader.LoadType(FileItem typeFile)
   at ECMA2Yaml.ECMALoader.LoadTypes(String basePath, Namespace ns)

  • Line 0, Column 0: [Error-ECMA2Yaml_File_LoadFailed] Failed to load 1 files, aborting...

For more details, please refer to the build report.

If you see build warnings/errors with permission issues, it might be due to single sign-on (SSO) enabled on Microsoft's GitHub organizations. Please follow instructions here to re-authorize your GitHub account to Docs Build.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

Note: Your PR may contain errors or warnings unrelated to the files you changed. This happens when external dependencies like GitHub alias, Microsoft alias, cross repo links are updated. Please use these instructions to resolve them.

For any questions, please:

@opbld30
Copy link

opbld30 commented Apr 27, 2021

Docs Build status updates of commit 2d279d3:

✅ Validation status: passed

File Status Preview URL Details
xml/System.Security.Cryptography.X509Certificates/X509VerificationFlags.xml ✅Succeeded View

For more details, please refer to the build report.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

For any questions, please:

@bartonjs bartonjs merged commit e64e640 into dotnet:main Apr 27, 2021
NickCraver added a commit to StackExchange/StackExchange.Redis that referenced this pull request Mar 7, 2024
This issue was brought to my attention last night (thanks reporter!): dotnet/dotnet-api-docs#6660

This changeset ensures that we do not honor self-signed certs or partial/broken chains as a result of `X509VerificationFlags.AllowUnknownCertificateAuthority` downstream and adds a few tests and utilities to generate test certificates (currently valid for ~9000 days). Instead we are checking that the certificate we're being told to trust is explicitly in the chain, given that the result of `.Build()` cannot be trusted for this case.
NickCraver added a commit to StackExchange/StackExchange.Redis that referenced this pull request Mar 9, 2024
This issue was brought to my attention last night (thanks to Badrish Chandramouli): dotnet/dotnet-api-docs#6660

This changeset ensures that we do not honor self-signed certs or partial/broken chains as a result of `X509VerificationFlags.AllowUnknownCertificateAuthority` downstream and adds a few tests and utilities to generate test certificates (currently valid for ~9000 days). Instead we are checking that the certificate we're being told to trust is explicitly in the chain, given that the result of `.Build()` cannot be trusted for this case.

This also resolves an issue where `TrustIssuer` could be called but we'd error when _no errors_ were detected (due to requiring chain errors in our validator), this means users couldn't temporarily trust a cert while getting it installed on the machine for instance and migrating between the 2 setups was difficult.

This needs careful eyes, please scrutinize heavily. It's possible this breaks an existing user, but...it should be broken if so unless there's a case I'm not seeing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants