socketio / socketio/socket.io

origins not checked when url has sid

Open
#5,077 1 comment 2 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

package:engine.io
Dominant language
TypeScript
Stars
63.2k
Forks
10.3k
Avg merge
11d 20h
Merged PRs (30d)
2

Description

You want to:
  • report a bug
  • request a feature
Current behaviour

When the url does not have the sid parameter the request is checked against the allowed origins here, after this subsequent requests are not checked against allowed origins.

Steps to reproduce

Set origins to mydomain.com
Make a request from evildomain.com spoof the origin header to match mydomain.com and get a valid sid.
Make request from evildomain with the valid sid and origin is no longer checked.

Expected behaviour

All requests to be checked against allowed origins

Other information (e.g. stacktraces, related issues, suggestions how to fix)

I changed the code to this and seems to work but wanted to get some feedback.

Server.prototype.verify = function (req, upgrade, fn) {
  // transport check
  var transport = req._query.transport;
  if (!~this.transports.indexOf(transport)) {
    debug('unknown transport "%s"', transport);
    return fn(Server.errors.UNKNOWN_TRANSPORT, false);
  }

  // 'Origin' header check
  var isOriginInvalid = checkInvalidHeaderChar(req.headers.origin);
  if (isOriginInvalid) {
    req.headers.origin = null;
    return fn(Server.errors.BAD_REQUEST, false);
  }

  // sid check
  var sid = req._query.sid;
  if (sid) {
    if (!this.clients.hasOwnProperty(sid)) {
      return fn(Server.errors.UNKNOWN_SID, false);
    }
    if (!upgrade && this.clients[sid].transport.name !== transport) {
      debug('bad request: unexpected transport without upgrade');
      return fn(Server.errors.BAD_REQUEST, false);
    }
  } else {
    // handshake is GET only
    if ('GET' !== req.method) return fn(Server.errors.BAD_HANDSHAKE_METHOD, false);
  }
  if (!this.allowRequest) return fn(null, true);
  return this.allowRequest(req, fn);
};

This calls allowRequest function for all requests instead of just the initial one.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in lib/server.js at Server.prototype.verify, focusing on the sid branch and the allowRequest call. Reproduce the described requests with an allowed origin and a sid from an untrusted origin, then verify that all requests are checked against allowed origins rather than only the initial handshake.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript, node.js
Domain
backend, security
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.