963978 - (picky-with-pickles) Faulty: Parent process crashes as Pickle::ReadBool asserts that the value is 0 or 1

Status: NEW

(Core :: IPC, defect)

Description

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

Attachment: Faulty session

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

The parent process needs to handle bad pickles more gracefully.

Comment 1

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

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.

Comment 2

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

Well, we could certainly change that to return false if the value isn't 0 or 1.

Comment 3

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

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.

Comment 4

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

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?

Comment 5

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

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.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 7

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

You mean something like this?

  MOZ_ASSERT_IF(!FuzzingIPC(), value == 0 || value == 1);

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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);