| Message ID | 20231207071030.GA1275835@coredump.intra.peff.net (mailing list archive) |
|---|---|
| Headers | show |
| Series | fix segfaults with implicit-bool config | expand |
On Thu, Dec 07, 2023 at 02:10:30AM -0500, Jeff King wrote: > Carlos reported to the security list a case where you can cause Git > to segfault by using an implicit bool like: > > [core] > someVariable > > when the parsing side for core.someVariable does not correctly check a > NULL "value" string. This is mostly harmless, as anybody who can feed > arbitrary config can already execute arbitrary code. There is one case > of this when parsing .gitmodules (which we don't trust), but even there > I don't think the security implications are that interesting. A > malicious repo can get "clone --recurse-submodules" to segfault, but > always with a strict NULL dereference, not any kind of controllable > pointer. See patch 5 for more details. > > I audited the whole code base for instances of the problem. It was > fairly manual, so it's possible I missed a spot, but I think this should > cover everything. > > The first patch has vanilla cases, and the rest are instances where I > thought it was worth calling out specific details. Thanks for working on this topic! I've left a couple of comments, most of which are about whether we should retain previous behaviour where we generate a warning instead of raising an error for unknown values. Patrick > [1/7]: config: handle NULL value when parsing non-bools > [2/7]: setup: handle NULL value when parsing extensions > [3/7]: trace2: handle NULL values in tr2_sysenv config callback > [4/7]: help: handle NULL value for alias.* config > [5/7]: submodule: handle NULL value when parsing submodule.*.branch > [6/7]: trailer: handle NULL value when parsing trailer-specific config > [7/7]: fsck: handle NULL value when parsing message config > > builtin/blame.c | 2 ++ > builtin/checkout.c | 2 ++ > builtin/clone.c | 2 ++ > builtin/log.c | 5 ++++- > builtin/pack-objects.c | 6 +++++- > builtin/receive-pack.c | 11 +++++++---- > compat/mingw.c | 2 ++ > config.c | 8 ++++++++ > diff.c | 19 ++++++++++++++++--- > fetch-pack.c | 12 ++++++++---- > fsck.c | 8 ++++++-- > help.c | 5 ++++- > mailinfo.c | 2 ++ > notes-utils.c | 2 ++ > setup.c | 2 ++ > submodule-config.c | 4 +++- > trace2/tr2_sysenv.c | 2 ++ > trailer.c | 8 ++++++++ > 18 files changed, 85 insertions(+), 17 deletions(-) >
On Thu, Dec 07, 2023 at 09:14:36AM +0100, Patrick Steinhardt wrote: > Thanks for working on this topic! I've left a couple of comments, most > of which are about whether we should retain previous behaviour where we > generate a warning instead of raising an error for unknown values. Thanks for taking a look. I see what you're saying about the warnings, but IMHO it's not worth the extra complexity. Returning early means the existing code can proceed without worrying about NULLs. Though I suppose we could have a "warn_error_nonbool()" which issues a warning and returns 0. Still, the "return config_error_nonbool()" pattern is pretty well-established and used for most options. I would go so far as to say the ones that warn for invalid values are the odd ones out, and probably should be returning errors as well (though doing so now may not be worth the trouble and risk of annoyance). And certainly there should be no regressions in this series; every case is currently a segfault, so returning an error is a strict improvement. I'd just as soon stay strict there, as it's easier to loosen later if we choose than to tighten. -Peff
On Mon, Dec 11, 2023 at 07:52:28PM -0500, Jeff King wrote: > On Thu, Dec 07, 2023 at 09:14:36AM +0100, Patrick Steinhardt wrote: > > > Thanks for working on this topic! I've left a couple of comments, most > > of which are about whether we should retain previous behaviour where we > > generate a warning instead of raising an error for unknown values. > > Thanks for taking a look. I see what you're saying about the warnings, > but IMHO it's not worth the extra complexity. Returning early means the > existing code can proceed without worrying about NULLs. Though I suppose > we could have a "warn_error_nonbool()" which issues a warning and > returns 0. > > Still, the "return config_error_nonbool()" pattern is pretty > well-established and used for most options. I would go so far as to say > the ones that warn for invalid values are the odd ones out, and probably > should be returning errors as well (though doing so now may not be worth > the trouble and risk of annoyance). > > And certainly there should be no regressions in this series; every case > is currently a segfault, so returning an error is a strict improvement. > I'd just as soon stay strict there, as it's easier to loosen later if we > choose than to tighten. Fair enough, I'm perfectly fine with this reasoning. Thanks! Patrick