Closed Bug 1194010 Opened 5 years ago Closed 2 years ago
ICE TCP might connect to any port
47 bytes, text/x-phabricator-request
|Details | Review|
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.
Yes, this seems like a good idea.
backlog: --- → webrtc/webaudio+
Priority: -- → P3
Hi, would like to work on this bug, please assign it to me, can you also provide more info.Thank you :)
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.
Assignee: nobody → bmanojkumar24
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 :)
Mass change P3->P4 to align with new Mozilla triage process.
Priority: P3 → P4
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.
Assignee: bmanojkumar24 → paul.m.vitale
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/f56dfb58f1c9 Block webrtc socket creation on restricted ports r=drno
You need to log in before you can comment on or make changes to this bug.