Skip to content

Commit

Permalink
Improved fix for open redirect allow list bypass
Browse files Browse the repository at this point in the history 
Co-authored-by: Jon Church <me@jonchurch.com>
Co-authored-by: Blake Embrey <hello@blakeembrey.com>
  • Loading branch information
@wesleytodd @jonchurch @blakeembrey
3 people committed Mar 25, 2024
1 parent 4f0f6cc commit 66a9778
Show file tree
Hide file tree
Showing 2 changed files with 276 additions and 64 deletions.
31 changes: 11 additions & 20 deletions lib/response.js
View file
Expand Up @@ -34,7 +34,6 @@ var extname = path.extname;
var mime = send.mime;
var resolve = path.resolve;
var vary = require('vary');
var urlParse = require('url').parse;

/**
* Response prototype.
Expand All @@ -56,6 +55,7 @@ module.exports = res
*/

var charsetRegExp = /;\s*charset\s*=/;
var schemaAndHostRegExp = /^(?:[a-zA-Z][a-zA-Z0-9+.-]*:)?\/\/[^\\\/\?]+/;

/**
* Set status `code`.
Expand Down Expand Up @@ -905,32 +905,23 @@ res.cookie = function (name, value, options) {
*/

res.location = function location(url) {
var loc = String(url);
var loc;

// "back" is an alias for the referrer
if (url === 'back') {
loc = this.req.get('Referrer') || '/';
} else {
loc = String(url);
}

var lowerLoc = loc.toLowerCase();
var encodedUrl = encodeUrl(loc);
if (lowerLoc.indexOf('https://') === 0 || lowerLoc.indexOf('http://') === 0) {
try {
var parsedUrl = urlParse(loc);
var parsedEncodedUrl = urlParse(encodedUrl);
// Because this can encode the host, check that we did not change the host
if (parsedUrl.host !== parsedEncodedUrl.host) {
// If the host changes after encodeUrl, return the original url
return this.set('Location', loc);
}
} catch (e) {
// If parse fails, return the original url
return this.set('Location', loc);
}
}
var m = schemaAndHostRegExp.exec(loc);
var pos = m ? m[0].length + 1 : 0;

// Only encode after host to avoid invalid encoding which can introduce
// vulnerabilities (e.g. `\\` to `%5C`).
loc = loc.slice(0, pos) + encodeUrl(loc.slice(pos));

// set location
return this.set('Location', encodedUrl);
return this.set('Location', loc);
};

/**
Expand Down
309 changes: 265 additions & 44 deletions test/res.location.js
View file
Expand Up @@ -2,9 +2,10 @@

var express = require('../')
, request = require('supertest')
, assert = require('assert')
, url = require('url');

describe('res', function(){
describe.only('res', function(){
describe('.location(url)', function(){
it('should set the header', function(done){
var app = express();
Expand Down Expand Up @@ -45,49 +46,6 @@ describe('res', function(){
.expect(200, done)
})

it('should not encode bad "url"', function (done) {
var app = express()

app.use(function (req, res) {
// This is here to show a basic check one might do which
// would pass but then the location header would still be bad
if (url.parse(req.query.q).host !== 'google.com') {
res.status(400).end('Bad url');
}
res.location(req.query.q).end();
});

request(app)
.get('/?q=http://google.com' + encodeURIComponent('\\@apple.com'))
.expect(200)
.expect('Location', 'http://google.com\\@apple.com')
.end(function (err) {
if (err) {
throw err;
}

// This ensures that our protocol check is case insensitive
request(app)
.get('/?q=HTTP://google.com' + encodeURIComponent('\\@apple.com'))
.expect(200)
.expect('Location', 'HTTP://google.com\\@apple.com')
.end(done)
});
});

it('should not touch already-encoded sequences in "url"', function (done) {
var app = express()

app.use(function (req, res) {
res.location('https://google.com?q=%A710').end()
})

request(app)
.get('/')
.expect('Location', 'https://google.com?q=%A710')
.expect(200, done)
})

describe('when url is "back"', function () {
it('should set location from "Referer" header', function (done) {
var app = express()
Expand Down Expand Up @@ -146,6 +104,79 @@ describe('res', function(){
})
})

it('should encode data uri', function (done) {
var app = express()
app.use(function (req, res) {
res.location('data:text/javascript,export default () => { }').end();
});

request(app)
.get('/')
.expect('Location', 'data:text/javascript,export%20default%20()%20=%3E%20%7B%20%7D')
.expect(200, done)
})

it('should encode data uri', function (done) {
var app = express()
app.use(function (req, res) {
res.location('data:text/javascript,export default () => { }').end();
});

request(app)
.get('/')
.expect('Location', 'data:text/javascript,export%20default%20()%20=%3E%20%7B%20%7D')
.expect(200, done)
})

it('should consistently handle non-string input: boolean', function (done) {
var app = express()
app.use(function (req, res) {
res.location(true).end();
});

request(app)
.get('/')
.expect('Location', 'true')
.expect(200, done)
});

it('should consistently handle non-string inputs: object', function (done) {
var app = express()
app.use(function (req, res) {
res.location({}).end();
});

request(app)
.get('/')
.expect('Location', '[object%20Object]')
.expect(200, done)
});

it('should consistently handle non-string inputs: array', function (done) {
var app = express()
app.use(function (req, res) {
res.location([]).end();
});

request(app)
.get('/')
.expect('Location', '')
.expect(200, done)
});

it('should consistently handle empty string input', function (done) {
var app = express()
app.use(function (req, res) {
res.location('').end();
});

request(app)
.get('/')
.expect('Location', '')
.expect(200, done)
});


if (typeof URL !== 'undefined') {
it('should accept an instance of URL', function (done) {
var app = express();
Expand All @@ -161,4 +192,194 @@ describe('res', function(){
});
}
})

describe('location header encoding', function() {
function createRedirectServerForDomain (domain) {
var app = express();
app.use(function (req, res) {
var host = url.parse(req.query.q, false, true).host;
// This is here to show a basic check one might do which
// would pass but then the location header would still be bad
if (host !== domain) {
res.status(400).end('Bad host: ' + host + ' !== ' + domain);
}
res.location(req.query.q).end();
});
return app;
}

function testRequestedRedirect (app, inputUrl, expected, expectedHost, done) {
return request(app)
// Encode uri because old supertest does not and is required
// to test older node versions. New supertest doesn't re-encode
// so this works in both.
.get('/?q=' + encodeURIComponent(inputUrl))
.expect('') // No body.
.expect(200)
.expect('Location', expected)
.end(function (err, res) {
if (err) {
console.log('headers:', res.headers)
console.error('error', res.error, err);
return done(err, res);
}

// Parse the hosts from the input URL and the Location header
var inputHost = url.parse(inputUrl, false, true).host;
var locationHost = url.parse(res.headers['location'], false, true).host;

assert.strictEqual(locationHost, expectedHost);

// Assert that the hosts are the same
if (inputHost !== locationHost) {
return done(new Error('Hosts do not match: ' + inputHost + " !== " + locationHost));
}

return done(null, res);
});
}

it('should not touch already-encoded sequences in "url"', function (done) {
var app = createRedirectServerForDomain('google.com');
testRequestedRedirect(
app,
'https://google.com?q=%A710',
'https://google.com?q=%A710',
'google.com',
done
);
});

it('should consistently handle relative urls', function (done) {
var app = createRedirectServerForDomain(null);
testRequestedRedirect(
app,
'/foo/bar',
'/foo/bar',
null,
done
);
});

it('should not encode urls in such a way that they can bypass redirect allow lists', function (done) {
var app = createRedirectServerForDomain('google.com');
testRequestedRedirect(
app,
'http://google.com\\@apple.com',
'http://google.com\\@apple.com',
'google.com',
done
);
});

it('should not be case sensitive', function (done) {
var app = createRedirectServerForDomain('google.com');
testRequestedRedirect(
app,
'HTTP://google.com\\@apple.com',
'HTTP://google.com\\@apple.com',
'google.com',
done
);
});

it('should work with https', function (done) {
var app = createRedirectServerForDomain('google.com');
testRequestedRedirect(
app,
'https://google.com\\@apple.com',
'https://google.com\\@apple.com',
'google.com',
done
);
});

it('should correctly encode schemaless paths', function (done) {
var app = createRedirectServerForDomain('google.com');
testRequestedRedirect(
app,
'//google.com\\@apple.com/',
'//google.com\\@apple.com/',
'google.com',
done
);
});

it('should percent encode backslashes in the path', function (done) {
var app = createRedirectServerForDomain('google.com');
testRequestedRedirect(
app,
'https://google.com/foo\\bar\\baz',
'https://google.com/foo%5Cbar%5Cbaz',
'google.com',
done
);
});

it('should encode backslashes in the path after the first backslash that triggered path parsing', function (done) {
var app = createRedirectServerForDomain('google.com');
testRequestedRedirect(
app,
'https://google.com\\@app\\l\\e.com',
'https://google.com\\@app%5Cl%5Ce.com',
'google.com',
done
);
});

it('should escape header splitting for old node versions', function (done) {
var app = createRedirectServerForDomain('google.com');
testRequestedRedirect(
app,
'http://google.com\\@apple.com/%0d%0afoo:%20bar',
'http://google.com\\@apple.com/%0d%0afoo:%20bar',
'google.com',
done
);
});

it('should encode unicode correctly', function (done) {
var app = createRedirectServerForDomain(null);
testRequestedRedirect(
app,
'/%e2%98%83',
'/%e2%98%83',
null,
done
);
});

it('should encode unicode correctly even with a bad host', function (done) {
var app = createRedirectServerForDomain('google.com');
testRequestedRedirect(
app,
'http://google.com\\@apple.com/%e2%98%83',
'http://google.com\\@apple.com/%e2%98%83',
'google.com',
done
);
});

it('should work correctly despite using deprecated url.parse', function (done) {
var app = createRedirectServerForDomain('google.com');
testRequestedRedirect(
app,
'https://google.com\'.bb.com/1.html',
'https://google.com\'.bb.com/1.html',
'google.com',
done
);
});

it('should encode file uri path', function (done) {
var app = createRedirectServerForDomain('');
testRequestedRedirect(
app,
'file:///etc\\passwd',
'file:///etc%5Cpasswd',
'',
done
);
});
});
})

0 comments on commit 66a9778

Please sign in to comment.