-
Notifications
You must be signed in to change notification settings - Fork 49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Process crashes when duplicate headers are received in a request #42
Comments
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:
I have tried forcing all those headers/values for some test calls, and it doesn't reproduce the crash. :/ |
My /etc/nginx/fastcgi_params that gets loaded for the host is:
|
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 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? |
Please check that you don't have duplicate fast_cgi parameters in nginx config. You issue looks like that #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 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 |
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. |
First thing, the duplicate header test. I just ran this, and on the first request it crashes the fastcgi process instantly. :(
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? |
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
|
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. |
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:
example.
HyperFastCgi should have an item in headers dictionary with the name |
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.
or
Some things are (from what I've seen) assumed to only contain one, examples:
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? |
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. |
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? |
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. |
Ok, want me to take a swing at it? |
All fixes will be appreciated |
Regarding:
To be clear, when you say "server variables" do you mean something matching Or does it mean the stuff pulled by name from the |
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 |
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. |
Right now "known headers" get completely overwritten by I'm assuming we should modify that to do the add or concatenate logic as well, right? |
You are right, known headers must be also modified by concatenating new value |
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.Dictionary
2[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.Dictionary
2[TKey,TValue].Add (System.Collections.Generic.TKey key, System.Collections.Generic.TValue value) <0x40ead0c0 + 0x00023> in :0at 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. :/
The text was updated successfully, but these errors were encountered: