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(-)