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

OpenSslX509ChainProcessor: ignore NotSignatureValid on last element. #69668

Merged
merged 5 commits into from
May 25, 2022

Conversation

tmds
Copy link
Member

@tmds tmds commented May 23, 2022

Fixes #65874 (comment).

Implements #65874 (comment).

Tests still need to be updated.

@bartonjs @vcsjones ptal.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label May 23, 2022
@ghost
Copy link

ghost commented May 23, 2022

Tagging subscribers to this area: @dotnet/area-system-security, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #65874 (comment).

Implements #65874 (comment).

Tests still need to be updated.

@bartonjs @vcsjones ptal.

Author: tmds
Assignees: -
Labels:

area-System.Security

Milestone: -

@tmds
Copy link
Member Author

tmds commented May 23, 2022

With this change, I see 12 less test failures on RHEL 9 in System.Security.Cryptography.X509Certificates.Tests:

ChainTests.BuildChain
ChainTests.BuildChainCustomTrustStore(chainBuildsSuccessfully: True, chainFlags: NoError, testArguments: MultipleCalls)
ChainTests.BuildChainCustomTrustStore(chainBuildsSuccessfully: True, chainFlags: NoError, testArguments: TrustedIntermediateTrustedRoot)
ChainTests.BuildChainCustomTrustStore(chainBuildsSuccessfully: True, chainFlags: NoError, testArguments: UntrustedIntermediateTrustedRoot)
ChainTests.VerifyExpiration_LocalTime(verificationTime: 2020-08-28T22:17:02.0000000+00:00, shouldBeValid: True)
ChainTests.VerifyExpiration_LocalTime(verificationTime: 2020-08-28T22:17:02.0000000, shouldBeValid: True)
ChainTests.VerifyExpiration_LocalTime(verificationTime: 2020-08-28T22:17:02.0000000Z, shouldBeValid: True)
ChainTests.VerifyExpiration_LocalTime(verificationTime: 2021-08-28T22:17:01.0000000+00:00, shouldBeValid: True)
ChainTests.VerifyExpiration_LocalTime(verificationTime: 2021-08-28T22:17:01.0000000, shouldBeValid: True)
ChainTests.VerifyExpiration_LocalTime(verificationTime: 2021-08-28T22:17:01.0000000Z, shouldBeValid: True)
CollectionTests.X509ChainElementCollection_CopyTo_NonZeroLowerBound_ThrowsIndexOutOfRangeException
CollectionTests.X509ChainElementCollection_IndexerVsEnumerator

There are still 104 test failures remaining due to the sha1 deprecation. Some of these should be skipped, similar to #67998. Updating these tests is for a separate PR(/PRs).

System.Reflection.PortableExecutable.Tests.PEBuilderTests.Checksum
System.Reflection.PortableExecutable.Tests.PEBuilderTests.BasicValidationSigned
System.Security.Cryptography.Pkcs.Tests.SignerInfoTests.CheckSignature_SHA1WithRSA
System.Security.Cryptography.Pkcs.Tests.SignerInfoTests.AddCounterSigner_RSA(identifierType: IssuerAndSerialNumber)
System.Security.Cryptography.Pkcs.Tests.SignerInfoTests.AddCounterSigner_RSA(identifierType: SubjectKeyIdentifier)
System.Security.Cryptography.Pkcs.Tests.SignerInfoTests.AddSecondCounterSignature_NoSignature_WithoutCert(addExtraCert: False)
System.Security.Cryptography.Pkcs.Tests.SignerInfoTests.AddSecondCounterSignature_NoSignature_WithoutCert(addExtraCert: True)
System.Security.Cryptography.Pkcs.Tests.SignerInfoTests.RemoveCounterSignature_MatchesSubjectKeyIdentifier
System.Security.Cryptography.Pkcs.Tests.SignerInfoTests.AddCounterSigner_ECDSA(identifierType: IssuerAndSerialNumber, digestOid: \"1.3.14.3.2.26\")
System.Security.Cryptography.Pkcs.Tests.SignerInfoTests.AddCounterSigner_ECDSA(identifierType: SubjectKeyIdentifier, digestOid: \"1.3.14.3.2.26\")
System.Security.Cryptography.Pkcs.Tests.SignerInfoTests.AddCounterSigner_ECDSA(identifierType: IssuerAndSerialNumber, digestOid: \"2.16.840.1.101.3.4.2.1\")
System.Security.Cryptography.Pkcs.Tests.SignerInfoTests.AddCounterSigner_ECDSA(identifierType: SubjectKeyIdentifier, digestOid: \"2.16.840.1.101.3.4.2.1\")
System.Security.Cryptography.Pkcs.Tests.SignerInfoTests.AddCounterSigner_ECDSA(identifierType: IssuerAndSerialNumber, digestOid: \"2.16.840.1.101.3.4.2.2\")
System.Security.Cryptography.Pkcs.Tests.SignerInfoTests.AddCounterSigner_ECDSA(identifierType: SubjectKeyIdentifier, digestOid: \"2.16.840.1.101.3.4.2.2\")
System.Security.Cryptography.Pkcs.Tests.SignerInfoTests.AddCounterSigner_ECDSA(identifierType: IssuerAndSerialNumber, digestOid: \"2.16.840.1.101.3.4.2.3\")
System.Security.Cryptography.Pkcs.Tests.SignerInfoTests.AddCounterSigner_ECDSA(identifierType: SubjectKeyIdentifier, digestOid: \"2.16.840.1.101.3.4.2.3\")
System.Security.Cryptography.Pkcs.Tests.SignerInfoTests.AddSecondCounterSignature_NoSignature_WithCert(addExtraCert: False)
System.Security.Cryptography.Pkcs.Tests.SignerInfoTests.AddSecondCounterSignature_NoSignature_WithCert(addExtraCert: True)
System.Security.Cryptography.Pkcs.Tests.SignerInfoTests.AddFirstCounterSigner_NoSignature
System.Security.Cryptography.Pkcs.Tests.SignerInfoTests.RemoveCounterSignature_MatchesIssuerAndSerialNumber
System.Security.Cryptography.Pkcs.Tests.SignerInfoTests.RemoveCounterSignature_MatchesNoSignature
System.Security.Cryptography.Pkcs.Tests.SignerInfoTests.AddCounterSigner_DuplicateCert_RSA
System.Security.Cryptography.Pkcs.Tests.SignerInfoTests.AddCounterSigner_DSA
System.Security.Cryptography.Pkcs.Tests.SignerInfoTests.AddFirstCounterSigner_NoSignature_NoPrivateKey
System.Security.Cryptography.Pkcs.Tests.SignerInfoTests.RemoveCounterSignature_UsesLiveState
System.Security.Cryptography.Pkcs.Tests.SignerInfoTests.CheckSignature_ExtraStore_IsAdditional
System.Security.Cryptography.Pkcs.Tests.SignedCmsWholeDocumentTests.ReadRsaPkcs1DoubleCounterSigned
System.Security.Cryptography.Pkcs.Tests.SignedCmsWholeDocumentTests.ReadRsaPkcs1CounterSigned
System.Security.Cryptography.Pkcs.Tests.SignedCmsWholeDocumentTests.CheckNoSignatureDocument
System.Security.Cryptography.Pkcs.Tests.SignedCmsWholeDocumentTests.ReadRsaPkcs1SimpleDocument
System.Security.Cryptography.Pkcs.Tests.SignedCmsWholeDocumentTests.NonEmbeddedCertificate
System.Security.Cryptography.Pkcs.Tests.TimestampTokenTests.ParseDocument_ExcessData(testDataName: \"FreeTsaDotOrg1\")
System.Security.Cryptography.Pkcs.Tests.TimestampTokenTests.ParseDocument(testDataName: \"FreeTsaDotOrg1\")
System.Security.Cryptography.Pkcs.Tests.SignedCmsTests.CreateSignature_RsaPss(digestOid: \"1.3.14.3.2.26\", assignByConstructor: True)
System.Security.Cryptography.Pkcs.Tests.SignedCmsTests.CreateSignature_RsaPss(digestOid: \"1.3.14.3.2.26\", assignByConstructor: False)
System.Security.Cryptography.Pkcs.Tests.SignedCmsTests.SignCmsUsingExplicitRSAKey
System.Security.Cryptography.Pkcs.Tests.SignedCmsTests.Decode_IgnoresExtraData
System.Security.Cryptography.Pkcs.Tests.SignedCmsTests.CheckSignature_ExtraStore_IsAdditional
System.Security.Cryptography.Pkcs.Tests.SignedCmsTests.AddCertificate
System.Security.Cryptography.Pkcs.Tests.SignedCmsTests.UntrustedCertFails_WhenTrustChecked
System.Security.Cryptography.Pkcs.Tests.SignedCmsTests.CheckSignedEncrypted_IssuerSerial_FromNetFx
System.Security.Cryptography.Pkcs.Tests.SignedCmsTests.CheckSignedEncrypted_SKID_FromNetFx
System.Security.Cryptography.Pkcs.Tests.SignedCmsTests.RemoveAllCertsAddBackSignerCert
System.Security.Cryptography.Pkcs.Tests.SignedCmsTests.CounterSignCmsUsingExplicitRSAKeyForFirstSignerAndDSAForCounterSignature
System.Security.Cryptography.Pkcs.Tests.SignedCmsTests.AddCertificateWithPrivateKey
System.Security.Cryptography.Pkcs.Tests.SignedCmsTests.CounterSignCmsUsingExplicitECDsaKeyForFirstSignerAndRSAForCounterSignature
System.Security.Cryptography.Pkcs.Tests.TimestampRequestTests.ProcessResponse_FreeTsa_WithCerts_NoNonce(expectedStatus: Accepted, variant: 0)
System.Security.Cryptography.Pkcs.Tests.TimestampRequestTests.ProcessResponse_FreeTsa_WithCerts_NoNonce(expectedStatus: HashMismatch, variant: 0)
System.Security.Cryptography.Pkcs.Tests.TimestampRequestTests.ProcessResponse_FreeTsa_WithCerts_NoNonce(expectedStatus: HashMismatch, variant: 1)
System.Security.Cryptography.Pkcs.Tests.TimestampRequestTests.ProcessResponse_FreeTsa_WithCerts_NoNonce(expectedStatus: NonceMismatch, variant: 0)
System.Security.Cryptography.Pkcs.Tests.TimestampRequestTests.ProcessResponse_FreeTsa_WithCerts_NoNonce(expectedStatus: UnexpectedCertificates, variant: 0)
System.Security.Cryptography.X509Certificates.Tests.PublicKeyTests.TestKey_RSA384_ValidatesSignature
System.Security.Cryptography.X509Certificates.Tests.ChainTests.BuildChain_WithCertificatePolicy_Match
System.Security.Cryptography.X509Certificates.Tests.ChainTests.TestResetMethod
System.Security.Cryptography.X509Certificates.Tests.ChainTests.BuildChainCustomTrustStore(chainBuildsSuccessfully: False, chainFlags: UntrustedRoot, testArguments: TrustedIntermediateUntrustedRoot)
System.Security.Cryptography.Xml.Tests.Signature_ArbitraryElements.ExtraAttributes_Lang
System.Security.Cryptography.Xml.Tests.Signature_ArbitraryElements.DifferentSignatureXMLNS
System.Security.Cryptography.Xml.Tests.Signature_ArbitraryElements.OutOfOrder
System.Security.Cryptography.Xml.Tests.Signature_ArbitraryElements.ExtraAttributes_Base
System.Security.Cryptography.Xml.Tests.Signature_ArbitraryElements.ExtraData(arbitraryData: \"<![CDATA[some stuff]]>\", checkSignatureSucceeds: True, loadThrows: False)
System.Security.Cryptography.Xml.Tests.Signature_ArbitraryElements.ExtraData(arbitraryData: \"<!-- comment -->\", checkSignatureSucceeds: True, loadThrows: False)
System.Security.Cryptography.Xml.Tests.Signature_ArbitraryElements.ExtraData(arbitraryData: \" \", checkSignatureSucceeds: True, loadThrows: False)
System.Security.Cryptography.Xml.Tests.Signature_ArbitraryElements.ExtraData(arbitraryData: \"this\", checkSignatureSucceeds: True, loadThrows: False)
System.Security.Cryptography.Xml.Tests.Signature_ArbitraryElements.ExtraData(arbitraryData: \"&amp;\", checkSignatureSucceeds: True, loadThrows: False)
System.Security.Cryptography.Xml.Tests.Signature_ArbitraryElements.ExtraData(arbitraryData: \"<?xml-stylesheet type='text / xsl' href='style.xsl\"..., checkSignatureSucceeds: True, loadThrows: False)
System.Security.Cryptography.Xml.Tests.Signature_ArbitraryElements.ExtraAttributes_WeirdXMLNS
System.Security.Cryptography.Xml.Tests.Signature_ArbitraryElements.ExtraAttributes_Preserve
System.Security.Cryptography.Xml.Tests.Signature_ArbitraryElements.CorrectAttributes
System.Security.Cryptography.Xml.Tests.Reference_ArbitraryElements.DuplicateLegalAttributes
System.Security.Cryptography.Xml.Tests.Reference_ArbitraryElements.ExtraAttributes
System.Security.Cryptography.Xml.Tests.Reference_ArbitraryElements.Transforms_ExtraData_XmlProcessingInstruction
System.Security.Cryptography.Xml.Tests.Reference_ArbitraryElements.OutOfOrder
System.Security.Cryptography.Xml.Tests.Reference_ArbitraryElements.Transforms_ExtraData_CData_Text
System.Security.Cryptography.Xml.Tests.KeyInfo_ArbitraryElements.ExtraData
System.Security.Cryptography.Xml.Tests.SignedInfo_ArbitraryElements.OutOfOrder
System.Security.Cryptography.Xml.Tests.SignedXmlTest.Constructor_Empty
System.Security.Cryptography.Xml.Tests.SignedXmlTest.AsymmetricRSAMixedCaseAttributesVerifyWindows
System.Security.Cryptography.Xml.Tests.SignedXmlTest.CheckSignatureDetectsTamperedDataOnMultipleEnvelopedSignatures(signatureParent: \"a\", tamperNode: \"a\", expected: False)
System.Security.Cryptography.Xml.Tests.SignedXmlTest.CheckSignatureDetectsTamperedDataOnMultipleEnvelopedSignatures(signatureParent: \"a\", tamperNode: \"b\", expected: True)
System.Security.Cryptography.Xml.Tests.SignedXmlTest.CheckSignatureDetectsTamperedDataOnMultipleEnvelopedSignatures(signatureParent: \"b\", tamperNode: \"b\", expected: False)
System.Security.Cryptography.Xml.Tests.SignedXmlTest.CheckSignatureDetectsTamperedDataOnMultipleEnvelopedSignatures(signatureParent: \"b\", tamperNode: \"c\", expected: False)
System.Security.Cryptography.Xml.Tests.SignedXmlTest.CheckSignatureDetectsTamperedDataOnMultipleEnvelopedSignatures(signatureParent: \"y\", tamperNode: \"b\", expected: True)
System.Security.Cryptography.Xml.Tests.SignedXmlTest.CheckSignatureDetectsTamperedDataOnMultipleEnvelopedSignatures(signatureParent: \"y\", tamperNode: \"c\", expected: False)
System.Security.Cryptography.Xml.Tests.SignedXmlTest.CheckSignatureDetectsTamperedDataOnMultipleEnvelopedSignatures(signatureParent: \"y\", tamperNode: \"y\", expected: False)
System.Security.Cryptography.Xml.Tests.SignedXmlTest.CheckSignatureDetectsTamperedDataOnMultipleEnvelopedSignatures(signatureParent: \"\", tamperNode: \"a\", expected: False)
System.Security.Cryptography.Xml.Tests.SignedXmlTest.CheckSignatureDetectsTamperedDataOnMultipleEnvelopedSignatures(signatureParent: \"\", tamperNode: \"b\", expected: False)
System.Security.Cryptography.Xml.Tests.SignedXmlTest.CheckSignatureDetectsTamperedDataOnMultipleEnvelopedSignatures(signatureParent: \"\", tamperNode: \"c\", expected: False)
System.Security.Cryptography.Xml.Tests.SignedXmlTest.CheckSignatureDetectsTamperedDataOnMultipleEnvelopedSignatures(signatureParent: \"\", tamperNode: \"y\", expected: False)
System.Security.Cryptography.Xml.Tests.SignedXmlTest.GetIdElement
System.Security.Cryptography.Xml.Tests.SignedXmlTest.CheckSignatureHandlesIncorrectOrTamperedReferenceWithMultipleEnvelopedSignatures(signatureParent: \"a\", newReference: \"b\")
System.Security.Cryptography.Xml.Tests.SignedXmlTest.CheckSignatureHandlesIncorrectOrTamperedReferenceWithMultipleEnvelopedSignatures(signatureParent: \"a\", newReference: \"nonexisting\")
System.Security.Cryptography.Xml.Tests.SignedXmlTest.Constructor_XmlElement
System.Security.Cryptography.Xml.Tests.SignedXmlTest.DigestValue_LF
System.Security.Cryptography.Xml.Tests.SignedXmlTest.SignedXML_CRLF_Invalid
System.Security.Cryptography.Xml.Tests.SignedXmlTest.CheckSignatureMultipleEnvelopedSignatures(signatureParent: \"a\")
System.Security.Cryptography.Xml.Tests.SignedXmlTest.CheckSignatureMultipleEnvelopedSignatures(signatureParent: \"b\")
System.Security.Cryptography.Xml.Tests.SignedXmlTest.CheckSignatureMultipleEnvelopedSignatures(signatureParent: \"y\")
System.Security.Cryptography.Xml.Tests.SignedXmlTest.CheckSignatureMultipleEnvelopedSignatures(signatureParent: \"\")
System.Security.Cryptography.Xml.Tests.SignedXmlTest.MultipleX509Certificates
System.Security.Cryptography.Xml.Tests.SignedXmlTest.AsymmetricRSAVerify
System.Security.Cryptography.Xml.Tests.SignedXmlTest.SignedXML_LF_Valid
System.Security.Cryptography.Xml.Tests.SignedXmlTest.DigestValue_CRLF
System.Security.Cryptography.Xml.Tests.SignedXmlTest.SignedXML_CRLF_Valid
System.Security.Cryptography.Xml.Tests.SignedXmlTest.Constructor_XmlDocument

@tmds
Copy link
Member Author

tmds commented May 23, 2022

Tests still need to be updated.

For tests, I think we'd like to combine a NotSignatureValid cert:
a. with no errors in the chain,
b. with UntrustedRoot, and
c. with PartialChain.

I'm not sure how to write these tests.

I assume I need to create my own root cert, intermediate cert and end certificate using openssl?
Somehow, I need to make the end certificate NotSignatureValid?

And then, using X509Chain I should be able to validate these 3 cases.

@bartonjs @vcsjones does this sound right? any advice on how I can create the certificates?

@vcsjones
Copy link
Member

@bartonjs @vcsjones does this sound right? any advice on how I can create the certificates?

We have helpers and some existing tests that are pretty close to this scenario.

MakeTestChain3 will give you a Leaf, Intermediate, and Root.

  1. Make the three certs.
  2. Use the TamperSignature helper on the root, it will give you a new root instance.
  3. Put the root in CustomTrustStore in the X509Chain (and set the appropriate TrustMode)
  4. Should successfully build a chain.

Then there are all of the negative tests that Jeremy mentioned, like PartialChain (so put the root in ExtraStore, not CustomTrustStore), etc.

@bartonjs
Copy link
Member

like PartialChain (so put the root in ExtraStore, not CustomTrustStore), etc.

Well, that'd be UntrustedRoot, since it found it (from ExtraStore), but doesn't trust it... PartialChain would be when the root can't be found at all.

Otherwise, yeah, what @vcsjones said 😄.

@vcsjones
Copy link
Member

Well, that'd be UntrustedRoot, since it found it (from ExtraStore)

Yeah, that.

@tmds
Copy link
Member Author

tmds commented May 24, 2022

@vcsjones @bartonjs thanks for these great pointers! I've added a test. ptal.

@tmds
Copy link
Member Author

tmds commented May 25, 2022

@bartonjs I've addressed your feedback. ptal.

@bartonjs
Copy link
Member

Test failure is #69806.

@bartonjs bartonjs merged commit dab9857 into dotnet:main May 25, 2022
@tmds
Copy link
Member Author

tmds commented May 26, 2022

@bartonjs can we backport this to .NET 6 so users don't run into issues on RHEL9?

cc @omajid

@bartonjs
Copy link
Member

/backport to release/6.0

@github-actions
Copy link
Contributor

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/2391548531

@github-actions
Copy link
Contributor

@bartonjs backporting to release/6.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: OpenSslX509ChainProcessor: ignore NotSignatureValid on last element.
Using index info to reconstruct a base tree...
A	src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/OpenSslX509ChainProcessor.cs
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/OpenSslX509ChainProcessor.cs
Applying: Add test
Applying: Test: verify NotSignatureValid is not filtered from end cert.
Applying: PR feedback.
Using index info to reconstruct a base tree...
A	src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/OpenSslX509ChainProcessor.cs
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/OpenSslX509ChainProcessor.cs
CONFLICT (content): Merge conflict in src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/OpenSslX509ChainProcessor.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0004 PR feedback.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

@vcsjones
Copy link
Member

Given all of the code shuffling in 7.0 I would pretty much expect every back port to fail in S.S.Cryptography 😔.

@bartonjs
Copy link
Member

Yeah. I was hoping it'd see backwards through the moves.

@tmds
Copy link
Member Author

tmds commented May 31, 2022

@bartonjs should I create a PR against the 6.0 branch? Or will you look into it?

@bartonjs
Copy link
Member

should I create a PR against the 6.0 branch?

Yes, please. I meant to, but keep getting distracted by other things.

@ghost ghost locked as resolved and limited conversation to collaborators Jun 30, 2022
@bartonjs bartonjs added the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Aug 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Security community-contribution Indicates that the PR has been added by a community member needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OpenSslCryptographicException: error:03000098:digital envelope routines::invalid digest on CentOS Stream 9
4 participants