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(-) >
Attachment:
signature.asc
Description: PGP signature
