From 4e0bdaa877336efc9d42fe7c2a57d4cfe60e66a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Fri, 5 Oct 2018 11:42:00 +0200 Subject: submodule: add failing test for option-injection protection in url and path --- tests/submodule/inject_option.c | 80 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+) create mode 100644 tests/submodule/inject_option.c diff --git a/tests/submodule/inject_option.c b/tests/submodule/inject_option.c new file mode 100644 index 000000000..182f088be --- /dev/null +++ b/tests/submodule/inject_option.c @@ -0,0 +1,80 @@ +#include "clar_libgit2.h" +#include "posix.h" +#include "path.h" +#include "submodule_helpers.h" +#include "fileops.h" +#include "repository.h" + +static git_repository *g_repo = NULL; + +void test_submodule_inject_option__initialize(void) +{ + g_repo = setup_fixture_submodule_simple(); +} + +void test_submodule_inject_option__cleanup(void) +{ + cl_git_sandbox_cleanup(); +} + +static int find_naughty(git_submodule *sm, const char *name, void *payload) +{ + int *foundit = (int *) payload; + + GIT_UNUSED(sm); + + if (!git__strcmp("naughty", name)) + *foundit = true; + + return 0; +} + +void test_submodule_inject_option__url(void) +{ + int foundit; + git_submodule *sm; + git_buf buf = GIT_BUF_INIT; + + cl_git_pass(git_buf_joinpath(&buf, git_repository_workdir(g_repo), ".gitmodules")); + cl_git_rewritefile(buf.ptr, + "[submodule \"naughty\"]\n" + " path = testrepo\n" + " url = -u./payload\n"); + git_buf_dispose(&buf); + + /* We do want to find it, but with the appropriate field empty */ + foundit = 0; + cl_git_pass(git_submodule_foreach(g_repo, find_naughty, &foundit)); + cl_assert_equal_i(1, foundit); + + cl_git_pass(git_submodule_lookup(&sm, g_repo, "naughty")); + cl_assert_equal_s("testrepo", git_submodule_path(sm)); + cl_assert_equal_p(NULL, git_submodule_url(sm)); + + git_submodule_free(sm); +} + +void test_submodule_inject_option__path(void) +{ + int foundit; + git_submodule *sm; + git_buf buf = GIT_BUF_INIT; + + cl_git_pass(git_buf_joinpath(&buf, git_repository_workdir(g_repo), ".gitmodules")); + cl_git_rewritefile(buf.ptr, + "[submodule \"naughty\"]\n" + " path = --something\n" + " url = blah.git\n"); + git_buf_dispose(&buf); + + /* We do want to find it, but with the appropriate field empty */ + foundit = 0; + cl_git_pass(git_submodule_foreach(g_repo, find_naughty, &foundit)); + cl_assert_equal_i(1, foundit); + + cl_git_pass(git_submodule_lookup(&sm, g_repo, "naughty")); + cl_assert_equal_s("naughty", git_submodule_path(sm)); + cl_assert_equal_s("blah.git", git_submodule_url(sm)); + + git_submodule_free(sm); +} -- cgit v1.2.1 From c8ca3caef68f31d553c131b471223ff934bb3cff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Fri, 5 Oct 2018 11:47:39 +0200 Subject: submodule: ignore path and url attributes if they look like options These can be used to inject options in an implementation which performs a recursive clone by executing an external command via crafted url and path attributes such that it triggers a local executable to be run. The library is not vulnerable as we do not rely on external executables but a user of the library might be relying on that so we add this protection. This matches this aspect of git's fix for CVE-2018-17456. --- src/submodule.c | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/src/submodule.c b/src/submodule.c index 9231f08b1..825e85a52 100644 --- a/src/submodule.c +++ b/src/submodule.c @@ -1865,6 +1865,14 @@ static int get_value(const char **out, git_config *cfg, git_buf *buf, const char return error; } +static bool looks_like_command_line_option(const char *s) +{ + if (s && s[0] == '-') + return true; + + return false; +} + static int submodule_read_config(git_submodule *sm, git_config *cfg) { git_buf key = GIT_BUF_INIT; @@ -1878,24 +1886,31 @@ static int submodule_read_config(git_submodule *sm, git_config *cfg) if ((error = get_value(&value, cfg, &key, sm->name, "path")) == 0) { in_config = 1; + /* We would warn here if we had that API */ + if (!looks_like_command_line_option(value)) { /* * TODO: if case insensitive filesystem, then the following strcmp * should be strcasecmp */ - if (strcmp(sm->name, value) != 0) { - if (sm->path != sm->name) - git__free(sm->path); - sm->path = git__strdup(value); - GITERR_CHECK_ALLOC(sm->path); + if (strcmp(sm->name, value) != 0) { + if (sm->path != sm->name) + git__free(sm->path); + sm->path = git__strdup(value); + GITERR_CHECK_ALLOC(sm->path); + } + } } else if (error != GIT_ENOTFOUND) { goto cleanup; } if ((error = get_value(&value, cfg, &key, sm->name, "url")) == 0) { - in_config = 1; - sm->url = git__strdup(value); - GITERR_CHECK_ALLOC(sm->url); + /* We would warn here if we had that API */ + if (!looks_like_command_line_option(value)) { + in_config = 1; + sm->url = git__strdup(value); + GITERR_CHECK_ALLOC(sm->url); + } } else if (error != GIT_ENOTFOUND) { goto cleanup; } -- cgit v1.2.1