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(webrtc): remove vulnerable unmaintained stun package dependency #2967

Merged
merged 2 commits into from
Feb 18, 2025

Conversation

devlux76
Copy link
Contributor

Note: This is my second attempt, I put it on a completely clean feature branch this time.

Title

Description

This PR removes the dependency on the abandoned stun package from the WebRTC transport implementation, which had critical security vulnerabilities through its dependencies:

  • ip: SSRF vulnerability in isPublic categorization
  • parse-path: Authorization bypass vulnerability
  • trim-newlines: Uncontrolled resource consumption
  • yargs-parser: Prototype pollution vulnerability

The change replaces the STUN message handling with the built-in implementation from @ipshipyard/node-datachannel, which we already use for most of our WebRTC functionality. This simplifies our dependency tree and removes these security issues without affecting functionality.

All tests are passing and npm audit now reports zero vulnerabilities.

Notes & open questions

The change was straightforward as we already had a more secure alternative available through our existing dependency. No API changes were required.

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if necessary (implementation changes only, no API changes)
  • I have added tests that prove my fix is effective or that my feature works (existing tests pass and verify the functionality)

@devlux76 devlux76 requested a review from a team as a code owner February 17, 2025 03:11
@achingbrain
Copy link
Member

Thanks for opening this. The stun dep was originally added because there was a question mark over whether libjuice would support multiple STUN listeners per process (necessary for things like testing where we run two or more libp2p nodes in the same process).

Support was added in paullouisageneau/libjuice#292 so this could simplify further and remove the dgramListener codepath entirely.

@achingbrain achingbrain merged commit 6d0f3ee into libp2p:main Feb 18, 2025
34 checks passed
@achingbrain achingbrain mentioned this pull request Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants