Skip to content

Don't panic on empty cert list or unsupported format. #231

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Don't panic on empty cert list or unsupported format. #231

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 5 additions & 6 deletions authenticode/src/signature.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,14 +265,13 @@ impl AuthenticodeSignature {
self.signed_data
.certificates
.as_ref()
.unwrap()
.0
.iter()
.map(|cert| {
.into_iter()
.flat_map(|v| v.0.iter())
.filter_map(|cert| {
if let cms::cert::CertificateChoices::Certificate(cert) = cert {
cert
Some(cert)
} else {
panic!()
None
Copy link
Collaborator

@nicholasbishop nicholasbishop Oct 13, 2025

Choose a reason for hiding this comment

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

I think perhaps we should perhaps change the iterator item type to something like Result<&Certificate, CertificateIterError>. Silently skipping over certificates matching CertificateChoices::Other seems like it could lead to incorrect validation against the certificate chain.

Copy link
Contributor Author

@roblabla roblabla Oct 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Well if we're going to be changing the signature of the function, we should probably just be returning the CertificateChoice directly in the iterator?

Silently skipping over certificates matching CertificateChoices::Other seems like it could lead to incorrect validation against the certificate chain.

I mean, the worse that can happen is that we fail to validate the chain due to a missing certificate. Which feels fine? And anyways, I'm pretty sure windows skips over certificates it doesn't know how to handle either - though I'll need to double check that (should be as simple as adding an unused Other certificate in the certificate set).

Copy link
Collaborator

@nicholasbishop nicholasbishop Oct 13, 2025

Choose a reason for hiding this comment

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

Good point about returning CertificateChoice directly, let's do that.

I could be convinced that skipping is the right thing to do if we confirm that Windows does that (and add a note about that behavior in the function's docstring).

Copy link
Contributor Author

@roblabla roblabla Dec 2, 2025

Choose a reason for hiding this comment

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

Sorry for taking so long to come back to this. I generated some test binaries of the various Certificatechoice possibilities to see how windows dealt with it. So it turns out windows really doesn't like having anything other than the standard "Certificate" in the CertificateChoice. It kinda makes sense since Authenticode used PKCS#7 as a base (and not CMS) when originally spec'd, and that only accepts Certificate or ExtendedCertificate. But even ExtendedCertificate don't seem to be allowed so 🤷 .

Here's an example binary with an Attribute Certificate V2 inside the certificates of the signed data (but not used in the cert chain, ofc):
attribute_cert_in_certs.exe.zip. Explorer doesn't show the Digital Signature tabs, as if the signature didn't even exist. ASN1 signature can be seen here

}
})
}
Expand Down