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

[5.8] Handle SuspiciousOperationException in router #28866

Merged
merged 4 commits into from Jun 18, 2019

Conversation

Projects
None yet
4 participants
@mpskovvang
Copy link
Contributor

commented Jun 17, 2019

Some vulnerability scanners manipulate HTTP headers sent to the server. Symfony checks for suspicious operations with the X-Http-Method-Override and X-Forwarded-For headers, and throws an exception if the value seems invalid.

I would expect Laravel to handle these requests as not found, but instead the server responds with a server error.

This PR catch the SuspiciousOperationException thrown by Symfony and throws a NotFoundHttpException instead.

This prevents our error reporting tools from keep tracking this "expected" error.

It seems X-Forwarded-Host isn't affected in the Laravel router, but I would expect it to be based on the source code. Perhaps my test cases just didn't trigger the exception.

@SjorsO

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2019

Would it be possible to add the SuspiciousOperationException to the $internalDontReport array of the exception handler instead?

@mpskovvang

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2019

Would it be possible to add the SuspiciousOperationException to the $internalDontReport array of the exception handler instead?

Sure, that would prevent it from showing up in the error reporting, but still returns a 500 HTTP error. We believe the 404 HTTP code is more correct, as the request is not found rather than an unexpected server error.

We use $dontReport and overrides the prepareException method to replace SuspiciousOperationException with NotFoundHttpException. This is a fine workaround, if this PR isn't of interest for the community. :)

@mpskovvang

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2019

I see this PR fails some tests because this exception was first added by Symfony on 2 April. I wasn't aware of this when writing the test.

Source: symfony/http-foundation@d325ae5#diff-7edb274bc39ed8c493badba7dd278826

try {
$routes = $this->get($request->getMethod());
} catch (SuspiciousOperationException $e) {
throw new NotFoundHttpException(null, $e);

This comment has been minimized.

Copy link
@GrahamCampbell

GrahamCampbell Jun 17, 2019

Member

Should we forward e->code too, and also set some kind of exception message?

@mpskovvang

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2019

@SjorsO, I believe, I misunderstood your answer at first sight. You're probably right it is better to handle the exception in the internal exception handler rather than in the router itself.

What are your thoughts regarding the 500 code vs 404 code? Until the recent change from Symfony the error code would have been 404. Perhaps that is an argument to continue using 404?

@mpskovvang

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2019

@GrahamCampbell, I followed @SjorsO suggestion and moved the check to the internal exception handler. This ensure that suspicious operations is handled with HTTP 404 as of before April 2, but allows the developer to change to his desires by overriding the exception handler.

Do you like this approach better as well?

@@ -196,7 +198,7 @@ public function render($request, Exception $e)
*/
protected function prepareException(Exception $e)
{
if ($e instanceof ModelNotFoundException) {
if ($e instanceof ModelNotFoundException || $e instanceof SuspiciousOperationException) {

This comment has been minimized.

Copy link
@GrahamCampbell

GrahamCampbell Jun 17, 2019

Member

You should not propagate the exception message from SuspiciousOperationException to NotFoundHttpException since we don't want to show it to users.

This comment has been minimized.

Copy link
@GrahamCampbell

GrahamCampbell Jun 17, 2019

Member

Unless, we do? Is the message you used in the tests actually realistic?

This comment has been minimized.

Copy link
@mpskovvang

mpskovvang Jun 17, 2019

Author Contributor

I believe we do. The 404 response was returned until the change was made by Symfony in April. The current 500 seems like an unhandled error. We see these attempts quite often. I believe attackers tries to scan for vulnerabilities in routing functionality (with no success in this case).
I found the same error reported here: octobercms/october#4359

This comment has been minimized.

Copy link
@GrahamCampbell

GrahamCampbell Jun 17, 2019

Member

Right, in which case we don't want to pass along the real exception message. Just having it available as the previous exception is enough. It is often the case that people will show the exception message of httpnotfoundexception to users, so we don't want to show anything that is not meant for users.

This comment has been minimized.

Copy link
@mpskovvang

mpskovvang Jun 17, 2019

Author Contributor

Sorry, I got your first reply wrong. I didn't thought of users showing the actual exception message, so you're probably right that we shouldn't pass the message a long.
I'll put this in a separate statement below the others with a null message in the NotFoundHttpException.

This comment has been minimized.

Copy link
@mpskovvang

mpskovvang Jun 18, 2019

Author Contributor

I updated this PR to prevent propagating the exception message.

@taylorotwell

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

Is it true that we want to bake the 404 response into the framework? You already have control over the response via the render method of the exception handler and can easily return a 404 there. Might some applications not want to pay careful attention to these types of requests or possibly ban IP addresses?

@mpskovvang

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2019

@taylorotwell that's a really good point! Having the option to react to these suspicious operations seems reasonable, and you're right that the developer can easily adjust the exception handler to return a 404 response like we did.
Based on that, this PR is probably not necessary. Perhaps a doc hint is better, unless you'll just leave it up to the developers of the affected applications to detect the exception from the logs?

@SjorsO

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2019

I'm in favor of the current implementation of this PR. I think this exception can be ignored by the framework by default.

In many of my projects I get either slack or email notifications when errors happen. I'm not a fan of having to add code to each project to prevent being notified about this harmless exception.

@mpskovvang

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

@SjorsO the notifications was the same reason for me to open a PR to prevent coping the error handling to others projects.

If someone likes to ban IP addresses as suggested by @taylorotwell, this can be done by overriding the exception handler.

The exception isn't harmful, but the default server error might indicate to an attacker, something isn't fully handled. A default 404 response would perhaps be a better choice.

Another consideration is the 400 response indicating a bad request which is just the case here. But Laravel doesn't come with a default 400 error view. However, no reel user should ever see this error as the application should never trigger the exception - only suspicious users.

@taylorotwell taylorotwell merged commit 21cb37d into laravel:5.8 Jun 18, 2019

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.