Open Bug 963978 (picky-with-pickles) Opened 6 years ago Updated 6 years ago

Faulty: Parent process crashes as Pickle::ReadBool asserts that the value is 0 or 1

Categories

(Core :: IPC, defect)

defect
Not set

Tracking

()

People

(Reporter: bjacob, Unassigned)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

Attached file Faulty session
Found by Christoph Diehl's "Faulty" fuzzer, see bug 777067

The parent process needs to handle bad pickles more gracefully.
Component: Graphics → IPC
OS: Linux → All
Hardware: x86_64 → All
I'm not sure about this kind of thing...

The abort is DEBUG-only, and I think we should keep it as a hard abort in DEBUG builds so that we notice it if it ever happens in our testing.

However, since the release mode code handles this case decently well I'm not sure we really want to change anything.
Well, we could certainly change that to return false if the value isn't 0 or 1.
See Also: → 925471
That feels uncomfortable. Asserts are typically used to guarantee consistent internal state. I would like to be able to run with Faulty and asserts, to ensure that a bad IPC child cannot corrupt the parent's internal state.
It's a tough call.

In ordinary operation neither the child nor the parent should ever send an invalid bool value, and it would be a programming error if that ever happened, so I'd say the assert is totally called for.

But in release builds we're trying to anticipate hacked child processes and make sure that we handle them intelligently (or, at least, safely). A hacked child process could easily send a bad bool value, so we need to make sure that we handle that case correctly.

Here I think that the assert proves its utility again when fuzzing. It forces us to look at this case and make sure that we're doing the right thing. We're currently clamping any non-0 value to 'true' and continuing on, but we're missing the opportunity to recognize a hacked child process and force it to be killed.

Does that make sense? Do you disagree?
Oh, but we obviously need some way to annotate this so that we don't abort on fuzzing runs after we've decided that a particular "hack case" is handled appropriately. Otherwise it will make fuzzing very tiresome.
I think we should have a global flag in the parent that indicates whether we're testing the parent+child or just the parent. We set the flag to the latter state when we're deliberately making the child malicious. This assertion would be conditional on testing parent+child. An invalid bool value should cause the child to be unconditionally killed after the assertion has been checked. This may make fuzzing a bit more tedious, but it's not much different to the standard fuzzing problem of having to make sure your generated input is *nearly* correct and not trivially syntactically invalid.
You mean something like this?

  MOZ_ASSERT_IF(!FuzzingIPC(), value == 0 || value == 1);
Blocks: 963976
Alias: picky-with-pickles
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #7)
> You mean something like this?
> 
>   MOZ_ASSERT_IF(!FuzzingIPC(), value == 0 || value == 1);

I would make it more explicit:
   MOZ_ASSERT_IF(!HasMaliciousChildren(), value == 0 || value == 1);
You need to log in before you can comment on or make changes to this bug.