"Safe Ports" feature cannot restrict port 80 or 443 (or any other implicit port derived from the scheme of a URL)

[robertwessen]

Description:

The Safe Ports setting is to limit the ports an embedded link preview might try to load. This feature relies on URL.parse().port which will be null for all implicit ports derived from a URL scheme. This makes it impossible to prevent resources from being loaded from port 80 or 443 if those ports are not explicitly set in the URL typed in by the user (which no one ever does). The default values are 80, 443, but if someone wanted to prevent insecure resources from loading (by removing port 80) it won't actually do anything.

Server Setup Information:

• Version of Rocket.Chat Server: 0.60.1
• Operating System: 4.4.0-112-generic
• Deployment Method(snap/docker/tar/etc): Docker
• Number of Running Instances: 1
• DB Replicaset Oplog: no
• Node Version: v8.9.3
• mongoDB Version: 3.4

Steps to Reproduce:

1. Go to Administration->Message->Embed Link Previews, make sure its on
2. Go to Administration->Message->Safe Ports
   2a. Remove Default values and put only 443 into the input field, Save changes
3. Go to any channel and type a HTTP URL into a message (ex: http://www.google.com)

4. Go to any channel and type a HTTP URL with an explicit (ex: http://www.google.com:80)

Expected behavior:

Both Preview contents do not load.

Actual behavior:

The first Preview content (step 3) is loaded over port 80 (HTTP).
The second Preview content (step 4) is blocked.

Relevant code:

packages/rocketchat-oembed/server/server.js
lines 55-70

nodejs URL lib will return null for any implicit port derived from a URL scheme, so when the safe port check happens a few lines later, even without 80 being present in the Safe Ports list, it still passes the check, even though the call eventually loads the content over port 80.

[localguru]

I can confirm this feature is still broken in 0.69.2.

See also
#7490
RocketChat/feature-requests#785

[engelgabriel]

So it should work if we also test for NULL and test the "protocol" part of the URL?

[robertwessen]

yes, i think the best option is to add a "permitted scheme" option as well, where admins can explicitly permit certain protocols [https,ftp, etc]. it allows for better flexibility for future RC capabilities. you don't really want to maintain a map of common schemes to implicit ports for those schemes within RC (at least i wouldn't).

the trade-off in this approach is that by permitting a scheme, you permit it's implicit port as well as any explicit port settings an admin has entered in the "safe ports" option. this does leave some potential edge cases where the behavior will be inconsistent (ie. admin wants to allow http only to port 8080, but inadvertently will be permitting port 80 and 8080)

the more complete solution is to give the admin the option to specify scheme and port tuples for each protocol they would like to permit. this is more complex, but more granular in control.

either would be an improvement over current behavior.