Make the Pepper TCP open the firewall on Cros.

Make the Pepper TCP open the firewall on Cros.

When listening on a an interface that is not the loopback interface, an
app using the pepper sockets api would not be able to accept connections
from a remote host due to the Chromeos firewall.

This opens up a hole in the firewall as is done in the
chrome.sockets.tcpServer javascript api.

Add header.

BUG=489853
Committed: https://crrev.com/2f11040f9262036c19b1e7cdf05422d4ed92706b
Cr-Commit-Position: refs/heads/master@{#340831}

[avallee, 2015-06-30 03:17:16]

avallee@chromium.org changed reviewers:
+ asvitkine@chromium.org, sadrul@chromium.org

[avallee, 2015-06-30 03:17:16]

PTAL.

[sadrul, 2015-06-30 04:54:22]

sadrul@chromium.org changed reviewers:
- sadrul@chromium.org

[sadrul, 2015-06-30 04:54:22]

I am not a good reviewer for this. Can you find someone from
//c/b/r/pepper/OWNERS?

https://codereview.chromium.org/1213203004/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/pepper/pepper_socket_utils.h (right):

https://codereview.chromium.org/1213203004/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/pepper/pepper_socket_utils.h:68: #endif  //
defined(OS_CHROMEOS)
Maybe move these into pepper_socket_utils_chromeos.h|cc?

[elijahtaylor1, 2015-06-30 17:04:53]

elijahtaylor@chromium.org changed reviewers:
+ elijahtaylor@chromium.org

[elijahtaylor1, 2015-06-30 17:04:54]

One top-level request (which can be discussed by reviewers): this targets TCP
listen, which matches the JavaScript API AFAIK, but leaves UDP traffic out of
the loop.  Can we do a similar thing for UDP bind? (we might need to loop in a
security reviewer, jorgelo maybe?)

[avallee, 2015-07-08 17:50:23]

Patchset #4 (id:60001) has been deleted

[avallee, 2015-07-08 18:20:45]

avallee@chromium.org changed reviewers:
+ bbudge@chromium.org
- asvitkine@chromium.org

[avallee, 2015-07-08 18:20:46]

PTAL.

[bbudge, 2015-07-09 19:55:17]

https://codereview.chromium.org/1213203004/diff/100001/content/browser/render...
File content/browser/renderer_host/pepper/pepper_socket_utils.cc (right):

https://codereview.chromium.org/1213203004/diff/100001/content/browser/render...
content/browser/renderer_host/pepper/pepper_socket_utils.cc:137: namespace {
nit: Blank line here.

https://codereview.chromium.org/1213203004/diff/100001/content/browser/render...
content/browser/renderer_host/pepper/pepper_socket_utils.cc:142: {0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1};
nit: insert blank line

https://codereview.chromium.org/1213203004/diff/100001/content/browser/render...
content/browser/renderer_host/pepper/pepper_socket_utils.cc:144: if
(address.size() == net::kIPv4AddressSize && address[0] == 0x7f) {
This would be a little easier to reason about if you simplify the 'if'
conditions:

if (address.size() == net::kIPv4AddressSize) {
  return address[0] == 0x7f;
} else if (address.size() == net::kIPv6AddressSize) {
  return std::equal(&kIPv6Loopback[0],
                    &kIPv6Loopback[net::kIPv6AddressSize],
                    address.begin());
}
...

Also, don't you need to check the other bytes of the address for the ipv4 case?

https://codereview.chromium.org/1213203004/diff/100001/content/browser/render...
content/browser/renderer_host/pepper/pepper_socket_utils.cc:173: }
nit: insert blank line

https://codereview.chromium.org/1213203004/diff/100001/content/browser/render...
File
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.cc
(right):

https://codereview.chromium.org/1213203004/diff/100001/content/browser/render...
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.cc:196:
MaybeOpenFirewallHole(context, net_result);
I think it would be clearer to have the chromeos-specific code here, i.e.:

#if defined(OS_CHROMEOS)
  OpenFirewallHole(context, net_result);
#else
  OnListenCompleted(context, net_result);
#endif

https://codereview.chromium.org/1213203004/diff/100001/content/browser/render...
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.cc:241:
return;
What if net_result == net::ERR_IO_PENDING?

https://codereview.chromium.org/1213203004/diff/100001/content/browser/render...
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.cc:251:
OnListenCompleted(context, net_result);
This changes the original behavior, where we checked:

if (net_result != net::ERR_IO_PENDING)

before calling OnListenCompleted.

https://codereview.chromium.org/1213203004/diff/100001/content/browser/render...
File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc
(right):

https://codereview.chromium.org/1213203004/diff/100001/content/browser/render...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:724:
MaybeOpenFirewallHole(context, pp_result);
Have chromeos-specific code here to make it clearer that original behavior is
preserved.

https://codereview.chromium.org/1213203004/diff/100001/content/browser/render...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:936:
void PepperTCPSocketMessageFilter::ListenComplete(
OnListenCompleted for consistency.

https://codereview.chromium.org/1213203004/diff/100001/content/browser/render...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:942: }
nit: blank line

https://codereview.chromium.org/1213203004/diff/100001/content/browser/render...
File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc
(right):

https://codereview.chromium.org/1213203004/diff/100001/content/browser/render...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:496: 
Have chromeos-specific code here to make it clearer that original behavior is
preserved.

https://codereview.chromium.org/1213203004/diff/100001/content/browser/render...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:497:
base::Closure bind_complete = base::Bind(
Do this inline, since there is only one consumer of bind_complete.

[avallee, 2015-07-13 18:29:10]

PTAL.

https://codereview.chromium.org/1213203004/diff/100001/content/browser/render...
File content/browser/renderer_host/pepper/pepper_socket_utils.cc (right):

https://codereview.chromium.org/1213203004/diff/100001/content/browser/render...
content/browser/renderer_host/pepper/pepper_socket_utils.cc:137: namespace {
On 2015/07/09 19:55:17, bbudge wrote:
> nit: Blank line here.

Done.

https://codereview.chromium.org/1213203004/diff/100001/content/browser/render...
content/browser/renderer_host/pepper/pepper_socket_utils.cc:142: {0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1};
On 2015/07/09 19:55:17, bbudge wrote:
> nit: insert blank line

Done.

https://codereview.chromium.org/1213203004/diff/100001/content/browser/render...
content/browser/renderer_host/pepper/pepper_socket_utils.cc:144: if
(address.size() == net::kIPv4AddressSize && address[0] == 0x7f) {
On 2015/07/09 19:55:17, bbudge wrote:
> This would be a little easier to reason about if you simplify the 'if'
> conditions:
> 
> if (address.size() == net::kIPv4AddressSize) {
>   return address[0] == 0x7f;
> } else if (address.size() == net::kIPv6AddressSize) {
>   return std::equal(&kIPv6Loopback[0],
>                     &kIPv6Loopback[net::kIPv6AddressSize],
>                     address.begin());
> }
> ...
> 

Done.

> Also, don't you need to check the other bytes of the address for the ipv4
case?

Nope the whole /8 netblock is loopback per RFC3330, page 2.

https://codereview.chromium.org/1213203004/diff/100001/content/browser/render...
content/browser/renderer_host/pepper/pepper_socket_utils.cc:173: }
On 2015/07/09 19:55:16, bbudge wrote:
> nit: insert blank line

Done.

https://codereview.chromium.org/1213203004/diff/100001/content/browser/render...
File
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.cc
(right):

https://codereview.chromium.org/1213203004/diff/100001/content/browser/render...
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.cc:241:
return;
On 2015/07/09 19:55:17, bbudge wrote:
> What if net_result == net::ERR_IO_PENDING?

Moved call to the body of if (net_result != net::ERR_IO_PENDING);

https://codereview.chromium.org/1213203004/diff/100001/content/browser/render...
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.cc:251:
OnListenCompleted(context, net_result);
On 2015/07/09 19:55:17, bbudge wrote:
> This changes the original behavior, where we checked:
> 
> if (net_result != net::ERR_IO_PENDING)
> 
> before calling OnListenCompleted.

Done.

https://codereview.chromium.org/1213203004/diff/100001/content/browser/render...
File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc
(right):

https://codereview.chromium.org/1213203004/diff/100001/content/browser/render...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:936:
void PepperTCPSocketMessageFilter::ListenComplete(
On 2015/07/09 19:55:17, bbudge wrote:
> OnListenCompleted for consistency.

Done.

https://codereview.chromium.org/1213203004/diff/100001/content/browser/render...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:942: }
On 2015/07/09 19:55:17, bbudge wrote:
> nit: blank line

Done.

https://codereview.chromium.org/1213203004/diff/100001/content/browser/render...
File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc
(right):

https://codereview.chromium.org/1213203004/diff/100001/content/browser/render...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:496: 
On 2015/07/09 19:55:17, bbudge wrote:
> Have chromeos-specific code here to make it clearer that original behavior is
> preserved.

Done.

https://codereview.chromium.org/1213203004/diff/100001/content/browser/render...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:497:
base::Closure bind_complete = base::Bind(
On 2015/07/09 19:55:17, bbudge wrote:
> Do this inline, since there is only one consumer of bind_complete.

Done.

[avallee, 2015-07-21 20:13:12]

Ping?

[bbudge, 2015-07-22 02:00:48]

Looking better thanks.

https://codereview.chromium.org/1213203004/diff/120001/content/browser/render...
File content/browser/renderer_host/pepper/pepper_socket_utils.cc (right):

https://codereview.chromium.org/1213203004/diff/120001/content/browser/render...
content/browser/renderer_host/pepper/pepper_socket_utils.cc:147: // All of
127.0.0.0/8 is loopback in ipv4. See RFC3330.
Maybe the comment is confusing me then: Could you say "All of 127.*.*.*/8 is
loopback" instead?

https://codereview.chromium.org/1213203004/diff/120001/content/browser/render...
File
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.cc
(right):

https://codereview.chromium.org/1213203004/diff/120001/content/browser/render...
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.cc:249:
// punch hole.
nit: comment doesn't add much information. The method is already named
OpenFirewallHole.

https://codereview.chromium.org/1213203004/diff/120001/content/browser/render...
File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc
(right):

https://codereview.chromium.org/1213203004/diff/120001/content/browser/render...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:479:
firewall_hole_.reset(nullptr);
nit: can just reset() with no param.

https://codereview.chromium.org/1213203004/diff/120001/content/browser/render...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:961:
base::Unretained(this), context, pp_result);
Why base::Unretained(this) instead of just 'this'?
Here and below.

https://codereview.chromium.org/1213203004/diff/120001/content/browser/render...
File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc
(right):

https://codereview.chromium.org/1213203004/diff/120001/content/browser/render...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:500:
base::Unretained(this), base::Passed(&socket),
Why base::Unretained(this) instead of just 'this'?
Here and below.

[avallee, 2015-07-24 18:33:00]

https://codereview.chromium.org/1213203004/diff/120001/content/browser/render...
File content/browser/renderer_host/pepper/pepper_socket_utils.cc (right):

https://codereview.chromium.org/1213203004/diff/120001/content/browser/render...
content/browser/renderer_host/pepper/pepper_socket_utils.cc:147: // All of
127.0.0.0/8 is loopback in ipv4. See RFC3330.
On 2015/07/22 02:00:48, bbudge wrote:
> Maybe the comment is confusing me then: Could you say "All of 127.*.*.*/8 is
> loopback" instead?

I guess it want's clear that my notation is not a subnet, not a specific host
address.

https://codereview.chromium.org/1213203004/diff/120001/content/browser/render...
File
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.cc
(right):

https://codereview.chromium.org/1213203004/diff/120001/content/browser/render...
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.cc:249:
// punch hole.
On 2015/07/22 02:00:48, bbudge wrote:
> nit: comment doesn't add much information. The method is already named
> OpenFirewallHole.

Done.

https://codereview.chromium.org/1213203004/diff/120001/content/browser/render...
File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc
(right):

https://codereview.chromium.org/1213203004/diff/120001/content/browser/render...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:479:
firewall_hole_.reset(nullptr);
On 2015/07/22 02:00:48, bbudge wrote:
> nit: can just reset() with no param.

Done.

https://codereview.chromium.org/1213203004/diff/120001/content/browser/render...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:961:
base::Unretained(this), context, pp_result);
On 2015/07/22 02:00:48, bbudge wrote:
> Why base::Unretained(this) instead of just 'this'?
> Here and below.

Was copying the other code in this file. Removed since it works without
unretained and is safer.

https://codereview.chromium.org/1213203004/diff/120001/content/browser/render...
File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc
(right):

https://codereview.chromium.org/1213203004/diff/120001/content/browser/render...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:500:
base::Unretained(this), base::Passed(&socket),
On 2015/07/22 02:00:48, bbudge wrote:
> Why base::Unretained(this) instead of just 'this'?
> Here and below.

Done.

[bbudge, 2015-07-29 00:44:58]

lgtm

[avallee, 2015-07-29 00:48:22]

The CQ bit was checked by avallee@chromium.org

[commit-bot: I haz the power, 2015-07-29 00:48:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1213203004/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1213203004/140001

[commit-bot: I haz the power, 2015-07-29 01:28:27]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2015-07-29 01:29:01]

Patchset 7 (id:??) landed as
https://crrev.com/2f11040f9262036c19b1e7cdf05422d4ed92706b
Cr-Commit-Position: refs/heads/master@{#340831}