1439267 - Review of the win32 flash sandbox SSL connection brokering

Status: RESOLVED FIXED

(Firefox Graveyard :: Security: Review Requests, enhancement)

Description

[Paul Theriault [:pauljt] (no longer reading bugmail)]

From PI list: 

"we recently landed bug 1382251 and some bugfix followups (bugs 1429643, 1433855, 1433856 and 1436253) on Nightly.  The gist of this is that it strengthens the Flash process' sandbox and adapts Flash networking behavior to run in the main process (outside of a sandbox).  We're requesting a security review, specifically around Flash behavior."

Targeting Firefox 60

Comment 1

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Taking this bug, aim to completey 27th Feb.

Comment 2

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Need to organise a meeting with David to run through code.

Comment 3

[Paul Theriault [:pauljt] (no longer reading bugmail)]

From email: "since the plugin networking commands are remoted and now run in the chrome process, can this be used to attack the chrome proc from the plugin proc?". That the main concern here, so we should run through IPC code, and maybe look and new process model to ensure we aren't adding a hole to our sandbox.

Comment 4

[Paul Theriault [:pauljt] (no longer reading bugmail)]

David, I took a look through all the functions that we have hooks setup for. I didn't get much just looking at what the functions were supposed to do. I have a couple observations but no risks/issues really (though see my questions below).

 - So we expose InternetOpen and related functions so the plugin can make net connections, that perhaps byapss restrictions we might place on the regular child process. Not sure what if any restrictions we have at the moment, but will be something to think about for future. I was worried that maybe you could open a file:// uri with these functions, but its seems that is not possible with InternetConnect. 

 - PrintDlgW[1] takes a PRINTDLG struct as its only argument [2]. Two of the members of this struct are hooks: LPPRINTHOOKPROC and LPSETUPHOOKPROC

These are described in the docs [2] as "A pointer to a PrintHookProc hook procedure that can process messages intended for the Print dialog box." I was wondering: does this give the child a way to redirect the parent's execution to an address in the parent's address space? There would need to be other bugs at play, to leak addresses, and for the child to inject code into the parent's address space, but I wonder if this is risk. My understanding here is patchy at best though, so this is a bit of a guess. 

It seems though that both of these are ignored based on the value of the "PRINTDLG->flag" value. So maybe we could/should validated the flag value in the parent to prevent these being passed? (if we actually allow this to be passed at all). I don't see a "shouldBroker" for this function though (looks like it existed prior to your patch).

What do you think?

That's about all I could see. Depending on your answers here I'll probably call this done.


[1] https://searchfox.org/mozilla-central/source/dom/plugins/ipc/FunctionHook.cpp#139
[2] https://msdn.microsoft.com/en-us/library/windows/desktop/ms646843(v=vs.85).aspx

Comment 5

[David Parks [:handyman]]

(discussed on IRC)

(In reply to Paul Theriault [:pauljt] from comment #4)

>  - So we expose InternetOpen and related functions so the plugin can make
> net connections, that perhaps byapss restrictions we might place on the
> regular child process. 

The sandbox may impose network restrictions beyond closing off the APIs but I don't know of any.  I think we are good here.

>  - PrintDlgW[1] takes a PRINTDLG struct as its only argument [2]. Two of the
> members of this struct are hooks: LPPRINTHOOKPROC and LPSETUPHOOKPROC

This is fubar so I have to submit a fix for the ParamTraits of IPCPrintDlg but the deal with the user (Adobe) supplied callbacks is that they are extraneous.  I have some other questions about my original implementation but the given callbacks were totally ignored.  I'll ping you once I've restored the original version.

Comment 6

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Ok thanks David, so I think we are done here for now, but I'll have a look again once you've finished up if need be.