Closed Bug 1244926 Opened 8 years ago Closed 8 years ago

Add a filter for TCP connection under e10s

Categories

(Core :: WebRTC: Networking, defect, P1)

Product:
Core
Component:
WebRTC: Networking
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla48
Tracking Flags:
Tracking Status
e10s + ---
firefox47 --- affected
firefox48 --- fixed
Blocking Flags:
backlog webrtc/webaudio+

People

(Reporter: drno, Assigned: drno)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

MozReview Request: Bug 1244926: added TCP socket filter to only allow outgoing STUN. r=jesup,mcmanus
58 bytes, text/x-review-board-request
jesup
: review+
mcmanus
: review+
Details 
To utilize that content is separated from the rest under e10s we have implemented a simple STUN UDP packet filter in stun_udp_socket_filter.cpp.
With the utilizing ICE TCP we should have a similar filter for TCP as well. This should prevent that a hijacked content process can abuse TCP connection for something else then STUN.
backlog: --- → webrtc/webaudio+
Rank: 12
Assignee: nobody → drno
Comment on attachment 8719011 [details]
MozReview Request: Bug 1244926: added TCP socket filter to only allow outgoing STUN.  r=jesup,mcmanus

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34843/diff/1-2/
Comment on attachment 8719011 [details]
MozReview Request: Bug 1244926: added TCP socket filter to only allow outgoing STUN.  r=jesup,mcmanus

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34843/diff/2-3/
Comment on attachment 8719011 [details]
MozReview Request: Bug 1244926: added TCP socket filter to only allow outgoing STUN.  r=jesup,mcmanus

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34843/diff/3-4/
Attachment #8719011 - Attachment description: MozReview Request: Bug 1244926: (WIP) add TCP socket filter → MozReview Request: Bug 1244926: (WIP) added optional TCP socket filter
Comment on attachment 8719011 [details]
MozReview Request: Bug 1244926: added TCP socket filter to only allow outgoing STUN.  r=jesup,mcmanus

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34843/diff/4-5/
Attachment #8719011 - Attachment description: MozReview Request: Bug 1244926: (WIP) added optional TCP socket filter → MozReview Request: Bug 1244926: added TCP socket filter to only allow outgoing STUN
Comment on attachment 8719011 [details]
MozReview Request: Bug 1244926: added TCP socket filter to only allow outgoing STUN.  r=jesup,mcmanus

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34843/diff/5-6/
Comment on attachment 8719011 [details]
MozReview Request: Bug 1244926: added TCP socket filter to only allow outgoing STUN.  r=jesup,mcmanus

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34843/diff/6-7/
Attachment #8719011 - Flags: review?(rjesup)
Attachment #8719011 - Flags: review?(josh)
Attachment #8719011 - Flags: review?(ekr)
Attachment #8719011 - Flags: review?(rjesup) → review+
Comment on attachment 8719011 [details]
MozReview Request: Bug 1244926: added TCP socket filter to only allow outgoing STUN.  r=jesup,mcmanus

https://reviewboard.mozilla.org/r/34843/#review37697

::: dom/network/TCPSocketParent.cpp:415
(Diff revision 7)
> +                                          dataA.Length(),
> +                                          nsIUDPSocketFilter::SF_INCOMING,
> +                                          &allowed);
> +    // receiving unallowed data, drop it.
> +    if (NS_WARN_IF(NS_FAILED(nsrv)) || !allowed) {
> +      // TODO: what is the right log for an error message here?

TCPSOCKET_LOG() IIRC (there's one for UDPSOCKET logging; clone that if you need).
https://reviewboard.mozilla.org/r/34841/#review38519

I think you really need to refactor the names.

::: media/mtransport/stun_udp_socket_filter.cpp:269
(Diff revision 7)
> +    if (!nr_is_stun_message(stun, length)) {
> +      //TODO should we really let pass non-STUN data here to mimic the UDP
> +      //     filter behavior?
> +      //     This has the benefit of solving the problem of messages less
> +      //     then the min STUN request size still being handed to the upper
> +      //     layers for re-assembly (which we otherwise would reject here).
> +      return true;
> +    }

TODO(drno@...) or whatever
Comment on attachment 8719011 [details]
MozReview Request: Bug 1244926: added TCP socket filter to only allow outgoing STUN.  r=jesup,mcmanus

https://reviewboard.mozilla.org/r/34843/#review38513

::: media/mtransport/stun_udp_socket_filter.cpp:341
(Diff revision 7)
>  NS_IMPL_ISUPPORTS(nsStunUDPSocketFilterHandler, nsIUDPSocketFilterHandler)
>  
>  NS_IMETHODIMP nsStunUDPSocketFilterHandler::NewFilter(nsIUDPSocketFilter **result)
>  {
>    nsIUDPSocketFilter *ret = new STUNUDPSocketFilter();
>    if (!ret) {
>      return NS_ERROR_OUT_OF_MEMORY;
>    }
>    NS_ADDREF(*result = ret);
>    return NS_OK;
>  }

Copy-paste is annoying here. Almost grounds for a template or refactor.. Almost.

::: media/mtransport/stun_udp_socket_filter.cpp:352
(Diff revision 7)
> +
> +NS_IMPL_ISUPPORTS(nsStunTCPSocketFilterHandler, nsIUDPSocketFilterHandler)
> +
> +NS_IMETHODIMP nsStunTCPSocketFilterHandler::NewFilter(nsIUDPSocketFilter **result)
> +{
> +  nsIUDPSocketFilter *ret = new STUNTCPSocketFilter();
> +  if (!ret) {
> +    return NS_ERROR_OUT_OF_MEMORY;

Kind of sad you didn't rename the interface. Why not nsISTUNSocketfilterHandler?
Attachment #8719011 - Flags: review?(ekr)
Comment on attachment 8719011 [details]
MozReview Request: Bug 1244926: added TCP socket filter to only allow outgoing STUN.  r=jesup,mcmanus

https://reviewboard.mozilla.org/r/34843/#review38539

I reviewed all the dom/network changes and the glue code that integrates with it; I stopped short of reviewing most of the lower-level STUN stuff and testing code.

::: dom/network/TCPSocketChild.h:79
(Diff revision 7)
>                              const CallbackData& aData,
>                              const uint32_t& aReadyState) override;
>    virtual bool RecvRequestDelete() override;
>    virtual bool RecvUpdateBufferedAmount(const uint32_t& aBufferred,
>                                          const uint32_t& aTrackingNumber) override;
> +  NS_IMETHODIMP SetFilterName(const nsACString& aFilterName);

There doesn't appear to be any reason for this to be NS_IMETHODIMP, since it's not implementing a method declared in an xpidl.

::: dom/network/TCPSocketChild.cpp:105
(Diff revision 7)
>  {
>    mSocket = aSocket;
>  
>    AddIPDLReference();
>    gNeckoChild->SendPTCPSocketConstructor(this, mHost, mPort);
> -  PTCPSocketChild::SendOpen(mHost, mPort, aUseSSL, aUseArrayBuffers);
> +  PTCPSocketChild::SendOpen(mHost, mPort, aUseSSL, aUseArrayBuffers, mFilterName);

Does this codepath actually get used with the filter? I only see SendWindowlessOpenBind being called after setting a filter on the child socket. I'd rather assert that this never happens if possible.

::: dom/network/TCPSocketParent.h:89
(Diff revision 7)
>  
>  private:
>    virtual void ActorDestroy(ActorDestroyReason why) override;
>    void SendEvent(const nsAString& aType, CallbackData aData, TCPReadyState aReadyState);
> +
> +  nsCOMPtr<nsIUDPSocketFilter> mFilter;

Maybe consider renaming this interface now that it's not UDP-specific?

::: dom/network/TCPSocketParent.cpp:242
(Diff revision 7)
>    if (NS_FAILED(rv)) {
>      FireInteralError(this, __LINE__);
>      return true;
>    }
>  
> +  if (!aFilter.IsEmpty()) {

Let's make this a helper function rather than duplicating it.

::: dom/network/TCPSocketParent.cpp:318
(Diff revision 7)
>    ErrorResult rv;
> +
> +  if (mFilter) {
> +    mozilla::net::NetAddr addr; // dummy value
> +    bool allowed;
> +    const InfallibleTArray<uint8_t>& data(aData.get_ArrayOfuint8_t());

This should assert that the type of aData is correct first.

::: dom/network/TCPSocketParent.cpp:325
(Diff revision 7)
> +                                          data.Length(),
> +                                          nsIUDPSocketFilter::SF_OUTGOING,
> +                                          &allowed);
> +
> +    // Reject sending of unallowed data
> +    if (NS_WARN_IF(NS_FAILED(nsrv)) || ! allowed) {

nit: extra space after !

::: dom/network/TCPSocketParent.cpp:326
(Diff revision 7)
> +                                          nsIUDPSocketFilter::SF_OUTGOING,
> +                                          &allowed);
> +
> +    // Reject sending of unallowed data
> +    if (NS_WARN_IF(NS_FAILED(nsrv)) || ! allowed) {
> +      return false;

Is the intent here to kill the child process if the filter rejects the data?

::: dom/network/TCPSocketParent.cpp:408
(Diff revision 7)
> -  SendEvent(NS_LITERAL_STRING("data"), SendableData(nsCString(aData)), aReadyState);
> +  SendableData data((nsCString(aData)));
> +
> +  if (mFilter) {
> +    bool allowed;
> +    mozilla::net::NetAddr addr;
> +    const InfallibleTArray<uint8_t>& dataA(data.get_ArrayOfuint8_t());

How can this possibly work? data's type has just been set to an nsCString a few lines previously.

::: media/mtransport/stun_udp_socket_filter.cpp:358
(Diff revision 7)
> +NS_IMPL_ISUPPORTS(nsStunTCPSocketFilterHandler, nsIUDPSocketFilterHandler)
> +
> +NS_IMETHODIMP nsStunTCPSocketFilterHandler::NewFilter(nsIUDPSocketFilter **result)
> +{
> +  nsIUDPSocketFilter *ret = new STUNTCPSocketFilter();
> +  if (!ret) {

new is infallible.

::: media/mtransport/test/ice_unittest.cpp:1773
(Diff revision 7)
> -    nsCOMPtr<nsIUDPSocketFilterHandler> handler =
> +    nsCOMPtr<nsIUDPSocketFilterHandler> udp_handler =
>        do_GetService(NS_STUN_UDP_SOCKET_FILTER_HANDLER_CONTRACTID);
> -    handler->NewFilter(getter_AddRefs(filter_));
> +    ASSERT_TRUE(udp_handler);
> +    udp_handler->NewFilter(getter_AddRefs(udp_filter_));
> +
> +    nsCOMPtr<nsIUDPSocketFilterHandler> tcp_handler =

This feels like a strange layer of indirection; why not just have STUNTCPSocketFilter/STUNUDPSocketFilter have an associated constract id and use do_CreateInstance directly, rather than getting this service which has a single method that returns a new instance of a single class?

::: netwerk/base/nsIUDPSocketFilter.idl:47
(Diff revision 7)
>   * Filter handlers are registered with XPCOM under the following CONTRACTID prefix:
>   */
>  #define NS_NETWORK_UDP_SOCKET_FILTER_HANDLER_PREFIX "@mozilla.org/network/udp-filter-handler;1?name="
> +#define NS_NETWORK_TCP_SOCKET_FILTER_HANDLER_PREFIX "@mozilla.org/network/tcp-filter-handler;1?name="
> +
> +#define NS_NETWORK_SOCKET_FILTER_HANDLER_STUN_POSTFIX "stun"

s/POSTFIX/SUFFIX/
Attachment #8719011 - Flags: review?(josh)
https://reviewboard.mozilla.org/r/34843/#review38539

> Is the intent here to kill the child process if the filter rejects the data?

That is actually a good question. I guess the assumption is that something really nasty is going on if the filter kicks in. Killing the child process sounds like a reasonable and safe choice in that case. How would/could I accomplish that?

> This feels like a strange layer of indirection; why not just have STUNTCPSocketFilter/STUNUDPSocketFilter have an associated constract id and use do_CreateInstance directly, rather than getting this service which has a single method that returns a new instance of a single class?

My guess is that this is historic artifact. There used to be #ifdef'ed code for external linkage which this whole filter would turn into a dummy implementation.
I can try to clean that up now where the #ifdef's are gone.
https://reviewboard.mozilla.org/r/34843/#review38539

> How can this possibly work? data's type has just been set to an nsCString a few lines previously.

Turns out it does not. It asserts as you probably expected. I did not notice because in my testing FireArrayBufferDataEvent() always delivers the received data as an array already.
I replaced this with a MOZ_ASSERT(!mFilter) for now (based on your request to an assert in line 318). Do I need to look into adding support for nsCString for the filter or convert nsCString's into array?
https://reviewboard.mozilla.org/r/34843/#review38539

> My guess is that this is historic artifact. There used to be #ifdef'ed code for external linkage which this whole filter would turn into a dummy implementation.
> I can try to clean that up now where the #ifdef's are gone.

So I tried to pursue this route for a couple of hours without much success. I needed to move the STUNTCPSocketFilter class into the header so it becomes visible to media bridge. But that then results in mess of dependencies getting pulled in from our ICE stack. It's probably doable to refactor this. But for now I would like to avoid that extra hassle and hope it is not a blocker from your point of view.
Comment on attachment 8719011 [details]
MozReview Request: Bug 1244926: added TCP socket filter to only allow outgoing STUN.  r=jesup,mcmanus

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34843/diff/7-8/
Attachment #8719011 - Flags: review?(josh)
Attachment #8719011 - Flags: review?(ekr)
Attachment #8719011 - Flags: review?(ekr)
So the two open questions from the first review are:
- How would I kill the child process in case the filter decides that the child tried to send illegal data?
- Is it okay to not have support for the nsCString based functions in TCPSocketParent?
Flags: needinfo?(josh)
https://reviewboard.mozilla.org/r/34841/#review39109

::: media/mtransport/nr_socket_prsock.cpp:1498
(Diff revision 8)
>    if (!socket_child_) {
>      socket_child_ = socketChild;
> -    socket_child_->SetFilterName(nsCString("stun"));
> +    socket_child_->SetFilterName(nsCString(NS_NETWORK_SOCKET_FILTER_HANDLER_STUN_SUFFIX));
>    } else {
>      socketChild = nullptr;

Why is this "suffix"?
https://reviewboard.mozilla.org/r/34841/#review39109

> Why is this "suffix"?

Because "stun" is the suffix part for finding the filter implementation. What gets passed into SetFilterName gets appended to the generic filter prefix and an implementation for the combination of prefix + suffix gets looked up as contract ID by the parent. I guess the idea here was/is that potentially more/other filters then STUN could get implemented.
I only disliked that fact that "stun" was hard coded in several places, even though it has a hard dependency on for the contract ID.
https://reviewboard.mozilla.org/r/34841/#review39109

> Because "stun" is the suffix part for finding the filter implementation. What gets passed into SetFilterName gets appended to the generic filter prefix and an implementation for the combination of prefix + suffix gets looked up as contract ID by the parent. I guess the idea here was/is that potentially more/other filters then STUN could get implemented.
> I only disliked that fact that "stun" was hard coded in several places, even though it has a hard dependency on for the contract ID.

OK
(In reply to Nils Ohlmeier [:drno] from comment #20)
> So the two open questions from the first review are:
> - How would I kill the child process in case the filter decides that the
> child tried to send illegal data?

Returning false from IPDL message handlers (ie. RecvFoo) makes this happen automatically.

> - Is it okay to not have support for the nsCString based functions in
> TCPSocketParent?

If you don't need it, I think we should avoid trying to support it.
Flags: needinfo?(josh)
Comment on attachment 8719011 [details]
MozReview Request: Bug 1244926: added TCP socket filter to only allow outgoing STUN.  r=jesup,mcmanus

https://reviewboard.mozilla.org/r/34843/#review39527

::: dom/network/TCPSocketParent.h:55
(Diff revision 8)
>  
>    TCPSocketParent() {}
>  
>    virtual bool RecvOpen(const nsString& aHost, const uint16_t& aPort,
> -                        const bool& useSSL, const bool& aUseArrayBuffers) override;
> +                        const bool& useSSL, const bool& aUseArrayBuffers,
> +                        const nsCString& aFilter) override;

No need for this argument now that we're blocking it in the child.

::: dom/network/TCPSocketParent.cpp:152
(Diff revision 8)
>    }
>    return refcnt;
>  }
>  
> +nsresult
> +TCPSocketParent::SetFilter(const nsCString& aFilter)

We can go back to inlining this in RecvOpenBind when we remove the caller from RecvOpen.
Attachment #8719011 - Flags: review?(josh)
https://reviewboard.mozilla.org/r/34843/#review38539

> That is actually a good question. I guess the assumption is that something really nasty is going on if the filter kicks in. Killing the child process sounds like a reasonable and safe choice in that case. How would/could I accomplish that?

The return false does that. It looks like newer code uses return true, which does not. I leave that judgement call in your hands.
https://reviewboard.mozilla.org/r/34843/#review38539

> The return false does that. It looks like newer code uses return true, which does not. I leave that judgement call in your hands.

I think the child crash makes it more clear to the end user that the website just did something pretty bad.
Comment on attachment 8719011 [details]
MozReview Request: Bug 1244926: added TCP socket filter to only allow outgoing STUN.  r=jesup,mcmanus

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34843/diff/8-9/
Attachment #8719011 - Flags: review?(josh)
Comment on attachment 8719011 [details]
MozReview Request: Bug 1244926: added TCP socket filter to only allow outgoing STUN.  r=jesup,mcmanus

https://reviewboard.mozilla.org/r/34843/#review41571

Someone from the necko team will need to review the changes to nsSocketTransportService2.cpp.
Attachment #8719011 - Flags: review?(josh)
Comment on attachment 8719011 [details]
MozReview Request: Bug 1244926: added TCP socket filter to only allow outgoing STUN.  r=jesup,mcmanus

r+ from mcmanus via IRC
Comment on attachment 8719011 [details]
MozReview Request: Bug 1244926: added TCP socket filter to only allow outgoing STUN.  r=jesup,mcmanus

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34843/diff/9-10/
Attachment #8719011 - Attachment description: MozReview Request: Bug 1244926: added TCP socket filter to only allow outgoing STUN → MozReview Request: Bug 1244926: added TCP socket filter to only allow outgoing STUN. r=jesup,mcmanus
Attachment #8719011 - Flags: review?(mcmanus)
Attachment #8719011 - Flags: review?(mcmanus) → review+
https://hg.mozilla.org/mozilla-central/rev/11edf75d48c6
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
We may roll e10s out in 47 to a subset of users, should we uplift this to get it in?
Flags: needinfo?(drno)
As this:
- only affects users on WebRTC calls which happen to over TURN TCP relayed connections
- and only adds extra security for e10s users (compared to non-e10s) and doesn't actually fix a real problem
I think it is not worth taking the extra risk of an uplift.
Flags: needinfo?(drno)
Blocks: 1279514
Blocks: 1275216
Blocks: 1285318
See Also: → 1285330
See Also: → 1287225
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: