From 11d2240518f9c121dde5933096de6975ac110b83 Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Tue, 29 Sep 2026 22:42:50 -0400 Subject: [PATCH 1/3] fix(ffi): stop AuthenticodeValidator from keeping borrowed dates The validator kept the `&UtcDate` passed to `ExactDate`/`IntervalDate`, but C# only holds the date for the duration of that call. Once the GC collected the date, `Verify()` read freed memory. The validator now owns a copy of the signature and records each setter call as a step, then builds picky's borrowing validator inside `verify()`. Nothing is borrowed across calls, and the setters take an exclusive lease since they mutate. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Devolutions.Picky.Tests/Pkcs7Tests.cs | 98 ++++++++++++- .../Generated/AuthenticodeSignature.cs | 6 +- .../Generated/AuthenticodeValidator.cs | 24 ++-- ffi/src/x509/authenticode.rs | 136 ++++++++++++------ 4 files changed, 196 insertions(+), 68 deletions(-) diff --git a/ffi/dotnet/Devolutions.Picky.Tests/Pkcs7Tests.cs b/ffi/dotnet/Devolutions.Picky.Tests/Pkcs7Tests.cs index 2d7675ab..07798970 100644 --- a/ffi/dotnet/Devolutions.Picky.Tests/Pkcs7Tests.cs +++ b/ffi/dotnet/Devolutions.Picky.Tests/Pkcs7Tests.cs @@ -1,4 +1,5 @@ using System; +using System.Runtime.CompilerServices; using Devolutions.Picky.Diplomat; using Xunit; @@ -68,6 +69,11 @@ public class Pkcs7Tests bOrBNDoLzSPaN6jy3JNeoQAxAA== -----END PKCS7-----"; + static readonly byte[] FILE_HASH = [ + 0xa7, 0x38, 0xda, 0x44, 0x46, 0xa4, 0xe7, 0x8a, 0xb6, 0x47, 0xdb, 0x7e, 0x53, 0x42, 0x7e, 0xb0, 0x79, 0x61, + 0xc9, 0x94, 0x31, 0x7f, 0x4c, 0x59, 0xd7, 0xed, 0xbe, 0xa5, 0xcc, 0x78, 0x6d, 0x80, + ]; + static readonly string private_key = @"-----BEGIN PRIVATE KEY----- MIIEvQIBADANBgkqhkiG9w0BAQEFAASCBKcwggSjAgEAAoIBAQDXTGfzB7A6FuCt Rt0wfM9Ew2ykHgU2L+//65T7/NTjC4ZDCYHEG15IR5EqMsp8P8FAL1h91PGTeL2H @@ -114,11 +120,6 @@ public void SelfSignedAuthenticodeSignatureBasicValidation() Pem pem = Pem.Parse(pkcs7_pem); Pkcs7 pkcs7 = Pkcs7.FromPem(pem); - byte[] FILE_HASH = [ - 0xa7, 0x38, 0xda, 0x44, 0x46, 0xa4, 0xe7, 0x8a, 0xb6, 0x47, 0xdb, 0x7e, 0x53, 0x42, 0x7e, 0xb0, 0x79, 0x61, - 0xc9, 0x94, 0x31, 0x7f, 0x4c, 0x59, 0xd7, 0xed, 0xbe, 0xa5, 0xcc, 0x78, 0x6d, 0x80, - ]; - VecU8 fileHashBuffer = VecU8.FromBytes(FILE_HASH); RsString program_name = RsString.FromString("decoding_into_authenticode_signature"); PrivateKey privateKey = PrivateKey.FromPemStr(private_key); @@ -156,4 +157,91 @@ public void SelfSignedAuthenticodeSignatureBasicValidation() } } + // The fixture's signing certificate is valid from 2021-07-08 to 2022-07-08. + [Theory] + [InlineData(2022, true)] + [InlineData(2030, false)] + public void AuthenticodeValidatorOutlivesItsInputs(ushort year, bool shouldPass) + { + AuthenticodeValidator validator = CreateValidator(); + SetHashAndDate(validator, year); + + // Nothing references the signature, the hash or the date anymore, so this frees them. + CollectGarbage(); + + validator.RequireSigningCertificateCheck(); + validator.RequireNotBeforeCheck(); + validator.RequireNotAfterCheck(); + validator.IgnoreChainCheck(); + + if (shouldPass) + { + validator.Verify(); + } + else + { + Assert.Throws(() => validator.Verify()); + } + } + + [Fact] + public void AlgorithmIdentifierParametersOutliveTheIdentifier() + { + AlgorithmIdentifierParameters parameters = GetDigestAlgorithmParameters(); + + // Nothing references the identifier anymore, except the view borrowing from it. + CollectGarbage(); + + Assert.Equal(AlgorithmIdentifierParametersType.Null, parameters.Type); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void SetHashAndDate(AuthenticodeValidator validator, ushort year) + { + validator.RequireBasicAuthenticodeValidation(VecU8.FromBytes(FILE_HASH)); + validator.ExactDate(UtcDate.Ymd(year, 1, 1)!); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static AlgorithmIdentifierParameters GetDigestAlgorithmParameters() + { + AlgorithmIdentifier algorithm = CreateSignedPkcs7().DigestAlgorithms().Next()!; + Assert.Equal("2.16.840.1.101.3.4.2.1", algorithm.Oid); + + return algorithm.Parameters; + } + + static void CollectGarbage() + { + GC.Collect(); + GC.WaitForPendingFinalizers(); + GC.Collect(); + GC.WaitForPendingFinalizers(); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static AuthenticodeSignature CreateSignature() + { + using Pem pem = Pem.Parse(pkcs7_pem); + using PrivateKey privateKey = PrivateKey.FromPemStr(private_key); + Pkcs7 pkcs7 = Pkcs7.FromPem(pem); + + return AuthenticodeSignature.New(pkcs7, VecU8.FromBytes(FILE_HASH), ShaVariant.Sha2256, privateKey, null); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static AuthenticodeValidator CreateValidator() + { + return CreateSignature().AuthenticodeVerifier(); + } + + static Pkcs7 CreateSignedPkcs7() + { + VecU8 der = CreateSignature().ToDer(); + + byte[] bytes = new byte[(int)der.Length]; + der.Fill(bytes); + + return Pkcs7.FromDer(bytes); + } } \ No newline at end of file diff --git a/ffi/dotnet/Devolutions.Picky/Generated/AuthenticodeSignature.cs b/ffi/dotnet/Devolutions.Picky/Generated/AuthenticodeSignature.cs index 11104c1f..3cf3dbf2 100644 --- a/ffi/dotnet/Devolutions.Picky/Generated/AuthenticodeSignature.cs +++ b/ffi/dotnet/Devolutions.Picky/Generated/AuthenticodeSignature.cs @@ -283,10 +283,6 @@ public Cert SigningCertificate(CertIterator cert) /// /// A AuthenticodeValidator allocated on Rust side. /// - /// - /// Lifetime: the returned native-backed value may borrow from the receiver or one or more inputs. - /// Source handles stay reachable while the returned value exists. - /// public AuthenticodeValidator AuthenticodeVerifier() { unsafe @@ -296,7 +292,7 @@ public AuthenticodeValidator AuthenticodeVerifier() { selfLease = _diplomatHandle.Lease(BorrowKind.Shared); Raw.AuthenticodeValidator* result = Raw.AuthenticodeSignature.AuthenticodeVerifier(selfLease!.Ptr); - return new AuthenticodeValidator(result, LifetimeEdge.Move(ref selfLease)); + return new AuthenticodeValidator(result); } finally { diff --git a/ffi/dotnet/Devolutions.Picky/Generated/AuthenticodeValidator.cs b/ffi/dotnet/Devolutions.Picky/Generated/AuthenticodeValidator.cs index ea17619b..09054891 100644 --- a/ffi/dotnet/Devolutions.Picky/Generated/AuthenticodeValidator.cs +++ b/ffi/dotnet/Devolutions.Picky/Generated/AuthenticodeValidator.cs @@ -66,7 +66,7 @@ public void ExactDate(UtcDate exact) BorrowLease? exactLease = null; try { - selfLease = _diplomatHandle.Lease(BorrowKind.Shared); + selfLease = _diplomatHandle.Lease(BorrowKind.Exclusive); exactLease = exact._diplomatHandle.Lease(BorrowKind.Shared); Raw.AuthenticodeValidator.ExactDate(selfLease!.Ptr, exactLease!.Ptr); } @@ -89,7 +89,7 @@ public void IntervalDate(UtcDate lower, UtcDate upper) BorrowLease? upperLease = null; try { - selfLease = _diplomatHandle.Lease(BorrowKind.Shared); + selfLease = _diplomatHandle.Lease(BorrowKind.Exclusive); lowerLease = lower._diplomatHandle.Lease(BorrowKind.Shared); upperLease = upper._diplomatHandle.Lease(BorrowKind.Shared); Raw.AuthenticodeValidator.IntervalDate(selfLease!.Ptr, lowerLease!.Ptr, upperLease!.Ptr); @@ -110,7 +110,7 @@ public void RequireNotBeforeCheck() BorrowLease? selfLease = null; try { - selfLease = _diplomatHandle.Lease(BorrowKind.Shared); + selfLease = _diplomatHandle.Lease(BorrowKind.Exclusive); Raw.AuthenticodeValidator.RequireNotBeforeCheck(selfLease!.Ptr); } finally @@ -127,7 +127,7 @@ public void RequireNotAfterCheck() BorrowLease? selfLease = null; try { - selfLease = _diplomatHandle.Lease(BorrowKind.Shared); + selfLease = _diplomatHandle.Lease(BorrowKind.Exclusive); Raw.AuthenticodeValidator.RequireNotAfterCheck(selfLease!.Ptr); } finally @@ -144,7 +144,7 @@ public void IgnoreNotBeforeCheck() BorrowLease? selfLease = null; try { - selfLease = _diplomatHandle.Lease(BorrowKind.Shared); + selfLease = _diplomatHandle.Lease(BorrowKind.Exclusive); Raw.AuthenticodeValidator.IgnoreNotBeforeCheck(selfLease!.Ptr); } finally @@ -161,7 +161,7 @@ public void IgnoreNotAfterCheck() BorrowLease? selfLease = null; try { - selfLease = _diplomatHandle.Lease(BorrowKind.Shared); + selfLease = _diplomatHandle.Lease(BorrowKind.Exclusive); Raw.AuthenticodeValidator.IgnoreNotAfterCheck(selfLease!.Ptr); } finally @@ -178,7 +178,7 @@ public void RequireSigningCertificateCheck() BorrowLease? selfLease = null; try { - selfLease = _diplomatHandle.Lease(BorrowKind.Shared); + selfLease = _diplomatHandle.Lease(BorrowKind.Exclusive); Raw.AuthenticodeValidator.RequireSigningCertificateCheck(selfLease!.Ptr); } finally @@ -195,7 +195,7 @@ public void IgnoreSigningCertificateCheck() BorrowLease? selfLease = null; try { - selfLease = _diplomatHandle.Lease(BorrowKind.Shared); + selfLease = _diplomatHandle.Lease(BorrowKind.Exclusive); Raw.AuthenticodeValidator.IgnoreSigningCertificateCheck(selfLease!.Ptr); } finally @@ -214,7 +214,7 @@ public void RequireBasicAuthenticodeValidation(VecU8 expectedFileHash) BorrowLease? expectedFileHashLease = null; try { - selfLease = _diplomatHandle.Lease(BorrowKind.Shared); + selfLease = _diplomatHandle.Lease(BorrowKind.Exclusive); expectedFileHashLease = expectedFileHash._diplomatHandle.Lease(BorrowKind.Shared); Raw.AuthenticodeValidator.RequireBasicAuthenticodeValidation(selfLease!.Ptr, expectedFileHashLease!.Ptr); } @@ -233,7 +233,7 @@ public void IgnoreBasicAuthenticodeValidation() BorrowLease? selfLease = null; try { - selfLease = _diplomatHandle.Lease(BorrowKind.Shared); + selfLease = _diplomatHandle.Lease(BorrowKind.Exclusive); Raw.AuthenticodeValidator.IgnoreBasicAuthenticodeValidation(selfLease!.Ptr); } finally @@ -250,7 +250,7 @@ public void RequireChainCheck() BorrowLease? selfLease = null; try { - selfLease = _diplomatHandle.Lease(BorrowKind.Shared); + selfLease = _diplomatHandle.Lease(BorrowKind.Exclusive); Raw.AuthenticodeValidator.RequireChainCheck(selfLease!.Ptr); } finally @@ -267,7 +267,7 @@ public void IgnoreChainCheck() BorrowLease? selfLease = null; try { - selfLease = _diplomatHandle.Lease(BorrowKind.Shared); + selfLease = _diplomatHandle.Lease(BorrowKind.Exclusive); Raw.AuthenticodeValidator.IgnoreChainCheck(selfLease!.Ptr); } finally diff --git a/ffi/src/x509/authenticode.rs b/ffi/src/x509/authenticode.rs index 4e8de293..1b1c59ca 100644 --- a/ffi/src/x509/authenticode.rs +++ b/ffi/src/x509/authenticode.rs @@ -97,9 +97,11 @@ pub mod ffi { Ok(Box::new(Cert(cert.clone()))) } - pub fn authenticode_verifier<'a>(&'a self) -> Box> { - let verifier = self.0.authenticode_verifier(); - Box::new(AuthenticodeValidator::new(verifier)) + pub fn authenticode_verifier(&self) -> Box { + Box::new(AuthenticodeValidator { + signature: self.0.clone(), + steps: Vec::new(), + }) } pub fn file_hash(&self) -> Option> { @@ -127,73 +129,82 @@ pub mod ffi { } } + // Holds a copy of the signature plus the requested settings, and only builds picky's borrowing + // validator inside `verify`. Nothing is borrowed across calls, so the signature and the dates + // passed in can be collected at any time. #[diplomat::opaque_mut] - pub struct AuthenticodeValidator<'a> { - pub inner: picky::x509::pkcs7::authenticode::AuthenticodeValidator<'a>, - //'exclude_cert_authorities' method down there a few lines takes a reference to a Vec, - // which I have to store in the struct so it have the same lifetime as the inner struct - pub excluded_cert_authorities: Option>, + pub struct AuthenticodeValidator { + signature: picky::x509::pkcs7::authenticode::AuthenticodeSignature, + steps: Vec, } - impl<'a> AuthenticodeValidator<'a> { - pub fn exact_date(&'a self, exact: &'a UtcDate) { - self.inner.exact_date(&exact.0); + impl AuthenticodeValidator { + pub fn exact_date(&mut self, exact: &UtcDate) { + self.steps.push(super::ValidatorStep::ExactDate(exact.0.clone())); } - pub fn interval_date(&'a self, lower: &'a UtcDate, upper: &'a UtcDate) { - self.inner.interval_date(&lower.0, &upper.0); + pub fn interval_date(&mut self, lower: &UtcDate, upper: &UtcDate) { + self.steps.push(super::ValidatorStep::IntervalDate { + lower: lower.0.clone(), + upper: upper.0.clone(), + }); } - pub fn require_not_before_check(&'a self) { - self.inner.require_not_before_check(); + pub fn require_not_before_check(&mut self) { + self.steps.push(super::ValidatorStep::RequireNotBeforeCheck); } - pub fn require_not_after_check(&'a self) { - self.inner.require_not_after_check(); + pub fn require_not_after_check(&mut self) { + self.steps.push(super::ValidatorStep::RequireNotAfterCheck); } - pub fn ignore_not_before_check(&'a self) { - self.inner.ignore_not_before_check(); + pub fn ignore_not_before_check(&mut self) { + self.steps.push(super::ValidatorStep::IgnoreNotBeforeCheck); } - pub fn ignore_not_after_check(&'a self) { - self.inner.ignore_not_after_check(); + pub fn ignore_not_after_check(&mut self) { + self.steps.push(super::ValidatorStep::IgnoreNotAfterCheck); } - pub fn require_signing_certificate_check(&'a self) { - self.inner.require_signing_certificate_check(); + pub fn require_signing_certificate_check(&mut self) { + self.steps.push(super::ValidatorStep::RequireSigningCertificateCheck); } - pub fn ignore_signing_certificate_check(&'a self) { - self.inner.ignore_signing_certificate_check(); + pub fn ignore_signing_certificate_check(&mut self) { + self.steps.push(super::ValidatorStep::IgnoreSigningCertificateCheck); } - pub fn require_basic_authenticode_validation(&'a self, expected_file_hash: &'a VecU8) { - self.inner - .require_basic_authenticode_validation(expected_file_hash.0.clone()); + pub fn require_basic_authenticode_validation(&mut self, expected_file_hash: &VecU8) { + self.steps + .push(super::ValidatorStep::RequireBasicAuthenticodeValidation( + expected_file_hash.0.clone(), + )); } - pub fn ignore_basic_authenticode_validation(&'a self) { - self.inner.ignore_basic_authenticode_validation(); + pub fn ignore_basic_authenticode_validation(&mut self) { + self.steps.push(super::ValidatorStep::IgnoreBasicAuthenticodeValidation); } - pub fn require_chain_check(&'a self) { - self.inner.require_chain_check(); + pub fn require_chain_check(&mut self) { + self.steps.push(super::ValidatorStep::RequireChainCheck); } - pub fn ignore_chain_check(&'a self) { - self.inner.ignore_chain_check(); + pub fn ignore_chain_check(&mut self) { + self.steps.push(super::ValidatorStep::IgnoreChainCheck); } - pub fn exclude_cert_authorities(&'a mut self, cert_auths: &'a DirectoryNameIterator) { - let vec: Vec = cert_auths.0.iter().map(|dn| dn.0.clone()).collect(); - self.excluded_cert_authorities = Some(vec); - self.inner - .exclude_cert_authorities(self.excluded_cert_authorities.as_ref().unwrap()); + pub fn exclude_cert_authorities(&mut self, cert_auths: &DirectoryNameIterator) { + let cert_auths = cert_auths.0.iter().map(|dn| dn.0.clone()).collect(); + self.steps + .push(super::ValidatorStep::ExcludeCertAuthorities(cert_auths)); } pub fn verify(&self) -> Result<(), Box> { - Ok(self.inner.verify()?) + let validator = self.signature.authenticode_verifier(); + for step in &self.steps { + step.apply(&validator); + } + Ok(validator.verify()?) } } @@ -223,11 +234,44 @@ pub mod ffi { } } -impl<'a> ffi::AuthenticodeValidator<'a> { - pub fn new(inner: picky::x509::pkcs7::authenticode::AuthenticodeValidator<'a>) -> Self { - Self { - inner, - excluded_cert_authorities: None, - } +/// A setting recorded on `ffi::AuthenticodeValidator`, replayed in call order onto picky's validator. +enum ValidatorStep { + ExactDate(picky::x509::date::UtcDate), + IntervalDate { + lower: picky::x509::date::UtcDate, + upper: picky::x509::date::UtcDate, + }, + RequireNotBeforeCheck, + IgnoreNotBeforeCheck, + RequireNotAfterCheck, + IgnoreNotAfterCheck, + RequireSigningCertificateCheck, + IgnoreSigningCertificateCheck, + RequireBasicAuthenticodeValidation(Vec), + IgnoreBasicAuthenticodeValidation, + RequireChainCheck, + IgnoreChainCheck, + ExcludeCertAuthorities(Vec), +} + +impl ValidatorStep { + fn apply<'a>(&'a self, validator: &picky::x509::pkcs7::authenticode::AuthenticodeValidator<'a>) { + match self { + Self::ExactDate(exact) => validator.exact_date(exact), + Self::IntervalDate { lower, upper } => validator.interval_date(lower, upper), + Self::RequireNotBeforeCheck => validator.require_not_before_check(), + Self::IgnoreNotBeforeCheck => validator.ignore_not_before_check(), + Self::RequireNotAfterCheck => validator.require_not_after_check(), + Self::IgnoreNotAfterCheck => validator.ignore_not_after_check(), + Self::RequireSigningCertificateCheck => validator.require_signing_certificate_check(), + Self::IgnoreSigningCertificateCheck => validator.ignore_signing_certificate_check(), + Self::RequireBasicAuthenticodeValidation(hash) => { + validator.require_basic_authenticode_validation(hash.clone()) + } + Self::IgnoreBasicAuthenticodeValidation => validator.ignore_basic_authenticode_validation(), + Self::RequireChainCheck => validator.require_chain_check(), + Self::IgnoreChainCheck => validator.ignore_chain_check(), + Self::ExcludeCertAuthorities(cert_auths) => validator.exclude_cert_authorities(cert_auths), + }; } } From bad7ffe9727e4681baaee8dbf30be6e21e56255b Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Fri, 30 Oct 2026 15:29:10 -0400 Subject: [PATCH 2/3] style(ffi): write the AuthenticodeValidator comment one sentence per line Co-Authored-By: Claude Opus 5.5 --- ffi/src/x509/authenticode.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/ffi/src/x509/authenticode.rs b/ffi/src/x509/authenticode.rs index 1b1c59ca..696624a5 100644 --- a/ffi/src/x509/authenticode.rs +++ b/ffi/src/x509/authenticode.rs @@ -129,9 +129,8 @@ pub mod ffi { } } - // Holds a copy of the signature plus the requested settings, and only builds picky's borrowing - // validator inside `verify`. Nothing is borrowed across calls, so the signature and the dates - // passed in can be collected at any time. + // Holds a copy of the signature plus the requested settings, and only builds picky's borrowing validator inside `verify`. + // Nothing is borrowed across calls, so the signature and the dates passed in can be collected at any time. #[diplomat::opaque_mut] pub struct AuthenticodeValidator { signature: picky::x509::pkcs7::authenticode::AuthenticodeSignature, From ceeee15c686fe9d7067f7fc2a54dd85fbfc49bf2 Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Fri, 30 Oct 2026 16:09:37 -0400 Subject: [PATCH 3/3] test(ffi): cover AuthenticodeValidator.IntervalDate outliving its dates Co-Authored-By: Claude Opus 5.5 --- .../Devolutions.Picky.Tests/Pkcs7Tests.cs | 22 ++++++++++++++----- 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/ffi/dotnet/Devolutions.Picky.Tests/Pkcs7Tests.cs b/ffi/dotnet/Devolutions.Picky.Tests/Pkcs7Tests.cs index 07798970..6f313006 100644 --- a/ffi/dotnet/Devolutions.Picky.Tests/Pkcs7Tests.cs +++ b/ffi/dotnet/Devolutions.Picky.Tests/Pkcs7Tests.cs @@ -159,12 +159,14 @@ public void SelfSignedAuthenticodeSignatureBasicValidation() // The fixture's signing certificate is valid from 2021-07-08 to 2022-07-08. [Theory] - [InlineData(2022, true)] - [InlineData(2030, false)] - public void AuthenticodeValidatorOutlivesItsInputs(ushort year, bool shouldPass) + [InlineData(2022, false, true)] + [InlineData(2030, false, false)] + [InlineData(2022, true, true)] + [InlineData(2030, true, false)] + public void AuthenticodeValidatorOutlivesItsInputs(ushort year, bool interval, bool shouldPass) { AuthenticodeValidator validator = CreateValidator(); - SetHashAndDate(validator, year); + SetHashAndDate(validator, year, interval); // Nothing references the signature, the hash or the date anymore, so this frees them. CollectGarbage(); @@ -196,10 +198,18 @@ public void AlgorithmIdentifierParametersOutliveTheIdentifier() } [MethodImpl(MethodImplOptions.NoInlining)] - static void SetHashAndDate(AuthenticodeValidator validator, ushort year) + static void SetHashAndDate(AuthenticodeValidator validator, ushort year, bool interval) { validator.RequireBasicAuthenticodeValidation(VecU8.FromBytes(FILE_HASH)); - validator.ExactDate(UtcDate.Ymd(year, 1, 1)!); + + if (interval) + { + validator.IntervalDate(UtcDate.Ymd(year, 1, 1)!, UtcDate.Ymd(year, 2, 1)!); + } + else + { + validator.ExactDate(UtcDate.Ymd(year, 1, 1)!); + } } [MethodImpl(MethodImplOptions.NoInlining)]