870660 - Packet filter for UDP e10s

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[u459114]

If the content process is compromised/ attacked, it could send arbitrary UDP packets to anywhere in the network that is accessible to the phone. Construct primitive packet filtering on the parent process side to block attack from content process

We put foucs on UDP here since application protocols in WebRTC, RTP/ICE, are base on UDP transport layer.

Draft from EKR:
* A socket maintains two tables:
**An outstanding STUN transaction table
**A "permissions" table of accepted remote addresses 
*When a content process tries to send a non-STUN formatted packet, the socket rejects it unless the remote address is in the permissions table
*When a content process sends a STUN-formatted packet, it gets transmitted and added to the outstanding STUN transaction table
*When packet is received, it is checked against the outstanding STUN transaction table. If a transaction completes, then the address is added to the permissions table.

Comment 1

[Shian-Yow Wu [:swu]]

If not only WebRTC has this concern, maybe we can think a further step to discuss a generic solution to cover this issue, to benefit to more applications.  It's fine too to have a short-term solution for WebRTC here and file another bug for general solution.

The question depends on priority of this issue(how difficult the content process will been compromised).  Glad to see if someone can provide attack scenarios or comments.

Comment 2

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Since we are exposing UDP socket to content process, I think we should do more general than just filter packet for WebRTC. I'd like to propose an approach: When someone try to create a UDPSocket in content process, he needs to provide the reason the UDPSocket is created for. For example, we may create a UDPSocket for WebRTC with code like |new UDPSocket("webrtc")|.

In chrome side, when a NeckoParent is asked to create a UDPSocketParent, it will try to get the a filter service for the reason ("webrtc" in our case). A filter service is a class implements a specific interface (assuming nsIFilterService) and has contract id like "mozilla/network/filter;1?type=webrtc" for example. So NeckoParent can get the filter service for webrtc through do_GetService(). If NeckoParent fail to get filter service, it will return null and kill the content process.

After UDPSocketParent is created, it keeps reference to the filter service. Each time when UDPSocketChild asks UDPSocketParent to send a packet, UDPSocketParent will pass the target IP/port and pointer to the data buffer to the filter service. If the filter service returns true, UDPSocketParent will send the packet, or it will return failure message to child. We can implement the logic and described in comment 0 as a filter service.


Proposed nsIFilterService interface

interface nsIFilterService
{
  [noscript]
  boolean checkPacket(in String host, in short aPort, in charPtr aBuf, in unsigned long aLength);
}

Comment 3

[Eric Rescorla (:ekr)]

Having a service here seems kind of like overkill, since it's not clear we have
another example of this service. Additionally, what happens if the child
process lies about the service they are implementing.

I suggest we just hardcode the filter now. We can always turn it into a service
later.

Was your thought that a filter service would represent a single socket? Or would
be global? I suggest the former. Note that you need to know send versus recv.

class SocketFilter {
public:
 enum { SF_INCOMING, SF_OUTGOING } Direction;

 SocketFilter(const PRNetAddr& local_addr);
 bool FilterPacket(
                   const PRNetAddr& remote_addr,
                   const uint8_t* data,
                   size_t length,
                   Direction direction
                  );
};

Comment 4

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: WIP Part 1: Add packet filter in UDPSocketParent.

Comment 5

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 798826 [details] [diff] [review]
Patch: WIP Part 1: Add packet filter in UDPSocketParent.

Review of attachment 798826 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/ipc/NeckoParent.cpp
@@ +360,5 @@
>    if (UsingNeckoIPCSecurity() &&
>        !AssertAppProcessPermission(Manager(), "udp-socket")) {
> +    // Content doesn't have udp-socket permission, try to apply filter.
> +    nsAutoCString contractId(NS_NETWORK_UDP_SOCKET_FILTER_HANDLER_PREFIX);
> +    contractId.Append(aFilter);

Will need to do a nullptr check after mark |filter| as nullable in PNecko.ipdl.

@@ +362,5 @@
> +    // Content doesn't have udp-socket permission, try to apply filter.
> +    nsAutoCString contractId(NS_NETWORK_UDP_SOCKET_FILTER_HANDLER_PREFIX);
> +    contractId.Append(aFilter);
> +    nsCOMPtr<nsIUDPSocketFilterHandler> filterHandler = do_GetService(contractId.get());
> +    if (!filterHandler) {

Checking for a valid packet filter should be placed before AssertAppProcessPermission, otherwise the child process will be killed immediately if App permission check is failed.

::: netwerk/ipc/PNecko.ipdl
@@ +56,4 @@
>  
>    PWebSocket(PBrowser browser, SerializedLoadContext loadContext);
>    PTCPServerSocket(uint16_t localPort, uint16_t backlog, nsString binaryType);
> +  PUDPSocket(nsCString host, uint16_t port, nsCString filter);

Should mark |filter| as nullable because there might not be a filter outside the usage of WebRTC.

Comment 6

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Thanks for the comment, Shih-Chiang.

(In reply to Shih-Chiang Chien [:schien] from comment #5)
> Comment on attachment 798826 [details] [diff] [review]
> Patch: WIP Part 1: Add packet filter in UDPSocketParent.
> 
> Review of attachment 798826 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/ipc/NeckoParent.cpp
> @@ +360,5 @@
> >    if (UsingNeckoIPCSecurity() &&
> >        !AssertAppProcessPermission(Manager(), "udp-socket")) {
> > +    // Content doesn't have udp-socket permission, try to apply filter.
> > +    nsAutoCString contractId(NS_NETWORK_UDP_SOCKET_FILTER_HANDLER_PREFIX);
> > +    contractId.Append(aFilter);
> 
> Will need to do a nullptr check after mark |filter| as nullable in
> PNecko.ipdl.
> 
> @@ +362,5 @@
> > +    // Content doesn't have udp-socket permission, try to apply filter.
> > +    nsAutoCString contractId(NS_NETWORK_UDP_SOCKET_FILTER_HANDLER_PREFIX);
> > +    contractId.Append(aFilter);
> > +    nsCOMPtr<nsIUDPSocketFilterHandler> filterHandler = do_GetService(contractId.get());
> > +    if (!filterHandler) {
> 
> Checking for a valid packet filter should be placed before
> AssertAppProcessPermission, otherwise the child process will be killed
> immediately if App permission check is failed.
> 

I just realized the AssertAppProcessPermission kills content process when content process doesn't have the required permission. I will arrange the flow or try to test permission without make content be killed.

> ::: netwerk/ipc/PNecko.ipdl
> @@ +56,4 @@
> >  
> >    PWebSocket(PBrowser browser, SerializedLoadContext loadContext);
> >    PTCPServerSocket(uint16_t localPort, uint16_t backlog, nsString binaryType);
> > +  PUDPSocket(nsCString host, uint16_t port, nsCString filter);
> 
> Should mark |filter| as nullable because there might not be a filter outside
> the usage of WebRTC.

IPDL doesn't allow to use nullable on nsCString, empty string may also be a choice to stand for an invalid value.

I am not sure if we should let content create a UDPSocket without specifying a filter before we implemented UDP Socket APU. I thought 'udp-socket' should only be granted to an app which dose use the UDP API.

Comment 7

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Patrick Wang [:kk1fff] from comment #6)
> 
> IPDL doesn't allow to use nullable on nsCString, empty string may also be a
> choice to stand for an invalid value.
  ^ "be a method"

Comment 8

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: WIP Part 1: Add packet filter in UDPSocketParent.

Comment 9

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: WIP Part 2: implement packet filter for stun.

Comment 10

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Comment on attachment 800128 [details] [diff] [review]
Patch: WIP Part 1: Add packet filter in UDPSocketParent.

Providing packet filter in UDPSocket's parent side to limit the usage of UDPSocket in content. In content process, if the process doesn't have permission to create a UDPSocket, creating a UDP socket child will need to provide the purpose of this socket, and in parent side, UDPSocketParent will ask packet filter if a packet can be sent when UDPSocketChild wants to send a packet.

This patch can only filter the UDP packets sent with NetAddr. Filtering packets sent with host name which needs DNS resolve haven't done yet.

Comment 11

[Eric Rescorla (:ekr)]

Comment on attachment 800129 [details] [diff] [review]
Patch: WIP Part 2: implement packet filter for stun.

Review of attachment 800129 [details] [diff] [review]:
-----------------------------------------------------------------

Patrick,

A more complex filter is needed here. We need three states for
an address pair:

1. Unknown
2. An outgoing STUN message has been seen (PENDING)
3. A complete STUN transaction has been seen. (COMPLETED)


Your filter logic should be approximately the following




               STUN-OUT         OUT         STUN-IN         IN
UNKNOWN            Pass        Drop            Drop        Drop
            S = PENDING

PENDING            Pass        Drop    if XID match {      Drop
                                        Pass
                                        S = COMPLETED
                                       } else {
				        Drop
                                       }

COMPLETED          Pass        Pass             Pass       Pass



When you move to PENDING you need to record the transacton ID.
If a STUN repsonse comes in with a separate transaction ID you
drop it.

Comment 12

[Patrick McManus [:mcmanus]]

Comment on attachment 800128 [details] [diff] [review]
Patch: WIP Part 1: Add packet filter in UDPSocketParent.

jason is probably the best person to read this

Comment 13

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: WIP Part 2: implement packet filter for stun.

Adding checking transaction id.

Comment 14

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: WIP Part 2: implement packet filter for stun.

Comment 15

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: WIP Part 1: Add packet filter in UDPSocketParent.

Filter packets sent with hostname (instead of address). Resolve hostname in UDPSocketParent.

Comment 16

[Eric Rescorla (:ekr)]

Patrick,

Is there any current application which requires us to send packets
with hostname? WebRTC certainly does not. I would suggest this is
a phase 2 thing.

Comment 17

[Eric Rescorla (:ekr)]

Comment on attachment 811976 [details] [diff] [review]
Patch: WIP Part 1: Add packet filter in UDPSocketParent.

Review of attachment 811976 [details] [diff] [review]:
-----------------------------------------------------------------

I haven't really gone over the code, but I'm sort of wondering why this
needs to be an XPCOM component. Is there a reason it can't be an ordinary
C++ class?

Comment 18

[Eric Rescorla (:ekr)]

Comment on attachment 811086 [details] [diff] [review]
Patch: WIP Part 2: implement packet filter for stun.

Review of attachment 811086 [details] [diff] [review]:
-----------------------------------------------------------------

This looks mostly technically correct.

Overall: Please follow the Googley mtransport code conventions for things in mtransport.

::: media/mtransport/nr_socket_prsock.cpp
@@ +867,5 @@
>      err_ = true;
>      MOZ_ASSERT(false, "Failed to create UDPSocketChild");
>    }
>  
> +  socket_child_->SetFilterName(nsCString("stun"));

What happens if you don't do this? Do you get no filter or some default filter? Remember we don't trust the child process.

::: media/mtransport/stun_udp_socket_filter.cpp
@@ +26,5 @@
> +  if (nr_transport_addr_fmt_addr_string(&addr)) {
> +    return false;
> +  }
> +  result = addr.as_string;
> +  return true;

This seems fairly expensive to use for every packet. Is there a reason we can't use NetAddr as the table lookup key directly?

@@ +49,5 @@
> +    for (int i = 0; i < 12; ++i) {
> +      if (id.octet[i] != rhs.id.octet[i]) {
> +        return id.octet[i] < rhs.id.octet[i];
> +      }
> +    }

I believe you can use memcmp() here.

@@ +80,5 @@
> +};
> +
> +NS_IMPL_ISUPPORTS1(nsStunUDPSocketFilter, nsIUDPSocketFilter)
> +
> +nsStunUDPSocketFilter::nsStunUDPSocketFilter() { }

Please have the ctor initialize the sets even though the defaults are sensible.

@@ +82,5 @@
> +NS_IMPL_ISUPPORTS1(nsStunUDPSocketFilter, nsIUDPSocketFilter)
> +
> +nsStunUDPSocketFilter::nsStunUDPSocketFilter() { }
> +
> +nsStunUDPSocketFilter::~nsStunUDPSocketFilter() { }

You can use the automatic dtor.

@@ +88,5 @@
> +NS_IMETHODIMP
> +nsStunUDPSocketFilter::SetLocalAddress(const mozilla::net::NetAddr *aLocalAddr,
> +                                       bool *aResult)
> +{
> +  *aResult = true;

Do something to indicate that aLocalAddr is unused.

@@ +127,5 @@
> +  // Check if this is a stun message.
> +  if (nr_is_stun_message(reinterpret_cast<UCHAR*>(const_cast<uint8_t*>(aData)), aLen) > 0) {
> +    // Check if we had sent any stun request to this destination. If we had sent a request
> +    // to this host, we check the transaction id, and we can add this address to whitelist.
> +    std::set<PendingSTUNRequest>::iterator i =

it rather than i

@@ +168,5 @@
> +NS_IMPL_ISUPPORTS1(nsStunUDPSocketFilterHandler, nsIUDPSocketFilterHandler)
> +
> +nsStunUDPSocketFilterHandler::nsStunUDPSocketFilterHandler() { }
> +
> +nsStunUDPSocketFilterHandler::~nsStunUDPSocketFilterHandler() { }

Is there a reason we can't use the defaults?

Comment 19

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 2: implement packet filter for stun.

Address comment 18.

Comment 20

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Eric Rescorla (:ekr) from comment #16)
> Patrick,
> 
> Is there any current application which requires us to send packets
> with hostname? WebRTC certainly does not. I would suggest this is
> a phase 2 thing.

UDPSocket provides an IPC protocol to send packet with hostname. To prevent child process from using hostname to send arbitery packets, packets with hostname need to pass the filter. But I am happy to remove them and simply block the packets with hostname, since it makes the logic more complex, and I didn't find a way to reuse the host resolving code which is already written in UDPSocket.

Comment 21

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 1: Add packet filter in UDPSocketParent.

Comment 22

[Jason Duell]

Comment on attachment 820285 [details] [diff] [review]
Part 1: Add packet filter in UDPSocketParent.

Review of attachment 820285 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry I'm confused.

All of this work seems to be based on top of some other work that hasn't landed in mozilla central yet--please mark this bug dependent on the basic e10s UDP socket bug.

What's the security model here?  Are we allowing privileged apps to open arbitrary UDP sockets, but limiting non-privileged ones to only using some whitelist ("webrtc", etc) of approved UDP protocols/ports, and we sniff the traffic to make sure nothing else is going on?

::: netwerk/base/public/nsIUDPSocketFilter.idl
@@ +9,5 @@
> +native NetAddr(mozilla::net::NetAddr);
> +[ptr] native NetAddrPtr(mozilla::net::NetAddr);
> +
> +[scriptable, uuid(24f20de4-09e9-42ab-947a-0d6a3d103d59)]
> +interface nsIUDPSocketFilter : nsISupports

add comment explaining at a high level what the class does.

Comment 23

[Eric Rescorla (:ekr)]

Jason,

I don't have an opinion on what privileged apps can do.

The browser (which is what I care about at this moment) should be able
to sockets that filter on whether STUN checks have completed or not.

Comment 24

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 1: Add packet filter in UDPSocketParent.

Adding comment to IDL.

Comment 25

[Jason Duell]

Comment on attachment 820950 [details] [diff] [review]
Patch: Part 1: Add packet filter in UDPSocketParent.

Review of attachment 820950 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.  I only have a question about whether we still need to handle cases where mFilter == null if we're now requiring it during construction.  Maybe I'm misunderstanding and that's still allowed.

::: dom/network/src/UDPSocketParent.cpp
@@ +1,3 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */

might as well add vim/emacs modelines here, and to any other files in this patch that are missing it.

@@ +97,5 @@
>  
>    net::NetAddr localAddr;
>    mSocket->GetAddress(&localAddr);
>  
> +  if (mFilter) {

I'm not clear how we could create sockets w/o filter any more--your patch seems to make them required?

@@ +129,5 @@
>  {
>    NS_ENSURE_TRUE(mSocket, true);
> +  if (mFilter) {
> +    // TODO, filter packets that are sent with hostname.
> +    // Block the traffic before we can filter this kind of packets.

I assume this is followup work?

@@ +152,5 @@
>                                       const mozilla::net::NetAddr& aAddr)
>  {
>    NS_ENSURE_TRUE(mSocket, true);
> +
> +  return SendDataInternal(aData, aAddr);

Not clear from patch why you need the new function--this appears to be the only call site.  OK if you've got some reason for it.

@@ +206,5 @@
>  
>    const char* buffer = data.get();
>    uint32_t len = data.Length();
>  
> +  if (mFilter) {

and again I'm not clear how filter could be null, or if we should allow traffic if it is.

@@ +258,5 @@
> +  NS_ENSURE_TRUE(mSocket, true);
> +
> +  uint32_t count;
> +  nsresult rv;
> +  if (mFilter) {

and again :)

::: dom/network/src/UDPSocketParent.h
@@ +19,5 @@
>  public:
>    NS_DECL_THREADSAFE_ISUPPORTS
>    NS_DECL_NSIUDPSOCKETLISTENER
>  
>    UDPSocketParent() : mIPCOpen(true) {}

Do we still need the empty arg constructor?  When would it be used if we now require aFilter for the creation to succeed?

::: netwerk/base/public/nsIUDPSocketFilter.idl
@@ +1,1 @@
> +/* -*- Mode: IDL; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */

emacs modeline ofset is wrong, and you need a vim modeline too:

   https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Mode_Line

@@ +11,5 @@
> +
> +/**
> + * A Filter is created and runs in parent side of UDP Socket to filter all
> + * UDP packets that content process asks parent to send. In content side
> + * the UDP socket can be created with name of a filter, and send packets

Change comment to just this?
 
"Filters are created and run on the parent, and filter all UDP packets, both ingoing and outgoing. The child must specify the name of a recognized filter in order to create a UDP socket."

@@ +14,5 @@
> + * UDP packets that content process asks parent to send. In content side
> + * the UDP socket can be created with name of a filter, and send packets
> + * with the rule of the specified protocol.
> + */
> +[scriptable, uuid(24f20de4-09e9-42ab-947a-0d6a3d103d59)]

why is the interface scriptable, yet all of its methods are [noscript]?  Is there a use case for that?

::: netwerk/ipc/NeckoParent.cpp
@@ +382,5 @@
> +                                   const nsCString& aFilter)
> +{
> +  UDPSocketParent* p;
> +
> +  // Try to apply a filter.

// Only allow socket if it specifies a valid packet filter.

@@ +393,5 @@
> +      nsCOMPtr<nsIUDPSocketFilter> filter;
> +      if (NS_SUCCEEDED(filterHandler->NewFilter(getter_AddRefs(filter)))) {
> +        p = new UDPSocketParent(filter);
> +      } else {
> +        printf_stderr("Cannot create filter that content specified. filter name: %s.", aFilter.get());

break line if it's over 80 chars (not sure if it is, but looks like it in my browser).

Is there a real scenario where NewFilter might fail?  It seem like we're basically guaranteed it will work if we find a handler for aFilter?   If there's some reason it might fail, you might as well capture rv and print it in the "Cannot create filter" message.

@@ +396,5 @@
> +      } else {
> +        printf_stderr("Cannot create filter that content specified. filter name: %s.", aFilter.get());
> +      }
> +    } else {
> +      printf_stderr("Content doesn't have a valid filter.");

print aFilter value in the error output.

@@ +408,2 @@
>    p->AddRef();
>    return p;

If you initialize p = nsnull at the top of the function you can skip the if(!p) logic and just do

  NS_IF_ADDREF(p);
  return p;

Comment 26

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Jason Duell (:jduell) from comment #25)
> Looks good.  I only have a question about whether we still need to handle
> cases where mFilter == null if we're now requiring it during construction. 
> Maybe I'm misunderstanding and that's still allowed.

I don't want to break the normal function in UDPSocket, since WebAPI for UDPSocket is likely going to reuse this class. I believe it would add other code to create a UDPSocketParent without a filter (probably permission or something like that), so I leave the original code unmodified if possible. But creating a UDPSocketParent without a filter is not allowed for now.

Comment 27

[Jason Duell]

> I don't want to break the normal function in UDPSocket, since WebAPI for UDPSocket is likely going to reuse this class

I think I'd prefer to just remove the non-filter constructor and enforce having a filter, and then if/when webAPI needs to do things differently, we can change it.  I hate leaving in dead code paths unless we're 100% sure we'll be using them very soon, and it doesn't sound like we're clear on how WebAPI will work yet with this.  

But if you really think we're likely to use the empty constructors again soon, you can leave them in.  Implementor's choice :)

Comment 28

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Jason Duell (:jduell) from comment #27)
> I think I'd prefer to just remove the non-filter constructor and enforce
> having a filter, and then if/when webAPI needs to do things differently, we
> can change it.  I hate leaving in dead code paths unless we're 100% sure
> we'll be using them very soon, and it doesn't sound like we're clear on how
> WebAPI will work yet with this.  

Okay. Yeah, I agree. I will remove them and make the UDPSocketParent only works with a filter. (I was pretty sure because I was assignee of that bug. But now other people took that bug, so I am not that sure if he would use this now.)

Comment 29

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 1: Add packet filter in UDPSocketParent.

Address comment 25 and removed the code of handling non-filter case.

Comment 30

[Jason Duell]

Patrick:  I'm in the middle of looking over the patch--don't land it yet.

Comment 31

[Jason Duell]

Comment on attachment 825221 [details] [diff] [review]
Patch: Part 1: Add packet filter in UDPSocketParent.

Review of attachment 825221 [details] [diff] [review]:
-----------------------------------------------------------------

Just a couple nit to fix.  No need to re-review once you've fixed them (you can mark next version of patch +r).

::: dom/network/src/UDPSocketParent.cpp
@@ +131,5 @@
>    NS_ENSURE_TRUE(mSocket, true);
> +  NS_ASSERTION(mFilter, "No packet filter");
> +  // TODO, Bug 933102, filter packets that are sent with hostname.
> +  // Block the traffic before we can filter this kind of packets.
> +  return true;

// TODO, Bug 933102, filter packets that are sent with hostname.
// Until then we simply throw away packets that are sent to a hostname.

@@ +135,5 @@
> +  return true;
> +
> +#if 0
> +  // This piece of code is not using until allowing UDPSocketParent to work
> +  // without packet filter or being able to filter packets with hostname.

// Enable this once we have filtering working with hostname delivery.

@@ +143,5 @@
>                                aData.Length(), &count);
>    mozilla::unused <<
>        PUDPSocketParent::SendCallback(NS_LITERAL_CSTRING("onsent"),
>                                       UDPSendResult(rv),
>                                       NS_LITERAL_CSTRING("connected"));

You had abstracted this into a single SendInternal function in last patch. I see now you're doing the same thing (albeit in one case within #if 0) but in your earlier patch you didn't replace both sites with a call to SendInternal--so you *do* have 2 uses. So go ahead and add SendInternal after all, to keep the code common.

@@ +145,5 @@
>        PUDPSocketParent::SendCallback(NS_LITERAL_CSTRING("onsent"),
>                                       UDPSendResult(rv),
>                                       NS_LITERAL_CSTRING("connected"));
>    NS_ENSURE_SUCCESS(rv, true);
>    NS_ENSURE_TRUE(count > 0, true);

I saw you just added the two ENSURE macros here.  Since you're having them return 'true', the only effect here will be to call NS_WARNING.  Use MOZ_ASSERT if this is something that should never happen.  If it can happen but things should just continue, what you've got is fine.

Comment 32

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Jason Duell (:jduell) from comment #31)
> @@ +143,5 @@
> >                                aData.Length(), &count);
> >    mozilla::unused <<
> >        PUDPSocketParent::SendCallback(NS_LITERAL_CSTRING("onsent"),
> >                                       UDPSendResult(rv),
> >                                       NS_LITERAL_CSTRING("connected"));
> 
> You had abstracted this into a single SendInternal function in last patch. I
> see now you're doing the same thing (albeit in one case within #if 0) but in
> your earlier patch you didn't replace both sites with a call to
> SendInternal--so you *do* have 2 uses. So go ahead and add SendInternal
> after all, to keep the code common.

Actually they are different: SendDataInternal sends data directly with IP, but RecvData sends data with hostname. SendDataInternal was written to be called in the callback of DNS, but we decided to add the ability to filter packet with hostname in another bug, SendDataInternal has only one caller now.

Comment 33

[Jason Duell]

> Actually they are different

OK then no need to make them a common function :)

Comment 34

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 1: Add packet filter in UDPSocketParent. (r=jduell)

Comment 35

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 2: implement packet filter for stun

Comment 36

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 3: Test case.

Test case for packet filter:
1. Not allowing to send or receive a non-STUN packet to or from an non-whilelisted address.
2. Allowing to send a STUN packet.
3. Not allowing to receive a STUN packet if its ID doesn't match any pending ID.
4. Not allowing to receive a STUN packet if its ID matches one of pending IDs but the address does not match the address corresponding to the pending ID.
5. Allowing to receive a STNU packet if its ID matches a pending ID, and this would make the remote address whitelisted.

Comment 37

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 2: implement packet filter for stun

Comment 38

[Eric Rescorla (:ekr)]

Comment on attachment 831465 [details] [diff] [review]
Patch: Part 2: implement packet filter for stun

Review of attachment 831465 [details] [diff] [review]:
-----------------------------------------------------------------

This looks generally good. Please clean up nits and re-submit.

::: media/mtransport/stun_udp_socket_filter.cpp
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +extern "C" {
> +#include "stun.h"
> +}

Please put this after the system includes.

@@ +7,5 @@
> +
> +#include <string>
> +#include <set>
> +
> +#include "nsAutoPtr.h"

Should not be needed (I don't see any use of AutoPtr below)

@@ +14,5 @@
> +#include "nr_socket_prsock.h"
> +
> +namespace {
> +
> +class NetAddressAdapter {

Everything in this directory should use Google style. Please reformat all below.
I noticed at least:

- public should be indented one space
- initializer lists should have commas at the end.
- don't name arguments "aFoo"; use foo or foo_bar

@@ +38,5 @@
> +    return (*this < rhs) || (rhs < *this);
> +  }
> +
> +private:
> +  uint32_t addr_;

Can these be const?

@@ +63,5 @@
> +
> +class STUNUDPSocketFilter : public nsIUDPSocketFilter {
> +public:
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIUDPSOCKETFILTER

Please put these after ctor.

@@ +85,5 @@
> +
> +NS_IMPL_ISUPPORTS1(STUNUDPSocketFilter, nsIUDPSocketFilter)
> +
> +NS_IMETHODIMP
> +STUNUDPSocketFilter::SetLocalAddress(const mozilla::net::NetAddr*,

What does this do? It seems to be a no-op.

@@ +98,5 @@
> +                                  uint32_t len,
> +                                  int32_t direction,
> +                                  bool *result) {
> +  switch (direction) {
> +  case nsIUDPSocketFilter::SF_INCOMING:

Indent inside switch.

@@ +147,5 @@
> +
> +  // Check if it is a stun packet. If yes, we put it into a pending list and wait for
> +  // response packet.
> +  if (nr_is_stun_message(reinterpret_cast<UCHAR*>(const_cast<uint8_t*>(data)), len) == 3) {
> +    pending_requests_.insert(PendingSTUNRequest(*remote_addr, ((nr_stun_message_header*)data)->id));

C++-style cast please.

@@ +164,5 @@
> +  nsIUDPSocketFilter *ret = new STUNUDPSocketFilter();
> +  if (!ret) {
> +    return NS_ERROR_OUT_OF_MEMORY;
> +  }
> +  nsCOMPtr<nsIUDPSocketFilter>(ret).forget(result);

Why not just directly addref?

Comment 39

[Eric Rescorla (:ekr)]

Comment on attachment 831461 [details] [diff] [review]
Patch: Part 3: Test case.

Review of attachment 831461 [details] [diff] [review]:
-----------------------------------------------------------------

Still needs a bit of cleanup.

::: media/mtransport/test/ice_unittest.cpp
@@ +903,5 @@
>  
> +class PacketFilterTest : public ::testing::Test {
> + public:
> +  PacketFilterTest():
> +    filter_(nullptr) {}

Move up one line.

@@ +917,5 @@
> +                    bool expected_result) {
> +    mozilla::net::NetAddr addr;
> +    MakeNetAddr(&addr, from_addr, from_port);
> +    bool result;
> +    nsresult rv = filter_->FilterPacket(&addr, data, len, nsIUDPSocketFilter::SF_INCOMING, &result);

Line too long

@@ +918,5 @@
> +    mozilla::net::NetAddr addr;
> +    MakeNetAddr(&addr, from_addr, from_port);
> +    bool result;
> +    nsresult rv = filter_->FilterPacket(&addr, data, len, nsIUDPSocketFilter::SF_INCOMING, &result);
> +    EXPECT_EQ(rv, NS_OK);

ASSERT_EQ(NS_OK, rv)

@@ +919,5 @@
> +    MakeNetAddr(&addr, from_addr, from_port);
> +    bool result;
> +    nsresult rv = filter_->FilterPacket(&addr, data, len, nsIUDPSocketFilter::SF_INCOMING, &result);
> +    EXPECT_EQ(rv, NS_OK);
> +    EXPECT_EQ(result, expected_result);

Let's do ASSERT_EQ here.

@@ +930,5 @@
> +    MakeNetAddr(&addr, to_addr, to_port);
> +    bool result;
> +    nsresult rv = filter_->FilterPacket(&addr, data, len, nsIUDPSocketFilter::SF_OUTGOING, &result);
> +    EXPECT_EQ(rv, NS_OK);
> +    EXPECT_EQ(result, expected_result);

Same EXPECT->ASSERT changes as above

@@ +1346,5 @@
>    HasLowerPreference("1", "5");
>    HasLowerPreference("5", "4");
>  }
>  
> +TEST_F(PacketFilterTest, TestSendNonStunPacket) {

These lines all look super-long.

@@ +1347,5 @@
>    HasLowerPreference("5", "4");
>  }
>  
> +TEST_F(PacketFilterTest, TestSendNonStunPacket) {
> +  const char *data = "12345abcde";

Let's start with unsigned chars to avoid the cast here and below.

@@ +1348,5 @@
>  }
>  
> +TEST_F(PacketFilterTest, TestSendNonStunPacket) {
> +  const char *data = "12345abcde";
> +  TestOutgoing(reinterpret_cast<const unsigned char*>(data), sizeof(data), 123, 45, false);

Please use a real address here and below.

@@ +1358,5 @@
> +}
> +
> +TEST_F(PacketFilterTest, TestSendStunPacket) {
> +  nr_stun_message *msg;
> +  ASSERT_EQ(nr_stun_build_req_no_auth(NULL, &msg), 0);

All ASSERT_EQ should have the expected thing first.

@@ +1383,5 @@
> +  nr_stun_message *msg;
> +  ASSERT_EQ(nr_stun_build_req_no_auth(NULL, &msg), 0);
> +  ASSERT_EQ(nr_stun_encode_message(msg), 0);
> +  TestOutgoing(msg->buffer, msg->length, 123, 45, true);
> +  TestIncoming(msg->buffer, msg->length, 123, 46, false);

Please do a test with a bogus address as well as a bogus port.

Comment 40

[Eric Rescorla (:ekr)]

Comment on attachment 831465 [details] [diff] [review]
Patch: Part 2: implement packet filter for stun

Review of attachment 831465 [details] [diff] [review]:
-----------------------------------------------------------------

This looks generally good. Please clean up nits and re-submit.

::: media/mtransport/stun_udp_socket_filter.cpp
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +extern "C" {
> +#include "stun.h"
> +}

Please put this after the system includes.

@@ +7,5 @@
> +
> +#include <string>
> +#include <set>
> +
> +#include "nsAutoPtr.h"

Should not be needed (I don't see any use of AutoPtr below)

@@ +14,5 @@
> +#include "nr_socket_prsock.h"
> +
> +namespace {
> +
> +class NetAddressAdapter {

Everything in this directory should use Google style. Please reformat all below.
I noticed at least:

- public should be indented one space
- initializer lists should have commas at the end.
- don't name arguments "aFoo"; use foo or foo_bar

@@ +38,5 @@
> +    return (*this < rhs) || (rhs < *this);
> +  }
> +
> +private:
> +  uint32_t addr_;

Can these be const?

@@ +63,5 @@
> +
> +class STUNUDPSocketFilter : public nsIUDPSocketFilter {
> +public:
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIUDPSOCKETFILTER

Please put these after ctor.

@@ +85,5 @@
> +
> +NS_IMPL_ISUPPORTS1(STUNUDPSocketFilter, nsIUDPSocketFilter)
> +
> +NS_IMETHODIMP
> +STUNUDPSocketFilter::SetLocalAddress(const mozilla::net::NetAddr*,

What does this do? It seems to be a no-op.

@@ +98,5 @@
> +                                  uint32_t len,
> +                                  int32_t direction,
> +                                  bool *result) {
> +  switch (direction) {
> +  case nsIUDPSocketFilter::SF_INCOMING:

Indent inside switch.

@@ +119,5 @@
> +    return true;
> +  }
> +
> +  // Check if this is a stun message.
> +  if (nr_is_stun_message(reinterpret_cast<UCHAR*>(const_cast<uint8_t*>(data)), len) > 0) {

Please check for this to be a response.

@@ +146,5 @@
> +  }
> +
> +  // Check if it is a stun packet. If yes, we put it into a pending list and wait for
> +  // response packet.
> +  if (nr_is_stun_message(reinterpret_cast<UCHAR*>(const_cast<uint8_t*>(data)), len) == 3) {

Please check for this to be a request.

@@ +147,5 @@
> +
> +  // Check if it is a stun packet. If yes, we put it into a pending list and wait for
> +  // response packet.
> +  if (nr_is_stun_message(reinterpret_cast<UCHAR*>(const_cast<uint8_t*>(data)), len) == 3) {
> +    pending_requests_.insert(PendingSTUNRequest(*remote_addr, ((nr_stun_message_header*)data)->id));

C++-style cast please.

@@ +164,5 @@
> +  nsIUDPSocketFilter *ret = new STUNUDPSocketFilter();
> +  if (!ret) {
> +    return NS_ERROR_OUT_OF_MEMORY;
> +  }
> +  nsCOMPtr<nsIUDPSocketFilter>(ret).forget(result);

Why not just directly addref?

Comment 41

[Randell Jesup [:jesup] (needinfo me)]

This is needed for peerconnections to work for 1.3, nominating

Comment 42

[Randell Jesup [:jesup] (needinfo me)]

With these patches I've been able to get http://mozilla.github.com/webrtc-landing/data_test.html to work (sometimes) on helix, using a custom gecko (from inbound with these patches).  This does start an Opus m=audio channel, though it doesn't play it out.

I've been unable to get apprtc.appspot.com, talky.io, or our own chat sites to work - I never seem to get the peerconnection fully connected (perhaps missing the final ICE candidates?)  I also tried http://mozilla.github.com/webrtc-landing/audio.html (a modified version of pc_test.html for audio-only calls) - it seemed to connect once or twice, but I never heard audio.

Note that gum_test.html works reliably on this phone.  I see the same behavior on the Peaks.

Are there any other patches I should be using?  What are you using to test your patches?  We seem to be very close to getting this up reliably; we just need to push it over the goal line and then we can assess quality and performance.

Comment 43

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Eric Rescorla (:ekr) from comment #40)
> @@ +119,5 @@
> > +    return true;
> > +  }
> > +
> > +  // Check if this is a stun message.
> > +  if (nr_is_stun_message(reinterpret_cast<UCHAR*>(const_cast<uint8_t*>(data)), len) > 0) {
> 
> Please check for this to be a response.
> 

If we only allow response, will we block the binding request from remote during the connectivity check?

> @@ +146,5 @@
> > +  }
> > +
> > +  // Check if it is a stun packet. If yes, we put it into a pending list and wait for
> > +  // response packet.
> > +  if (nr_is_stun_message(reinterpret_cast<UCHAR*>(const_cast<uint8_t*>(data)), len) == 3) {
> 
> Please check for this to be a request.
>

Comment 44

[Eric Rescorla (:ekr)]

(In reply to Patrick Wang [:kk1fff] from comment #43)
> (In reply to Eric Rescorla (:ekr) from comment #40)
> > @@ +119,5 @@
> > > +    return true;
> > > +  }
> > > +
> > > +  // Check if this is a stun message.
> > > +  if (nr_is_stun_message(reinterpret_cast<UCHAR*>(const_cast<uint8_t*>(data)), len) > 0) {
> > 
> > Please check for this to be a response.
> > 
> 
> If we only allow response, will we block the binding request from remote
> during the connectivity check?

That will happen anyway because it will have an unrecognized transaction ID.
In any case, that is the desired property.


> > @@ +146,5 @@
> > > +  }
> > > +
> > > +  // Check if it is a stun packet. If yes, we put it into a pending list and wait for
> > > +  // response packet.
> > > +  if (nr_is_stun_message(reinterpret_cast<UCHAR*>(const_cast<uint8_t*>(data)), len) == 3) {
> > 
> > Please check for this to be a request.
> >

Comment 45

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Eric Rescorla (:ekr) from comment #44)
> That will happen anyway because it will have an unrecognized transaction ID.
> In any case, that is the desired property.
I see.

Comment 46

[Ivan Tsay (:ITsay)]

Since this is the blocker for peer connection, plus it for v1.3

Comment 47

[Eric Rescorla (:ekr)]

I think we should also verify that the *response* contains an integrity field.

That will stop us from spamming non-ICE STUN servers.

Comment 48

[Eric Rescorla (:ekr)]

(In reply to Eric Rescorla (:ekr) from comment #47)
> I think we should also verify that the *response* contains an integrity
> field.
> 
> That will stop us from spamming non-ICE STUN servers.

Let's do a new bug for this. See 941992

Comment 49

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 1: Add packet filter in UDPSocketParent. (r=jduell)

Fix according to comment 31 and removing SetLocalAddress() from filter interface. The function is not used in our filter implementation. Carrying r+ since the modification didn't affect logic of this patch.

Comment 50

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 2: implement packet filter for stun

Comment 51

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 3: Test case.

Comment 52

[Eric Rescorla (:ekr)]

Comment on attachment 8337257 [details] [diff] [review]
Patch: Part 2: implement packet filter for stun

Review of attachment 8337257 [details] [diff] [review]:
-----------------------------------------------------------------

r- for MOZ_CRASH issue. Otherwise lgtm

::: media/mtransport/stun_udp_socket_filter.cpp
@@ +22,5 @@
> +    switch(netaddr.raw.family) {
> +      case AF_INET:
> +        break;
> +      case AF_INET6:
> +        MOZ_CRASH("NetAddressAdapter for IPv6 is not implemented.");

You can't use MOZ_CRASH here since that would allow the child to crash the
parent.

However, you also need to make sure that IPv6 addresses never match anything.

Will discuss with you over IRC

@@ +45,5 @@
> +class PendingSTUNRequest {
> + public:
> +  PendingSTUNRequest(const NetAddressAdapter& netaddr, const UINT12 &id)
> +    : id_(id),
> +      net_addr_(netaddr) { }

{}

@@ +103,5 @@
> +  return NS_OK;
> +}
> +
> +bool
> +STUNUDPSocketFilter::filter_incoming_packet(const mozilla::net::NetAddr *remote_addr,

return value goes on same line.

@@ +129,5 @@
> +  return false;
> +}
> +
> +bool
> +STUNUDPSocketFilter::filter_outgoing_packet(const mozilla::net::NetAddr *remote_addr,

return value goes on same line.

Comment 53

[Eric Rescorla (:ekr)]

Comment on attachment 8337258 [details] [diff] [review]
Patch: Part 3: Test case.

Review of attachment 8337258 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm. Please add the one additional test, but you can r+ without re-review.

::: media/mtransport/test/ice_unittest.cpp
@@ +1427,5 @@
> +  const unsigned char data[] = "12345abcde";
> +
> +  // 123:45 is white-listed.
> +  TestOutgoing(data, sizeof(data), 123, 45, true);
> +  TestIncoming(data, sizeof(data), 123, 45, true);

Please verify that STUN non-requests (i.e., indications)
also pass in and out

Comment 54

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 2: implement packet filter for stun

Comparing NetAddr directly and allowing indications to pass in and out.

Comment 55

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Part 3: Test case. (r=ekr)

Add test case for indications.

Comment 56

[Eric Rescorla (:ekr)]

Comment on attachment 8337270 [details] [diff] [review]
Patch: Part 2: implement packet filter for stun

Review of attachment 8337270 [details] [diff] [review]:
-----------------------------------------------------------------

The new comparison code looks fine. Please spin a new patch w/o the indication
exception.

::: media/mtransport/stun_udp_socket_filter.cpp
@@ +23,5 @@
> +  }
> +  if (lhs.raw.family == AF_INET) {
> +    return lhs.inet.ip < rhs.inet.ip ? true : (lhs.inet.port < rhs.inet.port);
> +  }
> +  return memcmp(&lhs, &rhs, sizeof(mozilla::net::NetAddr)) < 0;

Please add a comment about why memcmp is ok. Note that it's going to produce spurious
non-matches.

@@ +139,5 @@
> +  }
> +
> +  if (nr_is_stun_indication_message(reinterpret_cast<UCHAR*>(const_cast<uint8_t*>(data)), len)) {
> +    return true;
> +  }

No, we don't want this. Indications get ordinary whitelist processing. Remove this and below.

Comment 57

[Eric Rescorla (:ekr)]

Comment on attachment 8337271 [details] [diff] [review]
Patch: Part 3: Test case. (r=ekr)

Review of attachment 8337271 [details] [diff] [review]:
-----------------------------------------------------------------

The idea here is that indications get the same processing as everyone else....

::: media/mtransport/test/ice_unittest.cpp
@@ +1469,5 @@
> +  msg->header.type = NR_STUN_MSG_BINDING_INDICATION;
> +  ASSERT_EQ(0, nr_stun_encode_message(msg));
> +  TestOutgoing(msg->buffer, msg->length, 123, 45, true);
> +  TestIncoming(msg->buffer, msg->length, 123, 45, true);
> +  ASSERT_EQ(0, nr_stun_message_destroy(&msg));

This isn't the test we want. We want to test that indications pass after you establish a whitelist entry.

Comment 58

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Eric Rescorla (:ekr) from comment #57)
> This isn't the test we want. We want to test that indications pass after you
> establish a whitelist entry.

I see. Thanks, EKR.

Comment 59

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 2: implement packet filter for stun

Comment 60

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 3: Test case.

Comment 61

[Eric Rescorla (:ekr)]

Comment on attachment 8337331 [details] [diff] [review]
Part 3: Test case.

Review of attachment 8337331 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm

Comment 62

[Eric Rescorla (:ekr)]

Comment on attachment 8337330 [details] [diff] [review]
Part 2: implement packet filter for stun

Review of attachment 8337330 [details] [diff] [review]:
-----------------------------------------------------------------

Patrick,

I just realized I gave you stupid feedback in my previous review
of Patch #2.

Instead of replacing NetAddressWrapper, I suggest you just revert
to the way it was and then add the rejection filter as commented
below. That way, the only way to get into the wrapper will be
if we have already established that we have an IPv4 address,
and so the MOZ_CRASH is safe.

Sorry about that. My bad.

::: media/mtransport/stun_udp_socket_filter.cpp
@@ +90,5 @@
> +                                  const uint8_t *data,
> +                                  uint32_t len,
> +                                  int32_t direction,
> +                                  bool *result) {
> +  switch (direction) {

Reject anything that's not IPv4.

Comment 63

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 2: implement packet filter for stun

Bring the backup back :)
And reject packets that are not associated with an IPv4 address.

Comment 64

[Eric Rescorla (:ekr)]

Comment on attachment 8337374 [details] [diff] [review]
Part 2: implement packet filter for stun

Review of attachment 8337374 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm

::: media/mtransport/stun_udp_socket_filter.cpp
@@ +18,5 @@
> + public:
> +  NetAddressAdapter(const mozilla::net::NetAddr& netaddr)
> +    : addr_(ntohl(netaddr.inet.ip)),
> +      port_(ntohs(netaddr.inet.port)) {
> +    switch(netaddr.raw.family) {

Suggest replacing this switch with:

MOZ_ASSERT(netaddr.raw.family == AF_INET);

Because the initializer isn't going to work anyway if we ar enon-inet.
But it's OK as-is.

Comment 65

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 4: Disable UDPSocket test

UDPSocket test works by sending a arbitrary data through a UDPSocket. In this bug, we removed the support for non-filter udp socket, so this test case does not work anymore. We still have peerconnection test in content to test UDPSocket IPC.

Comment 66

[Eric Rescorla (:ekr)]

Comment on attachment 8337839 [details] [diff] [review]
Part 4: Disable UDPSocket test

Review of attachment 8337839 [details] [diff] [review]:
-----------------------------------------------------------------

This is fine with me if it's fine with jduell...

Comment 67

[Jason Duell]

Comment on attachment 8337839 [details] [diff] [review]
Part 4: Disable UDPSocket test

Review of attachment 8337839 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/network/tests/unit_ipc/xpcshell.ini
@@ +7,5 @@
>  [test_tcpsocket_ipc.js]
>  [test_tcpserversocket_ipc.js]
>  [test_udpsocket_ipc.js]
>  run-sequentially = Uses hardcoded port, bug 903830.
> +skip-if = true
\ No newline at end of file

as discussed with kk1fff, we might as well just delete test_udpsocket_ipc.js.  We get the same coverage from test_peerConnection_* tests, so there's no need to keep it around.

Comment 68

[Jason Smith [:jsmith]]

Per discussion offline - Peer Connection support is a targeted feature, not committed, which means we don't need to block on this. We should try to get this in for 1.3 still, however.

Comment 69

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 2: implement packet filter for stun

1. Updating filter to allow incoming non-response packet if there's an outgoing request to this remote address.
2. Allow sending response to an address if a request have been sent from this address before.
3. Fix less operator of NetAddressAdapter, which causes test failure after 1, 2 have been addressed.

Comment 70

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 3: test case

Comment 71

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 4: remove test_udpsocket test case.

Comment 72

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Comment on attachment 8339825 [details] [diff] [review]
Part 2: implement packet filter for stun

Review of attachment 8339825 [details] [diff] [review]:
-----------------------------------------------------------------

still failure on try.

Comment 73

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 2: implement packet filter for stun

Test fails in previous patch because it allows send response outgoing packet only once: if one peer received an incoming requests twice, and it tries to send response for each request, the second response will not be allowed.

Comment 74

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 3: Test case.

Comment 75

[Eric Rescorla (:ekr)]

Comment on attachment 8339965 [details] [diff] [review]
Part 2: implement packet filter for stun

Review of attachment 8339965 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm with changes below.

::: media/mtransport/stun_udp_socket_filter.cpp
@@ +44,5 @@
> +      net_addr_(netaddr),
> +      is_id_set_(true) {}
> +
> +  PendingSTUNRequest(const NetAddressAdapter& netaddr)
> +    : id_(),

Let's do id_(0)

@@ +147,5 @@
> +        std::set<PendingSTUNRequest>::iterator it =
> +          pending_requests_.find(PendingSTUNRequest(*remote_addr, msg->id));
> +        if (it != pending_requests_.end()) {
> +          pending_requests_.erase(it);
> +          white_list_.insert(*remote_addr);

Let's remove it from response allowed too, since not needed.

Comment 76

[Eric Rescorla (:ekr)]

Comment on attachment 8339966 [details] [diff] [review]
Part 3: Test case.

Review of attachment 8339966 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm

Comment 77

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Comment on attachment 8339828 [details] [diff] [review]
Part 4: remove test_udpsocket test case.

Review of attachment 8339828 [details] [diff] [review]:
-----------------------------------------------------------------

Josh, we are removing test case for udp socket child. It tests sockets with arbitrary data, but we require all UDP packet to pass filter in this bug. Its test coverage is still covered by PeerConnention test. Since it's Thanksgiving in the US, not sure if Jason can review in these days. Could you help to review this patch?  Thanks!

Comment 78

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Eric Rescorla (:ekr) from comment #75)
> > +  PendingSTUNRequest(const NetAddressAdapter& netaddr)
> > +    : id_(),
> 
> Let's do id_(0)

The structure can't be initialized with integer, fail to compile.

Comment 79

[Eric Rescorla (:ekr)]

(In reply to Patrick Wang [:kk1fff] from comment #78)
> (In reply to Eric Rescorla (:ekr) from comment #75)
> > > +  PendingSTUNRequest(const NetAddressAdapter& netaddr)
> > > +    : id_(),
> > 
> > Let's do id_(0)
> 
> The structure can't be initialized with integer, fail to compile.

OK

Comment 80

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 1: Add packet filter in UDPSocketParent. v-check in (r=jduell)

Comment 81

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 2: implement packet filter for stun v-check in (r=ekr)

Comment 82

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 3: Test case. v-check in (r=ekr)

Comment 83

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 4: remove test_udpsocket test case. v-check in (r=jdm)

Comment 84

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

https://hg.mozilla.org/integration/b2g-inbound/rev/3f2e8b809678
https://hg.mozilla.org/integration/b2g-inbound/rev/c039384c1dfa
https://hg.mozilla.org/integration/b2g-inbound/rev/61fdab1c8ab6
https://hg.mozilla.org/integration/b2g-inbound/rev/35b2064e87d4

Comment 85

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3f2e8b809678
https://hg.mozilla.org/mozilla-central/rev/c039384c1dfa
https://hg.mozilla.org/mozilla-central/rev/61fdab1c8ab6
https://hg.mozilla.org/mozilla-central/rev/35b2064e87d4

Comment 86

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

sec-