Prevent parent directory access, custom Errors #49

michalpokusa · 2023-04-14T07:23:22Z

Part of v3 Milestone, contains some minor breaking changes
If possible, please do not make it a separate "Release"

This PR prevents accessing files outside root server directory, Previously, when there was a ".." in path it was possible to do that, which was unsecure as users could access files with configuration or secrets, or any other file.

All paths are now checked by default, whether they contain ".." or "\" (backslash) inside. if that is teh sace a 403 Forbidden is returned.

In order to keep everything readable in clean, I intruduced custom exceptions, instead of raising e.g. RunTimeError or ValueError with a comment, now InvalidPathError or ResponseAlreadySentError.

Server's root_path is now specified in constructor, not in server_forever or pool methods.

Some minor refactor to separate logic for diffrent smaller tasks inside HTTPResponse.

Updated examples to use new constructor and to use settings.toml instead of secrets.py.

…ions

dhalbert

I looked at the code, but did not test it running. @anecdata, would you have a chance to test this?

dhalbert · 2023-04-19T16:37:50Z

adafruit_httpserver/response.py

+    Can be explicitly provided in the constructor, in ``send()`` or
+    implicitly determined from filename in ``send_file()``.


The documentation builder should be able to use single backticks and produce cross references to other methods/functions here. Did that fail?

I had problems with single ticks as it often couldn't find the member in X in module Y, not entirely sure why

Not a big deal; I've also found it doesn't work under strange circumstances.

@dhalbert Are there any more changes that you would like me to include?

…parent-directory-access

anecdata

Baseline testing with adafruit_httpserver 2.5.0, which allows serving static files from the filesystem root_path without a route. An explicit route will allow accessing a file outside of root_path.

But also, it is possible using a TCP socket to access files outside of the root_path without a route (by adding ../ to a path, for example). Thanks @Neradoc for identifying that in Discord. Typical browsers, and curl, filter that.

The current PR disallows ".." (and backslash) in a path. Tested successfully with:
Adafruit CircuitPython 8.1.0-beta.1-31-g4e25a4f6b on 2023-04-18; Adafruit QT Py ESP32S2 with ESP32S2. It is still possible for the user to explicitly allow "/" as the root_path.

LGTM

michalpokusa · 2023-04-19T20:22:00Z

Baseline testing with adafruit_httpserver 2.5.0, which allows serving static files from the filesystem root_path without a route. An explicit route will allow accessing a file outside of root_path.

But also, it is possible using a TCP socket to access files outside of the root_path without a route (by adding ../ to a path, for example). Thanks @Neradoc for identifying that in Discord. Typical browsers, and curl, filter that.

The current PR disallows ".." (and backslash) in a path. Tested successfully with: Adafruit CircuitPython 8.1.0-beta.1-31-g4e25a4f6b on 2023-04-18; Adafruit QT Py ESP32S2 with ESP32S2. It is still possible for the user to explicitly allow "/" as the root_path.

LGTM

Thank you for testing @anecdata.

Just a note. It is still also possible to do an explicit route to serve file outside root_path.

FoamyGuy · 2023-04-22T16:39:06Z

examples/httpserver_cpu_information.py

@@ -15,14 +15,15 @@
 from adafruit_httpserver.server import HTTPServer


-ssid, password = secrets.WIFI_SSID, secrets.WIFI_PASSWORD  # pylint: disable=no-member
+ssid = os.environ.get("WIFI_SSID")
+password = os.environ.get("WIFI_PASSWORD")


I tested this out on a Feather S2 TFT and found that I had this error raised:

File "code.py", line 18, in <module> AttributeError: 'module' object has no attribute 'environ'

I think maybe we don't have the environ api and instead may need to use getenv like: os.getenv('WIFI_SSID') I tried changing the code in one of the examples to use getenv instead and I was able to execute it successfully

Good catch. Is os.getenv('WIFI_SSID') the standard for examples now for wifi connect not via web workflow?

Good catch. Is os.getenv('WIFI_SSID') the standard for examples now for wifi connect not via web workflow?

Seems that it is, https://docs.circuitpython.org/en/latest/docs/environment.html, although libs like adafruit_requests or adafruit_minimqtt still use secrets in their example(s).

My bad for using the os.environ, I wrongly assumed that it will work the same way as in Python, already fixed in the latest commit. Thanks for testing @FoamyGuy

FoamyGuy

I think we may need to use a slightly different API to access the environment vars.

FoamyGuy

This looks good to me at this point.

I tested the latest version successfully on a Feather S2 TFT using the examples from this repo.

michalpokusa · 2023-04-24T14:59:17Z

Thank you for merging. I see that teh changes were released under 3.0.0 and the 3.0.1. Considering I have some more changes, improvements, some of which are not backwards compatible it might be necessary to release that the as 4.0.0. Maybe it is better to do it this way as it might take some time to review and correct everything in incoming 4.0.0 PR.

Updating https://github.com/adafruit/Adafruit_CircuitPython_AGS02MA to 1.0.4 from 1.0.3: > Merge pull request adafruit/Adafruit_CircuitPython_AGS02MA#3 from jposada202020/docs > Add upload url to release action > Add .venv to .gitignore > Update .pylintrc for v2.15.5 > Fix release CI files > Update pylint to 2.15.5 > Updated pylint version to 2.13.0 > Switching to composite actions Updating https://github.com/adafruit/Adafruit_CircuitPython_CLUE to 3.0.14 from 3.0.13: > Merge pull request adafruit/Adafruit_CircuitPython_CLUE#59 from julianaklulo/main > Add upload url to release action > Add .venv to .gitignore > Update .pylintrc for v2.15.5 > Fix release CI files > Update pylint to 2.15.5 > Updated pylint version to 2.13.0 > Switching to composite actions Updating https://github.com/adafruit/Adafruit_CircuitPython_EPD to 2.11.0 from 2.10.8: > Merge pull request adafruit/Adafruit_CircuitPython_EPD#61 from ladyada/master > Add upload url to release action > Add .venv to .gitignore > Update .pylintrc for v2.15.5 > Fix release CI files > Update pylint to 2.15.5 > Updated pylint version to 2.13.0 > Switching to composite actions Updating https://github.com/adafruit/Adafruit_CircuitPython_MCP2515 to 1.1.1 from 1.1.0: > Merge pull request adafruit/Adafruit_CircuitPython_MCP2515#18 from adafruit/use-core-message-rtr-classes Updating https://github.com/adafruit/Adafruit_CircuitPython_framebuf to 1.6.1 from 1.6.0: > Merge pull request adafruit/Adafruit_CircuitPython_framebuf#52 from steka/improve_font_to_bin Updating https://github.com/adafruit/Adafruit_CircuitPython_HTTPServer to 3.0.1 from 2.5.0: > Merge pull request adafruit/Adafruit_CircuitPython_HTTPServer#45 from foxy82/main > Merge pull request adafruit/Adafruit_CircuitPython_HTTPServer#49 from michalpokusa/prevent-parent-directory-access Updating https://github.com/adafruit/Adafruit_CircuitPython_Pixel_Framebuf to 1.1.10 from 1.1.9: > Merge pull request adafruit/Adafruit_CircuitPython_Pixel_Framebuf#9 from RossK1/adding_type_hints > Add upload url to release action > Add .venv to .gitignore > Update .pylintrc for v2.15.5 > Fix release CI files > Update pylint to 2.15.5 > Updated pylint version to 2.13.0 > Switching to composite actions Updating https://github.com/adafruit/Adafruit_CircuitPython_TinyLoRa to 2.2.13 from 2.2.12: > Merge pull request adafruit/Adafruit_CircuitPython_TinyLoRa#49 from awordforthat/issue47/fix-short-name-errors > Add upload url to release action > Add .venv to .gitignore Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA: > Updated download stats for the libraries

michalpokusa added 6 commits April 13, 2023 23:23

Moved root_path from start and server_forever methods to constructor

77bc821

Minor changes in comments

e7a2deb

Added checking for .. and backslash in path, introduced custom except…

d89e66e

…ions

Updated HTTPServer constructors to use "/static" as root path

0467a27

Changed secrets to settings.toml in examples

c24466b

Added missing FORBIDDEN_403

a545ca7

michalpokusa force-pushed the prevent-parent-directory-access branch from ab12bbc to a545ca7 Compare April 16, 2023 09:33

michalpokusa added 2 commits April 16, 2023 12:16

Extracted multiple send calls logic into decorator

8a4f5c4

Removing unnecessary slash in front of filename

bef9f76

michalpokusa force-pushed the prevent-parent-directory-access branch from 9b9adfc to bef9f76 Compare April 16, 2023 12:17

michalpokusa changed the title ~~Draft: Prevent parent directory access~~ Prevent parent directory access Apr 16, 2023

michalpokusa changed the title ~~Prevent parent directory access~~ Prevent parent directory access, custom Errors Apr 16, 2023

michalpokusa marked this pull request as ready for review April 16, 2023 12:31

dhalbert requested changes Apr 19, 2023

View reviewed changes

Merge commit 'a8b68f153b72b8c022eceec254abe129cf5f5802' into prevent-…

394c6f3

…parent-directory-access

anecdata approved these changes Apr 19, 2023

View reviewed changes

FoamyGuy reviewed Apr 22, 2023

View reviewed changes

FoamyGuy requested changes Apr 22, 2023

View reviewed changes

Replaced os.environ to os.getenv

9e173ef

michalpokusa requested review from dhalbert and FoamyGuy April 24, 2023 06:46

FoamyGuy approved these changes Apr 24, 2023

View reviewed changes

FoamyGuy merged commit c2e832a into adafruit:main Apr 24, 2023

michalpokusa deleted the prevent-parent-directory-access branch April 24, 2023 21:47

michalpokusa mentioned this pull request Apr 26, 2023

How do I safely disable all access to filesystem? #52

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent parent directory access, custom Errors #49

Prevent parent directory access, custom Errors #49

michalpokusa commented Apr 14, 2023 •

edited

Loading

dhalbert left a comment

dhalbert Apr 19, 2023

michalpokusa Apr 19, 2023 •

edited

Loading

dhalbert Apr 19, 2023

michalpokusa Apr 21, 2023 •

edited

Loading

anecdata left a comment •

edited

Loading

michalpokusa commented Apr 19, 2023 •

edited

Loading

FoamyGuy Apr 22, 2023

anecdata Apr 22, 2023

michalpokusa Apr 22, 2023 •

edited

Loading

FoamyGuy left a comment

FoamyGuy left a comment

michalpokusa commented Apr 24, 2023

		Can be explicitly provided in the constructor, in ``send()`` or
		implicitly determined from filename in ``send_file()``.

Prevent parent directory access, custom Errors #49

Prevent parent directory access, custom Errors #49

Conversation

michalpokusa commented Apr 14, 2023 • edited Loading

dhalbert left a comment

Choose a reason for hiding this comment

dhalbert Apr 19, 2023

Choose a reason for hiding this comment

michalpokusa Apr 19, 2023 • edited Loading

Choose a reason for hiding this comment

dhalbert Apr 19, 2023

Choose a reason for hiding this comment

michalpokusa Apr 21, 2023 • edited Loading

Choose a reason for hiding this comment

anecdata left a comment • edited Loading

Choose a reason for hiding this comment

michalpokusa commented Apr 19, 2023 • edited Loading

FoamyGuy Apr 22, 2023

Choose a reason for hiding this comment

anecdata Apr 22, 2023

Choose a reason for hiding this comment

michalpokusa Apr 22, 2023 • edited Loading

Choose a reason for hiding this comment

FoamyGuy left a comment

Choose a reason for hiding this comment

FoamyGuy left a comment

Choose a reason for hiding this comment

michalpokusa commented Apr 24, 2023

michalpokusa commented Apr 14, 2023 •

edited

Loading

michalpokusa Apr 19, 2023 •

edited

Loading

michalpokusa Apr 21, 2023 •

edited

Loading

anecdata left a comment •

edited

Loading

michalpokusa commented Apr 19, 2023 •

edited

Loading

michalpokusa Apr 22, 2023 •

edited

Loading