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

Strip query parameters from request URL in preview server #347

Merged
merged 4 commits into from Sep 14, 2021
Merged

Strip query parameters from request URL in preview server #347

merged 4 commits into from Sep 14, 2021

Conversation

edkelly-ovo
Copy link
Contributor

@edkelly-ovo edkelly-ovo commented Aug 26, 2021

What/Why/How?

The file packages/cli/src/commands/preview-docs/preview-server/preview-server.ts contains a security defect that allows directory traversal of the complete file path on a server and the download of files via query parameters on the request URI.

Example:

http://localhost:8080/jsp/help-sb-download.jsp?sbFileName=../../../../.redocly.yaml

Applying a request.url.split('?')[0] at the start of the preview server function has stopped this from occurring.

Reference

https://github.com/Redocly/openapi-cli/blob/master/packages/cli/src/commands/preview-docs/preview-server/preview-server.ts#L99

The usage of path.resolve() on this line can result in arbitrary download of any file on the server if query params are exploited.

Testing

Fix tested manually

Screenshots (optional)

Check yourself

  • Code is linted
  • Tested with redoc/reference-docs/workflows
  • All new/updated code is covered with tests

Security

  • Security impact of change has been considered
  • Code follows company security practices and guidelines

@RomanHotsiy
Copy link
Member

@edkelly-ovo I can't actually reproduce the bug you describe. And I can't understand how it can happen from code. path.resolve will keep the ? in place so it will just return 404.

Can you provide more details?

@edkelly-ovo
Copy link
Contributor Author

edkelly-ovo commented Sep 7, 2021

@RomanHotsiy if I navigate to http://localhost:8080/jsp/help-sb-download.jsp?sbFileName=../../../../../../../../../../../../etc/passwd in Google Chrome on OSX 11.5, the server triggers a download of my /etc/passwd file.

This is happening because path.resolve is being provided the following arguments:

  • ./static
  • ./jsp/help-sb-download.jsp?sbFileName=../../../../../../../../../../../../etc/passwd

which is yielding the path /etc/passwd when run.

path.resolve is interpreting (correctly) the second argument as a path to a file (which exists on disk). This file is then being sent in response via the respondWithGzip function.

If I strip off the query params from request.url, this stops the directory traversal from being possible

@edkelly-ovo
Copy link
Contributor Author

@RomanHotsiy any update on this please?

@RomanHotsiy
Copy link
Member

So I was able to reproduce it, it looks that in order to exploit it the file with the question mark must actually exists on the filesystem: help-sb-download.jsp?sbFileName=...

I would implement a more robust solution by checking if the file path is within the current working dir. I pushed changes to this PR. @edkelly-ovo could you please verify it resolved the issue?

@edkelly-ovo
Copy link
Contributor Author

Hi @RomanHotsiy yes that works, thanks! I've reverted my original code which was a bit heavy handed, your solution is much more elegant.

I spotted a typo in preview-server.ts on the isSubdir import which I've also fixed :)

@RomanHotsiy RomanHotsiy merged commit 69a1580 into Redocly:master Sep 14, 2021
@edkelly-ovo
Copy link
Contributor Author

Thanks @RomanHotsiy when is the next release going out please?

@RomanHotsiy
Copy link
Member

#366

@bmocanu
Copy link

bmocanu commented Oct 12, 2021

I just tried this issue with latest version (1.0.0-beta.62) and I still can reproduce it. Calling something like "http://127.0.0.1:8080/jsp/help-sb-download.jsp?sbFileName=../../../../../../../../../test.txt" gives me a download file with the name "help-sb-download.jsp" but with the content of "test.txt", which is located in my D:\ root (Windows machine). So in theory I can download anything from the current drive.

@edkelly-ovo is the latest version working for you? What do you get when you call your attack URL? 404?

@edkelly-ovo
Copy link
Contributor Author

edkelly-ovo commented Oct 12, 2021

@bmocanu with version 1.0.0-beta.62, I am not able to reproduce the issue I saw where my /etc/passwd file was downloadable using the URL left in comment #347 (comment). I receive a 404 message

@RomanHotsiy
Copy link
Member

@bmocanu it may be related to windows maybe... We're looking into this.

@bmocanu
Copy link

bmocanu commented Oct 12, 2021

I confirm, I am not able to reproduce it on a Debian box. So I guess it is just an issue for Windows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants