Unsafe directory traversal #3
Conversation
Paths are now verified with filename:safe_relative_path/1 to ensure they don't refer to files outside of the user-supplied static directory.
jhlywa
changed the title
|
I'll backport |
|
Thanks for this! I look forward to the backport of Further PRs are most welcome too! As it stands, elli_static is not quite ready for wide adoption. |
jhlywa
force-pushed
the
develop
branch
3 times, most recently
from
9008952 to
fa50764
Compare
src/elli_static.erl
Outdated
| %% OTP_RELEASE macro was introduced in 21, `filename:safe_relative_path/1' in | ||
| %% 19.3, so the code below is safe | ||
| -ifdef(OTP_RELEASE). | ||
| -ifdef(?OTP_RELEASE >= 21). |
There was a problem hiding this comment.
I don't think I've seen this sort of ifdef before, but it appears to work as desired.
There was a problem hiding this comment.
My preference would be to defer to filename:safe_relative_path/1 if available. However, checking this at runtime (via module_info(exports)) caused lots of coverall coverage complaints.
Xref and dialyzer are run under the test profile, so in-module eunit tests ended up failing xref right out of the gate (elli_static:safe_relative_path_test/0 is unused export). Doh!
I didn't want to introduce a new module, but I suppose I could create something like elli_static_utils, place safe_relative_path/1 in there, and test independently of the OTP version. The utils module would be a cleaner approach IMO.
There was a problem hiding this comment.
👍 to a utils module, if you're up for it.
| @@ -48,6 +54,12 @@ unsafe_traversal() -> | |||
| {ok, Response} = httpc:request("http://localhost:3000/elli_static/" ++ Path), | |||
| ?assertMatch({{"HTTP/1.1",404,"Not Found"}, _Headers, "Not Found"}, Response). | |||
|
|
|||
| invalid_path_separator() -> | |||
|
Thoughts, @elli-lib/team? |
|
hi, since we are there let's learn from https://www.owasp.org/index.php/Path_Traversal I would suggest reusing their examples as test cases and maybe even throw proper |
|
I'm under the assumption that we should we be calling the I'm not sure there's any way to silence xref without either:
Thoughts? |
|
The common approach, I think, is to use Rebar3's |
|
LGTM. What do you think, @deadtrickster? |
|
I probably need to add additional test cases per @deadtrickster's suggestions. |
|
That would be great. I'd also be amenable to pulling that out to a new issue, I think. |
The
maybe_file/3function doesn't sanitize input which allows unsafe directory traversal. I've added a failingunsafe_traversaltest which demonstrates this by reading/etc/passwd. I'll follow up shortly with a fix.BTW - I think elli is awesome!