fix(ffi): unsound FreeContextBuffer #141
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
FreeContextBuffer
function is usingBox::<T>::from_raw
to freememory from a raw pointer of type
*mut c_void
. Sadly, this is unsound.Indeed, knowledge of the original type is required when deallocating memory in
Rust. At the bare minimum, knowledge of the original type’s
Layout
is requiredsince this information is required by the default global allocator (actually
it is a safety invariant of any global allocator in Rust).
The correct layout for a given type can easily be retrieved by using
Layout::new::<T>
orLayout::for_value::<T>
, but this requires knowledge ofthe concrete type
T
. That’s whyBox::<T>::from_raw
safety invariant is definedas follows:
References:
This invariant is violated because
c_void
definitely does not have the samelayout as structs such as
SecPkgInfoW
orSecurityBuffer
.We could allocate a global table protected by a
Mutex
. This table wouldassociate each address pointing to a block we allocated with the layout or some
hint for the original type, and using that information we could cast back into
the original type when freeing the memory. However, I finally settled on using
libc allocator:
1/ libc allocator is doing its own bookkeeping by reserving a metadata chunk in
front of the actual "user" data chunk so that it can retrieve the length of the
allocated block later when
free
is called. By using libc allocator, we don’thave to re-invent our own bookkeeping logic.
2/ Currently, some structures are tightly packed in memory by allocating a
bigger chunk in order to hold both the "top level" struct as well as
other data that fields from the said top level struct are pointing to.
The sec_pkg_info.rs file contains several occurrence
of this pattern. A
Layout
is manually constructed: its size is set to thesum of the sizes of all the elements packed together and the aligment is
assumed to be the same as the top level struct (hardcoded to 8). There is
no comment explaining why this "crime" is safe, but this doesn’t sound quite
right to me. This also breaks the invariant for
Box::<T>::from_raw
even ifwe know the type of the original ("top level") type, because we didn’t use the
correct layout for a
T
when allocating the memory in the first place.That’s why, as of now, I think that using libc allocator is best.
I was able to inject the patched dll into FreeRDP and connect to my Devolutions Lab
without getting any crash, so it doesn’t appear to break anything so far.
(Note: username and password on this screenshot are not sensitive
since this is a local Devolutions Lab)
It’s likely that we still need to audit the unsafe code more in depth.
cc @RRRadicalEdward
I’m not very familiar with sspi-rs codebase, so if you spot anything wrong let me know. Thank you!