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

feat: Add headers, panic, echo and store utilities #728

Merged
merged 6 commits into from
Feb 7, 2025

Conversation

guikcd
Copy link
Contributor

@guikcd guikcd commented Jan 23, 2025

Closes: #596

  • GET /headers Print the heads of the inbound request
  • GET /panic Shutdown the application with an error code
  • POST /echo Write back the payload sent to it
  • POST /store Write the payload to a file and return a hash
  • GET /store/{hash} Return the payload from the file system previously stored

My tests:

$ curl localhost:8080/utility/headers
{"Host":["localhost:8080"],"User-Agent":["curl/8.7.1"],"Accept":["*/*"],"X-Session-ID":["ac60f659-0b4e-4d49-9b2a-5f6fbcf8c250"]}

$ curl --include localhost:8080/utility/panic
HTTP/1.1 500 Internal Server Error
Content-Type: text/plain;charset=UTF-8
Content-Length: 16
set-cookie: SESSIONID=ff033cf5-08f6-4f6c-b4c9-4fb89f9b9fdd

Shutting down...

$ curl localhost:8080/utility/echo -X POST -H "Content-Type: application/json" -d '{"store": "store2"}'
{"store": "store2"}

$ curl localhost:8080/utility/store -X POST -H "Content-Type: application/json" -d '{"store": "store1"}'
{"hash": "1698894751"}

$ curl localhost:8080/utility/store/1698894751
{"store": "store1"}

# fill /tmp....
curl localhost:8080/utility/store -X POST -H "Content-Type: application/json" -d '{"store": "store2}"'
HTTP/1.1 500 Internal Server Error
Content-Type: text/plain;charset=UTF-8
Content-Length: 43
set-cookie: SESSIONID=52ab08e3-b084-432a-97c4-7f444eaebcc9

Error writing file: No space left on device

# try reading nonexistent hash
$ curl --include localhost:8080/utility/store/123456789
HTTP/1.1 404 Not Found
Content-Type: text/plain;charset=UTF-8
Content-Length: 67
set-cookie: SESSIONID=6b6855f1-56e2-4818-93bb-16ef5fdbc4ca

Error reading file: /tmp/123456789.json (No such file or directory)

# try path traversal
$ curl --include "http://localhost:8080/utility/store/%2e%2e"
HTTP/1.1 400 Bad Request
Content-Type: text/plain;charset=UTF-8
Content-Length: 19
set-cookie: SESSIONID=8c81f0e6-71ab-4960-8ecd-8409127cb496

Invalid hash format

curl -i "http://localhost:8080/utility/store/../etc/passwd"

HTTP/1.1 404 Not Found
Content-Type: application/json
Content-Length: 114
set-cookie: SESSIONID=4f72d053-59eb-41f5-ab3e-0a54062e2d99

{"timestamp":1738924559726,"path":"/utility/etc/passwd","status":404,"error":"Not Found","requestId":"984ee8bf-2"}

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

// function /store/{hash} that read a local hash file
@GetMapping("/store/{hash}")
@ResponseBody
public ResponseEntity<String> read(@PathVariable String hash) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guikcd can we put in protections to make sure this can only open files within /tmp? Right now you could probably use something like ../root/whatever to traverse the filesystem. You'll probably have to get the absolute path to the file it tries to load and verify its in /tmp.

Copy link
Contributor Author

@guikcd guikcd Feb 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 mechanisms already prevented us having "complete" path traversal:

  • GetMapping /store/{hash} didn't understand ../root/...
  • The code always add a suffix .json only these files was subject to path traversal.

As it is still a threat, I've changed the code to:

  • filter the hash format to numbers only
  • verify that the file always start from /tmp/. Due to previous control, nearly not possible to reach here

@niallthomson niallthomson merged commit c4f703b into aws-containers:main Feb 7, 2025
5 of 6 checks passed
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.

Add demo utility functions to the UI component
2 participants