From 4ba99b741b9d703e8a76fb6629668614826ac135 Mon Sep 17 00:00:00 2001 From: Jeremy Banks Date: Thu, 27 Aug 2015 12:26:43 -0700 Subject: [PATCH] Potential XSS mitigations and encoding corrections. Encoding current directory name in directory listings. HTML in directory names (unlikely but possible) could previously have been rendered in the page. Encoding query strings in subdirectory links in directory listings. It is unclear if the query string values could actually have been rendered in any cases, but the unencoded values could produce inconsequentially incorrect links (e.g. "/?>" would produce subdirectory links like "/dir/>") and cause HTML validation to fail. Replacing text/plain responses messages for 400 and 500 errors with text/html response pages. This should not be neccessary in theory, but older versions of some browsers including Internet Explorer (and possibly newer versions in certain contexts) sometimes render text/plain content as text/html for backwards compatibility with poorly-configured servers, even if `X-Content-Type-Options: nosniff` is specified. Since the errors messages in these handlers could potentially include content reflected from the requests, the most conservative and safe approach is to use an HTML response with the error messages encoded. Also updates LICENSE.txt to include the current year and some additional contributors. --- LICENSE.txt | 3 ++- lib/ecstatic/showdir.js | 6 ++--- lib/ecstatic/status-handlers.js | 40 +++++++++++++++++++++++++++---- test/html-reflection.js | 5 ++-- test/public//a.js | 0 test/showdir-href-encoding.js | 2 +- test/showdir-pathname-encoding.js | 34 ++++++++++++++++++++++++++ test/showdir-search-encoding.js | 34 ++++++++++++++++++++++++++ 8 files changed, 112 insertions(+), 12 deletions(-) create mode 100644 test/public//a.js create mode 100644 test/showdir-pathname-encoding.js create mode 100644 test/showdir-search-encoding.js diff --git a/LICENSE.txt b/LICENSE.txt index a90fd5e..a244de6 100644 --- a/LICENSE.txt +++ b/LICENSE.txt @@ -1,6 +1,7 @@ The MIT License (MIT) -Copyright (c) 2013 Joshua Holbrook +Copyright (c) 2013-2015 Joshua Holbrook, Jon Ege Ronnenberg, James Halliday, +and Google Inc. Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated documentation files (the "Software"), to deal diff --git a/lib/ecstatic/showdir.js b/lib/ecstatic/showdir.js index 731a323..5b56a82 100644 --- a/lib/ecstatic/showdir.js +++ b/lib/ecstatic/showdir.js @@ -108,10 +108,10 @@ module.exports = function (opts, stat) { ' ', ' ', ' ', - ' Index of ' + pathname +'', + ' Index of ' + he.encode(pathname) +'', ' ', ' ', - '

Index of ' + pathname + '

' + '

Index of ' + he.encode(pathname) + '

' ].join('\n') + '\n'; html += ''; @@ -124,7 +124,7 @@ module.exports = function (opts, stat) { // append trailing slash and query for dir entry if (isDir) { - href += '/' + ((parsed.search)? parsed.search:''); + href += '/' + he.encode((parsed.search)? parsed.search:''); } var displayName = he.encode(file[0]) + ((isDir)? '/':''); diff --git a/lib/ecstatic/status-handlers.js b/lib/ecstatic/status-handlers.js index 9874534..348c99d 100644 --- a/lib/ecstatic/status-handlers.js +++ b/lib/ecstatic/status-handlers.js @@ -1,3 +1,5 @@ +var he = require('he'); + // not modified exports['304'] = function (res, next) { res.statusCode = 304; @@ -60,13 +62,43 @@ exports['416'] = function (res, next) { // flagrant error exports['500'] = function (res, next, opts) { res.statusCode = 500; - res.setHeader('content-type', 'text/plain'); - res.end(opts.error.stack || opts.error.toString() || "No specified error"); + res.setHeader('content-type', 'text/html'); + var error = String(opts.error.stack || opts.error || "No specified error"), + html = [ + '', + '', + ' ', + ' ', + ' 500 Internal Server Error', + ' ', + ' ', + '

', + ' ' + he.encode(error), + '

', + ' ', + '' + ].join('\n') + '\n'; + res.end(html); }; // bad request exports['400'] = function (res, next, opts) { res.statusCode = 400; - res.setHeader('content-type', 'text/plain'); - res.end(opts && opts.error ? String(opts.error) : 'Malformed request.'); + res.setHeader('content-type', 'text/html'); + var error = opts && opts.error ? String(opts.error) : 'Malformed request.', + html = [ + '', + '', + ' ', + ' ', + ' 400 Bad Request', + ' ', + ' ', + '

', + ' ' + he.encode(error), + '

', + ' ', + '' + ].join('\n') + '\n'; + res.end(html); }; diff --git a/test/html-reflection.js b/test/html-reflection.js index f399096..93574e7 100644 --- a/test/html-reflection.js +++ b/test/html-reflection.js @@ -10,9 +10,8 @@ test('html reflection prevented', function (t) { var port = server.address().port; var attack = ''; request.get('http://localhost:' + port + '/more-problematic/' + attack, function (err, res, body) { - if ((!res.headers['content-type'] || res.headers['content-type'] == 'text/html') && - body.indexOf(attack) != -1) { - t.fail('Unescaped HTML reflected with vulnerable or missing content-type.'); + if (body.indexOf('