ICE TCP might connect to any port

RESOLVED FIXED in Firefox 65

Status

()

defect
P4
normal
Rank:
37
RESOLVED FIXED
4 years ago
7 months ago

People

(Reporter: drno, Assigned: iidebyo)

Tracking

(Blocks 1 bug)

Trunk
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 affected, firefox65 fixed)

Details

(Whiteboard: [good first bug])

Attachments

(1 attachment)

Reporter

Description

4 years ago
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.
Reporter

Updated

4 years ago
Whiteboard: [good first bug]
backlog: --- → webrtc/webaudio+
Rank: 37
Priority: -- → P3

Comment 2

4 years ago
Hi, would like to work on this bug, please assign it to me, can you also provide more info.Thank you :)
Flags: needinfo?(drno)
Reporter

Comment 3

4 years ago
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
Flags: needinfo?(drno)

Comment 4

4 years ago
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 :)
Flags: needinfo?(drno)
Mass change P3->P4 to align with new Mozilla triage process.
Priority: P3 → P4
Assignee

Comment 6

7 months ago
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

Updated

7 months ago
Assignee: bmanojkumar24 → paul.m.vitale
Keywords: checkin-needed
Assignee

Updated

7 months ago
Flags: needinfo?(drno)

Comment 8

7 months ago
Pushed by rvandermeulen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f56dfb58f1c9
Block webrtc socket creation on restricted ports r=drno
Keywords: checkin-needed

Comment 9

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f56dfb58f1c9
Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.