Process crashes when duplicate headers are received in a request

[jpasichnyk]

I'm seeing the process crash due to duplicate key exception on some requests, and I was originally thinking maybe duplicate headers or url parameters, but running TCP dump doesn't show anything of concern.

This same code base works fine on fastcgi-mono-server4.

Unhandled Exception: System.ArgumentException: An item with the same key has already been added. at System.ThrowHelper.ThrowArgumentException (ExceptionResource resource) <0x41d7df30 + 0x0002b> in :0 at System.Collections.Generic.Dictionary2[TKey,TValue].Insert (System.Collections.Generic.TKey key, System.Collections.Generic.TValue value, Boolean add) <0x40ead100 + 0x0018b> in <filename unknown>:0 at System.Collections.Generic.Dictionary2[TKey,TValue].Add (System.Collections.Generic.TKey key, System.Collections.Generic.TValue value) <0x40ead0c0 + 0x00023> in :0 at Mono.WebServer.HyperFastCgi.Request.ParseParameters (System.Byte[] data, Boolean parseHeaders) <0x40fa70c0 + 0x001e7> in :0 at Mono.WebServer.HyperFastCgi.Request.AddParameterData (System.Byte[] data, Boolean parseHeaders) <0x40fa6e20 + 0x0009b> in :0 at Mono.WebServer.HyperFastCgi.NetworkConnector.ProcessRecord (Record record) <0x40fa62f0 + 0x00153> in :0 at Mono.WebServer.HyperFastCgi.NetworkConnector.ReceiveCallback (IAsyncResult ar) <0x40fa58e0 + 0x0058f> in :0

Anything hints at where the duplicate might be coming from? I'm not sure what it means when it says "ParseParameters". Parameters for what?

I'm running on the current 0.3.4 stable, and i'm not able to find any method matching ParseParameters (System.Byte[] data, Boolean parseHeaders) to investigate what might be happening in the source. :/

[jpasichnyk]

Ok, i found the method, odd, but github search didn't find it for some reason. https://github.com/xplicit/HyperFastCgi/blob/v0.3_stable/src/Mono.WebServer.HyperFastCgi/Request.cs#L134

Here is an example set of headers from tcpdump that cause it to crash, as you'll notice there are no duplicate keys:

User-Agent: Mozilla/5.0 (iPad; CPU OS 6_1_3 like Mac OS X) AppleWebKit/536.26 (KHTML, like Gecko) Mobile/10B329
Accept-Language: zh-cn
Accept: */*
X-Akamai-CONFIG-LOG-DETAIL: true
TE:  chunked;q=1.0
Connection: TE
Accept-Encoding: gzip
Akamai-Origin-Hop: 1
Via: 1.1 akamai.net(ghost) (AkamaiGHost)
X-Forwarded-For: 1.2.3.4

I have tried forcing all those headers/values for some test calls, and it doesn't reproduce the crash. :/

[jpasichnyk]

My /etc/nginx/fastcgi_params that gets loaded for the host is:

fastcgi_param  QUERY_STRING       $query_string;
fastcgi_param  REQUEST_METHOD     $request_method;
fastcgi_param  CONTENT_TYPE       $content_type;
fastcgi_param  CONTENT_LENGTH     $content_length;

fastcgi_param  SCRIPT_NAME        $fastcgi_script_name;
fastcgi_param  REQUEST_URI        $request_uri;
fastcgi_param  DOCUMENT_URI       $document_uri;
fastcgi_param  DOCUMENT_ROOT      $document_root;
fastcgi_param  SERVER_PROTOCOL    $server_protocol;
fastcgi_param  HTTPS              $https if_not_empty;

fastcgi_param  GATEWAY_INTERFACE  CGI/1.1;
fastcgi_param  SERVER_SOFTWARE    nginx/$nginx_version;

fastcgi_param  REMOTE_ADDR        $remote_addr;
fastcgi_param  REMOTE_PORT        $remote_port;
fastcgi_param  SERVER_ADDR        $server_addr;
fastcgi_param  SERVER_PORT        $server_port;
fastcgi_param  SERVER_NAME        $server_name;

# mono
fastcgi_param PATH_INFO "";
fastcgi_param SCRIPT_FILENAME $document_root$fastcgi_script_name;

[jpasichnyk]

I may have misidentified the request that was crashing the service. Now i'm seeing these problematic requests have two X-Forwarded-For headers, which seem to be injected by my haproxy loadbalancer.

Looking at their documentation at https://cbonte.github.io/haproxy-dconv/configuration-1.5.html#4.2-option%20forwardfor It says: Since this header is always appended at the end of the existing header list, the server must be configured to always use the last occurrence of this header only. See the server's manual to find how to enable use of this standard header. Note that only the last occurrence of the header must be used, since it is really possible that the client has already brought one.

It looks like this is likely my case, where there is already one supplied, and its adding another instead of appending to a list of values.

Is it really the expected behavior of hyperfastcgi that it should crazy if multiple of the same header are received? This seems like a huge potential for a DoS attack, right?

[xplicit]

Please check that you don't have duplicate fast_cgi parameters in nginx config. You issue looks like that https://github.com/xplicit/HyperFastCgi/issues/18. Duplicate headers should be handled normally.

To localize the parameter, which breaks your request you can wrap method ParseParameters into try/catch and log the variable name.

If hyperfastcgi can be broken by duplicate headers, it should be easily reproduced by plain HTTP request with two strings of the same header name. If you have such one request, please post it here, because this request should not generate an exception

[jpasichnyk]

Yeah, I triple checked that i don't have duplicate values in nginx.conf / fastcgi_params, as i came across that ticket first.

I'll recompile a copy of the binary with the try/catch to try to isolate the parameter causing the issue as you suggested. I'll also try breaking it with some calls with duplicate headers, to ensure that's not a point of attack.

[jpasichnyk]

First thing, the duplicate header test. I just ran this, and on the first request it crashes the fastcgi process instantly. :(

curl -X GET -H "User-Agent: Mozilla/5.0 Gecko/2010 Firefox/5" -H "User-Agent: Mozilla/5.0 Gecko/2010 Firefox/6" "http://192.168.1.2:80/" 

I think that header parsing needs to check for existence of a given header in the collection, and overwrite it if it already exists (last one wins) or something. Because this is not good for production use if someone can take down a process this easy. :/

Before I put together a pull request, is that a change you'd be open to?

[xplicit]

Headers should not be owerwriten if they already exist in the array. According to RFC multiple headers should be concatenated by comma. I supposed that nginx should do that work on the front-end before sending requests to FastCGI backend. Need to check this nginx behaviour before doing fix. Also the fix should work carefully with nginx fast_cgi server variables (so it should concatenate only headers, but not server variables).

http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2

Multiple message-header fields with the same field-name MAY be present in a message if and only if the entire field-value for that header field is defined as a comma-separated list [i.e., #(values)]. It MUST be possible to combine the multiple header fields into one "field-name: field-value" pair, without changing the semantics of the message, by appending each subsequent field-value to the first, each separated by a comma. The order in which header fields with the same field-name are received is therefore significant to the interpretation of the combined field value, and thus a proxy MUST NOT change the order of these field values when a message is forwarded.

[jpasichnyk]

My test above was running through nginx and both still made it separately to the fastcgi process. So it doesn't look like nginx is doing anything fancy here (at least by default) to combine those duplicate headers into single header with multiple values, etc.

How I understand this, is if someone did want send multiple header values with the same name,then they are expected to comma separate them but only pass a single header name with the multiple values. If they did that then we don't have this issue. If they however sent a malformed request (as i did), I think all bets are off as it's not supported by the protocol. In this case of the malformed request, I'd personally be OK taking either the first or the last, as long as the request doesn't crash the server, but maybe i'm missing something in protocol spec on how to handle it.

That said, I think this exposes a bigger issue in that there isn't enough isolation in the current request handling to prevent a malformed request from taking down the process, making it very susceptible to DoS attacks if any bug is found in request handling. If there is a malformed request, or something unexpected happens while handling the request, it should only impact that specific request, not the entire process, IMO.

[xplicit]

RFC says that multiple headers values must be equal to comma-separated values of single header. So the logic in hyperfastcgi (if nginx does not combine multiple headers into single line) should be:

1. check the header exists in the dictionary
2. if not, add the header name and value
3. if exists concatenate new value with the old one.

example. After parsing

X-Test: value1
X-Test: value2

HyperFastCgi should have an item in headers dictionary with the name X-Test and value value1, value2. And yes, it must not become down.

[jpasichnyk]

It seems to me that concatenating them together (if mulitple values are intended) would be the responsibility of the caller (thing adding the additional value) though, and if they didn't adhere to the RFC in sending the request then that's their problem.

Here's the issue I see with just concatenating everything:

Some things are expected to contain multiple values.
Example:

Accept-Encoding: gzip, deflate

or

Cache-Control: no-cache, max-age=0

Some things are (from what I've seen) assumed to only contain one, examples:

User-Agent
Host
Referer

If someone decided to send multiple values of one of those headers that are more or less assumed to have just one, and multiple get concatenated together, do we end up breaking more things downstream in frameworks/applications that have always assumed a single value?

[xplicit]

We don't know which headers supports lists and which are not (also note that some headers can be user defined). User can send malformed HTTP request with two headers which do not support lists, or it can send the same request with one header with manually concatenated values. Both will be malformed and detecting and dropping out these malformed request is not the job of fastcgi backend, this should be done by front-end or even by routers/firewalls. FastCGI just should pass HTTP request from nginx front-end to web application and return the response from web application to nginx front end, without any additional logic and without crashes.

[jpasichnyk]

Ok, so if it's not the job of fastcgi to scrub these requests, I'm fine with that. But that said, if one malformed reques tis received, and the entire process crashes, that is not good...

So I guess in your description of what fastcgi responsibility is and is not... The headers probably shouldn't be stored in a Dictionary, if the process doesn't want to take responsibility for enforcing uniqueness of the headers. Does the fastcgi even need to have access to these headers at all, or can it just proxy them on through to the application?

[xplicit]

As I wrote before, the working logic with headers should be changed in HFC. Multiple headers should be concatenated into single one in HyperFastCGI to avoid crashing.

The headers are just collected before sending to web application by creating corresponding ASP.NET HttpRequest. In ASP.NET headers are stored as NameValueCollection, so we can't cardinally change the behaviour of headers storing (for example in NodeJs multiple headers values stored as array). Anyway we must add only one header name to the final NameValueCollection in ASP.NET HttpRequest.

[jpasichnyk]

Ok, want me to take a swing at it?

[xplicit]

All fixes will be appreciated

[jpasichnyk]

Regarding:

Also the fix should work carefully with nginx fast_cgi server variables (so it should concatenate only headers, but not server variables).

To be clear, when you say "server variables" do you mean something matching GetKnownRequestHeaderIndex(header)?

Or does it mean the stuff pulled by name from the parameters_list like GetParameter ("SCRIPT_NAME") ? And if so, is there an enum or comprehensive list of those anywhere?

[xplicit]

All headers are sent by FCGI protocol (except some special one like "Content-Length") start from the prefix "HTTP". During parsing FCGI request prefix "HTTP" is removing and headers are adding to the headers dictionary. All other parameters not started with "HTTP_" prefix are server variables. So only headers should be concatenated, but not server variables.

In v0.4 there are functions ParseParameters and ReformatHttpHeader which do this job. If ReformatHttpHeader returns empty string then we deal with server variable, if string is not empty then we've got header. In v0.3 the logic is the same.

[jpasichnyk]

OK thanks for the clarification. I will do my changes based on v0.3, since it is stable release and I think this is a stability fix.

[jpasichnyk]

Right now "known headers" get completely overwritten by knownHeaders [idx] = value;

I'm assuming we should modify that to do the add or concatenate logic as well, right?

[xplicit]

You are right, known headers must be also modified by concatenating new value