-
nika
-
tjr
jld: To get a IPC tainting POC going, I started modifying the ipdl .py files, then found I need to go edit ipc/chromium/src/chrome/common/ipc_message.h and other stuff there. Is that okay? We're not worried about upstream compatible changes, right?
-
nika
We aren't keeping compatible with upstream, no
-
nika
I'd probably go about un-chromiumifying them at some point, but it's a bunch of work
-
tjr
So I got
phabricator.services.mozilla.com/D60306 going, but I'm having a bit more trouble figuring out the C++ codegen part of things.
-
tjr
I made ExprCast have an option for doing Tainted<foo> but I'm trying to figure out where to put those for affect the arguments....
-
tjr
Ah, found invokeRecvHandler....
-
nika
tjr: what is the intended behaviour of `tainted`?
-
tjr
nika: all arguments of the Recv message will be wrapped with Tainted<>. So Tainted<uint32_t>, etc
-
tjr
(all argument types)
-
nika
What does that get us?
-
» nika is probably unfamiliar with a meaning of the word "tainted" which you're using here
-
nika
I've seen
bug 1610005, but it doesn't have a motivation recorded
-
firebot
bugzil.la/1610005 — NEW, nobody⊙mo — [meta] Support tainting data received from IPC
-
tjr
We'll be forcing explicit validation of the data before its use in the parent process; to avoid trusting data we can't trust. Validation will be context dependent
-
nika
How does this enable that?
-
nika
Is it just a newtype wrapper which requires explicit unwrapping?
-
tjr
Yeah, pretty much. I'm playing around with the ergonomics, but this is the POc I'm working with right now:
gist.github.com/tomrittervg/5ca3512…2f4fcddd1aef4b5eb#file-main-cpp-L22
-
nika
So this is just something like a custom enum serializer?
-
nika
But you have to manually do checks at unwrap sites?
-
tjr
You do have to manually do checks at unwrap sites; I'm not sure if it's like a custom enum serializer.
-
tjr
When I get to applying it to non-builtin types I intend to improve ergonomics where appropriate for classes/structs/etc.
-
nika
What's the benefit of doing them manually at unwrap sites instead of having them validated as we deserialize the type from IPC?
-
nika
It sounds mostly like opt-in syntactic salt right now, but maybe I'm missing the benefit
-
tjr
If the type *can* be validated in codegen that's great - but that seems like a syntactic validation (or maybe a lower-level semantic validation). e.g. "Okay the enum value you sent me was one of the correct enum values and not 0xFFFFFF" But it can't check that only values 1, 2, and 3 shoudl come from the content process and values 4 and 5 are only permissible from the parent. That's the type of semantic validation we're aiming for.
-
nika
So the approach is to blanket mark everything as `Tainted` to force people to think about writing validators?
-
tjr
Obviously I can't blanket everything, but I want to enable the use, encourage adoption, and better understand where and why people don't want/need to validate stuff to iterate on the design.
-
nika
I'm mostly trying to understand the sell for updating your message to use this over writing the validation without the `Tainted` newtype.
-
tjr
Right, it's a fair question. If people remembered to write the validation every time, it wouldn't be needed. So there's that. The other component is making the location of the validation explicit. Tainted<> parameters can and will be passed down to consumers where validation must happen later - this makes the need for validation explicit as you get further from the IPC layer and also allows security review to be more reliable and
-
tjr
targeted
-
nika
So it's effectively a way to say "we've considered validation for this message" to security review folks?
-
tjr
And also a way to make sure people do it. And a way to how reviewers exactly where the validation occurs.
-
nika
Ok, so we'd make it required for new messages?
-
nika
Or, say, only required on messages which are received in the parent process?
-
tjr
Yeah, I think at the other end of the content process it would ideally be required once we got the ergonomics worked out