Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Third-party transport access attempt fix. #624

Closed
wants to merge 1 commit into from

6 participants

@Irrelon

Updated handleRequest method to provide protection from third-party client access attempt to same transport ID. See issue 619 #619 for more information about why this fix is required.

@Irrelon Irrelon Updated handleRequest method to provide protection from third-party c…
…lient access attempt to same transport ID. See issue 619 Automattic#619 for more information about why this fix is required.
b80bda9
@shoe

Am I missing something, or doesn't this change actually close a DoS security vulnerability?

@tj tj commented on the diff
lib/manager.js
@@ -525,7 +525,24 @@ Manager.prototype.onDisconnect = function (id, local) {
Manager.prototype.handleRequest = function (req, res) {
var data = this.checkRequest(req);
-
+
+ // Check if the request id is already assigned to a transport
+ if (this.transports[data.id]) {
+ // Get the real client IPs if we are begind a proxy like node-http-proxy
+ var finalRemoteAddress1 = this.transports[data.id].req.headers['x-forwarded-for'] || this.transports[data.id].req.connection.remoteAddress;
@tj
tj added a note

trusting x-forwarded-for should be a setting that you trust when behind a reverse proxy otherwise it's easy to spoof (unless im missing that here somewhere)

@Irrelon
Irrelon added a note

Agreed, that would make sense. Easy to spoof otherwise for sure! I'll see if I can add a new option into socket.io called "trust forwarded header" or something like that. Then it can do a quick check in here.

@tj
tj added a note

in Express I called it "trust proxy" I think, that sounds kinda weird though actually .enable('trust proxy').. hmm..

@Irrelon
Irrelon added a note

I like "trust proxy", more terse. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@binocarlos

Wow this was very helpful - fired up my socket.io (0.9.6) + express (2.5.9) app - worked perfectly using all browsers... Rang friend - the first 30 seconds perfect then everytime a:

client disconnect
warn: websocket connection invalid

Turns out he was on Talk-Talk and the extra callback was killing the connection - inserting the code above fixed it a treat and you can see the console logging when the extra evil connection is attempted - happy cos it was the first external connection I tested and it didn't work - fixed a treat now though : )

@Irrelon

Great, glad the code I wrote could help someone else other than myself. I actually moved away from TalkTalk after this incident because of the way they were tracking and checking every URL I browsed. I'm surprised this pull request hasn't been added to the latest builds of socket.io though! I guess that it hasn't been reviewed in a while though.

Rob (Irrelon Software)

@binocarlos

Hey man - this should defo be added - it's not just Talk-Talk, this would stop a 'ping-back' from any ISP, it seems logical to block a connection that is trying to grab a socket already allocated to another IP? either ways - thanking u kindly once again!

@markwillis82

I can confirm this worked for my site using socket.io on talk talk.
Would like to see this merged to the main branch.

@rauchg rauchg closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 4, 2011
  1. @Irrelon

    Updated handleRequest method to provide protection from third-party c…

    Irrelon authored
    …lient access attempt to same transport ID. See issue 619 Automattic#619 for more information about why this fix is required.
This page is out of date. Refresh to see the latest.
Showing with 18 additions and 1 deletion.
  1. +18 −1 lib/manager.js
View
19 lib/manager.js
@@ -525,7 +525,24 @@ Manager.prototype.onDisconnect = function (id, local) {
Manager.prototype.handleRequest = function (req, res) {
var data = this.checkRequest(req);
-
+
+ // Check if the request id is already assigned to a transport
+ if (this.transports[data.id]) {
+ // Get the real client IPs if we are begind a proxy like node-http-proxy
+ var finalRemoteAddress1 = this.transports[data.id].req.headers['x-forwarded-for'] || this.transports[data.id].req.connection.remoteAddress;
@tj
tj added a note

trusting x-forwarded-for should be a setting that you trust when behind a reverse proxy otherwise it's easy to spoof (unless im missing that here somewhere)

@Irrelon
Irrelon added a note

Agreed, that would make sense. Easy to spoof otherwise for sure! I'll see if I can add a new option into socket.io called "trust forwarded header" or something like that. Then it can do a quick check in here.

@tj
tj added a note

in Express I called it "trust proxy" I think, that sounds kinda weird though actually .enable('trust proxy').. hmm..

@Irrelon
Irrelon added a note

I like "trust proxy", more terse. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ var finalRemoteAddress2 = req.headers['x-forwarded-for'] || req.connection.remoteAddress;
+
+ // Check if the new request's client IP matches the original transport client IP
+ if (finalRemoteAddress1 != finalRemoteAddress2) {
+ // The new connection comes from a different IP from the original so drop it
+ console.log('Access to transport from third-party IP attempted from ' + finalRemoteAddress2);
+ res.writeHead(404);
+ res.end('404');
+
+ return;
+ }
+ }
+
if (!data) {
for (var i = 0, l = this.oldListeners.length; i < l; i++) {
this.oldListeners[i].call(this.server, req, res);
Something went wrong with that request. Please try again.