is_trivially_copyable does not mean deserializing with memcpy is safe
Categories
(Core :: IPC, task)
Tracking
()
Tracking | Status | |
---|---|---|
firefox105 | --- | fixed |
People
(Reporter: jgilbert, Assigned: jgilbert)
References
Details
Attachments
(2 files)
In particular PlainOldDataSerializer should be removed:
https://searchfox.org/mozilla-central/rev/657dc24e7254dac44880c33a2434523d0849d28f/ipc/glue/IPCMessageUtils.h#67
Would you have some more information to support this claim, please?
Going on my own confirmation-biased research:
According to https://en.cppreference.com/w/cpp/types/is_trivially_copyable#Notes :
Objects of trivially-copyable types that are not potentially-overlapping subobjects are the only C++ objects that may be safely copied with
std::memcpy
or serialized to/from binary files withstd::ofstream::write()
/std::ifstream::read()
.
The C++ standard also has explicit allowance to memcpy trivially-copyable objects between themselves, and to a byte buffer and back (to the same object though).
And there's some more obscure language:
For trivially-copyable types, the value representation is a set of bits in the object representation that determines a value
which I interpret to mean that if you can modify the bits/bytes of a live trivially-copyable object in a way that corresponds to a known valid value (say, you memcpy'd it from another object), then your object really has this new value now.
But if you're correct (thank you very much for shattering my world! 😱), it's used in quite a few places, so we'd have to review them all.
Assignee | ||
Comment 2•2 years ago
•
|
||
Sorry that I didn't explain more!
It's safe for within the same security regime, and of course it's mostly[1] safe to copy from these types. However, it's not necessarily safe to copy to these types if you don't trust the source of the copy, such as receiving untrusted bytes from over IPC.
Generally speaking C++ only guarantees the behavior of operations on type T for objects that are backed by valid-for-T bits.
- If you memcpy 0x02 into a bool, things can get weird from what I can tell?
- Our serialization code does generally specialize for
bool
, butis_trivially_copyable<bool>::value
istrue
, as would any struct-of-bools be.
- Our serialization code does generally specialize for
- If you cast an out-of-bounds value to an enum, that's actually totally valid C++. (even though it's also valid C++ to exhaustively enumerate an enum without having a default label. . .)
- Copying padding bytes can be a vulnerability vector (this caveats [1])
We also of course have the more general problem of (struct) invariants, such that not every possible bit pattern represents a safe and valid struct.
(E.g. We have a lot of code that assumes gfx::IntSize
s are non-negative, and would break if you copied in bits from rand())
I think this is generally a different level of problem, and that what we're wanting here is to have safe transit of scalarizeable structs.
I have an approach that makes it fairly easy to serialize structs field by field, adds bool and enum handling for fields of structs, and also adds a new safe approach for enum validation. (And generally tries to move such things closer to the struct declaration, rather than being spread between files)
Thank you for the explanation, and for thinking about the safety/security aspect of receiving bytes from untrusted sources -- it's indeed quite the can of worms, good luck to you! 😅
Assignee | ||
Comment 4•2 years ago
|
||
// We guarantee our robustness via these requirements:
// * Object.MutTiedFields() gives us a tuple,
// * where the combined sizeofs all field types sums to sizeof(Object),
// * (thus we know we are exhaustively listing all fields)
// * where feeding each field back into ParamTraits succeeds,
// * and ParamTraits is only automated for BytesAlwaysValidT<T> types.
// (BytesAlwaysValidT rejects bool and enum types, and only accepts int/float
// types, or array or std::arrays of such types)
// (Yes, bit-field fields are rejected by MutTiedFields too)
BytesAlwaysValidT<T> is the same as the IsTriviallySerializable<T> that
it replaces, however the emphasis is different, and should discourage
tagging structs as IsTriviallySerializable, since they more clearly
aren't BytesAlwaysValid.
Assignee | ||
Updated•2 years ago
|
Updated•2 years ago
|
Assignee | ||
Comment 5•2 years ago
|
||
Pushed by jgilbert@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6042409d7caf Use stricter TiedFields instead of IsTriviallySerializable in WebGL's QueueParamTraits. r=lsalzman,nika https://hg.mozilla.org/integration/autoland/rev/290f3b1c7102 Trim unused code, particularly in TiedFields.h. r=nika
Comment 7•2 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6042409d7caf
https://hg.mozilla.org/mozilla-central/rev/290f3b1c7102
Description
•