OpenSslX509ChainProcessor: ignore NotSignatureValid on last element. by tmds · Pull Request #69668 · dotnet/runtime

tmds · 2022-05-23T08:46:17Z

Tests still need to be updated.

ghost · 2022-05-23T08:46:24Z

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 · 2022-05-23T11:32:55Z

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 · 2022-05-23T14:00:53Z

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 · 2022-05-23T15:03:45Z

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

runtime/src/libraries/System.Security.Cryptography.X509Certificates/tests/DynamicChainTests.cs

Line 77 in a0ff92c

TestDataGenerator.MakeTestChain3(

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

Make the three certs.
Use the TamperSignature helper on the root, it will give you a new root instance.
Put the root in CustomTrustStore in the X509Chain (and set the appropriate TrustMode)
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 · 2022-05-23T16:43:53Z

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 · 2022-05-23T19:46:23Z

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

Yeah, that.

tmds · 2022-05-24T07:33:08Z

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

....Cryptography/src/System/Security/Cryptography/X509Certificates/OpenSslX509ChainProcessor.cs

src/libraries/System.Security.Cryptography.X509Certificates/tests/ChainTests.cs

....Cryptography/src/System/Security/Cryptography/X509Certificates/OpenSslX509ChainProcessor.cs

src/libraries/System.Security.Cryptography.X509Certificates/tests/ChainTests.cs

tmds · 2022-05-25T07:16:05Z

@bartonjs I've addressed your feedback. ptal.

bartonjs · 2022-05-25T17:58:08Z

Test failure is #69806.

tmds · 2022-05-26T05:34:08Z

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

cc @omajid

bartonjs · 2022-05-26T15:25:36Z

/backport to release/6.0

github-actions · 2022-05-26T15:25:49Z

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

github-actions · 2022-05-26T15:27:34Z

@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 · 2022-05-26T15:34:46Z

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

bartonjs · 2022-05-26T15:38:53Z

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

tmds · 2022-05-31T06:41:40Z

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

bartonjs · 2022-05-31T16:43:50Z

should I create a PR against the 6.0 branch?

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

OpenSslX509ChainProcessor: ignore NotSignatureValid on last element.

dce3f86

ghost added area-System.Security community-contribution Indicates that the PR has been added by a community member labels May 23, 2022

jeffhandley assigned bartonjs May 23, 2022

Add test

056ca8d

Test: verify NotSignatureValid is not filtered from end cert.

c491aaa

stephentoub reviewed May 24, 2022

View reviewed changes

....Cryptography/src/System/Security/Cryptography/X509Certificates/OpenSslX509ChainProcessor.cs Outdated Show resolved Hide resolved

stephentoub reviewed May 24, 2022

View reviewed changes

....Cryptography/src/System/Security/Cryptography/X509Certificates/OpenSslX509ChainProcessor.cs Show resolved Hide resolved

PR feedback.

97156d2