968823 - Shmem::Alloc and Shmem::OpenExisting should not abort on bad params

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Found this while fuzzing IPC with Faulty. See dependent bugs. I only hit crashes in OpenExisting(), but looking at this file, Alloc() also is to be called by IPDL-generated code and to be passed untrusted param values. These functions should not abort (note that they abort even in release builds) based on untrusted parameter values.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Make Shmem more graceful

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8372439 [details] [diff] [review]
Make Shmem more graceful

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

This is opening up some previously untested code paths, are you sure callers can actually handle these null values appropriately?

::: ipc/glue/Shmem.cpp
@@ +370,5 @@
>    else if (aType == SharedMemory::TYPE_SYSV)
>      segment = CreateSegment(segmentSize, SharedMemorySysV::NULLHandle());
>  #endif
> +  else {
> +    NS_WARNING("unknown shmem type");

Hrm, if we're making this warn instead of crash (or even assert) could you please file a followup to make this do whatever we decide to do for fuzzing assertions? I'm worried that we'll forget about these.

@@ +513,1 @@
>      // Unhandled!!

Nit: This comment isn't valid any more.

@@ +513,2 @@
>      // Unhandled!!
> +    NS_WARNING("unhandled shmem alloc!");

Odd, but this is inside a #ifndef DEBUG block, so this warning and all the rest of these warnings below are never going to be used. Just nuke them.

@@ +597,4 @@
>  #endif  // if defined(DEBUG)
>  
>  int
>  Shmem::GetSysVID() const

FYI it looks like this is entirely unused. Feel free to remove if try is happy?

@@ +636,5 @@
>      return new ShmemCreated(routingId, mId, mSize, seg->GetHandle());
>    }
>  #endif
>    else {
> +    NS_ABORT_IF_FALSE(false, "unknown shmem type (here?!)");

Did you mean to leave this as an abort after changing the rest to warnings?

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8372439 [details] [diff] [review]
Make Shmem more graceful

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

r=me with previous comments addressed.

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #2)
> Comment on attachment 8372439 [details] [diff] [review]
> Make Shmem more graceful
> 
> Review of attachment 8372439 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is opening up some previously untested code paths, are you sure callers
> can actually handle these null values appropriately?
> 
> ::: ipc/glue/Shmem.cpp
> @@ +370,5 @@
> >    else if (aType == SharedMemory::TYPE_SYSV)
> >      segment = CreateSegment(segmentSize, SharedMemorySysV::NULLHandle());
> >  #endif
> > +  else {
> > +    NS_WARNING("unknown shmem type");
> 
> Hrm, if we're making this warn instead of crash (or even assert) could you
> please file a followup to make this do whatever we decide to do for fuzzing
> assertions? I'm worried that we'll forget about these.

In fact, good catch: I've replaced NS_WARNING by NS_ERROR in my patch so that if this actually happens while running tests, this will recorded like an assertion failure. Is that enough? (By default I suppose that it is).

> >  
> >  int
> >  Shmem::GetSysVID() const
> 
> FYI it looks like this is entirely unused. Feel free to remove if try is
> happy?

This is used in a unit test: ipc/ipdl/test/cxx/TestSysVShmem.cpp

> 
> @@ +636,5 @@
> >      return new ShmemCreated(routingId, mId, mSize, seg->GetHandle());
> >    }
> >  #endif
> >    else {
> > +    NS_ABORT_IF_FALSE(false, "unknown shmem type (here?!)");
> 
> Did you mean to leave this as an abort after changing the rest to warnings?

Actually, yes. That is because, if I understand correctly, the "(here?!)" in this message is saying that this place is internal enough that any such "unknown shmem type" should have been validated against earlier, and it should be impossible for such bad state to exist at this point.

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

https://tbpl.mozilla.org/?tree=Try&rev=89b9896cc4d4

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/86350b7fe15f

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/86350b7fe15f