Re: [PATCH 0/7] fix segfaults with implicit-bool config

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 2513 bytes --]

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]