Closed Bug 1414901 Opened 2 years ago Closed 2 years ago

make Maybe::reset() poison the underlying storage to catch errors earlier

Categories

(Core :: MFBT, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox-esr52 --- wontfix
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

(Keywords: sec-want, Whiteboard: [adv-main60-][post-critsmash-triage])

Attachments

(3 files, 1 obsolete file)

On the assumption that people can do things like:

1. Call Maybe::ref() to get a reference to the Maybe's value;
2. Call Maybe::reset() to destroy said value; and
3. Use the result from step 1, touching memory they really shouldn't.

We should poison Maybe::mStorage in step 2 to make the erroneous step 3 show up (ideally) a little earlier.

Bug 1410186 is similar, but would only catch problems at the point of access (e.g. if there was a step 2.5 above that called Maybe::ref()).

One question is how many channels we should do the poisoning on.  This bug is motivated by bug 1342420, where the crashes only seem to happen on release and very late beta, so doing this when we have MOZ_DIAGNOSTIC_ASSERT or similar would be less desirable.  OTOH, implementing this poisoning scheme would bloat Maybe<T> instances for small T and may be unwelcome from a performance standpoint, so doing it everywhere would be less desirable.
Keywords: sec-want
Group: core-security → dom-core-security
I think I know how to do this, at least for decently-sized T.
Assignee: nobody → nfroyd
The documentation for mozWritePoision says that only an event number of
sizeof(uintptr_t) bytes are overwritten; any trailing bytes are not
touched.  This documentation doesn't correspond to how the function
actually works.  The function as written will happily overwrite trailing
bytes and any bytes not contained in the object, if the passed-in size
isn't divisible by sizeof(uintptr_t).  Let's fix that.
Attachment #8952232 - Flags: review?(jwalden+bmo)
Try looks surprisingly green, for the most part, though there do appear to be
some persistent mochitest-1 failures...
Attachment #8952233 - Flags: review?(jwalden+bmo)
(In reply to Nathan Froyd [:froydnj] from comment #2)
> Created attachment 8952232 [details] [diff] [review]
> part 1 - make mozWritePoision respect its documentation

mozWritePoison, of course.

> The documentation for mozWritePoision says that only an event number of

Same, as well as an *even* number of bytes, sigh.
Here's a better version that adds some Valgrind support, so people have a
better chance of tracking down their bug.
Attachment #8952525 - Flags: review?(jwalden+bmo)
Attachment #8952233 - Attachment is obsolete: true
Attachment #8952233 - Flags: review?(jwalden+bmo)
Attachment #8952232 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8952525 [details] [diff] [review]
part 2 - poison Maybe<T> instances when not in use

Review of attachment 8952525 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/Maybe.h
@@ +28,5 @@
>  
> +namespace detail {
> +
> +template<typename T>
> +struct Poisoner

Mild preference for MaybePoisoner, as this seems like the sort of thing someone's going to bootleg and I'd like to at least be aware if/when that happens.

@@ +35,5 @@
> +
> +  static void poison(void* aPtr)
> +  {
> +    // Avoid MOZ_ASSERTs in mozWritePoison.
> +    if (MOZ_ALIGNOF(T) % sizeof(uintptr_t) == 0

Can you not use alignof here?  Still feels like MOZ_ALIGNOF is a silly distraction and we should be able to remove it, and avoiding use of it in pursuit of that aim seems good.

@@ +114,5 @@
>    // T*.  Indirecting through these functions addresses the problem.
>    void* data() { return mStorage; }
>    const void* data() const { return mStorage; }
>  
> +  void poisonThis()

Rename this to |poisonData|.  This name suggests all of |*this| is poisoned, which obviously is not what anyone would want, given where this is called -- namely, in places where |this->mIsSome| is expected to still have a particular value.
Attachment #8952525 - Flags: review?(jwalden+bmo) → review+
FWIW I generally prefer that this sort of thing be diagnostic, and if specific information is needed for a specific Maybe, particularized poisoning is done rather than force poisoning overhead on everything.  It would not be difficult to add something like a maybePoison() function to Maybe to support this, IMO, and I somewhat prefer that we do that.
(In reply to Jeff Walden [:Waldo] from comment #6)
> @@ +35,5 @@
> > +
> > +  static void poison(void* aPtr)
> > +  {
> > +    // Avoid MOZ_ASSERTs in mozWritePoison.
> > +    if (MOZ_ALIGNOF(T) % sizeof(uintptr_t) == 0
> 
> Can you not use alignof here?  Still feels like MOZ_ALIGNOF is a silly
> distraction and we should be able to remove it, and avoiding use of it in
> pursuit of that aim seems good.

I don't know that alignof will necessarily give you the same alignment as MOZ_ALIGNOF.  There's also the weird different alignments for types in structs vs. bare types thing.

In any event, this assert is there because mozWritePoison checks for alignment, because it blindly casts pointers to uintptr_t*.  Maybe we should just change mozWritePoison to eliminate that requirement and use memcpy to store poison values, which most (x86ish) compilers will just optimize into a store instruction anyway?

> @@ +114,5 @@
> >    // T*.  Indirecting through these functions addresses the problem.
> >    void* data() { return mStorage; }
> >    const void* data() const { return mStorage; }
> >  
> > +  void poisonThis()
> 
> Rename this to |poisonData|.  This name suggests all of |*this| is poisoned,
> which obviously is not what anyone would want, given where this is called --
> namely, in places where |this->mIsSome| is expected to still have a
> particular value.

Good call.  ni? for the alignment question above.

(In reply to Jeff Walden [:Waldo] from comment #7)
> FWIW I generally prefer that this sort of thing be diagnostic, and if
> specific information is needed for a specific Maybe, particularized
> poisoning is done rather than force poisoning overhead on everything.  It
> would not be difficult to add something like a maybePoison() function to
> Maybe to support this, IMO, and I somewhat prefer that we do that.

This is a reasonable complaint.

I slightly prefer the shotgun approach here, because that means that everybody who does something dumb with Maybe gets caught, rather than having to discover it and add appropriate diagnostics after the fact.  (Bug 1410186 is a similar-in-spirit bug to this one, and turned up at one problem we might not have seen otherwise, and potentially others once we re-land it here in a bit.)  OTOH, adding blanket poisoning does seem like it could wreak havoc with e.g. types containing integers that have UAF problems, because then they're dealing with integers much larger than they probably expected to ever deal with.  Whereas a more targeted approach might encourage people to use poisoning specifically for things they know contain many pointers and whatnot, which minimizes the risk.

On the third hand, we poison memory for frames, which have widely varying data members, and AFAIK we haven't run into many problems with that.  So maybe the concern about havoc-wreaking doesn't really apply?
Flags: needinfo?(jwalden+bmo)
(In reply to Nathan Froyd [:froydnj] from comment #8)
> Maybe we should just change mozWritePoison to eliminate that requirement and
> use memcpy to store poison values, which most (x86ish) compilers will just
> optimize into a store instruction anyway?

Yes.  Please!

> (In reply to Jeff Walden [:Waldo] from comment #7)
> > FWIW I generally prefer that this sort of thing be diagnostic, and if
> > specific information is needed for a specific Maybe, particularized
> > poisoning is done rather than force poisoning overhead on everything.  It
> > would not be difficult to add something like a maybePoison() function to
> > Maybe to support this, IMO, and I somewhat prefer that we do that.
> 
> This is a reasonable complaint.
> 
> I slightly prefer the shotgun approach here, because that means that
> everybody who does something dumb with Maybe gets caught, rather than having
> to discover it and add appropriate diagnostics after the fact.  (Bug 1410186
> is a similar-in-spirit bug to this one, and turned up at one problem we
> might not have seen otherwise, and potentially others once we re-land it
> here in a bit.)

What I was suggesting was poison in release/beta -- only don't poison in release.  That catches everything that manifests any time short of release, and it doesn't impose a runtime cost on release.  And if release poisoning is required, that cost is affirmatively opted into.

> OTOH, adding blanket poisoning does seem like it could
> wreak havoc with e.g. types containing integers that have UAF problems,
> because then they're dealing with integers much larger than they probably
> expected to ever deal with.  Whereas a more targeted approach might
> encourage people to use poisoning specifically for things they know contain
> many pointers and whatnot, which minimizes the risk.

I don't think we disagree on the viability of poisoning everything, even small stuff, actually.

> On the third hand, we poison memory for frames, which have widely varying
> data members, and AFAIK we haven't run into many problems with that.  So
> maybe the concern about havoc-wreaking doesn't really apply?

Yeah, havoc-wreaking is not really a concern.  I care more about the perf sacrifice -- for things that want always-poisoning like frames, have them explicitly poison.
Flags: needinfo?(jwalden+bmo)
mozWritePoison secretly depended on the passed-in pointer being aligned
as though it were a pointer to uintptr_t, as it used bare stores to
C-casted pointers to accomplish poisoning.  But this is an unnecessary
limitation: we can use memcpy and rely on the compiler to appropriately
inline the store to an unaligned store instruction if necessary.

This is as discussed in comment 8 and comment 9; it is a prerequisite for
removing the MOZ_ALIGNOF requirement when poisoning.

I verified the Right Thing happens on x86-64 Linux.  clang should be similar,
and I *believe* MSVC will DTRT, though I should probably double-check.
Attachment #8954399 - Flags: review?(jwalden+bmo)
Comment on attachment 8954399 [details] [diff] [review]
part 1b - eliminate alignment requirements for poisoning memory

Review of attachment 8954399 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/Poison.h
@@ +47,1 @@
>    }

Maybe in a separate patch, add one final go of

  memcpy(limit, &POISON, aSize % sizeof(uintptr_t));

(or the equivalent, with the entire function phrased more cleanly) and get rid of this silly only-partially-overwrite documented behavior.  But this looks cool as-is.
Attachment #8954399 - Flags: review?(jwalden+bmo) → review+
(In reply to Jeff Walden [:Waldo] from comment #11)
> Maybe in a separate patch, add one final go of
> 
>   memcpy(limit, &POISON, aSize % sizeof(uintptr_t));
> 
> (or the equivalent, with the entire function phrased more cleanly) and get
> rid of this silly only-partially-overwrite documented behavior.  But this
> looks cool as-is.

I am disinclined to do this, because then the trailing bytes aren't pointing at a guaranteed-to-be-bogus address, but are pointing off to some random address, which seems not great.
The trailing bytes aren't a pointer/address anyway, not if they're < sizeof(uintptr_t).  Doesn't seem to me like it matters if they're a "random" address.  We just prefer that they contain more-garbage values than they previously would have.
Group: dom-core-security → core-security-release
Whiteboard: [adv-main60-]
Flags: qe-verify-
Whiteboard: [adv-main60-] → [adv-main60-][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.