This branch fixes two soundness holes in the current yara-rust implementation:
It removes the impl TryFrom<*mut YR_RULES> for Rules, which would allow anyone to create a Rules structure from an invalid, null, or shared raw pointers, allowing safe code to trigger segfaults trivially.
It properly binds the lifetimes in the scan_x functions. Currently, those functions have a signature that looks like fn scan_x<'r>(&self, /* ... */) -> Result<Vec<Rule<'r>>, YaraError>. The 'r lifetime is unbound, meaning the caller gets to chose how large it is! As such, the caller may keep the output Rule longer than the Rules instance, causing use-after-frees.
Note that part of the problem with this second case is that the safety boundary of this library is sort of misplaced. The internals function are all marked as safe, despite being unsafe to use. While this is not really a problem (those functions aren't exposed to the outside world), it makes it harder to spot these kinds of mistakes during review, as the code that binds the lifetime (scan_x in src/rules.rs) is far from the code that does the unsafe operation (scan_x from src/internals/rules.rs).
I believe those problems would be easier to spot if the internals function were marked unsafe, with their safety invariants properly spelled out. If this is something that's of interest, I can submit a PR that moves to such a design.
This branch fixes two soundness holes in the current yara-rust implementation:
impl TryFrom<*mut YR_RULES> for Rules
, which would allow anyone to create aRules
structure from an invalid, null, or shared raw pointers, allowing safe code to trigger segfaults trivially.fn scan_x<'r>(&self, /* ... */) -> Result<Vec<Rule<'r>>, YaraError>
. The'r
lifetime is unbound, meaning the caller gets to chose how large it is! As such, the caller may keep the outputRule
longer than theRules
instance, causing use-after-frees.Note that part of the problem with this second case is that the safety boundary of this library is sort of misplaced. The
internals
function are all marked as safe, despite being unsafe to use. While this is not really a problem (those functions aren't exposed to the outside world), it makes it harder to spot these kinds of mistakes during review, as the code that binds the lifetime (scan_x
insrc/rules.rs
) is far from the code that does the unsafe operation (scan_x
fromsrc/internals/rules.rs
).I believe those problems would be easier to spot if the internals function were marked unsafe, with their safety invariants properly spelled out. If this is something that's of interest, I can submit a PR that moves to such a design.