Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(parse-http): prevent announcement from peers with invalid ports #513

Merged
merged 1 commit into from
Mar 12, 2024

Conversation

SilentBot1
Copy link
Member

@SilentBot1 SilentBot1 commented Mar 10, 2024

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[x] Bug fix
[ ] New feature
[ ] Other, please explain:

What changes did you make? (Give an overview)

This change implements a small check within parse-http.js to prevent peers with invalid announcement ports from having their announcements accepted or added to the swarm. Valid ports within this implementation are considered any port within the range of 1 - 65535.

Note: Port 0 is excluded as it is invalid for communication between hosts and is only used for local dynamic port binding.

Which issue (if any) does this pull request address?

#512

Is there anything you'd like reviewers to focus on?

Copy link

welcome bot commented Mar 10, 2024

🙌 Thanks for opening this pull request! You're awesome.

@ThaUnknown
Copy link
Member

wtf, how can this ever occur on an actual network stack?

@SilentBot1
Copy link
Member Author

SilentBot1 commented Mar 11, 2024

It is not possible in an actual TCP or UDP connection, any port lower than 1 and higher than 65535 is invalid.

In the UDP tracker, this is handled as the port is a read as a UInt16BE, limiting the value to between 0 to 65535 (of which 0 is still invalid, but doesn't cause a crash).

In the HTTP tracker, this port is cast as a Number directly from user the input, allowing port values outside of the valid port range.

These values then crash the server elsewhere as there are assumptions that only valid port ranges would added to the swarm.

@SilentBot1 SilentBot1 changed the title fix(swarm): prevent announcement from peers with invalid ports fix(parse-http): prevent announcement from peers with invalid ports Mar 11, 2024
@ThaUnknown
Copy link
Member

idk, I don't think I'll ever have time to review this, as there's too many things to understand and consider, such as:
wont this still be an issue here https://github.com/SilentBot1/bittorrent-tracker/blob/20d777a90472874fb02793d0da3dac749cf6d32d/lib/server/parse-websocket.js#L70

@SilentBot1
Copy link
Member Author

SilentBot1 commented Mar 12, 2024

wont this still be an issue here https://github.com/SilentBot1/bittorrent-tracker/blob/20d777a90472874fb02793d0da3dac749cf6d32d/lib/server/parse-websocket.js#L70

Due to the port in this case coming from socket.updateReq.connection.remotePort on the line above, this would not be susceptible to the same issue as the port is from the underlaying HTTP(s) connection itself (not a query param), which couldn't establish with an invalid port.

@SilentBot1 SilentBot1 merged commit fe75272 into webtorrent:master Mar 12, 2024
3 checks passed
Copy link

welcome bot commented Mar 12, 2024

🎉 Congrats on getting your first pull request landed!

@SilentBot1 SilentBot1 deleted the issue/512-invalid-ports branch March 12, 2024 17:40
webtorrent-bot pushed a commit that referenced this pull request Mar 12, 2024
## [11.0.2](v11.0.1...v11.0.2) (2024-03-12)

### Bug Fixes

* **parse-http:** ignore announcements from peers with invalid announcement ports. ([#513](#513)) ([fe75272](fe75272))
@webtorrent-bot
Copy link

🎉 This PR is included in version 11.0.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants