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

pe: support basic certificates enumeration #354

Merged
merged 1 commit into from
Mar 5, 2023

Conversation

RaitoBezarius
Copy link
Contributor

@RaitoBezarius RaitoBezarius commented Feb 24, 2023

This is a rough PR to enable basic certificate enumeration in the PE structure.

I am planning to add support to transform the string binary data into a structured format using a PKCS#7 compatible library gated behind a feature flag maybe (?).

I'm not sure my implementation is optimal, let me know if I should simplify stuff.

For tests, I was planning to add PE binaries with a certificate and multiple certificates attached, checking the number of certificates found, would that work?

Later, I am also planning for write support, including attaching arbitrary signatures.

@m4b
Copy link
Owner

m4b commented Feb 25, 2023

CI failing because needs a rustfmt

Copy link
Owner

@m4b m4b left a comment

Choose a reason for hiding this comment

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

let's fix the potential infinite loop/make it less likely with random inputs and some of the underflow/overflow questions, but otherwise this looks great, thank you!

src/pe/certificate_table.rs Outdated Show resolved Hide resolved
src/pe/certificate_table.rs Outdated Show resolved Hide resolved
src/pe/certificate_table.rs Outdated Show resolved Hide resolved
src/pe/certificate_table.rs Outdated Show resolved Hide resolved
src/pe/certificate_table.rs Outdated Show resolved Hide resolved
@RaitoBezarius
Copy link
Contributor Author

Thank you for the mindful comments, will address them :) ; do you have an opinion on the tests @m4b ?

src/pe/certificate_table.rs Outdated Show resolved Hide resolved
@baloo
Copy link
Contributor

baloo commented Feb 26, 2023

nitpick: CertificateAttribute should probably be named AttributeCertificate as this is referred as such in the spec, but it's a nitpick.

src/pe/certificate_table.rs Outdated Show resolved Hide resolved
@RaitoBezarius
Copy link
Contributor Author

Thanks for the comments @baloo ; will address them in the next hours 👍

Copy link
Owner

@m4b m4b left a comment

Choose a reason for hiding this comment

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

let's fix up the nits i posted about pub stuff and other details, and the comments as @baloo suggested and then this is ready to go

src/pe/certificate_table.rs Outdated Show resolved Hide resolved
src/pe/certificate_table.rs Show resolved Hide resolved
src/pe/certificate_table.rs Outdated Show resolved Hide resolved
src/pe/certificate_table.rs Outdated Show resolved Hide resolved
src/pe/certificate_table.rs Outdated Show resolved Hide resolved
src/pe/certificate_table.rs Outdated Show resolved Hide resolved
src/pe/certificate_table.rs Outdated Show resolved Hide resolved
src/pe/mod.rs Outdated Show resolved Hide resolved
src/pe/certificate_table.rs Outdated Show resolved Hide resolved
src/pe/certificate_table.rs Outdated Show resolved Hide resolved
@RaitoBezarius
Copy link
Contributor Author

Everything has been addressed, @m4b :)

Copy link
Owner

@m4b m4b left a comment

Choose a reason for hiding this comment

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

This is good to go but we need to fix last issue with unimplemented, then we can merge. thank you for your patience!

src/pe/certificate_table.rs Outdated Show resolved Hide resolved
@RaitoBezarius
Copy link
Contributor Author

RaitoBezarius commented Mar 5, 2023

thank you for your patience!

it was quite fast for NixOS contributor standards :P -- thank you for your guidance

Copy link
Owner

@m4b m4b left a comment

Choose a reason for hiding this comment

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

this is great thank you @RaitoBezarius

@m4b m4b merged commit 170395f into m4b:master Mar 5, 2023
@m4b
Copy link
Owner

m4b commented Mar 5, 2023

this will likely be rolled up into the 0.7 release which includes 1 minor breaking change, are you ok to wait a bit @RaitoBezarius i prefer to have at least a few changes between releases (1-2 month cadence seems the pattern). if not i could cherry-pick this into a 0.6.2 branch if it's something urgent.

@RaitoBezarius
Copy link
Contributor Author

this will likely be rolled up into the 0.7 release which includes 1 minor breaking change, are you ok to wait a bit @RaitoBezarius i prefer to have at least a few changes between releases (1-2 month cadence seems the pattern). if not i could cherry-pick this into a 0.6.2 branch if it's something urgent.

can definitely wait because I want to send you more PRs on that subject so we can bundle a lot of them for this 0.7 :)

@m4b
Copy link
Owner

m4b commented Jun 12, 2023

released in 0.7.0, thank you so much for your patience!

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.

4 participants