search

google / authenticode-rs

Rust tools for working with Authenticode
Apache License 2.0
26 stars 9 forks source link

No error handling on AttributeCertificateIterator can cause uncatchable error #29

Closed N0fix closed 1 year ago

N0fix commented 1 year ago

Hi, the following code can throw uncatchable error if length field from security directory is invalid, , which can lead to an out-of-bounds read on line 188:

https://github.com/google/authenticode-rs/blob/81a4e5cde559c282975ef3c3f710e342d0c65cc4/authenticode/src/win_cert.rs#L166-L200

Some malwares do have malformed certificates.

I would like to contribute, but I am unsure of what to modify since this is coming from an iterator. Should we ignore the error and return None as if any issue causes the end of the certificate list?

nicholasbishop commented 1 year ago

Thanks for filing this. I think a good solution is to change Item to return a Result, e.g.:

type Item = Result<AttributeCertificate<'a>, AttributeCertificateError>;

Then we can convert panics into proper errors. The iterator becomes a little clunkier to use, but I think that's OK. (This pattern is used, for example, in std::fs::read_dir.)

Happy to review PR(s) if you are interested in working on it :)

N0fix commented 1 year ago

The PR is ready. I had a few issues to build authenticode since command-run cannot be built on windows due to the std::os::unix::ffi::OsStrExt import.

N0fix commented 1 year ago

I apologies for this comment, may you please release a new version that includes this patch ?

nicholasbishop commented 1 year ago

Thanks for the reminder, released 0.3.0 that includes this change.