Fixed Apache range header DOS and proxy disclosure issue #5193
Conversation
pull-request-size
bot
added
the
size/XS
m4dcoder
left a comment
m4dcoder
left a comment
There was a problem hiding this comment.
Got some questions. Please clarify. Thanks.
|
|
||
| if ($request_method !~ ^(GET|HEAD|POST)$ ) { | ||
| return 405; | ||
| } |
There was a problem hiding this comment.
Is this section required below for the main API server on 443 ssl?
There was a problem hiding this comment.
If we also restrict this on the 443 server, note these
- There are API functons that use
PUT. Ref: https://github.com/StackStorm/st2/blob/master/st2api/st2api/controllers/v1/actions.py#L156 - There are API functions that use
DELETEto remove resources. How will this impact these functions? Ref: https://github.com/StackStorm/st2/blob/master/st2api/st2api/controllers/v1/actions.py#L208
There was a problem hiding this comment.
Yeah, as @m4dcoder said this is likely untested and won't work - API operations also utilize PUT, POST, OPTIONS (for cors) HTTP method.
And likely correct CORS headers we send should already be sufficient, but if we can correctly identify unused methods, I'm also find with filtering those out at the nginx level.
There was a problem hiding this comment.
@m4dcoder - I have modified the code and the only TRACE will not be allowed.
conf/nginx/st2.conf
Outdated
| root /opt/stackstorm/static/webui/; | ||
| index index.html; | ||
|
|
||
| proxy_set_header Range ""; |
There was a problem hiding this comment.
How about the other locations above (i.e. /auth/, /api/, etc.)?
There was a problem hiding this comment.
Is this more of a hypothetical / possible vulnerability or the code is actually vulnerable to this issue?
Because afaik, st2api doesn't handle Range header anywhere (so it should just be ignored). Or does it apply to nginx server static files?
pull-request-size
bot
added
size/S
conf/nginx/st2.conf
Outdated
| if ($request_method !~ ^(GET|HEAD|POST|PUT|DELETE)$ ) { | ||
| return 405; | ||
| } |
There was a problem hiding this comment.
| if ($request_method !~ ^(GET|HEAD|POST|PUT|DELETE)$ ) { | |
| return 405; | |
| } |
Similar to what @m4dcoder was saying before, looks like the directive added is not effective for the HTTP server section here as it's just redirecting to the HTTPS one.
There was a problem hiding this comment.
I have added a directive for HTTPS as well.
TRACE will be not be allowed in the HTTP method. Screenshots of testing for your reference.
I have added max_ranges 0 to disable the partial content responses on Nginx. If a range is set as a byte then it is more vulnerable. It is recommended to not set the range as a byte.
There was a problem hiding this comment.
Thanks for the update.
As per my comment above - we also needs to support / whitelist OPTIONS that's used by st2web for CORS purposes.
|
Thanks for the contribution. I will add a changelog entry and merge it into master. |
4 participants
The default Nginx configuration can have many vulnerabilities, a fix for a few of them.