1194010 - ICE TCP might connect to any port

Status: RESOLVED FIXED

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

Description

[Nils Ohlmeier [:drno]]

We should consider re-using the port blacklist from here https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsIOService.cpp#88 for ICE TCP.
One argument is that we are just going to waste resources trying to connect to these. The other argument might be to not crash services on these ports.

Comment 1

[Eric Rescorla (:ekr)]

Yes, this seems like a good idea.

Comment 2

[simplyblue24]

Hi, would like to work on this bug, please assign it to me, can you also provide more info.Thank you :)

Comment 3

[Nils Ohlmeier [:drno]]

So if you look at this code: 
https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsIOService.cpp#99
That is a list of port numbers which Firefox HTTP client should not connect to. The list is being converted into an internal array here:
https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsIOService.cpp#223
And then the actual checking seems to happen here:
https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsIOService.cpp#1161

Right now you can ask our ICE TCP implementation as well to connect to any port number. As we seem to know from experience that it is not a good idea to connect to the ports in |gBadPortList| it would be good if we could re-use that list and prevent ICE TCP from making connections to these ports as well.

I think we try to re-use the |gBadPortList| list. So probably you want to move that list into a header file to make it accessible from the nICEr code. From looking at the code in the AllowPort() function it looks like in case of the HTTP client this list can be over-written by the URL scheme. As ICE does not use URLs I would simply write our own little function for nICEr which simply checks if a given port number is on the blacklist or not.
Theoretically you could also introduce a IsOnBackList() function in the netwerk code, which then gets internally called by the existing AllowPort() function and by nICEr. Then AllowPort() would become a wrapper around your new function which also allows a URL scheme to overwrite the blacklisting, where nICEr simply just calls only your new blacklisting function directly without the option to over-write the result.

Let me know if this make it clear what is need, or if you have further question.

Comment 4

[simplyblue24]

Hi, thank you for the detailed explanation.I got few questions, in which header file should i write the function "IsOnBlackList()" and where should i call this function in "nICEr" code, I understood "AllowPort()" should call "IsOnBlackList()", apart from URL scheme.Thank you :)

Comment 5

[Bulk Bug Changes for mreavy's org]

Mass change P3->P4 to align with new Mozilla triage process.

Comment 6

[Paul Vitale [:iidebyo]]

Attachment: Bug 1194010 - Block webrtc socket creation on restricted ports r?drno

Port restriction is delegated to the io service.  Port 0 is explicitly
unrestricted.

NS_CheckPortSafety emits a warning which pollutes the gtests a bit.  It is only
tested if the port is not 0.

Comment 7

[Paul Vitale [:iidebyo]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f946adf7782bfb227735085adf727094598f030

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f56dfb58f1c9
Block webrtc socket creation on restricted ports r=drno

Comment 9

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/f56dfb58f1c9