Node security release makes http-proxy crash #964

fjakobs · 2016-02-22T15:50:04Z

The recent security release https://nodejs.org/en/blog/vulnerability/february-2016-security-releases/ of node.js makes header parsing much more strict:

To fix this defect, HTTP header parsing in Node.js, for both requests and responses, is moving closer to the formal HTTP specification. HTTP headers containing characters outside of the valid set for tokens will be rejected. This check is performed for both requests and responses, for Node.js HTTP servers and clients.

Setting headers can throw now at https://github.com/nodejitsu/node-http-proxy/blob/master/lib/http-proxy/passes/web-outgoing.js#L86.

Here the change in node.js https://github.com/nodejs/node/blob/fab240a886b69ef9fa78573fc210c15cfe0018f0/lib/_http_outgoing.js#L309

I'm getting The header content contains invalid characters error and then I get an uncaught exception.

Since I don't control the app being proxied and I can't wrap this call into a try/catch, this needs to be fixed in http-proxy.

The text was updated successfully, but these errors were encountered:

jcrugzz · 2016-02-22T15:54:58Z

@fjakobs what strings are causing this to throw? Could you log the headers that are being set here? Would love a test case for this

fjakobs · 2016-02-22T16:58:41Z

@jcrugzz I found this in the wild:

content-type: "�?@"

Not sure why someone would do this but is certainly should not crash the app.

fjakobs · 2016-02-22T17:02:09Z

here you can see what node is expecting nodejs/node@7bef1b7#diff-9ef7b1e52407f06cd19696d2f726e498R241

fjakobs · 2016-02-22T20:29:46Z

easy to reproduce. This works on node 0.10.41 and throws in 0.10.42

var httpProxy = require('http-proxy');
var net = require("net");
var http = require("http");
var proxy = httpProxy.createProxyServer({target:'http://localhost:9000'});

proxy.on('error', function(err, req, res) {
    console.log(err);
});
proxy.listen(8080);

var server = net.createServer(function(socket) {
  socket.on("data", function(data) {
    socket.write("HTTP/1.1 200 OK\n");
    socket.write("Content-Type: ");
    socket.write(new Buffer([0xfd, 0x40, 0x40]));
    socket.write("\n");
    socket.write("\n");
    socket.write(JSON.stringify({
      "name": "foo",
      "version": "4.4.4"
    }, null, 2));
    socket.end();
  });
});

server.listen(9000);

http.get("http://localhost:8080", function(res) {
    process.exit();
});

on node 0.10.42

http.js:733
      throw new TypeError('The header content contains invalid characters');
            ^
TypeError: The header content contains invalid characters
    at ServerResponse.OutgoingMessage.setHeader (http.js:733:13)
    at /home/ubuntu/workspace/lib/http-proxy/passes/web-outgoing.js:86:13
    at Array.forEach (native)
    at Array.writeHeaders [as 3] (/home/ubuntu/workspace/lib/http-proxy/passes/web-outgoing.js:84:35)
    at ClientRequest.<anonymous> (/home/ubuntu/workspace/lib/http-proxy/passes/web-incoming.js:150:20)
    at ClientRequest.emit (events.js:95:17)
    at HTTPParser.parserOnIncomingClient [as onIncoming] (http.js:1744:21)
    at HTTPParser.parserOnHeadersComplete [as onHeadersComplete] (http.js:152:23)
    at Socket.socketOnData [as ondata] (http.js:1639:20)
    at TCP.onread (net.js:528:27)

jcrugzz · 2016-02-22T22:23:25Z

@fjakobs hmm yea thats kind of unfortunate that we are receiving invalid headers. I'd almost rather do this check ourselves to prevent the need for a try..catch. Ill look at this later but would definitely take a PR that fixed this.

fjakobs · 2016-02-22T22:24:59Z

Would you prefer sanitizing the headers or passing on the error?

jcrugzz · 2016-02-22T22:34:42Z

@fjakobs how do you feel about the behavior of ignoring bad headers? I guess we could make it an option to receive it as an error but I dont think the proxy server should have to care if its receiving bad headers from a client, it just doesnt pass them along since they are not spec compliant.

What im thinking about is using that internal check they added to node core and using that as a signal to ignore the header.

ppotoplyak · 2016-06-14T15:41:18Z

@fjakobs @jcrugzz

I am using latest Meteor and hitting something with the same signature. I am trying to figure out a workaround and you guys seem to have the best handle on this.

Here is the stack dump:

http.js:733
      throw new TypeError('The header content contains invalid characters');
      ^
TypeError: The header content contains invalid characters
    at ClientRequest.OutgoingMessage.setHeader (http.js:733:13)
    at new ClientRequest (http.js:1429:14)
    at Object.exports.request (http.js:1899:10)
    at Array.stream [as 3] (/pit/root/.meteor/packages/meteor-tool/.1.3.2_4.1l0jier++os.linux.x86_64+web.browser+web.cordova/mt-os.linux.x86_64/dev_bundle/lib/node_modules/http-proxy/lib/http-proxy/passes/web-incoming.js:108:74)
    at ProxyServer.<anonymous> (/pit/root/.meteor/packages/meteor-tool/.1.3.2_4.1l0jier++os.linux.x86_64+web.browser+web.cordova/mt-os.linux.x86_64/dev_bundle/lib/node_modules/http-proxy/lib/http-proxy/index.js:80:21)
    at Proxy._tryHandleConnections (/tools/runners/run-proxy.js:182:20)
    at Server.<anonymous> (/tools/runners/run-proxy.js:50:12)
    at Server.emit (events.js:98:17)
    at HTTPParser.parser.onIncoming (http.js:2164:12)
    at HTTPParser.parserOnHeadersComplete [as onHeadersComplete] (http.js:152:23)
    at Socket.socket.ondata (http.js:2022:22)
    at TCP.onread (net.js:528:27)

A couple questions, this looks like an inbound request which crashes the server?

I wanted to simply comment the throw but it appears that the respective code gets compiled into the node elf executable?

Thanks in advance!

ppotoplyak · 2016-06-14T16:28:18Z

Refining my question a little.

So it looks like a request comes in, nothing necessarily particular about the request, but then on the response invalid characters are entered into the header?

Would the following be a reasonable workaround to provide immediate relief ? i.e. I need to prevent a hard crash when hitting this error.

diff --git a/web-outgoing.js b/web-outgoing.js
index 20b0f3c..e72b65e 100644
--- a/web-outgoing.js
+++ b/web-outgoing.js
@@ -81,11 +81,15 @@ var redirectRegex = /^30(1|2|7|8)$/;
    * @api private
    */
   function writeHeaders(req, res, proxyRes) {
-    Object.keys(proxyRes.headers).forEach(function(key) {
-      if(proxyRes.headers[key] != undefined){
-        res.setHeader(String(key).trim(), proxyRes.headers[key]);
-      }
-    });
+    try {
+      Object.keys(proxyRes.headers).forEach(function(key) {
+        if(proxyRes.headers[key] != undefined){
+          res.setHeader(String(key).trim(), proxyRes.headers[key]);
+        }
+      });
+    } catch(err) {
+      console.log(err);
+    }
   },

Thanks again!

ppotoplyak · 2016-06-14T17:06:34Z

Sorry about the noise. It appears that the issue can occur on the receive and send side. I notified the Meteor folks that the issue exists on the receive side. This is pretty nasty as a crafted request brings the server down.

jcrugzz · 2016-06-14T19:17:41Z

@ppotoplyak if you want to rebase from this branch #965 id merge it until we can figure out a better way that doesnt de-opt from the try catch.

ppotoplyak · 2016-06-15T02:57:37Z

@jcrugzz sure, if you are okay waiting until next week

There is a worse problem on the incoming side. A similarly crafted request brings the node/Meteor process down.

This is my workaround to prevent the process from terminating:

diff --git a/packages/meteor-tool/.1.3.3.1t63v9e++os.linux.x86_64+web.browser+web.cordova/mt-os.linux.x86_64/dev_bundle/lib/node_modules/http-proxy/lib/http-proxy/passes/web-inc
index 4070eb3..6812371 100644
--- a/packages/meteor-tool/.1.3.3.1t63v9e++os.linux.x86_64+web.browser+web.cordova/mt-os.linux.x86_64/dev_bundle/lib/node_modules/http-proxy/lib/http-proxy/passes/web-incoming.j
+++ b/packages/meteor-tool/.1.3.3.1t63v9e++os.linux.x86_64+web.browser+web.cordova/mt-os.linux.x86_64/dev_bundle/lib/node_modules/http-proxy/lib/http-proxy/passes/web-incoming.j
@@ -104,10 +104,17 @@ web_o = Object.keys(web_o).map(function(pass) {
       if(!options.target) { return res.end(); }
     }

-    // Request initalization
-    var proxyReq = (options.target.protocol === 'https:' ? https : http).request(
-      common.setupOutgoing(options.ssl || {}, options, req)
-    );
+    try {
+      // Request initalization
+      var proxyReq = (options.target.protocol === 'https:' ? https : http).request(
+        common.setupOutgoing(options.ssl || {}, options, req)
+      );
+    }
+    catch(err) {
+      console.log(err);
+      // must do an early return as to not take further errors
+      return;
+    }

     // Enable developers to modify the proxyReq before headers are sent
     proxyReq.on('socket', function(socket) {

The malformed request will take an "Error: socket hang up" but the server will stay up.

TooBug · 2016-09-14T07:10:59Z

I'm experiencing the same problem, any news on this recently?

blade254353074 · 2016-10-05T09:47:36Z

Have the same problem with _http_outgoing.js

bubenshchykov · 2016-11-01T14:22:09Z

+1

dlaha21 · 2016-11-10T11:26:05Z

@jcrugzz @fjakobs, is there any estimate on when you are thinking of releasing a fix? This issue is affecting us in production. Is there any easy workaround we can apply other than monkey patching the code?

fjakobs linked a pull request Feb 23, 2016 that will close this issue

Catch invalid headers #965

Open

kimown mentioned this issue May 21, 2016

The header content contains invalid characters TypeError: The header content contains invalid characters at ServerResponse. expressjs/express#3003

Closed

ppotoplyak mentioned this issue Jun 15, 2016

[1.3] Most basic projects not working in windows 7 meteor/meteor#6687

Closed

TooBug mentioned this issue Sep 14, 2016

http头不标准时崩溃 TooBug/CharlesX#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Node security release makes http-proxy crash #964

Node security release makes http-proxy crash #964

fjakobs commented Feb 22, 2016

jcrugzz commented Feb 22, 2016

fjakobs commented Feb 22, 2016

fjakobs commented Feb 22, 2016

fjakobs commented Feb 22, 2016

jcrugzz commented Feb 22, 2016

fjakobs commented Feb 22, 2016

jcrugzz commented Feb 22, 2016

ppotoplyak commented Jun 14, 2016

ppotoplyak commented Jun 14, 2016

ppotoplyak commented Jun 14, 2016

jcrugzz commented Jun 14, 2016

ppotoplyak commented Jun 15, 2016

TooBug commented Sep 14, 2016

blade254353074 commented Oct 5, 2016

bubenshchykov commented Nov 1, 2016

dlaha21 commented Nov 10, 2016

Node security release makes http-proxy crash #964

Node security release makes http-proxy crash #964

Comments

fjakobs commented Feb 22, 2016

jcrugzz commented Feb 22, 2016

fjakobs commented Feb 22, 2016

fjakobs commented Feb 22, 2016

fjakobs commented Feb 22, 2016

jcrugzz commented Feb 22, 2016

fjakobs commented Feb 22, 2016

jcrugzz commented Feb 22, 2016

ppotoplyak commented Jun 14, 2016

ppotoplyak commented Jun 14, 2016

ppotoplyak commented Jun 14, 2016

jcrugzz commented Jun 14, 2016

ppotoplyak commented Jun 15, 2016

TooBug commented Sep 14, 2016

blade254353074 commented Oct 5, 2016

bubenshchykov commented Nov 1, 2016

dlaha21 commented Nov 10, 2016