[PATCH v2 0/4] core: update our SHA-1 docs, use sha1collisiondetection on OSX too

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: git@vger.kernel.org
Cc: "Junio C Hamano" <gitster@pobox.com>,
	"Mike Hommey" <mh@glandium.org>,
	"brian m . carlson" <sandals@crustytoothpaste.net>,
	"Carlo Marcelo Arenas Belón" <carenas@gmail.com>,
	"Eric Sunshine" <sunshine@sunshineco.com>,
	"Glen Choo" <chooglen@google.com>,
	"Eric DeCosta" <edecosta@mathworks.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: [PATCH v2 0/4] core: update our SHA-1 docs, use sha1collisiondetection on OSX too
Date: Wed, 19 Oct 2022 03:03:18 +0200	[thread overview]
Message-ID: <cover-v2-0.4-00000000000-20221019T010222Z-avarab@gmail.com> (raw)
In-Reply-To: <cover-0.5-00000000000-20220422T094624Z-avarab@gmail.com>

The sha1collisiondetection alternative to SHA-1 has been our default
everywhere for a while, except on OSX. As 3/4 notes this seems to have
been a mistake.

Furthermore our documentation (INSTALL) was still claiming that we
used OpenSSL for SHA-1 by default, etc. See 3/4 for all the details.

This series was previously submitted in April[1], at the time it
included a change to remove the SHA-1 implementation in PPC
assembly. That part has since landed on master in fd1ec82547d (Merge
branch 'ab/retire-ppc-sha1', 2022-09-09).

I meant to re-send this anyway, but was prompted by the failure
report(s) in [2]. The 1/4 here un-breaks "master", and could be peeled
off.

For a branch for this & passing CI see:
https://github.com/avar/git/tree/avar/sha1dc-really-everywhere-and-no-ppc-sha1-2

1. https://lore.kernel.org/git/cover-0.5-00000000000-20220422T094624Z-avarab@gmail.com/
2. https://lore.kernel.org/git/xmqqo7u9wyt7.fsf@gitster.g/

Ævar Arnfjörð Bjarmason (4):
  fsmonitor OSX: compile with DC_SHA1=YesPlease
  Makefile: create and use sections for "define" flag listing
  Makefile: really use and document sha1collisiondetection by default
  Makefile: rephrase the discussion of *_SHA1 knobs

 INSTALL                             |  10 +-
 Makefile                            | 288 +++++++++++++++++-----------
 ci/lib.sh                           |   3 +
 compat/fsmonitor/fsm-ipc-darwin.c   |  10 +-
 contrib/buildsystems/CMakeLists.txt |   3 +-
 hash.h                              |  10 +-
 t/t0013-sha1dc.sh                   |   4 +-
 7 files changed, 198 insertions(+), 130 deletions(-)

Range-diff against v1:
-:  ----------- > 1:  392fabdb456 fsmonitor OSX: compile with DC_SHA1=YesPlease
1:  3a80fcb6784 ! 2:  7ae22276aa7 Makefile: create and use sections for "define" flag listing
    @@ Makefile: include shared.mak
     -# Define BLK_SHA1 environment variable to make use of the bundled
     -# optimized C SHA1 routine.
     -#
    --# Define PPC_SHA1 environment variable when running make to make use of
    --# a bundled SHA1 routine optimized for PowerPC.
    --#
     -# Define DC_SHA1 to unconditionally enable the collision-detecting sha1
     -# algorithm. This is slower, but may detect attempted collision attacks.
     -# Takes priority over other *_SHA1 knobs.
    @@ Makefile: include shared.mak
     -#
     -# Define BLK_SHA256 to use the built-in SHA-256 routines.
     -#
    +-# Define NETTLE_SHA256 to use the SHA-256 routines in libnettle.
    +-#
     -# Define GCRYPT_SHA256 to use the SHA-256 routines in libgcrypt.
     -#
     -# Define OPENSSL_SHA256 to use the SHA-256 routines in OpenSSL.
    @@ Makefile: include shared.mak
      #
      # Define NEEDS_SSL_WITH_CRYPTO if you need -lssl when using -lcrypto (Darwin).
     @@ Makefile: include shared.mak
    - # `compat/fsmonitor/fsm-listen-<name>.c` that implements the
    - # `fsm_listen__*()` routines.
    + # to the "<name>" of the corresponding `compat/fsmonitor/fsm-settings-<name>.c`
    + # that implements the `fsm_os_settings__*()` routines.
      #
     +# === Optional library: libintl ===
     +#
    @@ Makefile: include shared.mak
     +# Define BLK_SHA1 environment variable to make use of the bundled
     +# optimized C SHA1 routine.
     +#
    -+# Define PPC_SHA1 environment variable when running make to make use of
    -+# a bundled SHA1 routine optimized for PowerPC.
    -+#
     +# Define DC_SHA1 to unconditionally enable the collision-detecting sha1
     +# algorithm. This is slower, but may detect attempted collision attacks.
     +# Takes priority over other *_SHA1 knobs.
    @@ Makefile: include shared.mak
     +#
     +# Define BLK_SHA256 to use the built-in SHA-256 routines.
     +#
    ++# Define NETTLE_SHA256 to use the SHA-256 routines in libnettle.
    ++#
     +# Define GCRYPT_SHA256 to use the SHA-256 routines in libgcrypt.
     +#
     +# Define OPENSSL_SHA256 to use the SHA-256 routines in OpenSSL.
2:  3e250dc9d09 ! 3:  78ef8636c57 Makefile: really use and document sha1collisiondetection by default
    @@ Commit message
         * Have a rewritten discussion of SHA-1 and SHA-256 in the Makefile
           which covers all of this.

    +    Let's also change the CI for "osx-clang" to test with the new
    +    APPLE_SHA1 knob ("osx-gcc" uses the new sha1collisiondetection
    +    default).
    +
    +    In practice this will spot issues like the one noted in [7], as
    +    testing with just two backends should be enough to spot unportable
    +    code. Ideally we'd have other CI jobs to test the various SHA-1
    +    combinations, but for now we have better CI coverage than before.
    +
         1. 48b3693d3ce (Merge branch 'jk/sha1dc', 2017-03-24)
         2. 8325e43b82d (Makefile: add DC_SHA1 knob, 2017-03-16)
         3. e6b07da2780 (Makefile: make DC_SHA1 the default, 2017-03-17)
    @@ Commit message
            2013-05-19)
         6. 3ef2bcad02e (imap-send: use Apple's Security framework for base64
            encoding, 2013-07-29)
    +    7. https://lore.kernel.org/git/kl6l7d0yyu6r.fsf@chooglen-macbookpro.roam.corp.google.com/

         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

    @@ INSTALL: Issues of note:

     -	  By default, git uses OpenSSL for SHA1 but it will use its own
     -	  library (inspired by Mozilla's) with either NO_OPENSSL or
    --	  BLK_SHA1.  Also included is a version optimized for PowerPC
    --	  (PPC_SHA1).
    +-	  BLK_SHA1.
     +	- Git uses an altered version of SHA-1 by default which
    -+          detects the SHAttered attack via the sha1collisiondetection
    -+          counter-cryptanalysis library. For SHA-256 we'll select a
    -+          working implementation (and ship a fallback
    -+          implementation). See the "SHA-1 and SHA-256 defines" section
    -+          in the Makefile for details. You should not need to tweak
    -+          those settings.
    ++	  detects the SHAttered attack via the sha1collisiondetection
    ++	  counter-cryptanalysis library. For SHA-256 we'll select a
    ++	  working implementation (and ship a fallback
    ++	  implementation). See the "SHA-1 and SHA-256 defines" section
    ++	  in the Makefile for details. You should not need to tweak
    ++	  those settings.

      	- "libcurl" library is used for fetching and pushing
      	  repositories over http:// or https://, as well as by
    @@ Makefile: include shared.mak
     -# Define BLK_SHA1 environment variable to make use of the bundled
     -# optimized C SHA1 routine.
     +# === SHA-1 backend ===
    - #
    --# Define PPC_SHA1 environment variable when running make to make use of
    --# a bundled SHA1 routine optimized for PowerPC.
    ++#
     +# Due to the SHAttered (https://shattered.io) attack vector on SHA-1
     +# Git uses the sha1collisiondetection counter-cryptanalysis library
     +# for SHA-1 hashing.
     +#
     +# You're strongly advised not to override this for any usage of Git
     +# where you don't 100% trust the repository content.
    -+#
    -+# ==== Options common to all SHA-1 implementations ====
    -+#
    -+# Define SHA1_MAX_BLOCK_SIZE to limit the amount of data that will be hashed
    -+# in one call to the platform's SHA1_Update(). e.g. APPLE_COMMON_CRYPTO
    -+# wants 'SHA1_MAX_BLOCK_SIZE=1024L*1024L*1024L' defined.
      #
     -# Define DC_SHA1 to unconditionally enable the collision-detecting sha1
     -# algorithm. This is slower, but may detect attempted collision attacks.
     -# Takes priority over other *_SHA1 knobs.
    -+# ===== Options for the default sha1collisiondetection implementations =====
    ++# ==== Options common to all SHA-1 implementations ====
      #
     -# Define DC_SHA1_EXTERNAL in addition to DC_SHA1 if you want to build / link
    ++# Define SHA1_MAX_BLOCK_SIZE to limit the amount of data that will be hashed
    ++# in one call to the platform's SHA1_Update(). e.g. APPLE_COMMON_CRYPTO
    ++# wants 'SHA1_MAX_BLOCK_SIZE=1024L*1024L*1024L' defined.
    ++#
    ++# ===== Options for the default sha1collisiondetection implementations =====
    ++#
     +# Define DC_SHA1_EXTERNAL if you want to build / link
      # git with the external SHA1 collision-detect library.
      # Without this option, i.e. the default behavior is to build git with its
    @@ Makefile: include shared.mak
     -# Define SHA1_MAX_BLOCK_SIZE to limit the amount of data that will be hashed
     -# in one call to the platform's SHA1_Update(). e.g. APPLE_COMMON_CRYPTO
     -# wants 'SHA1_MAX_BLOCK_SIZE=1024L*1024L*1024L' defined.
    -+# Define PPC_SHA1 environment variable when running make to make use of
    -+# a bundled SHA1 routine optimized for PowerPC.
    -+#
     +# Define APPLE_SHA1 to use Apple's CommonCrypto SHA-1 routines on
     +# Darwin/Mac OS X.
     +#
    @@ Makefile: ifeq ($(uname_S),Darwin)
     -		APPLE_COMMON_CRYPTO = YesPlease
      		COMPAT_CFLAGS += -DAPPLE_COMMON_CRYPTO
      	endif
    - 	NO_REGEX = YesPlease
    + 	PTHREAD_LIBS =
     @@ Makefile: ifdef NO_POSIX_GOODIES
      	BASIC_CFLAGS += -DNO_POSIX_GOODIES
      endif
    @@ Makefile: ifdef NO_POSIX_GOODIES
     -ifdef APPLE_COMMON_CRYPTO
     -	# Apple CommonCrypto requires chunking
     -	SHA1_MAX_BLOCK_SIZE = 1024L*1024L*1024L
    -+ifdef DC_SHA1
    -+$(error the DC_SHA1 flag is no longer used, and has become the default. Adjust your build scripts accordingly)
    - endif
    +-endif
     -
    + ifdef PPC_SHA1
    + $(error the PPC_SHA1 flag has been removed along with the PowerPC-specific SHA-1 implementation.)
    + endif
    + 
     -ifdef OPENSSL_SHA1
     -	EXTLIBS += $(LIB_4_CRYPTO)
     -	BASIC_CFLAGS += -DSHA1_OPENSSL
    @@ Makefile: ifdef NO_POSIX_GOODIES
     -	LIB_OBJS += block-sha1/sha1.o
     -	BASIC_CFLAGS += -DSHA1_BLK
     -else
    --ifdef PPC_SHA1
    --	LIB_OBJS += ppc/sha1.o ppc/sha1ppc.o
    --	BASIC_CFLAGS += -DSHA1_PPC
    --else
     -ifdef APPLE_COMMON_CRYPTO
     -	COMPAT_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL
     -	BASIC_CFLAGS += -DSHA1_APPLE
     -else
     -	DC_SHA1 := YesPlease
     -	BASIC_CFLAGS += -DSHA1_DC
    ++ifdef DC_SHA1
    ++$(error the DC_SHA1 flag is no longer used, and has become the default. Adjust your build scripts accordingly)
    ++endif
     +ifndef NO_DC_SHA1
    -+	ifneq ($(OPENSSL_SHA1)$(BLK_SHA1)$(PPC_SHA1)$(APPLE_SHA1),)
    ++	ifneq ($(OPENSSL_SHA1)$(BLK_SHA1)$(APPLE_SHA1),)
     +$(error no other *_SHA1 option can be defined unless NO_DC_SHA1 is defined)
     +	endif
      	LIB_OBJS += sha1dc_git.o
    @@ Makefile: endif
     +	LIB_OBJS += block-sha1/sha1.o
     +	BASIC_CFLAGS += -DSHA1_BLK
     +else
    -+ifdef PPC_SHA1
    -+	LIB_OBJS += ppc/sha1.o ppc/sha1ppc.o
    -+	BASIC_CFLAGS += -DSHA1_PPC
    -+else
     +ifdef APPLE_SHA1
     +	COMPAT_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL
     +	BASIC_CFLAGS += -DSHA1_APPLE
    @@ Makefile: endif
      endif
      endif
     @@ Makefile: GIT-BUILD-OPTIONS: FORCE
    - 	@echo NO_PYTHON=\''$(subst ','\'',$(subst ','\'',$(NO_PYTHON)))'\' >>$@+
    + 	@echo NO_REGEX=\''$(subst ','\'',$(subst ','\'',$(NO_REGEX)))'\' >>$@+
      	@echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst ','\'',$(NO_UNIX_SOCKETS)))'\' >>$@+
      	@echo PAGER_ENV=\''$(subst ','\'',$(subst ','\'',$(PAGER_ENV)))'\' >>$@+
     -	@echo DC_SHA1=\''$(subst ','\'',$(subst ','\'',$(DC_SHA1)))'\' >>$@+
    @@ Makefile: GIT-BUILD-OPTIONS: FORCE
      	@echo SANITIZE_ADDRESS=\''$(subst ','\'',$(subst ','\'',$(SANITIZE_ADDRESS)))'\' >>$@+
      	@echo X=\'$(X)\' >>$@+

    + ## ci/lib.sh ##
    +@@ ci/lib.sh: macos-latest)
    + esac
    + 
    + case "$jobname" in
    ++osx-clang)
    ++	MAKEFLAGS="$MAKEFLAGS NO_DC_SHA1=Yes APPLE_SHA1=Yes"
    ++	;;
    + linux32)
    + 	CC=gcc
    + 	;;
    +
      ## contrib/buildsystems/CMakeLists.txt ##
     @@ contrib/buildsystems/CMakeLists.txt: set(NO_PERL )
      set(NO_PTHREADS )
    @@ hash.h
      #include "git-compat-util.h"
      #include "repository.h"

    --#if defined(SHA1_PPC)
    +-#if defined(SHA1_APPLE)
     +#if !defined(NO_SHA1_DC)
     +#include "sha1dc_git.h"
    -+#elif defined(SHA1_PPC)
    - #include "ppc/sha1.h"
    - #elif defined(SHA1_APPLE)
    ++#elif defined(SHA1_APPLE)
      #include <CommonCrypto/CommonDigest.h>
      #elif defined(SHA1_OPENSSL)
      #include <openssl/sha.h>
    @@ hash.h
     +#error "need a SHA1_* implementation defined"
      #endif

    - #if defined(SHA256_GCRYPT)
    + #if defined(SHA256_NETTLE)

      ## t/t0013-sha1dc.sh ##
     @@ t/t0013-sha1dc.sh: TEST_PASSES_SANITIZE_LEAK=true
3:  1422c6fd497 ! 4:  f1fb9775b33 Makefile: rephrase the discussion of *_SHA1 knobs
    @@ Makefile: include shared.mak
     +# Define OPENSSL_SHA1 to link to the the SHA-1 routines from
     +# the OpenSSL library.
      #
    --# Define PPC_SHA1 environment variable when running make to make use of
    --# a bundled SHA1 routine optimized for PowerPC.
    -+# Define PPC_SHA1 to make use of optimized (in assembly)
    -+# PowerPC SHA-1 routines.
    - #
      # Define APPLE_SHA1 to use Apple's CommonCrypto SHA-1 routines on
      # Darwin/Mac OS X.
4:  ae464dbd228 < -:  ----------- Makefile + hash.h: remove PPC_SHA1 implementation
5:  730d08da6fe < -:  ----------- Makefile: use $(OBJECTS) instead of $(C_OBJ)
-- 
2.38.0.1093.gcd4a685f0b1