From 02c69197c6492b3ccc1040cf0c225d65e5be2d9f Mon Sep 17 00:00:00 2001 From: Jeff Hlywa Date: Wed, 3 Oct 2018 22:32:16 -0400 Subject: [PATCH 1/7] Add test demonstrating unsafe directory traversal --- test/elli_static_tests.erl | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/test/elli_static_tests.erl b/test/elli_static_tests.erl index b90adeb..f6e8c49 100644 --- a/test/elli_static_tests.erl +++ b/test/elli_static_tests.erl @@ -7,7 +7,9 @@ elli_static_test_() -> fun setup/0, fun teardown/1, [?_test(readme()), ?_test(no_file()), - ?_test(not_found())]}. + ?_test(not_found()), + ?_test(safe_traversal()), + ?_test(unsafe_traversal())]}. readme() -> @@ -28,6 +30,23 @@ not_found() -> {ok, Response} = httpc:request("http://localhost:3000/not_found"), ?assertMatch({{"HTTP/1.1",404,"Not Found"}, _Headers, "Not Found"}, Response). +safe_traversal() -> + {ok, Response} = httpc:request("http://localhost:3000/elli_static/" + "../elli_static/README.md"), + {ok, File} = file:read_file("README.md"), + Expected = binary_to_list(File), + ?assertEqual([integer_to_list(iolist_size(Expected))], + proplists:get_all_values("content-length", element(2, Response))), + ?assertMatch({_Status, _Headers, Expected}, Response). + +unsafe_traversal() -> + %% compute the relative path to /etc/passwd + {ok, Cwd} = file:get_cwd(), + PasswdPath = [".." || _ <- filename:split(Cwd)] ++ ["etc", "passwd"], + Path = filename:join(PasswdPath), + + {ok, Response} = httpc:request("http://localhost:3000/elli_static/" ++ Path), + ?assertMatch({{"HTTP/1.1",404,"Not Found"}, _Headers, "Not Found"}, Response). setup() -> {ok, Dir} = file:get_cwd(), From 89ad1722504daf2e96d0d6bfc2195b005b1eab6e Mon Sep 17 00:00:00 2001 From: Jeff Hlywa Date: Wed, 3 Oct 2018 22:41:39 -0400 Subject: [PATCH 2/7] Prevent unsafe directory traversal 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. --- src/elli_static.erl | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/src/elli_static.erl b/src/elli_static.erl index cf38536..ee71698 100644 --- a/src/elli_static.erl +++ b/src/elli_static.erl @@ -9,7 +9,7 @@ -include_lib("kernel/include/file.hrl"). -define(TABLE, elli_static_table). - +-define(NOT_FOUND, {404, [], <<"Not Found">>}). %% elli_handler callbacks -export([handle/2, handle_event/3]). @@ -81,15 +81,31 @@ file_info(Filename) -> {error, Reason} -> throw(Reason) end. - maybe_file(Req, Prefix, Dir) -> + %% all paths start with a slash which `safe_relative_path/1' interprets as + %% unsafe (absolute path), so temporarily remove it + <<"/", RawPath/binary>> = elli_request:raw_path(Req), + + %% santize the path ensuring the request doesn't access any parent + %% directories ... and reattach the slash if deemed safe + SafePath = case filename:safe_relative_path(RawPath) of + unsafe -> + throw(?NOT_FOUND); + %% return type quirk work around + [] -> + <<"/">>; + Sanitized -> + <<"/", Sanitized/binary>> + end, + Size = byte_size(Prefix), - case elli_request:raw_path(Req) of + case SafePath of + %% ensure that `SafePath' starts with the correct prefix <> -> Filename = filename:join(Dir, Path), case filelib:is_regular(Filename) of true -> {just, Filename}; - false -> throw({404, [], <<"Not Found">>}) + false -> throw(?NOT_FOUND) end; _ -> nothing From 9be3b8023b87af495e1f907ae4f5d1957504a8f1 Mon Sep 17 00:00:00 2001 From: Jeff Hlywa Date: Wed, 3 Oct 2018 23:24:04 -0400 Subject: [PATCH 3/7] Backport safe_relative_path/1 from OTP 19.3 --- src/elli_static.erl | 42 +++++++++++++++++++++++++++++++++++++- test/elli_static_tests.erl | 20 ++++++++++++++---- 2 files changed, 57 insertions(+), 5 deletions(-) diff --git a/src/elli_static.erl b/src/elli_static.erl index ee71698..db48384 100644 --- a/src/elli_static.erl +++ b/src/elli_static.erl @@ -88,7 +88,7 @@ maybe_file(Req, Prefix, Dir) -> %% santize the path ensuring the request doesn't access any parent %% directories ... and reattach the slash if deemed safe - SafePath = case filename:safe_relative_path(RawPath) of + SafePath = case safe_relative_path(RawPath) of unsafe -> throw(?NOT_FOUND); %% return type quirk work around @@ -110,3 +110,43 @@ maybe_file(Req, Prefix, Dir) -> _ -> nothing end. + + +%% 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). +safe_relative_path(Path) -> + filename:safe_relative_path(Path). + -endif. +-else. + +%% @doc Backport of `filename:safe_relative_path/1' from 19.3. This code was +%% lifted from: +%% https://github.com/erlang/otp/blob/master/lib/stdlib/src/filename.erl#L811 +-spec safe_relative_path(binary()) -> unsafe | file:name_all(). +safe_relative_path(Path) -> + case filename:pathtype(Path) of + relative -> + Cs0 = filename:split(Path), + safe_relative_path_1(Cs0, []); + _ -> + unsafe + end. + +safe_relative_path_1([<<".">>|T], Acc) -> + safe_relative_path_1(T, Acc); +safe_relative_path_1([<<"..">>|T], Acc) -> + climb(T, Acc); +safe_relative_path_1([H|T], Acc) -> + safe_relative_path_1(T, [H|Acc]); +safe_relative_path_1([], []) -> + []; +safe_relative_path_1([], Acc) -> + filename:join(lists:reverse(Acc)). + +climb(_, []) -> + unsafe; +climb(T, [_|Acc]) -> + safe_relative_path_1(T, Acc). +-endif. diff --git a/test/elli_static_tests.erl b/test/elli_static_tests.erl index f6e8c49..56a889d 100644 --- a/test/elli_static_tests.erl +++ b/test/elli_static_tests.erl @@ -9,7 +9,8 @@ elli_static_test_() -> ?_test(no_file()), ?_test(not_found()), ?_test(safe_traversal()), - ?_test(unsafe_traversal())]}. + ?_test(unsafe_traversal()), + ?_test(invalid_path_separator())]}. readme() -> @@ -31,13 +32,18 @@ not_found() -> ?assertMatch({{"HTTP/1.1",404,"Not Found"}, _Headers, "Not Found"}, Response). safe_traversal() -> - {ok, Response} = httpc:request("http://localhost:3000/elli_static/" - "../elli_static/README.md"), {ok, File} = file:read_file("README.md"), Expected = binary_to_list(File), + + {ok, Response} = httpc:request("http://localhost:3000/elli_static/" + "../elli_static/README.md"), ?assertEqual([integer_to_list(iolist_size(Expected))], proplists:get_all_values("content-length", element(2, Response))), - ?assertMatch({_Status, _Headers, Expected}, Response). + ?assertMatch({_Status, _Headers, Expected}, Response), + + + %% `Response' should match the same request above + {ok, Response} = httpc:request("http://localhost:3000/elli_static/./README.md"). unsafe_traversal() -> %% compute the relative path to /etc/passwd @@ -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() -> + %% https://www.ietf.org/rfc/rfc2396.txt defines a path separator to be a + %% single slash + {ok, Response} = httpc:request("http://localhost:3000////elli_static/README.md"), + ?assertMatch({{"HTTP/1.1",404,"Not Found"}, _Headers, "Not Found"}, Response). + setup() -> {ok, Dir} = file:get_cwd(), Args = [{<<"/elli_static">>, {dir, Dir}}], From e04221f6b9ad8e1d92b962be3643e655ecf5f1ed Mon Sep 17 00:00:00 2001 From: Jeff Hlywa Date: Sun, 14 Oct 2018 21:37:04 -0400 Subject: [PATCH 4/7] Add utils module to hold safe_relative_path/1 --- src/elli_static.erl | 45 ++++++-------------------------- src/elli_static_utils.erl | 39 +++++++++++++++++++++++++++ test/elli_static_utils_tests.erl | 25 ++++++++++++++++++ 3 files changed, 72 insertions(+), 37 deletions(-) create mode 100644 src/elli_static_utils.erl create mode 100644 test/elli_static_utils_tests.erl diff --git a/src/elli_static.erl b/src/elli_static.erl index db48384..278fb9a 100644 --- a/src/elli_static.erl +++ b/src/elli_static.erl @@ -111,42 +111,13 @@ maybe_file(Req, Prefix, Dir) -> nothing end. - -%% 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). -safe_relative_path(Path) -> - filename:safe_relative_path(Path). - -endif. --else. - -%% @doc Backport of `filename:safe_relative_path/1' from 19.3. This code was -%% lifted from: -%% https://github.com/erlang/otp/blob/master/lib/stdlib/src/filename.erl#L811 --spec safe_relative_path(binary()) -> unsafe | file:name_all(). +-spec safe_relative_path(binary()) -> unsafe | [] | binary(). safe_relative_path(Path) -> - case filename:pathtype(Path) of - relative -> - Cs0 = filename:split(Path), - safe_relative_path_1(Cs0, []); - _ -> - unsafe + %% prefer the stdlib implementation of `safe_relative_path/1' if available + %% (found in OTP 19.3 or higher) + case lists:member(safe_relative_path, filename:module_info(exports)) of + true -> + filename:safe_relative_path(Path); + false -> + elli_static_utils:safe_relative_path(Path) end. - -safe_relative_path_1([<<".">>|T], Acc) -> - safe_relative_path_1(T, Acc); -safe_relative_path_1([<<"..">>|T], Acc) -> - climb(T, Acc); -safe_relative_path_1([H|T], Acc) -> - safe_relative_path_1(T, [H|Acc]); -safe_relative_path_1([], []) -> - []; -safe_relative_path_1([], Acc) -> - filename:join(lists:reverse(Acc)). - -climb(_, []) -> - unsafe; -climb(T, [_|Acc]) -> - safe_relative_path_1(T, Acc). --endif. diff --git a/src/elli_static_utils.erl b/src/elli_static_utils.erl new file mode 100644 index 0000000..9309e3f --- /dev/null +++ b/src/elli_static_utils.erl @@ -0,0 +1,39 @@ +-module(elli_static_utils). + +-export([safe_relative_path/1]). + +%% @doc Backport of `filename:safe_relative_path/1' from 19.3. This code was +%% lifted from: +%% https://github.com/erlang/otp/blob/master/lib/stdlib/src/filename.erl#L811 +-spec safe_relative_path(Filename) -> 'unsafe' | SafeFilename when + Filename :: file:name_all(), + SafeFilename :: file:name_all(). + +safe_relative_path(Path) -> + case filename:pathtype(Path) of + relative -> + Cs0 = filename:split(Path), + safe_relative_path_1(Cs0, []); + _ -> + unsafe + end. + +safe_relative_path_1(["."|T], Acc) -> + safe_relative_path_1(T, Acc); +safe_relative_path_1([<<".">>|T], Acc) -> + safe_relative_path_1(T, Acc); +safe_relative_path_1([".."|T], Acc) -> + climb(T, Acc); +safe_relative_path_1([<<"..">>|T], Acc) -> + climb(T, Acc); +safe_relative_path_1([H|T], Acc) -> + safe_relative_path_1(T, [H|Acc]); +safe_relative_path_1([], []) -> + []; +safe_relative_path_1([], Acc) -> + filename:join(lists:reverse(Acc)). + +climb(_, []) -> + unsafe; +climb(T, [_|Acc]) -> + safe_relative_path_1(T, Acc). diff --git a/test/elli_static_utils_tests.erl b/test/elli_static_utils_tests.erl new file mode 100644 index 0000000..5da21f7 --- /dev/null +++ b/test/elli_static_utils_tests.erl @@ -0,0 +1,25 @@ +-module(elli_static_utils_tests). +-include_lib("eunit/include/eunit.hrl"). +-compile(export_all). + +elli_static_utils_test_() -> + [?_test(safe_relative_path())]. + +safe_relative_path() -> + ?assertEqual("a", elli_static_utils:safe_relative_path("a")), + ?assertEqual("a/b", elli_static_utils:safe_relative_path("a/b")), + ?assertEqual("a/b", elli_static_utils:safe_relative_path("a/./b")), + ?assertEqual("a/b", elli_static_utils:safe_relative_path("a/./b/.")), + ?assertEqual("", elli_static_utils:safe_relative_path("a/..")), + ?assertEqual("", elli_static_utils:safe_relative_path("a/./..")), + ?assertEqual("", elli_static_utils:safe_relative_path("a/../.")), + ?assertEqual("a", elli_static_utils:safe_relative_path("a/b/..")), + ?assertEqual("a", elli_static_utils:safe_relative_path("a/../a")), + ?assertEqual("a", elli_static_utils:safe_relative_path("a/../a/../a")), + ?assertEqual("a/b/c", elli_static_utils:safe_relative_path("a/../a/b/c")), + + ?assertEqual(unsafe, elli_static_utils:safe_relative_path("a/../..")), + ?assertEqual(unsafe, elli_static_utils:safe_relative_path("a/../../..")), + ?assertEqual(unsafe, elli_static_utils:safe_relative_path("a/./../..")), + ?assertEqual(unsafe, elli_static_utils:safe_relative_path("a/././../../..")), + ?assertEqual(unsafe, elli_static_utils:safe_relative_path("a/b/././../../..")). From 0c27dc79a5876a5c1fee7cef0ff25432ab9b907f Mon Sep 17 00:00:00 2001 From: Jeff Hlywa Date: Sun, 14 Oct 2018 22:36:01 -0400 Subject: [PATCH 5/7] Add additional safe_relative_path tests --- src/elli_static.erl | 3 ++- test/elli_static_utils_tests.erl | 26 ++++++++++++++++++++++++-- 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/src/elli_static.erl b/src/elli_static.erl index 278fb9a..aed9ced 100644 --- a/src/elli_static.erl +++ b/src/elli_static.erl @@ -115,7 +115,8 @@ maybe_file(Req, Prefix, Dir) -> safe_relative_path(Path) -> %% prefer the stdlib implementation of `safe_relative_path/1' if available %% (found in OTP 19.3 or higher) - case lists:member(safe_relative_path, filename:module_info(exports)) of + case lists:member({safe_relative_path, 1}, + filename:module_info(exports)) of true -> filename:safe_relative_path(Path); false -> diff --git a/test/elli_static_utils_tests.erl b/test/elli_static_utils_tests.erl index 5da21f7..bdd35e7 100644 --- a/test/elli_static_utils_tests.erl +++ b/test/elli_static_utils_tests.erl @@ -6,6 +6,7 @@ elli_static_utils_test_() -> [?_test(safe_relative_path())]. safe_relative_path() -> + %% lists ?assertEqual("a", elli_static_utils:safe_relative_path("a")), ?assertEqual("a/b", elli_static_utils:safe_relative_path("a/b")), ?assertEqual("a/b", elli_static_utils:safe_relative_path("a/./b")), @@ -17,9 +18,30 @@ safe_relative_path() -> ?assertEqual("a", elli_static_utils:safe_relative_path("a/../a")), ?assertEqual("a", elli_static_utils:safe_relative_path("a/../a/../a")), ?assertEqual("a/b/c", elli_static_utils:safe_relative_path("a/../a/b/c")), - ?assertEqual(unsafe, elli_static_utils:safe_relative_path("a/../..")), ?assertEqual(unsafe, elli_static_utils:safe_relative_path("a/../../..")), ?assertEqual(unsafe, elli_static_utils:safe_relative_path("a/./../..")), ?assertEqual(unsafe, elli_static_utils:safe_relative_path("a/././../../..")), - ?assertEqual(unsafe, elli_static_utils:safe_relative_path("a/b/././../../..")). + ?assertEqual(unsafe, elli_static_utils:safe_relative_path("a/b/././../../..")), + ?assertEqual(unsafe, elli_static_utils:safe_relative_path("/root")), + + %% binaries + ?assertEqual(<<"a">>, elli_static_utils:safe_relative_path(<<"a">>)), + ?assertEqual(<<"a/b">>, elli_static_utils:safe_relative_path(<<"a/b">>)), + ?assertEqual(<<"a/b">>, elli_static_utils:safe_relative_path(<<"a/./b">>)), + ?assertEqual(<<"a/b">>, elli_static_utils:safe_relative_path(<<"a/./b/.">>)), + ?assertEqual([], elli_static_utils:safe_relative_path(<<"a/..">>)), + ?assertEqual([], elli_static_utils:safe_relative_path(<<"a/./..">>)), + ?assertEqual([], elli_static_utils:safe_relative_path(<<"a/../.">>)), + ?assertEqual(<<"a">>, elli_static_utils:safe_relative_path(<<"a/b/..">>)), + ?assertEqual(<<"a">>, elli_static_utils:safe_relative_path(<<"a/../a">>)), + ?assertEqual(<<"a">>, elli_static_utils:safe_relative_path(<<"a/../a/../a">>)), + ?assertEqual(<<"a/b/c">>, elli_static_utils:safe_relative_path(<<"a/../a/b/c">>)), + ?assertEqual(unsafe, elli_static_utils:safe_relative_path(<<"a/../..">>)), + ?assertEqual(unsafe, elli_static_utils:safe_relative_path(<<"a/../../..">>)), + ?assertEqual(unsafe, elli_static_utils:safe_relative_path(<<"a/./../..">>)), + ?assertEqual(unsafe, elli_static_utils:safe_relative_path(<<"a/././../../..">>)), + ?assertEqual(unsafe, elli_static_utils:safe_relative_path(<<"a/b/././../../..">>)), + ?assertEqual(unsafe, elli_static_utils:safe_relative_path(<<"/root">>)). + + From c360da3936bb7bdaaf405fe785efc0afaa533c02 Mon Sep 17 00:00:00 2001 From: Jeff Hlywa Date: Mon, 22 Oct 2018 21:08:20 -0400 Subject: [PATCH 6/7] Add platform_define for filename:safe_relative_path/1 inclusion --- rebar.config | 3 ++- src/elli_static.erl | 14 +------------- src/elli_static_utils.erl | 5 +++++ test/elli_static_utils_tests.erl | 2 -- 4 files changed, 8 insertions(+), 16 deletions(-) diff --git a/rebar.config b/rebar.config index 337dbb3..72ca09a 100644 --- a/rebar.config +++ b/rebar.config @@ -1,7 +1,8 @@ %% -*- mode: erlang; flycheck-mode: nil -*- {erl_opts, - [debug_info]}. + [debug_info, + {platform_define, "^(18|(19\.[012]))", custom_safe_relative_path}]}. %% HACK {overrides, diff --git a/src/elli_static.erl b/src/elli_static.erl index aed9ced..e4330b6 100644 --- a/src/elli_static.erl +++ b/src/elli_static.erl @@ -88,7 +88,7 @@ maybe_file(Req, Prefix, Dir) -> %% santize the path ensuring the request doesn't access any parent %% directories ... and reattach the slash if deemed safe - SafePath = case safe_relative_path(RawPath) of + SafePath = case elli_static_utils:safe_relative_path(RawPath) of unsafe -> throw(?NOT_FOUND); %% return type quirk work around @@ -110,15 +110,3 @@ maybe_file(Req, Prefix, Dir) -> _ -> nothing end. - --spec safe_relative_path(binary()) -> unsafe | [] | binary(). -safe_relative_path(Path) -> - %% prefer the stdlib implementation of `safe_relative_path/1' if available - %% (found in OTP 19.3 or higher) - case lists:member({safe_relative_path, 1}, - filename:module_info(exports)) of - true -> - filename:safe_relative_path(Path); - false -> - elli_static_utils:safe_relative_path(Path) - end. diff --git a/src/elli_static_utils.erl b/src/elli_static_utils.erl index 9309e3f..3136d5b 100644 --- a/src/elli_static_utils.erl +++ b/src/elli_static_utils.erl @@ -9,6 +9,10 @@ Filename :: file:name_all(), SafeFilename :: file:name_all(). +-ifndef(custom_safe_relative_path). +safe_relative_path(Path) -> + filename:safe_relative_path(Path). +-else. safe_relative_path(Path) -> case filename:pathtype(Path) of relative -> @@ -37,3 +41,4 @@ climb(_, []) -> unsafe; climb(T, [_|Acc]) -> safe_relative_path_1(T, Acc). +-endif. diff --git a/test/elli_static_utils_tests.erl b/test/elli_static_utils_tests.erl index bdd35e7..e440b10 100644 --- a/test/elli_static_utils_tests.erl +++ b/test/elli_static_utils_tests.erl @@ -43,5 +43,3 @@ safe_relative_path() -> ?assertEqual(unsafe, elli_static_utils:safe_relative_path(<<"a/././../../..">>)), ?assertEqual(unsafe, elli_static_utils:safe_relative_path(<<"a/b/././../../..">>)), ?assertEqual(unsafe, elli_static_utils:safe_relative_path(<<"/root">>)). - - From ed51c0721c22ef18aa0b1d0f4adbd4a0648b5a91 Mon Sep 17 00:00:00 2001 From: Jeff Hlywa Date: Mon, 22 Oct 2018 21:35:32 -0400 Subject: [PATCH 7/7] Add test for 404 edge case --- test/elli_static_tests.erl | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/elli_static_tests.erl b/test/elli_static_tests.erl index 56a889d..a0b14d7 100644 --- a/test/elli_static_tests.erl +++ b/test/elli_static_tests.erl @@ -29,8 +29,13 @@ no_file() -> not_found() -> {ok, Response} = httpc:request("http://localhost:3000/not_found"), + ?assertMatch({{"HTTP/1.1",404,"Not Found"}, _Headers, "Not Found"}, Response), + + %% return 404 if the user doesn't provide a path + {ok, Response} = httpc:request("http://localhost:3000"), ?assertMatch({{"HTTP/1.1",404,"Not Found"}, _Headers, "Not Found"}, Response). + safe_traversal() -> {ok, File} = file:read_file("README.md"), Expected = binary_to_list(File), @@ -41,7 +46,6 @@ safe_traversal() -> proplists:get_all_values("content-length", element(2, Response))), ?assertMatch({_Status, _Headers, Expected}, Response), - %% `Response' should match the same request above {ok, Response} = httpc:request("http://localhost:3000/elli_static/./README.md").