Jeff King <peff@xxxxxxxx> writes:
>> + } else {
>> + if (cf->name)
>> + return error("bad config file line %d in %s",
>> + cf->linenr, cf->name);
>> + else
>> + return error("bad config file line %d", cf->linenr);
>> + }
>
> I think I preferred the earlier version where you had "<stdin>" in the
> name field, and this hunk could just go away. I know you switched it to
> NULL here to avoid making bogus relative filenames in includes.
Exactly the same comment here. I really like the way how this
series becomes more polished every time we see it.
> But that would be pretty straightforward to fix by splitting the "name"
> field into an additional "path" field. It looks like "git config --blob"
> has the same problem (it will try relative lookups in the filesystem,
> even though that is nonsensical from the blob). So we could fix the
> existing bug at the same time. :)
>
> Perhaps this could go at the start of your series?
Sounds like the right thing to do.
Thanks.
> -- >8 --
> Subject: config: disallow relative include paths from blobs
>
> When we see a relative config include like:
>
> [include]
> path = foo
>
> we make it relative to the containing directory of the file
> that contains the snippet. This makes no sense for config
> read from a blob, as it is not on the filesystem. Something
> like "HEAD:some/path" could have a relative path within the
> tree, but:
>
> 1. It would not be part of include.path, which explicitly
> refers to the filesystem.
>
> 2. It would need different parsing rules anyway to
> determine that it is a tree path.
>
> The current code just uses the "name" field, which is wrong.
> Let's split that into "name" and "path" fields, use the
> latter for relative includes, and fill in only the former
> for blobs.
>
> Signed-off-by: Jeff King <peff@xxxxxxxx>
> ---
> I don't think we considered includes at all when adding --blob. I cannot
> think of any good reason to have even an absolute filesystem include
> from a blob. And I wonder if it is possible to cause security mischief
> by convincing git to look at /etc/passwd or similar (it would not parse,
> of course, but you might be able to glean information from the error
> messages or something).
>
> It's probably OK, though, because you would generally not read real
> config from an untrusted source (there are many bad things they could
> set), and other code which uses the config reader explicitly does not
> turn on includes.
>
> config.c | 10 ++++++----
> t/t1305-config-include.sh | 16 ++++++++++++++++
> 2 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/config.c b/config.c
> index d969a5a..b295310 100644
> --- a/config.c
> +++ b/config.c
> @@ -21,6 +21,7 @@ struct config_source {
> } buf;
> } u;
> const char *name;
> + const char *path;
> int die_on_error;
> int linenr;
> int eof;
> @@ -97,12 +98,12 @@ static int handle_path_include(const char *path, struct config_include_data *inc
> if (!is_absolute_path(path)) {
> char *slash;
>
> - if (!cf || !cf->name)
> + if (!cf || !cf->path)
> return error("relative config includes must come from files");
>
> - slash = find_last_dir_sep(cf->name);
> + slash = find_last_dir_sep(cf->path);
> if (slash)
> - strbuf_add(&buf, cf->name, slash - cf->name + 1);
> + strbuf_add(&buf, cf->path, slash - cf->path + 1);
> strbuf_addstr(&buf, path);
> path = buf.buf;
> }
> @@ -1040,7 +1041,7 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
> struct config_source top;
>
> top.u.file = f;
> - top.name = filename;
> + top.name = top.path = filename;
> top.die_on_error = 1;
> top.do_fgetc = config_file_fgetc;
> top.do_ungetc = config_file_ungetc;
> @@ -1062,6 +1063,7 @@ int git_config_from_buf(config_fn_t fn, const char *name, const char *buf,
> top.u.buf.len = len;
> top.u.buf.pos = 0;
> top.name = name;
> + top.path = NULL;
> top.die_on_error = 0;
> top.do_fgetc = config_buf_fgetc;
> top.do_ungetc = config_buf_ungetc;
> diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
> index a707076..6edd38b 100755
> --- a/t/t1305-config-include.sh
> +++ b/t/t1305-config-include.sh
> @@ -122,6 +122,22 @@ test_expect_success 'relative includes from command line fail' '
> test_must_fail git -c include.path=one config test.one
> '
>
> +test_expect_success 'absolute includes from blobs work' '
> + echo "[test]one = 1" >one &&
> + echo "[include]path=$(pwd)/one" >blob &&
> + blob=$(git hash-object -w blob) &&
> + echo 1 >expect &&
> + git config --blob=$blob test.one >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'relative includes from blobs fail' '
> + echo "[test]one = 1" >one &&
> + echo "[include]path=one" >blob &&
> + blob=$(git hash-object -w blob) &&
> + test_must_fail git config --blob=$blob test.one
> +'
> +
> test_expect_success 'include cycles are detected' '
> cat >.gitconfig <<-\EOF &&
> [test]value = gitconfig
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html