Guard every with_capacity call with a bounds check #298
Conversation
The crate does a lot of pre-allocation based on untrusted input, which can lead to unbounded allocation.
|
I’m sorry for being "that guy", but may I ask for this to be merged and eventually released? ;-) |
src/pe/export.rs
Outdated
| "Buffer is too short for {} address table entries", | ||
| address_table_entries | ||
| ); | ||
| return Err(error::Error::Malformed(message)); |
There was a problem hiding this comment.
I’m wondering if we should introduce a TooShort(count, &’static str) error variant which would do:
“Buffer is too short for {0} {1}” since you write the same boilerplate message every time (and technically allocate)
There was a problem hiding this comment.
Good idea! However I noticed that goblin::Error is not flagged as #[non_exhaustive], so adding a new variant would be technically a breaking change :-(
So how about doing that as a followup at a later time so it does not block releasing this fix.
There was a problem hiding this comment.
There is a breaking change pending anyway, so it’s perfect timing.
There was a problem hiding this comment.
That is good to know, I will do that tomorrow then
|
Ok merged, @Swatinem thanks for your patience! Hopefully can release this tonight or tomorrow |
|
0k goblin 0.5 is out, thanks for your patience! |
|
oof, i forgot scroll went to 2021 edition, that's a bit annoying |
|
ok i had to yank it, because it's technically broken on the msrv, 1.40; so i have to bump that to edition 2021, which is, unfortunately, 1.56.0 Hopefully we don't have a change of MSRV until the next edition :) |
The crate does a lot of pre-allocation based on untrusted input, which
can lead to unbounded allocation.
This came up in getsentry/symbolic#479, and I see the crate already had some bounds checks for certain parts, though not for Mach-O parsing.
I went a bit overboard and added explicit bounds checks for every place where a
with_capacityis called.