Added verification of X-HTTP-Method-Override before use #1556
Conversation
Verify that the method in X-HTTP-Method-Override is a valid method before use using it. Validation is performed using the same logic as net/http dose. This prevents a routing confusion vulnerability that allowed an attacker to control the entire internal revel routing path (used to find a controller) via the override header. This issue can be problematic in instances where authentication is based on the path for example in a revel.Filter or if the revel app is behind a reverse proxy.
|
this fix is almost identical to #1555. The functional difference is that my fix ignores the header when its invalid while movitz-s fix will return nil if the header is invalid. |
|
This looks good to me - @brendensoares thoughts? |
| @@ -169,7 +180,7 @@ type Router struct { | |||
|
|
|||
| func (router *Router) Route(req *Request) (routeMatch *RouteMatch) { | |||
| // Override method if set in header | |||
| if method := req.GetHttpHeader("X-HTTP-Method-Override"); method != "" && req.Method == "POST" { | |||
| if method := req.GetHttpHeader("X-HTTP-Method-Override"); method != "" && req.Method == "POST" && validMethod(method) { | |||
There was a problem hiding this comment.
It would be valuable to add a comment here for the context of why this line is necessary. Context for our future coders and our future selves.
There was a problem hiding this comment.
I mean we should note that this is required to negate the vulnerability. For example, you described to me privately this way:
We found that it was possible to control the http req path via the header X-HTTP-Method-Override. This issue can be problematic in instances where authentication is based on the path for example in a revel.Filter or if the revel app is behind a reverse proxy.
There was a problem hiding this comment.
added more meat to the comment
|
I appreciate the unit tests you included, but could you also provide steps to reproduce the vulnerability so we can verify this correctly addresses it? Thank you for contributing to Revel and OSS! |
| @@ -169,7 +180,7 @@ type Router struct { | |||
|
|
|||
| func (router *Router) Route(req *Request) (routeMatch *RouteMatch) { | |||
| // Override method if set in header | |||
| if method := req.GetHttpHeader("X-HTTP-Method-Override"); method != "" && req.Method == "POST" { | |||
| if method := req.GetHttpHeader("X-HTTP-Method-Override"); method != "" && req.Method == "POST" && validMethod(method) { | |||
There was a problem hiding this comment.
I mean we should note that this is required to negate the vulnerability. For example, you described to me privately this way:
We found that it was possible to control the http req path via the header X-HTTP-Method-Override. This issue can be problematic in instances where authentication is based on the path for example in a revel.Filter or if the revel app is behind a reverse proxy.
|
Added new code review comments. Also, I got your proof of concept code to test for this vulnerability in gitter. Thank you. |
.. why it's inportant that validMethod is called on repalce method
Verify that the method in X-HTTP-Method-Override is a valid method
before use using it. Validation is performed using the same logic
as net/http dose. This prevents a routing confusion vulnerability
that allowed an attacker to control the entire internal revel routing
path (used to find a controller) via the override header. This issue can
be problematic in instances where authentication is based on the path
for example in a revel.Filter or if the revel app is behind a reverse
proxy.