Filter out invalid URI and HTTP method in the error message of no handler found for a REST request

[tlfeng]

Description

Filter out invalid URI and HTTP method of a error message, which shown when there is no handler found for a REST request sent by user, so that HTML special characters <>&"' will not shown in the error message.

The error message is return as mine-type application/json, which can't contain active (script) content, so it's not a vulnerability. Besides, no browsers are going to render as html when the mine-type is that.
While the common security scanners will raise a false-positive alarm for having HTML tags in the response without escaping the HTML special characters, so the solution only aims to satisfy the code security scanners.

If the URI is valid, the error message remains.
If the URI is not valid, the change is:
old error message: {"error":"no handler found for uri [/<script>cross_site_scripting.nasl</script>.asp] and method [GET]"}
new error message: {"error":"invalid uri has been requested"}

If the HTTP method is not defined in the enum: https://github.com/opensearch-project/OpenSearch/blob/2.0.0/server/src/main/java/org/opensearch/rest/RestRequest.java#L236, the change of error message is:
old error message: {"error":"Unexpected http method: <script>alert(\"!\")</script>","status":405}
new error message: {"error":"Unexpected http method","status":405}

Reference: #3459 (comment)

Issues Resolved

Resolve #3453

Check List

• New functionality includes testing.
  • All tests pass
• New functionality has been documented.
  • New functionality has javadoc added
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[opensearch-ci-bot]

❌   Gradle Check failure ec2012e127c0514e9022fbc09dccc98645b700a7
Log 5623

Reports 5623

[opensearch-ci-bot]

❌   Gradle Check failure 17b7947628a8cdbb54d298c1a7827c4c7dfc453d
Log 5624

Reports 5624

[opensearch-ci-bot]

✅   Gradle Check success 5e1934be34eb90390e5ef82f33256b4b59ec3d8c
Log 5625

Reports 5625

[kotwanikunal]

OWasp also recommends '/'

I think that will conflict with the URI/route path though.

[tlfeng]

Thanks for your review! 👍
Ah, I didn't realized that the slash "/" is also a character that recommended to deal with. 😂 But your thought is correct that it will make unreadable for the URI in the error message.
Frankly speaking, I even think my current code to do the HTML escape is over-engineering. URLEncode (which will escape the <, >, and & characters to %3C, %3E, and %26, respectively) is likely enough to satisfy the requirement, and there is native Java API to do that, but it's just not a professional solution for XSS problem (https://security.stackexchange.com/questions/8867/urlencode-vs-htmlencode-for-xss-prevention).

[reta]

@tlfeng I am a bit concerned with rolling out own sanitization schemes, may be we should use OWASP HTML sanitizer instead [1]?

[1] https://github.com/OWASP/java-html-sanitizer/blob/master/docs/getting_started.md

[tlfeng]

Hi @reta, thanks for the opinion!
My reason for not using the additional library for sanitizing HTML is:
The purpose of the code change is to satisfy the security scanners so that they won't complain, instead of resolving actual vulnerability, so I don't think it worth to add a new Java dependency to the code base. Using additional library to the code will enlarge the file size for the build artifact, and adding maintenance burden for upgrading the library.

So I would rather using a simpler solution, to use a Java native API URLEncoder, URLEncoder.encode(String s, String enc), which achieves the goal in the issue, but it's not a quite correct way to solve XSS problem.

That's my personal thought, so may not reasonable. What do you think of it 😄

[reta]

thanks @tlfeng, I think URLEncoder.encode won't fit here: {"error":"no handler found for uri [/a/b/c] and method [DELETE]"} would become something like {"error":"no handler found for uri [%2Fa%2Fb%2Fc] and method [DELETE]"}, not very readable.

What we are dealing with is actually the fact that any "garbage" could be sent to Opensearch, for example {"error":"no handler found for uri [/a/b/c<script>alert(\"!\")</script>] and method [DELETE]"} - this is not even valid URI. May be what we should do instead, is to distinguish those two issue:

• uri is not valid: return {"error":"invalid uri has been requested"}
• uri is valid but not found, in this case it is sufficient to include uri.getPath() only: return {"error":"no handler found for uri [${uri.getPath()}] and method [DELETE]"}

The funny part is that HTTP method is also vulnerable to XSS attacks:

 curl -ik -X '<script>alert("!")</script>' 'https://localhost:9200/a/b/c'

{"error":"Unexpected http method: <script>alert(\"!\")</script>","status":405}

We should probably follow the same approach, the list of HTTP methods is more or less stable, we could reply to the "garbage" something like: {"error":"Unsupported http method","status":405}

[tlfeng]

@reta Thanks for your comprehensive solution! And you are so careful to find the HTTP method will be shown in the error message as well. 😂
I think it's really reasonable. Instead of encoding HTML special characters, not showing the invalid user input sounds like a simple and effective solution. 😄 I will update the code based on it.

[opensearch-ci-bot]

✅   Gradle Check success 39d0951777db309459b64de12ac05b2d80eadfa0
Log 5639

Reports 5639

[opensearch-ci-bot]

✅   Gradle Check success ef4e190
Log 5640

Reports 5640

[reta]

LGTM, do you want to sanitize method as well?

[tlfeng]

Your review is so quick!
Sure, I can sanitize the error message for the HTTP method, although it may not necessary, since there is no code security scanners have an alarm on that. 😄
I will update the code then.

Note:
After taking a look at the code, the error message of invalid HTTP method is directly obtained from the
https://github.com/opensearch-project/OpenSearch/blob/2.0.0/server/src/main/java/org/opensearch/rest/RestController.java#L450
https://github.com/opensearch-project/OpenSearch/blob/2.0.0/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpRequest.java#L148

[tlfeng]

Updated in the new commit 863e272 . 👍

[opensearch-ci-bot]

✅   Gradle Check success 34f1b8f
Log 5706

Reports 5706

[opensearch-ci-bot]

❌   Gradle Check failure cb681d542046b48c1c4f7f9306c5ae5763c4c98f
Log 5708

Reports 5708

[reta]

Super nit-pick (sorry): Unexpected HTTP method (just to make it concise with previous error Incorrect HTTP method phrasing.

[tlfeng]

Haha, that definitely makes the messages neat.

[tlfeng]

Updated in the commit c586b5b

[reta]

I think this case case throws an exception when getHttpMethod() is requested, could you please add a test case with HTTP method like "<script>alert('xss');alert(\"&#x6A&#x61&#x76&#x61\");</script>" to make sure it won't sneak in? Thank you.

[tlfeng]

Looks like this test case shows the result of any unsupported HTTP method, since the handler for HTTP method will always throw an exception https://github.com/opensearch-project/OpenSearch/blob/2.0.0/server/src/test/java/org/opensearch/rest/RestControllerTests.java#L560.
I will add a separate test for it, to use the real scenario.

[tlfeng]

@reta Sorry I find it very hard to simulate the real process after receiving an specific wrong HTTP method in the unit test.
The HTTP method that used to build RestRequest comes from interface HttpRequest, and it has to be an item of the enum. I couldn't find an existing class that implements interface HttpRequest can accept arbitrary HTTP method, and can be used in the test class.
In real scenario, using any HTTP method other than those listed in the enum will get an exception, and the current code directly throws an exception, so I think the current unit test is likely enough.
To make the assertion more effective, I changed to use equalTo instead of containString to validate the error message, in the new commit 3a7a484

[reta]

Got it, sorry about that @tlfeng , thank you!

[tlfeng]

Never mind at all 😁. Thanks for your opinion.

[opensearch-ci-bot]

✅   Gradle Check success 863e272
Log 5709

Reports 5709

[opensearch-ci-bot]

❌   Gradle Check failure c586b5b
Log 5711

Reports 5711

[opensearch-ci-bot]

✅   Gradle Check success 3a7a484
Log 5715

Reports 5715