* [PATCH 1/4] config: turn die_on_error into caller-facing enum
2018-06-28 22:03 ` Jeff King
@ 2018-06-28 22:05 ` Jeff King
2018-06-28 22:05 ` [PATCH 2/4] config: add CONFIG_ERROR_SILENT handler Jeff King
` (3 subsequent siblings)
4 siblings, 0 replies; 31+ messages in thread
From: Jeff King @ 2018-06-28 22:05 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, Jason, GIT Mailing-list
The config code has a die_on_error flag, which lets us emit
an error() instead of dying when we see a bogus config file.
But there's no way for a caller of the config code to set
this: it's auto-set based on whether we're reading a file or
a blob.
Instead, let's add it to the config_options struct. When
it's not set (or we have no options) we'll continue to fall
back to the existing file/blob behavior.
Signed-off-by: Jeff King <peff@peff.net>
---
config.c | 18 +++++++++++++-----
config.h | 5 +++++
2 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/config.c b/config.c
index a0a6ae1980..b5c23369d0 100644
--- a/config.c
+++ b/config.c
@@ -31,7 +31,7 @@ struct config_source {
enum config_origin_type origin_type;
const char *name;
const char *path;
- int die_on_error;
+ enum config_error_action default_error_action;
int linenr;
int eof;
struct strbuf value;
@@ -809,10 +809,18 @@ static int git_parse_source(config_fn_t fn, void *data,
cf->linenr, cf->name);
}
- if (cf->die_on_error)
+ switch (opts && opts->error_action ?
+ opts->error_action :
+ cf->default_error_action) {
+ case CONFIG_ERROR_DIE:
die("%s", error_msg);
- else
+ break;
+ case CONFIG_ERROR_ERROR:
error_return = error("%s", error_msg);
+ break;
+ case CONFIG_ERROR_UNSET:
+ BUG("config error action unset");
+ }
free(error_msg);
return error_return;
@@ -1520,7 +1528,7 @@ static int do_config_from_file(config_fn_t fn,
top.origin_type = origin_type;
top.name = name;
top.path = path;
- top.die_on_error = 1;
+ top.default_error_action = CONFIG_ERROR_DIE;
top.do_fgetc = config_file_fgetc;
top.do_ungetc = config_file_ungetc;
top.do_ftell = config_file_ftell;
@@ -1569,7 +1577,7 @@ int git_config_from_mem(config_fn_t fn, const enum config_origin_type origin_typ
top.origin_type = origin_type;
top.name = name;
top.path = NULL;
- top.die_on_error = 0;
+ top.default_error_action = CONFIG_ERROR_ERROR;
top.do_fgetc = config_buf_fgetc;
top.do_ungetc = config_buf_ungetc;
top.do_ftell = config_buf_ftell;
diff --git a/config.h b/config.h
index 626d4654bd..ce75bf1e5c 100644
--- a/config.h
+++ b/config.h
@@ -54,6 +54,11 @@ struct config_options {
const char *git_dir;
config_parser_event_fn_t event_fn;
void *event_fn_data;
+ enum config_error_action {
+ CONFIG_ERROR_UNSET = 0, /* use source-specific default */
+ CONFIG_ERROR_DIE, /* die() on error */
+ CONFIG_ERROR_ERROR, /* error() on error, return -1 */
+ } error_action;
};
typedef int (*config_fn_t)(const char *, const char *, void *);
--
2.18.0.604.g8c4f59c959
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH 2/4] config: add CONFIG_ERROR_SILENT handler
2018-06-28 22:03 ` Jeff King
2018-06-28 22:05 ` [PATCH 1/4] config: turn die_on_error into caller-facing enum Jeff King
@ 2018-06-28 22:05 ` Jeff King
2018-06-28 22:05 ` [PATCH 3/4] config: add options parameter to git_config_from_mem Jeff King
` (2 subsequent siblings)
4 siblings, 0 replies; 31+ messages in thread
From: Jeff King @ 2018-06-28 22:05 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, Jason, GIT Mailing-list
We can currently die() or error(), but there's not yet any
way for callers to ask us just to quietly return an error.
Let's give them one.
Signed-off-by: Jeff King <peff@peff.net>
---
config.c | 3 +++
config.h | 1 +
2 files changed, 4 insertions(+)
diff --git a/config.c b/config.c
index b5c23369d0..0797e284f4 100644
--- a/config.c
+++ b/config.c
@@ -818,6 +818,9 @@ static int git_parse_source(config_fn_t fn, void *data,
case CONFIG_ERROR_ERROR:
error_return = error("%s", error_msg);
break;
+ case CONFIG_ERROR_SILENT:
+ error_return = -1;
+ break;
case CONFIG_ERROR_UNSET:
BUG("config error action unset");
}
diff --git a/config.h b/config.h
index ce75bf1e5c..c02809ffdc 100644
--- a/config.h
+++ b/config.h
@@ -58,6 +58,7 @@ struct config_options {
CONFIG_ERROR_UNSET = 0, /* use source-specific default */
CONFIG_ERROR_DIE, /* die() on error */
CONFIG_ERROR_ERROR, /* error() on error, return -1 */
+ CONFIG_ERROR_SILENT, /* return -1 */
} error_action;
};
--
2.18.0.604.g8c4f59c959
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH 3/4] config: add options parameter to git_config_from_mem
2018-06-28 22:03 ` Jeff King
2018-06-28 22:05 ` [PATCH 1/4] config: turn die_on_error into caller-facing enum Jeff King
2018-06-28 22:05 ` [PATCH 2/4] config: add CONFIG_ERROR_SILENT handler Jeff King
@ 2018-06-28 22:05 ` Jeff King
2018-06-28 22:06 ` [PATCH 4/4] fsck: silence stderr when parsing .gitmodules Jeff King
2018-06-29 1:10 ` [PATCH] fsck: check skiplist for object in fsck_blob() Ramsay Jones
4 siblings, 0 replies; 31+ messages in thread
From: Jeff King @ 2018-06-28 22:05 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, Jason, GIT Mailing-list
The underlying config parser knows how to handle a
config_options struct, but git_config_from_mem() always
passes NULL. Let's allow our callers to specify the options
struct.
We could add a "_with_options" variant, but since there are
only a handful of callers, let's just update them to pass
NULL.
Signed-off-by: Jeff King <peff@peff.net>
---
config.c | 11 +++++++----
config.h | 7 +++++--
fsck.c | 2 +-
submodule-config.c | 2 +-
4 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/config.c b/config.c
index 0797e284f4..4548edb1e5 100644
--- a/config.c
+++ b/config.c
@@ -1569,8 +1569,10 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
return git_config_from_file_with_options(fn, filename, data, NULL);
}
-int git_config_from_mem(config_fn_t fn, const enum config_origin_type origin_type,
- const char *name, const char *buf, size_t len, void *data)
+int git_config_from_mem(config_fn_t fn,
+ const enum config_origin_type origin_type,
+ const char *name, const char *buf, size_t len,
+ void *data, const struct config_options *opts)
{
struct config_source top;
@@ -1585,7 +1587,7 @@ int git_config_from_mem(config_fn_t fn, const enum config_origin_type origin_typ
top.do_ungetc = config_buf_ungetc;
top.do_ftell = config_buf_ftell;
- return do_config_from(&top, fn, data, NULL);
+ return do_config_from(&top, fn, data, opts);
}
int git_config_from_blob_oid(config_fn_t fn,
@@ -1606,7 +1608,8 @@ int git_config_from_blob_oid(config_fn_t fn,
return error("reference '%s' does not point to a blob", name);
}
- ret = git_config_from_mem(fn, CONFIG_ORIGIN_BLOB, name, buf, size, data);
+ ret = git_config_from_mem(fn, CONFIG_ORIGIN_BLOB, name, buf, size,
+ data, NULL);
free(buf);
return ret;
diff --git a/config.h b/config.h
index c02809ffdc..f2063ceb86 100644
--- a/config.h
+++ b/config.h
@@ -68,8 +68,11 @@ extern int git_config_from_file(config_fn_t fn, const char *, void *);
extern int git_config_from_file_with_options(config_fn_t fn, const char *,
void *,
const struct config_options *);
-extern int git_config_from_mem(config_fn_t fn, const enum config_origin_type,
- const char *name, const char *buf, size_t len, void *data);
+extern int git_config_from_mem(config_fn_t fn,
+ const enum config_origin_type,
+ const char *name,
+ const char *buf, size_t len,
+ void *data, const struct config_options *opts);
extern int git_config_from_blob_oid(config_fn_t fn, const char *name,
const struct object_id *oid, void *data);
extern void git_config_push_parameter(const char *text);
diff --git a/fsck.c b/fsck.c
index 0b8b20b6c4..aa7a52cc80 100644
--- a/fsck.c
+++ b/fsck.c
@@ -1012,7 +1012,7 @@ static int fsck_blob(struct blob *blob, const char *buf,
data.options = options;
data.ret = 0;
if (git_config_from_mem(fsck_gitmodules_fn, CONFIG_ORIGIN_BLOB,
- ".gitmodules", buf, size, &data))
+ ".gitmodules", buf, size, &data, NULL))
data.ret |= report(options, &blob->object,
FSCK_MSG_GITMODULES_PARSE,
"could not parse gitmodules blob");
diff --git a/submodule-config.c b/submodule-config.c
index 388ef1f892..2ca3272dd1 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -561,7 +561,7 @@ static const struct submodule *config_from(struct submodule_cache *cache,
parameter.gitmodules_oid = &oid;
parameter.overwrite = 0;
git_config_from_mem(parse_config, CONFIG_ORIGIN_SUBMODULE_BLOB, rev.buf,
- config, config_size, ¶meter);
+ config, config_size, ¶meter, NULL);
strbuf_release(&rev);
free(config);
--
2.18.0.604.g8c4f59c959
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH 4/4] fsck: silence stderr when parsing .gitmodules
2018-06-28 22:03 ` Jeff King
` (2 preceding siblings ...)
2018-06-28 22:05 ` [PATCH 3/4] config: add options parameter to git_config_from_mem Jeff King
@ 2018-06-28 22:06 ` Jeff King
2018-06-28 22:12 ` Jeff King
2018-06-29 1:10 ` [PATCH] fsck: check skiplist for object in fsck_blob() Ramsay Jones
4 siblings, 1 reply; 31+ messages in thread
From: Jeff King @ 2018-06-28 22:06 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, Jason, GIT Mailing-list
If there's a parsing error we'll already report it via the
usual fsck report() function (or not, if the user has asked
to skip this object or warning type). The error message from
the config parser just adds confusion. Let's suppress it.
Note that we didn't test this case at all, so I've added
coverage in t7415. We may end up toning down or removing
this fsck check in the future. So take this test as checking
what happens now with a focus on stderr, and not any
ironclad guarantee that we must detect and report parse
failures in the future.
Signed-off-by: Jeff King <peff@peff.net>
---
fsck.c | 4 +++-
t/t7415-submodule-names.sh | 15 +++++++++++++++
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/fsck.c b/fsck.c
index aa7a52cc80..87b0e228bd 100644
--- a/fsck.c
+++ b/fsck.c
@@ -992,6 +992,7 @@ static int fsck_blob(struct blob *blob, const char *buf,
unsigned long size, struct fsck_options *options)
{
struct fsck_gitmodules_data data;
+ struct config_options config_opts = { 0 };
if (!oidset_contains(&gitmodules_found, &blob->object.oid))
return 0;
@@ -1011,8 +1012,9 @@ static int fsck_blob(struct blob *blob, const char *buf,
data.obj = &blob->object;
data.options = options;
data.ret = 0;
+ config_opts.error_action = CONFIG_ERROR_SILENT;
if (git_config_from_mem(fsck_gitmodules_fn, CONFIG_ORIGIN_BLOB,
- ".gitmodules", buf, size, &data, NULL))
+ ".gitmodules", buf, size, &data, &config_opts))
data.ret |= report(options, &blob->object,
FSCK_MSG_GITMODULES_PARSE,
"could not parse gitmodules blob");
diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh
index b68c5f5e85..ba8af785a5 100755
--- a/t/t7415-submodule-names.sh
+++ b/t/t7415-submodule-names.sh
@@ -176,4 +176,19 @@ test_expect_success 'fsck detects non-blob .gitmodules' '
)
'
+test_expect_success 'fsck detects corrupt .gitmodules' '
+ git init corrupt &&
+ (
+ cd corrupt &&
+
+ echo "[broken" >.gitmodules &&
+ git add .gitmodules &&
+ git commit -m "broken gitmodules" &&
+
+ test_must_fail git fsck 2>output &&
+ grep gitmodulesParse output &&
+ test_i18ngrep ! "bad config" output
+ )
+'
+
test_done
--
2.18.0.604.g8c4f59c959
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 4/4] fsck: silence stderr when parsing .gitmodules
2018-06-28 22:06 ` [PATCH 4/4] fsck: silence stderr when parsing .gitmodules Jeff King
@ 2018-06-28 22:12 ` Jeff King
2018-06-29 1:14 ` Ramsay Jones
0 siblings, 1 reply; 31+ messages in thread
From: Jeff King @ 2018-06-28 22:12 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, Jason, GIT Mailing-list
On Thu, Jun 28, 2018 at 06:06:03PM -0400, Jeff King wrote:
> Note that we didn't test this case at all, so I've added
> coverage in t7415. We may end up toning down or removing
> this fsck check in the future. So take this test as checking
> what happens now with a focus on stderr, and not any
> ironclad guarantee that we must detect and report parse
> failures in the future.
And such a patch _could_ look something like this. Though we could also
perhaps leave it in place and tone it down to "ignore" by default.
There's another case that triggers gitmodulesParse, too, which is a blob
so gigantic that we try not to hold it in memory. Technically that could
also happen due to somebody using .gitmodules for something unrelated.
But that seems even more far-fetched. And it _is_ dangerous to leave,
because I think existing vulnerable clients will try to load a 500MB
.gitmodules file in memory and parse it.
---
diff --git a/fsck.c b/fsck.c
index 87b0e228bd..296e8a8a7c 100644
--- a/fsck.c
+++ b/fsck.c
@@ -1013,11 +1013,9 @@ static int fsck_blob(struct blob *blob, const char *buf,
data.options = options;
data.ret = 0;
config_opts.error_action = CONFIG_ERROR_SILENT;
- if (git_config_from_mem(fsck_gitmodules_fn, CONFIG_ORIGIN_BLOB,
- ".gitmodules", buf, size, &data, &config_opts))
- data.ret |= report(options, &blob->object,
- FSCK_MSG_GITMODULES_PARSE,
- "could not parse gitmodules blob");
+ /* ignore errors */
+ git_config_from_mem(fsck_gitmodules_fn, CONFIG_ORIGIN_BLOB,
+ ".gitmodules", buf, size, &data, &config_opts);
return data.ret;
}
diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh
index ba8af785a5..9a7dae88a5 100755
--- a/t/t7415-submodule-names.sh
+++ b/t/t7415-submodule-names.sh
@@ -176,7 +176,7 @@ test_expect_success 'fsck detects non-blob .gitmodules' '
)
'
-test_expect_success 'fsck detects corrupt .gitmodules' '
+test_expect_success 'fsck does not complain about corrupt .gitmodules' '
git init corrupt &&
(
cd corrupt &&
@@ -185,9 +185,8 @@ test_expect_success 'fsck detects corrupt .gitmodules' '
git add .gitmodules &&
git commit -m "broken gitmodules" &&
- test_must_fail git fsck 2>output &&
- grep gitmodulesParse output &&
- test_i18ngrep ! "bad config" output
+ git fsck 2>output &&
+ ! test -s output
)
'
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 4/4] fsck: silence stderr when parsing .gitmodules
2018-06-28 22:12 ` Jeff King
@ 2018-06-29 1:14 ` Ramsay Jones
0 siblings, 0 replies; 31+ messages in thread
From: Ramsay Jones @ 2018-06-29 1:14 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Jason, GIT Mailing-list
On 28/06/18 23:12, Jeff King wrote:
> On Thu, Jun 28, 2018 at 06:06:03PM -0400, Jeff King wrote:
>
>> Note that we didn't test this case at all, so I've added
>> coverage in t7415. We may end up toning down or removing
>> this fsck check in the future. So take this test as checking
>> what happens now with a focus on stderr, and not any
>> ironclad guarantee that we must detect and report parse
>> failures in the future.
>
> And such a patch _could_ look something like this. Though we could also
> perhaps leave it in place and tone it down to "ignore" by default.
>
> There's another case that triggers gitmodulesParse, too, which is a blob
> so gigantic that we try not to hold it in memory. Technically that could
> also happen due to somebody using .gitmodules for something unrelated.
> But that seems even more far-fetched. And it _is_ dangerous to leave,
> because I think existing vulnerable clients will try to load a 500MB
> .gitmodules file in memory and parse it.
I also applied and tested the patch below. I think this patch
must be included in the series.
ATB,
Ramsay Jones
> ---
> diff --git a/fsck.c b/fsck.c
> index 87b0e228bd..296e8a8a7c 100644
> --- a/fsck.c
> +++ b/fsck.c
> @@ -1013,11 +1013,9 @@ static int fsck_blob(struct blob *blob, const char *buf,
> data.options = options;
> data.ret = 0;
> config_opts.error_action = CONFIG_ERROR_SILENT;
> - if (git_config_from_mem(fsck_gitmodules_fn, CONFIG_ORIGIN_BLOB,
> - ".gitmodules", buf, size, &data, &config_opts))
> - data.ret |= report(options, &blob->object,
> - FSCK_MSG_GITMODULES_PARSE,
> - "could not parse gitmodules blob");
> + /* ignore errors */
> + git_config_from_mem(fsck_gitmodules_fn, CONFIG_ORIGIN_BLOB,
> + ".gitmodules", buf, size, &data, &config_opts);
>
> return data.ret;
> }
> diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh
> index ba8af785a5..9a7dae88a5 100755
> --- a/t/t7415-submodule-names.sh
> +++ b/t/t7415-submodule-names.sh
> @@ -176,7 +176,7 @@ test_expect_success 'fsck detects non-blob .gitmodules' '
> )
> '
>
> -test_expect_success 'fsck detects corrupt .gitmodules' '
> +test_expect_success 'fsck does not complain about corrupt .gitmodules' '
> git init corrupt &&
> (
> cd corrupt &&
> @@ -185,9 +185,8 @@ test_expect_success 'fsck detects corrupt .gitmodules' '
> git add .gitmodules &&
> git commit -m "broken gitmodules" &&
>
> - test_must_fail git fsck 2>output &&
> - grep gitmodulesParse output &&
> - test_i18ngrep ! "bad config" output
> + git fsck 2>output &&
> + ! test -s output
> )
> '
>
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] fsck: check skiplist for object in fsck_blob()
2018-06-28 22:03 ` Jeff King
` (3 preceding siblings ...)
2018-06-28 22:06 ` [PATCH 4/4] fsck: silence stderr when parsing .gitmodules Jeff King
@ 2018-06-29 1:10 ` Ramsay Jones
2018-07-03 14:34 ` Jeff King
4 siblings, 1 reply; 31+ messages in thread
From: Ramsay Jones @ 2018-06-29 1:10 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Jason, GIT Mailing-list
On 28/06/18 23:03, Jeff King wrote:
> On Thu, Jun 28, 2018 at 07:53:27PM +0100, Ramsay Jones wrote:
[snip]
> Yes, it can go in quickly. But I'd prefer not to keep it in the long
> term if it's literally doing nothing.
Hmm, I don't think you can say its doing nothing!
"Yeah, this solution seems sensible. Given that we would
never report any error for that blob, there is no point
in even looking at it."
... is no less true, with or without additional patches! ;-)
> I have some patches which I think solve your problem. They apply on
> v2.18.0, but not on v2.17.1 (because they rely on Dscho's increased
> passing of config_options in v2.18). Is that good enough?
Heh, I was also writing patches to address this tonight (but
I was also watching the football, so I was somewhat behind you).
My patches were not too dissimilar to yours, except I was aiming
to allow even do_config_from_file() etc., to suppress errors.
Your patches were cleaner and more focused than mine. (Instead of
turning die_on_error into an enum, I added an additional 'quiet'
flag. When pushing the stack (eg. for include files), I had to
copy the quiet flag from the parent struct, etc, ... ;-) ).
> Yes, it would include any syntax error. I also have a slight worry about
> that, but nobody seems to have screamed _yet_. :)
Hmm, I don't think we can ignore this. :(
> Here are the patches I came up with.
Yes, I applied these locally and tested them. All OK here.
So, FWIW, Ack!
[I still think my original patch, with the 'to_be_skipped'
function name changed to 'object_on_skiplist', should be
the first patch of the series!]
ATB,
Ramsay Jones
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH] fsck: check skiplist for object in fsck_blob()
2018-06-29 1:10 ` [PATCH] fsck: check skiplist for object in fsck_blob() Ramsay Jones
@ 2018-07-03 14:34 ` Jeff King
2018-07-04 0:12 ` Ramsay Jones
0 siblings, 1 reply; 31+ messages in thread
From: Jeff King @ 2018-07-03 14:34 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, Jason, GIT Mailing-list
On Fri, Jun 29, 2018 at 02:10:59AM +0100, Ramsay Jones wrote:
> On 28/06/18 23:03, Jeff King wrote:
> > On Thu, Jun 28, 2018 at 07:53:27PM +0100, Ramsay Jones wrote:
> [snip]
> > Yes, it can go in quickly. But I'd prefer not to keep it in the long
> > term if it's literally doing nothing.
>
> Hmm, I don't think you can say its doing nothing!
>
> "Yeah, this solution seems sensible. Given that we would
> never report any error for that blob, there is no point
> in even looking at it."
>
> ... is no less true, with or without additional patches! ;-)
True that we don't even bother doing the parsing with your patch. But I
think I talked myself out of that part being a significant savings
elsewhere.
I guess it would be OK to leave it in. It just feels like it would be
vestigial after the rest of the patches.
> > I have some patches which I think solve your problem. They apply on
> > v2.18.0, but not on v2.17.1 (because they rely on Dscho's increased
> > passing of config_options in v2.18). Is that good enough?
>
> Heh, I was also writing patches to address this tonight (but
> I was also watching the football, so I was somewhat behind you).
> My patches were not too dissimilar to yours, except I was aiming
> to allow even do_config_from_file() etc., to suppress errors.
I think this should work via do_config_from_file(). The thing it really
misses is that git_config_with_options() will not respect it, but the
handling of options there is already a bug (well, I don't think there's
anything triggerable either before or after my patches, but it feels
like a bug waiting to happen).
> Your patches were cleaner and more focused than mine. (Instead of
> turning die_on_error into an enum, I added an additional 'quiet'
> flag. When pushing the stack (eg. for include files), I had to
> copy the quiet flag from the parent struct, etc, ... ;-) ).
Yes, I think that's what you have to do pre-v2.18, where we don't pass
the options struct around.
> > Yes, it would include any syntax error. I also have a slight worry about
> > that, but nobody seems to have screamed _yet_. :)
>
> Hmm, I don't think we can ignore this. :(
I'm not sure. This has been running on every push to GitHub for the past
6 weeks, and this is the first report. It's hard to say what that means,
and technically speaking of course this _is_ a regression.
There's a nearby thread of interest, too, which I cc'd you on:
https://public-inbox.org/git/20180703070650.b3drk5a6kb4k4tnp@glandium.org/
> > Here are the patches I came up with.
>
> Yes, I applied these locally and tested them. All OK here.
>
> So, FWIW, Ack!
>
> [I still think my original patch, with the 'to_be_skipped'
> function name changed to 'object_on_skiplist', should be
> the first patch of the series!]
Thanks. If we're going to do any loosening, I think we may want to
address that _first_, so it can go directly on top of the patches in
v2.17.1 (because it's a bigger issue than the stray message, IMHO).
-Peff
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] fsck: check skiplist for object in fsck_blob()
2018-07-03 14:34 ` Jeff King
@ 2018-07-04 0:12 ` Ramsay Jones
2018-07-07 1:32 ` Jeff King
0 siblings, 1 reply; 31+ messages in thread
From: Ramsay Jones @ 2018-07-04 0:12 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Jason, GIT Mailing-list
On 03/07/18 15:34, Jeff King wrote:
> On Fri, Jun 29, 2018 at 02:10:59AM +0100, Ramsay Jones wrote:
>
>> On 28/06/18 23:03, Jeff King wrote:
>>> On Thu, Jun 28, 2018 at 07:53:27PM +0100, Ramsay Jones wrote:
>> [snip]
>>> Yes, it can go in quickly. But I'd prefer not to keep it in the long
>>> term if it's literally doing nothing.
>>
>> Hmm, I don't think you can say its doing nothing!
>>
>> "Yeah, this solution seems sensible. Given that we would
>> never report any error for that blob, there is no point
>> in even looking at it."
>>
>> ... is no less true, with or without additional patches! ;-)
>
> True that we don't even bother doing the parsing with your patch. But I
> think I talked myself out of that part being a significant savings
> elsewhere.
[Sorry for the late reply - watching football again!]
I'm not interested in any savings - it would have to be a pretty
wacky repo for there to be much in the way of savings!
Simply, I have found (for many different reasons) that, if there
is no good reason to execute some code, it is _far_ better to not
do so! ;-)
> I guess it would be OK to leave it in. It just feels like it would be
> vestigial after the rest of the patches.
>
[snip]
>>> Yes, it would include any syntax error. I also have a slight worry about
>>> that, but nobody seems to have screamed _yet_. :)
>>
>> Hmm, I don't think we can ignore this. :(
>
> I'm not sure. This has been running on every push to GitHub for the past
> 6 weeks, and this is the first report. It's hard to say what that means,
> and technically speaking of course this _is_ a regression.
Hmm, are you concerned about old clients 'transmitting' the
bad data via large hosting sites? (New clients would complain
about a dodgy .gitmodules file, no matter were it came from,
right?)
Has the definition of the config file syntax changed recently?
If not, then old client versions will see the same syntax errors
as recently 'fixed' versions. So they should error out without
'looking' at the bad data, right? (ignoring the 'lets fix the
obvious syntax error' idea).
> There's a nearby thread of interest, too, which I cc'd you on:
>
> https://public-inbox.org/git/20180703070650.b3drk5a6kb4k4tnp@glandium.org/
Yeah, I don't quite follow what's going on there - I would have
to read up some more. ;-)
>> So, FWIW, Ack!
>>
>> [I still think my original patch, with the 'to_be_skipped'
>> function name changed to 'object_on_skiplist', should be
>> the first patch of the series!]
>
> Thanks. If we're going to do any loosening, I think we may want to
> address that _first_, so it can go directly on top of the patches in
> v2.17.1 (because it's a bigger issue than the stray message, IMHO).
Agreed. I probably haven't given it sufficient thought, but my
immediate reaction is to loosen the check - I don't see how
this could be exploited to significantly reduce security. (My lack
of imagination has been noted several times in the past, however!)
Having said that, I am no security expert, so I will let those who
have security expertise decide what is best to do in this situation.
Thanks!
ATB,
Ramsay Jones
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] fsck: check skiplist for object in fsck_blob()
2018-07-04 0:12 ` Ramsay Jones
@ 2018-07-07 1:32 ` Jeff King
2018-07-11 19:31 ` Ramsay Jones
0 siblings, 1 reply; 31+ messages in thread
From: Jeff King @ 2018-07-07 1:32 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, Jason, GIT Mailing-list
On Wed, Jul 04, 2018 at 01:12:40AM +0100, Ramsay Jones wrote:
> > True that we don't even bother doing the parsing with your patch. But I
> > think I talked myself out of that part being a significant savings
> > elsewhere.
>
> [Sorry for the late reply - watching football again!]
>
> I'm not interested in any savings - it would have to be a pretty
> wacky repo for there to be much in the way of savings!
>
> Simply, I have found (for many different reasons) that, if there
> is no good reason to execute some code, it is _far_ better to not
> do so! ;-)
Heh. I also agree with that as a guiding principle. But I _also_ like
the principle of "if you do not need to do add this code, do not add
it". So the two are a little at odds here. :)
> > I'm not sure. This has been running on every push to GitHub for the past
> > 6 weeks, and this is the first report. It's hard to say what that means,
> > and technically speaking of course this _is_ a regression.
>
> Hmm, are you concerned about old clients 'transmitting' the
> bad data via large hosting sites? (New clients would complain
> about a dodgy .gitmodules file, no matter were it came from,
> right?)
I just meant above that anybody with a broken .gitmodules would have had
their push rejected, and we haven't gotten any such reports beyond yours
and Mike's. So that's some evidence that it's relatively rare.
As far as why those fsck checks are there in the first place, yes, it's
about transmitting bad data to unpatched clients. Ultimately the
responsibility for not being vulnerable is on the clients themselves.
But in practice large hosting sites can help by not being vectors.
> Has the definition of the config file syntax changed recently?
> If not, then old client versions will see the same syntax errors
> as recently 'fixed' versions. So they should error out without
> 'looking' at the bad data, right? (ignoring the 'lets fix the
> obvious syntax error' idea).
No, it hasn't change. So as far as I know, loosening the syntax check
does not impact _this_ vulnerability, because it requires a file that
can be parsed, and the parser is the same for both cases. It might
affect future vulnerabilities. We could also tighten when those come to
light, or tighten in a way that blocks the specific bug. But there's
potentially some value in putting our foot down _now_ and saying that
we're going to enforce certain properties of special files, before it
gets any further out-of-hand.
And of course I could be wrong. We do occasionally fix bugs in the
parser, so I won't be surprised if there's some obscure case where git
<2.0 would not be protected or something like that. But frankly,
anything that old is probably already vulnerable to other ancient bugs,
too.
> > Thanks. If we're going to do any loosening, I think we may want to
> > address that _first_, so it can go directly on top of the patches in
> > v2.17.1 (because it's a bigger issue than the stray message, IMHO).
>
> Agreed. I probably haven't given it sufficient thought, but my
> immediate reaction is to loosen the check - I don't see how
> this could be exploited to significantly reduce security. (My lack
> of imagination has been noted several times in the past, however!)
>
> Having said that, I am no security expert, so I will let those who
> have security expertise decide what is best to do in this situation.
I think you have a grasp on the situation from what you wrote above.
What next? I was kind of leaning towards loosening, but it sounded like
Junio thought the opposite. One thing I didn't like about the patch I
sent earlier is that it removes the option for the admin to say "no, I
really do want to enforce this". I don't know if that's of value or not.
In an ideal world, I think we'd detect the problem and then react
according to the receiver's receive.fsck.* config. And we could just
flip the default for receive.fsck.gitmodulesParse to "warning". But
IIRC, the fsck code in index-pack does not bother distinguishing between
warnings and errors. I think it ought to, but that's too big a change to
go on maint.
It _might_ work to just flip the default to "ignore". That leaves the
paranoid admin with a way to re-enable it if they want, and I _think_ it
would be respected by index-pack.
[goes and looks at the code]
Hmm, we seem to have "info" these days, so maybe that would do what I
want. I.e., I wonder if the patch below does everything we'd want. It's
late here and I probably won't get back to this until Monday, but you
may want to play with it in the meantime.
diff --git a/fsck.c b/fsck.c
index 48e7e36869..0b0003055e 100644
--- a/fsck.c
+++ b/fsck.c
@@ -61,7 +61,6 @@ static struct oidset gitmodules_done = OIDSET_INIT;
FUNC(ZERO_PADDED_DATE, ERROR) \
FUNC(GITMODULES_MISSING, ERROR) \
FUNC(GITMODULES_BLOB, ERROR) \
- FUNC(GITMODULES_PARSE, ERROR) \
FUNC(GITMODULES_NAME, ERROR) \
FUNC(GITMODULES_SYMLINK, ERROR) \
/* warnings */ \
@@ -76,6 +75,7 @@ static struct oidset gitmodules_done = OIDSET_INIT;
FUNC(NUL_IN_COMMIT, WARN) \
/* infos (reported as warnings, but ignored by default) */ \
FUNC(BAD_TAG_NAME, INFO) \
+ FUNC(GITMODULES_PARSE, INFO) \
FUNC(MISSING_TAGGER_ENTRY, INFO)
#define MSG_ID(id, msg_type) FSCK_MSG_##id,
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH] fsck: check skiplist for object in fsck_blob()
2018-07-07 1:32 ` Jeff King
@ 2018-07-11 19:31 ` Ramsay Jones
2018-07-13 19:37 ` Ramsay Jones
2018-07-13 19:38 ` Jeff King
0 siblings, 2 replies; 31+ messages in thread
From: Ramsay Jones @ 2018-07-11 19:31 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Jason, GIT Mailing-list
On 07/07/18 02:32, Jeff King wrote:
[snip]
>> I'm not interested in any savings - it would have to be a pretty
>> wacky repo for there to be much in the way of savings!
>>
>> Simply, I have found (for many different reasons) that, if there
>> is no good reason to execute some code, it is _far_ better to not
>> do so! ;-)
>
> Heh. I also agree with that as a guiding principle. But I _also_ like
> the principle of "if you do not need to do add this code, do not add
> it". So the two are a little at odds here. :)
I agree with that also! ;-) However, in this case, I can't
imagine having to do less, to do nothing - if you see what
I mean! So, I think "don't execute code you don't need to"
trumps "don't add code you don't need to" here.
[snip]
> What next? I was kind of leaning towards loosening, but it sounded like
> Junio thought the opposite. One thing I didn't like about the patch I
> sent earlier is that it removes the option for the admin to say "no, I
> really do want to enforce this". I don't know if that's of value or not.
Yes, it would be good to let the admin set the policy.
> In an ideal world, I think we'd detect the problem and then react
> according to the receiver's receive.fsck.* config. And we could just
> flip the default for receive.fsck.gitmodulesParse to "warning". But
> IIRC, the fsck code in index-pack does not bother distinguishing between
> warnings and errors. I think it ought to, but that's too big a change to
> go on maint.
>
> It _might_ work to just flip the default to "ignore". That leaves the
> paranoid admin with a way to re-enable it if they want, and I _think_ it
> would be respected by index-pack.
Ah, that would be good, if it works.
> [goes and looks at the code]
>
> Hmm, we seem to have "info" these days, so maybe that would do what I
> want. I.e., I wonder if the patch below does everything we'd want. It's
> late here and I probably won't get back to this until Monday, but you
> may want to play with it in the meantime.
Sorry, I've been busy with other things and have not had the
time to try the patch below (still trying to catch up with
the mailing-list emails!).
> diff --git a/fsck.c b/fsck.c
> index 48e7e36869..0b0003055e 100644
> --- a/fsck.c
> +++ b/fsck.c
> @@ -61,7 +61,6 @@ static struct oidset gitmodules_done = OIDSET_INIT;
> FUNC(ZERO_PADDED_DATE, ERROR) \
> FUNC(GITMODULES_MISSING, ERROR) \
> FUNC(GITMODULES_BLOB, ERROR) \
> - FUNC(GITMODULES_PARSE, ERROR) \
> FUNC(GITMODULES_NAME, ERROR) \
> FUNC(GITMODULES_SYMLINK, ERROR) \
> /* warnings */ \
> @@ -76,6 +75,7 @@ static struct oidset gitmodules_done = OIDSET_INIT;
> FUNC(NUL_IN_COMMIT, WARN) \
> /* infos (reported as warnings, but ignored by default) */ \
> FUNC(BAD_TAG_NAME, INFO) \
> + FUNC(GITMODULES_PARSE, INFO) \
> FUNC(MISSING_TAGGER_ENTRY, INFO)
>
> #define MSG_ID(id, msg_type) FSCK_MSG_##id,
>
So, just squinting at this in my email client, if this allowed
a push/fetch to succeed (along with an 'info' message), while
providing an admin the means to configure it to loudly deny
the push/fetch - then I think we have a winner! ;-)
Sorry for not testing the patch.
ATB,
Ramsay Jones
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] fsck: check skiplist for object in fsck_blob()
2018-07-11 19:31 ` Ramsay Jones
@ 2018-07-13 19:37 ` Ramsay Jones
2018-07-13 19:41 ` Jeff King
2018-07-13 19:38 ` Jeff King
1 sibling, 1 reply; 31+ messages in thread
From: Ramsay Jones @ 2018-07-13 19:37 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Jason, GIT Mailing-list
On 11/07/18 20:31, Ramsay Jones wrote:
> On 07/07/18 02:32, Jeff King wrote:
[snip]
>> Hmm, we seem to have "info" these days, so maybe that would do what I
>> want. I.e., I wonder if the patch below does everything we'd want. It's
>> late here and I probably won't get back to this until Monday, but you
>> may want to play with it in the meantime.
>
> Sorry, I've been busy with other things and have not had the
> time to try the patch below (still trying to catch up with
> the mailing-list emails!).
>
>> diff --git a/fsck.c b/fsck.c
>> index 48e7e36869..0b0003055e 100644
>> --- a/fsck.c
>> +++ b/fsck.c
>> @@ -61,7 +61,6 @@ static struct oidset gitmodules_done = OIDSET_INIT;
>> FUNC(ZERO_PADDED_DATE, ERROR) \
>> FUNC(GITMODULES_MISSING, ERROR) \
>> FUNC(GITMODULES_BLOB, ERROR) \
>> - FUNC(GITMODULES_PARSE, ERROR) \
>> FUNC(GITMODULES_NAME, ERROR) \
>> FUNC(GITMODULES_SYMLINK, ERROR) \
>> /* warnings */ \
>> @@ -76,6 +75,7 @@ static struct oidset gitmodules_done = OIDSET_INIT;
>> FUNC(NUL_IN_COMMIT, WARN) \
>> /* infos (reported as warnings, but ignored by default) */ \
>> FUNC(BAD_TAG_NAME, INFO) \
>> + FUNC(GITMODULES_PARSE, INFO) \
>> FUNC(MISSING_TAGGER_ENTRY, INFO)
>>
>> #define MSG_ID(id, msg_type) FSCK_MSG_##id,
>>
>
> So, just squinting at this in my email client, if this allowed
> a push/fetch to succeed (along with an 'info' message), while
> providing an admin the means to configure it to loudly deny
> the push/fetch - then I think we have a winner! ;-)
>
> Sorry for not testing the patch.
OK, so I found some time to test this tonight. It is not good
news (assuming that I haven't messed up the testing, of course). :(
On top of 'pu' (@9026cfc855), I reverted commit d4c5675233
("fsck: silence stderr when parsing .gitmodules", 2018-06-28) and
added the patch given below.
Unfortunately, the final test fails, thus:
$ cd t
$ ./t7415-submodule-names.sh
ok 1 - check names
ok 2 - create innocent subrepo
ok 3 - submodule add refuses invalid names
ok 4 - add evil submodule
ok 5 - add other submodule
ok 6 - clone evil superproject
ok 7 - fsck detects evil superproject
ok 8 - transfer.fsckObjects detects evil superproject (unpack)
ok 9 - transfer.fsckObjects detects evil superproject (index)
ok 10 - create oddly ordered pack
ok 11 - transfer.fsckObjects handles odd pack (unpack)
ok 12 - transfer.fsckObjects handles odd pack (index)
ok 13 - index-pack --strict works for non-repo pack
ok 14 - fsck detects symlinked .gitmodules file
ok 15 - fsck detects non-blob .gitmodules
ok 16 - fsck detects corrupt .gitmodules
ok 17 - push warns about corrupt .gitmodules
not ok 18 - push rejects corrupt .gitmodules (policy)
#
# rm -rf dst.git &&
# git init --bare dst.git &&
# git -C dst.git config transfer.fsckObjects true &&
# git -C dst.git config fsck.gitmodulesParse error &&
# test_must_fail git -C corrupt push ../dst.git HEAD 2>output &&
# grep gitmodulesParse output &&
# test_i18ngrep ! "bad config" output
#
# failed 1 among 18 test(s)
1..18
$
i.e. the test_must_fail doesn't! ;-)
I have to go out now, but I will hopefully take a look at
this again tomorrow. (Do the test additions look OK?)
ATB,
Ramsay Jones
-- >8 --
Subject: [PATCH] WIP: try jeff's last patch
Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---
fsck.c | 2 +-
t/t7415-submodule-names.sh | 26 ++++++++++++++++++++++++++
2 files changed, 27 insertions(+), 1 deletion(-)
diff --git a/fsck.c b/fsck.c
index 17b3a51fa..b74856cee 100644
--- a/fsck.c
+++ b/fsck.c
@@ -64,7 +64,6 @@ static struct oidset gitmodules_done = OIDSET_INIT;
FUNC(ZERO_PADDED_DATE, ERROR) \
FUNC(GITMODULES_MISSING, ERROR) \
FUNC(GITMODULES_BLOB, ERROR) \
- FUNC(GITMODULES_PARSE, ERROR) \
FUNC(GITMODULES_NAME, ERROR) \
FUNC(GITMODULES_SYMLINK, ERROR) \
/* warnings */ \
@@ -79,6 +78,7 @@ static struct oidset gitmodules_done = OIDSET_INIT;
FUNC(NUL_IN_COMMIT, WARN) \
/* infos (reported as warnings, but ignored by default) */ \
FUNC(BAD_TAG_NAME, INFO) \
+ FUNC(GITMODULES_PARSE, INFO) \
FUNC(MISSING_TAGGER_ENTRY, INFO)
#define MSG_ID(id, msg_type) FSCK_MSG_##id,
diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh
index ba8af785a..ef9535a44 100755
--- a/t/t7415-submodule-names.sh
+++ b/t/t7415-submodule-names.sh
@@ -185,10 +185,36 @@ test_expect_success 'fsck detects corrupt .gitmodules' '
git add .gitmodules &&
git commit -m "broken gitmodules" &&
+ # issues an "info" warning, but does not fail
+ git fsck 2>output &&
+ grep gitmodulesParse output &&
+ test_i18ngrep ! "bad config" output &&
+
+ # up-rate gitmodulesParse to error
+ git config fsck.gitmodulesParse error &&
test_must_fail git fsck 2>output &&
grep gitmodulesParse output &&
test_i18ngrep ! "bad config" output
)
'
+test_expect_success 'push warns about corrupt .gitmodules' '
+ rm -rf dst.git &&
+ git init --bare dst.git &&
+ git -C dst.git config transfer.fsckObjects true &&
+ git -C corrupt push ../dst.git HEAD 2>output &&
+ grep gitmodulesParse output &&
+ test_i18ngrep ! "bad config" output
+'
+
+test_expect_success 'push rejects corrupt .gitmodules (policy)' '
+ rm -rf dst.git &&
+ git init --bare dst.git &&
+ git -C dst.git config transfer.fsckObjects true &&
+ git -C dst.git config fsck.gitmodulesParse error &&
+ test_must_fail git -C corrupt push ../dst.git HEAD 2>output &&
+ grep gitmodulesParse output &&
+ test_i18ngrep ! "bad config" output
+'
+
test_done
--
2.18.0
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH] fsck: check skiplist for object in fsck_blob()
2018-07-13 19:37 ` Ramsay Jones
@ 2018-07-13 19:41 ` Jeff King
2018-07-13 19:46 ` Jeff King
0 siblings, 1 reply; 31+ messages in thread
From: Jeff King @ 2018-07-13 19:41 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, Jason, GIT Mailing-list
On Fri, Jul 13, 2018 at 08:37:46PM +0100, Ramsay Jones wrote:
> OK, so I found some time to test this tonight. It is not good
> news (assuming that I haven't messed up the testing, of course). :(
I think you may have. :)
> not ok 18 - push rejects corrupt .gitmodules (policy)
> #
> # rm -rf dst.git &&
> # git init --bare dst.git &&
> # git -C dst.git config transfer.fsckObjects true &&
> # git -C dst.git config fsck.gitmodulesParse error &&
> # test_must_fail git -C corrupt push ../dst.git HEAD 2>output &&
> # grep gitmodulesParse output &&
> # test_i18ngrep ! "bad config" output
There are separate config slots for local fsck versus receiving objects.
So I think you need to be setting receive.fsck.gitmodulesParse.
-Peff
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] fsck: check skiplist for object in fsck_blob()
2018-07-13 19:41 ` Jeff King
@ 2018-07-13 19:46 ` Jeff King
2018-07-13 20:08 ` Ramsay Jones
0 siblings, 1 reply; 31+ messages in thread
From: Jeff King @ 2018-07-13 19:46 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, Jason, GIT Mailing-list
On Fri, Jul 13, 2018 at 03:41:19PM -0400, Jeff King wrote:
> > not ok 18 - push rejects corrupt .gitmodules (policy)
> > #
> > # rm -rf dst.git &&
> > # git init --bare dst.git &&
> > # git -C dst.git config transfer.fsckObjects true &&
> > # git -C dst.git config fsck.gitmodulesParse error &&
> > # test_must_fail git -C corrupt push ../dst.git HEAD 2>output &&
> > # grep gitmodulesParse output &&
> > # test_i18ngrep ! "bad config" output
>
> There are separate config slots for local fsck versus receiving objects.
> So I think you need to be setting receive.fsck.gitmodulesParse.
I confirmed that s/fsck/receive.fsck/ in your patch makes the tests
pass.
I didn't bother adding extra push tests in the patch I just sent, since
upgrading of config error classes is already covered elsewhere in t5504.
That said, I'm not opposed to adding more tests on top even if they are
slightly redundant (well, not redundant if you're into black-box
testing, but our current tests are usually written with an assumption of
where the module boundaries are, and what is likely to be a problem).
-Peff
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] fsck: check skiplist for object in fsck_blob()
2018-07-13 19:46 ` Jeff King
@ 2018-07-13 20:08 ` Ramsay Jones
0 siblings, 0 replies; 31+ messages in thread
From: Ramsay Jones @ 2018-07-13 20:08 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Jason, GIT Mailing-list
On 13/07/18 20:46, Jeff King wrote:
> On Fri, Jul 13, 2018 at 03:41:19PM -0400, Jeff King wrote:
>
>>> not ok 18 - push rejects corrupt .gitmodules (policy)
>>> #
>>> # rm -rf dst.git &&
>>> # git init --bare dst.git &&
>>> # git -C dst.git config transfer.fsckObjects true &&
>>> # git -C dst.git config fsck.gitmodulesParse error &&
>>> # test_must_fail git -C corrupt push ../dst.git HEAD 2>output &&
>>> # grep gitmodulesParse output &&
>>> # test_i18ngrep ! "bad config" output
>>
>> There are separate config slots for local fsck versus receiving objects.
>> So I think you need to be setting receive.fsck.gitmodulesParse.
>
> I confirmed that s/fsck/receive.fsck/ in your patch makes the tests
> pass.
Doh! Thanks for catching my stupid mistake! I was rushing a bit
just before going out (yes, I'm going to be late now!).
> I didn't bother adding extra push tests in the patch I just sent, since
> upgrading of config error classes is already covered elsewhere in t5504.
yeah, I like to 'test' by adding tests if I can (makes repeating
the steps less effort ...). So, I was just 'showing my working',
as it were.
> That said, I'm not opposed to adding more tests on top even if they are
> slightly redundant (well, not redundant if you're into black-box
> testing, but our current tests are usually written with an assumption of
> where the module boundaries are, and what is likely to be a problem).
I don't mind either way. I will let you and Junio decide.
Thanks!
ATB,
Ramsay Jones
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] fsck: check skiplist for object in fsck_blob()
2018-07-11 19:31 ` Ramsay Jones
2018-07-13 19:37 ` Ramsay Jones
@ 2018-07-13 19:38 ` Jeff King
2018-07-13 19:39 ` [PATCH 1/2] fsck: split ".gitmodules too large" error from parse failure Jeff King
2018-07-13 19:39 ` [PATCH 2/2] fsck: downgrade gitmodulesParse default to "info" Jeff King
1 sibling, 2 replies; 31+ messages in thread
From: Jeff King @ 2018-07-13 19:38 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, Jason, GIT Mailing-list
On Wed, Jul 11, 2018 at 08:31:34PM +0100, Ramsay Jones wrote:
> >> Simply, I have found (for many different reasons) that, if there
> >> is no good reason to execute some code, it is _far_ better to not
> >> do so! ;-)
> >
> > Heh. I also agree with that as a guiding principle. But I _also_ like
> > the principle of "if you do not need to do add this code, do not add
> > it". So the two are a little at odds here. :)
>
> I agree with that also! ;-) However, in this case, I can't
> imagine having to do less, to do nothing - if you see what
> I mean! So, I think "don't execute code you don't need to"
> trumps "don't add code you don't need to" here.
Fair enough. I'm OK with it either way, then.
> > @@ -76,6 +75,7 @@ static struct oidset gitmodules_done = OIDSET_INIT;
> > FUNC(NUL_IN_COMMIT, WARN) \
> > /* infos (reported as warnings, but ignored by default) */ \
> > FUNC(BAD_TAG_NAME, INFO) \
> > + FUNC(GITMODULES_PARSE, INFO) \
> > FUNC(MISSING_TAGGER_ENTRY, INFO)
> >
> > #define MSG_ID(id, msg_type) FSCK_MSG_##id,
> >
>
> So, just squinting at this in my email client, if this allowed
> a push/fetch to succeed (along with an 'info' message), while
> providing an admin the means to configure it to loudly deny
> the push/fetch - then I think we have a winner! ;-)
>
> Sorry for not testing the patch.
No problem. I didn't get back to it until today. And indeed, the patch
works as advertised, but there's one additional bit needed (in the
preparatory patch below).
So here's what I came up with, which I think is pretty reasonable. The
commit message for the second one is quite long, but I tried to lay out
the pros and cons from our discussion. And I think what we discussed
here may end up being the blueprint for how we consider similar cases in
the future, so I tried to be exhaustive.
I built these two on top of my earlier four patches (which Junio has
queued as jk/fsck-gitmodules-gently). The code change itself is
orthogonal to silencing the config code, but I built on top of the test
added earlier. If we want to back-port this to v2.17 or earlier, I can
build it the other way around.
If this is what we decide to do upstream, then I'll lobby to flip
GitHub's defaults to match. Other hosters (especially ones using other
implementations) may consider doing the same.
[1/2]: fsck: split ".gitmodules too large" error from parse failure
[2/2]: fsck: downgrade gitmodulesParse default to "info"
fsck.c | 5 +++--
t/t7415-submodule-names.sh | 2 +-
2 files changed, 4 insertions(+), 3 deletions(-)
-Peff
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 1/2] fsck: split ".gitmodules too large" error from parse failure
2018-07-13 19:38 ` Jeff King
@ 2018-07-13 19:39 ` Jeff King
2018-07-13 19:39 ` [PATCH 2/2] fsck: downgrade gitmodulesParse default to "info" Jeff King
1 sibling, 0 replies; 31+ messages in thread
From: Jeff King @ 2018-07-13 19:39 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, Jason, GIT Mailing-list
Since ed8b10f631 (fsck: check .gitmodules content,
2018-05-02), we'll report a gitmodulesParse error for two
conditions:
- a .gitmodules entry is not syntactically valid
- a .gitmodules entry is larger than core.bigFileThreshold
with the intent that we can detect malicious files and
protect downstream clients. E.g., from the issue in
0383bbb901 (submodule-config: verify submodule names as
paths, 2018-04-30).
But these conditions are actually quite different with
respect to that bug:
- a syntactically invalid file cannot trigger the problem,
as the victim would barf before hitting the problematic
code
- a too-big .gitmodules _can_ trigger the problem. Even
though it is obviously silly to have a 500MB .gitmodules
file, the submodule code will happily parse it if you
have enough memory.
So it may be reasonable to configure their severity
separately. Let's add a new class for the "too large" case
to allow that.
Signed-off-by: Jeff King <peff@peff.net>
---
fsck.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fsck.c b/fsck.c
index 87b0e228bd..4129935d86 100644
--- a/fsck.c
+++ b/fsck.c
@@ -63,6 +63,7 @@ static struct oidset gitmodules_done = OIDSET_INIT;
FUNC(GITMODULES_MISSING, ERROR) \
FUNC(GITMODULES_BLOB, ERROR) \
FUNC(GITMODULES_PARSE, ERROR) \
+ FUNC(GITMODULES_LARGE, ERROR) \
FUNC(GITMODULES_NAME, ERROR) \
FUNC(GITMODULES_SYMLINK, ERROR) \
/* warnings */ \
@@ -1005,7 +1006,7 @@ static int fsck_blob(struct blob *blob, const char *buf,
* that an error.
*/
return report(options, &blob->object,
- FSCK_MSG_GITMODULES_PARSE,
+ FSCK_MSG_GITMODULES_LARGE,
".gitmodules too large to parse");
}
--
2.18.0.433.gb9621797ee
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 2/2] fsck: downgrade gitmodulesParse default to "info"
2018-07-13 19:38 ` Jeff King
2018-07-13 19:39 ` [PATCH 1/2] fsck: split ".gitmodules too large" error from parse failure Jeff King
@ 2018-07-13 19:39 ` Jeff King
2018-07-13 20:21 ` Stefan Beller
2018-07-16 18:04 ` Junio C Hamano
1 sibling, 2 replies; 31+ messages in thread
From: Jeff King @ 2018-07-13 19:39 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, Jason, GIT Mailing-list
We added an fsck check in ed8b10f631 (fsck: check
.gitmodules content, 2018-05-02) as a defense against the
vulnerability from 0383bbb901 (submodule-config: verify
submodule names as paths, 2018-04-30). With the idea that
up-to-date hosting sites could protect downstream unpatched
clients that fetch from them.
As part of that defense, we reject any ".gitmodules" entry
that is not syntactically valid. The theory is that if we
cannot even parse the file, we cannot accurately check it
for vulnerabilities. And anybody with a broken .gitmodules
file would eventually want to know anyway.
But there are a few reasons this is a bad tradeoff in
practice:
- for this particular vulnerability, the client has to be
able to parse the file. So you cannot sneak an attack
through using a broken file, assuming the config parsers
for the process running fsck and the eventual victim are
functionally equivalent.
- a broken .gitmodules file is not necessarily a problem.
Our fsck check detects .gitmodules in _any_ tree, not
just at the root. And the presence of a .gitmodules file
does not necessarily mean it will be used; you'd have to
also have gitlinks in the tree. The cgit repository, for
example, has a file named .gitmodules from a
pre-submodule attempt at sharing code, but does not
actually have any gitlinks.
- when the fsck check is used to reject a push, it's often
hard to work around. The pusher may not have full control
over the destination repository (e.g., if it's on a
hosting server, they may need to contact the hosting
site's support). And the broken .gitmodules may be too
far back in history for rewriting to be feasible (again,
this is an issue for cgit).
So we're being unnecessarily restrictive without actually
improving the security in a meaningful way. It would be more
convenient to downgrade this check to "info", which means
we'd still comment on it, but not reject a push. Site admins
can already do this via config, but we should ship sensible
defaults.
There are a few counterpoints to consider in favor of
keeping the check as an error:
- the first point above assumes that the config parsers for
the victim and the fsck process are equivalent. This is
pretty true now, but as time goes on will become less so.
Hosting sites are likely to upgrade their version of Git,
whereas vulnerable clients will be stagnant (if they did
upgrade, they'd cease to be vulnerable!). So in theory we
may see drift over time between what two config parsers
will accept.
In practice, this is probably OK. The config format is
pretty established at this point and shouldn't change a
lot. And the farther we get from the announcement of the
vulnerability, the less interesting this extra layer of
protection becomes. I.e., it was _most_ valuable on day
0, when everybody's client was still vulnerable and
hosting sites could protect people. But as time goes on
and people upgrade, the population of vulnerable clients
becomes smaller and smaller.
- In theory this could protect us from other
vulnerabilities in the future. E.g., .gitmodules are the
only way for a malicious repository to feed data to the
config parser, so this check could similarly protect
clients from a future (to-be-found) bug there.
But that's trading a hypothetical case for real-world
pain today. If we do find such a bug, the hosting site
would need to be updated to fix it, too. At which point
we could figure out whether it's possible to detect
_just_ the malicious case without hurting existing
broken-but-not-evil cases.
- Until recently, we hadn't made any restrictions on
.gitmodules content. So now in tightening that we're
hitting cases where certain things used to work, but
don't anymore. There's some moderate pain now. But as
time goes on, we'll see more (and more varied) cases that
will make tightening harder in the future. So there's
some argument for putting rules in place _now_, before
users grow more cases that violate them.
Again, this is trading pain now for hypothetical benefit
in the future. And if we try hard in the future to keep
our tightening to a minimum (i.e., rejecting true
maliciousness without hurting broken-but-not-evil repos),
then that reduces even the hypothetical benefit.
Considering both sets of arguments, it makes sense to loosen
this check for now.
Note that we have to tweak the test in t7415 since fsck will
no longer consider this a fatal error. But we still check
that it reports the warning, and that we don't get the
spurious error from the config code.
Signed-off-by: Jeff King <peff@peff.net>
---
fsck.c | 2 +-
t/t7415-submodule-names.sh | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/fsck.c b/fsck.c
index 4129935d86..69ea8d5321 100644
--- a/fsck.c
+++ b/fsck.c
@@ -62,7 +62,6 @@ static struct oidset gitmodules_done = OIDSET_INIT;
FUNC(ZERO_PADDED_DATE, ERROR) \
FUNC(GITMODULES_MISSING, ERROR) \
FUNC(GITMODULES_BLOB, ERROR) \
- FUNC(GITMODULES_PARSE, ERROR) \
FUNC(GITMODULES_LARGE, ERROR) \
FUNC(GITMODULES_NAME, ERROR) \
FUNC(GITMODULES_SYMLINK, ERROR) \
@@ -77,6 +76,7 @@ static struct oidset gitmodules_done = OIDSET_INIT;
FUNC(ZERO_PADDED_FILEMODE, WARN) \
FUNC(NUL_IN_COMMIT, WARN) \
/* infos (reported as warnings, but ignored by default) */ \
+ FUNC(GITMODULES_PARSE, INFO) \
FUNC(BAD_TAG_NAME, INFO) \
FUNC(MISSING_TAGGER_ENTRY, INFO)
diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh
index ba8af785a5..293e2e1963 100755
--- a/t/t7415-submodule-names.sh
+++ b/t/t7415-submodule-names.sh
@@ -185,7 +185,7 @@ test_expect_success 'fsck detects corrupt .gitmodules' '
git add .gitmodules &&
git commit -m "broken gitmodules" &&
- test_must_fail git fsck 2>output &&
+ git fsck 2>output &&
grep gitmodulesParse output &&
test_i18ngrep ! "bad config" output
)
--
2.18.0.433.gb9621797ee
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 2/2] fsck: downgrade gitmodulesParse default to "info"
2018-07-13 19:39 ` [PATCH 2/2] fsck: downgrade gitmodulesParse default to "info" Jeff King
@ 2018-07-13 20:21 ` Stefan Beller
2018-07-16 18:04 ` Junio C Hamano
1 sibling, 0 replies; 31+ messages in thread
From: Stefan Beller @ 2018-07-13 20:21 UTC (permalink / raw)
To: Jeff King; +Cc: Ramsay Jones, Junio C Hamano, Jason, git
> Considering both sets of arguments, it makes sense to loosen
> this check for now.
>
I agree with this reasoning,
>
> Signed-off-by: Jeff King <peff@peff.net>
This and the previous patch are
Reviewed-by: Stefan Beller <sbeller@google.com>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 2/2] fsck: downgrade gitmodulesParse default to "info"
2018-07-13 19:39 ` [PATCH 2/2] fsck: downgrade gitmodulesParse default to "info" Jeff King
2018-07-13 20:21 ` Stefan Beller
@ 2018-07-16 18:04 ` Junio C Hamano
2018-07-16 18:30 ` Jeff King
1 sibling, 1 reply; 31+ messages in thread
From: Junio C Hamano @ 2018-07-16 18:04 UTC (permalink / raw)
To: Jeff King; +Cc: Ramsay Jones, Jason, GIT Mailing-list
Jeff King <peff@peff.net> writes:
> site's support). And the broken .gitmodules may be too
> far back in history for rewriting to be feasible (again,
> this is an issue for cgit).
"again" but this is the first mention that hints cgit has some
problem (but not exactly what problem). Is that the "cgit has a
file called .gitmodules that predates the submodule support on our
side?" thing?
> So we're being unnecessarily restrictive without actually
> improving the security in a meaningful way. It would be more
> convenient to downgrade this check to "info", which means
> we'd still comment on it, but not reject a push. Site admins
> can already do this via config, but we should ship sensible
> defaults.
> ...
> Considering both sets of arguments, it makes sense to loosen
> this check for now.
>
> Note that we have to tweak the test in t7415 since fsck will
> no longer consider this a fatal error. But we still check
> that it reports the warning, and that we don't get the
> spurious error from the config code.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
Thanks.
> fsck.c | 2 +-
> t/t7415-submodule-names.sh | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fsck.c b/fsck.c
> index 4129935d86..69ea8d5321 100644
> --- a/fsck.c
> +++ b/fsck.c
> @@ -62,7 +62,6 @@ static struct oidset gitmodules_done = OIDSET_INIT;
> FUNC(ZERO_PADDED_DATE, ERROR) \
> FUNC(GITMODULES_MISSING, ERROR) \
> FUNC(GITMODULES_BLOB, ERROR) \
> - FUNC(GITMODULES_PARSE, ERROR) \
> FUNC(GITMODULES_LARGE, ERROR) \
> FUNC(GITMODULES_NAME, ERROR) \
> FUNC(GITMODULES_SYMLINK, ERROR) \
> @@ -77,6 +76,7 @@ static struct oidset gitmodules_done = OIDSET_INIT;
> FUNC(ZERO_PADDED_FILEMODE, WARN) \
> FUNC(NUL_IN_COMMIT, WARN) \
> /* infos (reported as warnings, but ignored by default) */ \
> + FUNC(GITMODULES_PARSE, INFO) \
> FUNC(BAD_TAG_NAME, INFO) \
> FUNC(MISSING_TAGGER_ENTRY, INFO)
>
> diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh
> index ba8af785a5..293e2e1963 100755
> --- a/t/t7415-submodule-names.sh
> +++ b/t/t7415-submodule-names.sh
> @@ -185,7 +185,7 @@ test_expect_success 'fsck detects corrupt .gitmodules' '
> git add .gitmodules &&
> git commit -m "broken gitmodules" &&
>
> - test_must_fail git fsck 2>output &&
> + git fsck 2>output &&
> grep gitmodulesParse output &&
> test_i18ngrep ! "bad config" output
> )
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 2/2] fsck: downgrade gitmodulesParse default to "info"
2018-07-16 18:04 ` Junio C Hamano
@ 2018-07-16 18:30 ` Jeff King
2018-07-16 21:08 ` Junio C Hamano
0 siblings, 1 reply; 31+ messages in thread
From: Jeff King @ 2018-07-16 18:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ramsay Jones, Jason, GIT Mailing-list
On Mon, Jul 16, 2018 at 11:04:04AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > site's support). And the broken .gitmodules may be too
> > far back in history for rewriting to be feasible (again,
> > this is an issue for cgit).
>
> "again" but this is the first mention that hints cgit has some
> problem (but not exactly what problem). Is that the "cgit has a
> file called .gitmodules that predates the submodule support on our
> side?" thing?
I think you missed it. In the paragraph above the one you
quoted, I said:
The cgit repository, for example, has a file named
.gitmodules from a pre-submodule attempt at sharing code,
but does not actually have any gitlinks.
> > So we're being unnecessarily restrictive without actually
> > improving the security in a meaningful way. It would be more
> > convenient to downgrade this check to "info", which means
> > we'd still comment on it, but not reject a push. Site admins
> > can already do this via config, but we should ship sensible
> > defaults.
> > ...
> > Considering both sets of arguments, it makes sense to loosen
> > this check for now.
> >
> > Note that we have to tweak the test in t7415 since fsck will
> > no longer consider this a fatal error. But we still check
> > that it reports the warning, and that we don't get the
> > spurious error from the config code.
> >
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
>
> Thanks.
So I'm curious if you found the argument in my commit
message compelling. :)
My recollection from the earlier discussion was that you
were more in favor of keeping things tight. E.g.,:
https://public-inbox.org/git/xmqqh8lgrz5c.fsf@gitster-ct.c.googlers.com/
but reading it again:
- there we were talking about non-blob objects as
.gitmodules
- I think your main concern was that there be a way for the
user to loosen/tighten, which there is.
-Peff
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 2/2] fsck: downgrade gitmodulesParse default to "info"
2018-07-16 18:30 ` Jeff King
@ 2018-07-16 21:08 ` Junio C Hamano
0 siblings, 0 replies; 31+ messages in thread
From: Junio C Hamano @ 2018-07-16 21:08 UTC (permalink / raw)
To: Jeff King; +Cc: Ramsay Jones, Jason, GIT Mailing-list
Jeff King <peff@peff.net> writes:
> I think you missed it. In the paragraph above the one you
> quoted, I said:
>
> The cgit repository, for example, has a file named
> .gitmodules from a pre-submodule attempt at sharing code,
> but does not actually have any gitlinks.
Indeed.
> So I'm curious if you found the argument in my commit
> message compelling. :)
> ...
> - I think your main concern was that there be a way for the
> user to loosen/tighten, which there is.
Yeah, I think the solution looks good.
^ permalink raw reply [flat|nested] 31+ messages in thread