Closed Bug 1778808 Opened 2 years ago Closed 2 years ago

is_trivially_copyable does not mean deserializing with memcpy is safe

Categories

(Core :: IPC, task)

task

Tracking

()

RESOLVED FIXED
105 Branch
Tracking Status
firefox105 --- fixed

People

(Reporter: jgilbert, Assigned: jgilbert)

References

Details

Attachments

(2 files)

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 with std::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.

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, but is_trivially_copyable<bool>::value is true, as would any struct-of-bools be.
  • 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::IntSizes 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! 😅

// 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.

Flags: needinfo?(nika)
Flags: needinfo?(nika)
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
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 105 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: