| Message ID | 20231207071129.GE1276005@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 34b1a0d320e3a1531997d6681bacffbe9da7a046 |
| Headers | show |
| Series | fix segfaults with implicit-bool config | expand |
On Thu, Dec 07, 2023 at 02:11:29AM -0500, Jeff King wrote: > We record the submodule branch config value as a string, so config that > uses an implicit bool like: > > [submodule "foo"] > branch > > will cause us to segfault. Note that unlike most other config-parsing > bugs of this class, this can be triggered by parsing a bogus .gitmodules > file (which we might do after cloning a malicious repository). > > I don't think the security implications are important, though. It's > always a strict NULL dereference, not an out-of-bounds read or write. So > we should reliably kill the process. That may be annoying, but the > impact is limited to the attacker preventing the victim from > successfully using "git clone --recurse-submodules", etc, on the > malicious repo. > > The "branch" entry is the only one with this problem; other strings like > "path" and "url" already check for NULL. > > Signed-off-by: Jeff King <peff@peff.net> > --- > submodule-config.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/submodule-config.c b/submodule-config.c > index 6a48fd12f6..f4dd482abc 100644 > --- a/submodule-config.c > +++ b/submodule-config.c > @@ -516,7 +516,9 @@ static int parse_config(const char *var, const char *value, > submodule->recommend_shallow = > git_config_bool(var, value); > } else if (!strcmp(item.buf, "branch")) { > - if (!me->overwrite && submodule->branch) > + if (!value) > + ret = config_error_nonbool(var); > + else if (!me->overwrite && submodule->branch) > warn_multiple_config(me->treeish_name, submodule->name, > "branch"); > else { I was about to say that I'd rather expect us to handle this gracefully so that Git doesn't die when parsing an invalid gitmodules file. But there are other cases where we already fail in the same way, so this looks good to me. Patrick
On Thu, Dec 07, 2023 at 09:14:48AM +0100, Patrick Steinhardt wrote: > > diff --git a/submodule-config.c b/submodule-config.c > > index 6a48fd12f6..f4dd482abc 100644 > > --- a/submodule-config.c > > +++ b/submodule-config.c > > @@ -516,7 +516,9 @@ static int parse_config(const char *var, const char *value, > > submodule->recommend_shallow = > > git_config_bool(var, value); > > } else if (!strcmp(item.buf, "branch")) { > > - if (!me->overwrite && submodule->branch) > > + if (!value) > > + ret = config_error_nonbool(var); > > + else if (!me->overwrite && submodule->branch) > > warn_multiple_config(me->treeish_name, submodule->name, > > "branch"); > > else { > > I was about to say that I'd rather expect us to handle this gracefully > so that Git doesn't die when parsing an invalid gitmodules file. But > there are other cases where we already fail in the same way, so this > looks good to me. We're just returning the error here, so it's really up to the caller to decide what to do. The config API has an "error_action" field in the options struct. By default for reading from a blob, this will propagate the error, and I think that's what we use in most of the submodule code. For the code in fsck which looks at gitmodules, it suppresses the error text from the config API (in favor of its own fsck-specific message). Of course it does not suppress the error() from config_error_nonbool, which writes straight to stderr. So there may be some room for improvement, but I doubt anybody cares too much in practice if fsck is a little chatty when it sees breakage. -Peff
diff --git a/submodule-config.c b/submodule-config.c index 6a48fd12f6..f4dd482abc 100644 --- a/submodule-config.c +++ b/submodule-config.c @@ -516,7 +516,9 @@ static int parse_config(const char *var, const char *value, submodule->recommend_shallow = git_config_bool(var, value); } else if (!strcmp(item.buf, "branch")) { - if (!me->overwrite && submodule->branch) + if (!value) + ret = config_error_nonbool(var); + else if (!me->overwrite && submodule->branch) warn_multiple_config(me->treeish_name, submodule->name, "branch"); else {
We record the submodule branch config value as a string, so config that uses an implicit bool like: [submodule "foo"] branch will cause us to segfault. Note that unlike most other config-parsing bugs of this class, this can be triggered by parsing a bogus .gitmodules file (which we might do after cloning a malicious repository). I don't think the security implications are important, though. It's always a strict NULL dereference, not an out-of-bounds read or write. So we should reliably kill the process. That may be annoying, but the impact is limited to the attacker preventing the victim from successfully using "git clone --recurse-submodules", etc, on the malicious repo. The "branch" entry is the only one with this problem; other strings like "path" and "url" already check for NULL. Signed-off-by: Jeff King <peff@peff.net> --- submodule-config.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)