Skip to content
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

Handle 400 Request Header Or Cookie Too Large (494) #3112

Closed
wants to merge 1 commit into from
Closed

Handle 400 Request Header Or Cookie Too Large (494) #3112

wants to merge 1 commit into from

Conversation

ti-mo
Copy link

@ti-mo ti-mo commented Dec 19, 2017

Summary

Handle HTTP 494 with /kong_error_handler.

This recently came up in a security audit, we were leaking nginx error pages when sending an oversized header. (which also led us to include server_tokens off;, perhaps Kong should include this by default, too)

Can easily be triggered by eg. configuring key-auth and sending a large header value.

Upon inspection of the nginx source (https://github.com/nginx/nginx/blob/master/src/http/ngx_http_special_response.c#L244), I noticed that an 400 Request Header Or Cookie Too Large was merely an internal alias of HTTP 494, which is nginx-specific.

The block at https://github.com/nginx/nginx/blob/master/src/http/ngx_http_special_response.c#L394 revealed more of these corner cases:

    ngx_string(ngx_http_error_494_page), /* 494, request header too large */
    ngx_string(ngx_http_error_495_page), /* 495, https certificate error */
    ngx_string(ngx_http_error_496_page), /* 496, https no certificate */
    ngx_string(ngx_http_error_497_page), /* 497, http to https */
    ngx_string(ngx_http_error_404_page), /* 498, canceled */
    ngx_null_string, /* 499, client has closed connection */

Would you be interested in handling all of these with /kong_error_handler, perhaps? I'd have to investigate how to trigger them (except 499..).

Full changelog

  • Add HTTP code 494 to errors handled by /kong_error_handler

@p0pr0ck5
Copy link
Contributor

p0pr0ck5 commented Dec 20, 2017

This makes me wish the error_page directive could handle wildcards or negative exceptions, as I feel like we're never gonna stop seeing these ;) Seems sane overall. As for disabling server_tokens, that can be handled through a custom Nginx template: https://getkong.org/docs/0.11.x/configuration/#custom-nginx-configuration-embedding-kong... which, now that I think about it, would solve the specific need of this PR.

@ti-mo
Copy link
Author

ti-mo commented Dec 21, 2017

@p0pr0ck5 indeed, wildcards would be a good thing to have. (or even just a single catch-all would be useful to ensure no error pages are generated by nginx)

We use a custom template, I was just suggesting it wouldn't be a bad thing if it would be enabled by default. As you hopefully noticed, this is a PR to the default template to avoid people from having to customize it. Of course it can already be customized by the user. :)

@thibaultcha
Copy link
Member

@ti-mo Could you also edit kong/core/error_handlers.lua to that Kong responds with the appropriate message for this status code? Could you also provide a test? Thanks!

@thibaultcha thibaultcha added the pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. label Jan 20, 2018
thibaultcha pushed a commit that referenced this pull request Apr 10, 2018
Catch HTTP 494 errors from nginx into the native Kong error handler.

From #3112

Signed-off-by: Thibault Charbonnier <[email protected]>
@thibaultcha
Copy link
Member

Manually merged with a tiny addition. Thank you!

@darrenjennings
Copy link
Contributor

✨ 💯 ✨

@ti-mo
Copy link
Author

ti-mo commented Apr 16, 2018

@thibaultcha sorry, fell off my radar. Thanks for picking this up!

deltasquare4 added a commit to deltasquare4/kong-dashboard that referenced this pull request May 3, 2018
deltasquare4 added a commit to deltasquare4/kong-dashboard that referenced this pull request Jun 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants