On Tue, Jul 25, 2017 at 7:57 AM, Takashi Iwai <tiwai@xxxxxxx> wrote:
> Some distros provide SHA1 collision detect code as a shared library.
> It's the very same code as we have in git tree, and git can link with
> it as well; at least, it may make maintenance easier, according to our
> security guys.
>
> This patch allows user to build git linking with the external sha1dc
> library instead of the built-in sha1dc code. User needs to define
> DC_SHA1_EXTERNAL explicitly. As default, the built-in sha1dc code is
> used like before.
This whole thing sounds sensible. I reviewed this (but like Junio
haven't tested it with a lib) and I think it would be worth noting the
following in the commit message / Makefile documentation:
* The "sha1detectcoll" *.so name for the "sha1collisiondetection"
library is not something you or suse presumably) made up, it's a name
the sha1collisiondetection.git itself creates for its library. I think
the Makefile docs you've added here are a bit confusing, you talk
about the "external sha1collisiondetection library" but then link
against sha1detectcoll". It's worth calling out this difference in the
docs IMO. I.e. not talk about the sha1detectcoll.so library form of
sha1collisiondetection, not the sha1collisiondetection project name as
a library.
* It might be worth noting that this is *not* linking against the same
code we ship ourselves due to the difference in defining
SHA1DC_INIT_SAFE_HASH_DEFAULT for the git project's needs in the one
we build, hence your need to have a git_SHA1DCInit() wrapper whereas
we call SHA1DCInit() directly. It might be interesting to note that
the library version will always be *slightly* slower (although the
difference will be trivial).
* Nothing in your commit message or docs explains why DC_SHA1_LINK is
needed. We don't have these sorts of variables for other external
libraries we link to, why the difference?
Some other things I observed:
* We now have much of the same header code copy/pasted between
sha1dc_git.h and sha1dc_git_ext.h, did you consider just always
including the former but making what it's doing conditional on
DC_SHA1_EXTERNAL? I don't know if it would be worth it from a cursory
glance, but again your commit message doesn't list that among options
considered & discarded.
* I think it makes sense to spew out a "not both!" error in the
Makefile if you set DC_SHA1_EXTERNAL=Y and DC_SHA1_SUBMODULE=Y. See my
94da9193a6 ("grep: add support for PCRE v2", 2017-06-01) for an
example of how to do this.
* The whole business of "#include <sha1.h>" looks very fragile, are
there really no other packages in e.g. suse that ship a sha1.h? Debian
has libmd-dev that ships /usr/include/sha1.h that conflicts with this:
https://packages.debian.org/search?searchon=contents&keywords=sha1.h&mode=exactfilename&suite=unstable&arch=any
Shipping a sha1.h as opposed to a sha1collisiondetection.h or
sha1detectcoll.h or whatever seems like a *really* bad decision by
upstream that should be the subject of at least seeing if they'll take
a pull request to fix it before you package it or before we include
something that'll probably need to be fixed / worked around anyway in
Git.
> Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
> ---
> Makefile | 12 ++++++++++++
> hash.h | 4 +++-
> sha1dc_git_ext.c | 11 +++++++++++
> sha1dc_git_ext.h | 25 +++++++++++++++++++++++++
> 4 files changed, 51 insertions(+), 1 deletion(-)
> create mode 100644 sha1dc_git_ext.c
> create mode 100644 sha1dc_git_ext.h
>
> diff --git a/Makefile b/Makefile
> index 461c845d33cb..f1a262d56254 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -162,6 +162,12 @@ all::
> # algorithm. This is slower, but may detect attempted collision attacks.
> # Takes priority over other *_SHA1 knobs.
> #
> +# Define DC_SHA1_EXTERNAL in addition to DC_SHA1 if you want to build / link
> +# git with the external sha1collisiondetection library.
> +# Without this option, i.e. the default behavior is to build git with its
> +# own sha1dc code. If any extra linker option is required, define them in
> +# DC_SHA1_LINK variable in addition.
> +#
> # Define DC_SHA1_SUBMODULE in addition to DC_SHA1 to use the
> # sha1collisiondetection shipped as a submodule instead of the
> # non-submodule copy in sha1dc/. This is an experimental option used
> @@ -1472,6 +1478,11 @@ ifdef APPLE_COMMON_CRYPTO
> BASIC_CFLAGS += -DSHA1_APPLE
> else
> DC_SHA1 := YesPlease
> +ifdef DC_SHA1_EXTERNAL
> + LIB_OBJS += sha1dc_git_ext.o
> + BASIC_CFLAGS += -DSHA1_DC -DDC_SHA1_EXTERNAL
> + EXTLIBS += $(DC_SHA1_LINK) -lsha1detectcoll
> +else
> ifdef DC_SHA1_SUBMODULE
> LIB_OBJS += sha1collisiondetection/lib/sha1.o
> LIB_OBJS += sha1collisiondetection/lib/ubc_check.o
> @@ -1492,6 +1503,7 @@ endif
> endif
> endif
> endif
> +endif
>
> ifdef SHA1_MAX_BLOCK_SIZE
> LIB_OBJS += compat/sha1-chunked.o
> diff --git a/hash.h b/hash.h
> index bef3e630a093..dce327d58d07 100644
> --- a/hash.h
> +++ b/hash.h
> @@ -8,7 +8,9 @@
> #elif defined(SHA1_OPENSSL)
> #include <openssl/sha.h>
> #elif defined(SHA1_DC)
> -#ifdef DC_SHA1_SUBMODULE
> +#if defined(DC_SHA1_EXTERNAL)
> +#include "sha1dc_git_ext.h"
> +#elif defined(DC_SHA1_SUBMODULE)
> #include "sha1collisiondetection/lib/sha1.h"
> #else
> #include "sha1dc/sha1.h"
> diff --git a/sha1dc_git_ext.c b/sha1dc_git_ext.c
> new file mode 100644
> index 000000000000..359439fc3d93
> --- /dev/null
> +++ b/sha1dc_git_ext.c
> @@ -0,0 +1,11 @@
> +/* Only for DC_SHA1_EXTERNAL; sharing the same hooks as built-in sha1dc */
> +
> +#include "cache.h"
> +#include <sha1.h>
> +#include "sha1dc_git.c"
> +
> +void git_SHA1DCInit(SHA1_CTX *ctx)
> +{
> + SHA1DCInit(ctx);
> + SHA1DCSetSafeHash(ctx, 0);
> +}
> diff --git a/sha1dc_git_ext.h b/sha1dc_git_ext.h
> new file mode 100644
> index 000000000000..d0ea8ce518db
> --- /dev/null
> +++ b/sha1dc_git_ext.h
> @@ -0,0 +1,25 @@
> +/*
> + * This file is included by hash.h for DC_SHA1_EXTERNAL
> + */
> +
> +#include <sha1.h>
> +
> +/*
> + * Same as SHA1DCInit, but with default save_hash=0
> + */
> +void git_SHA1DCInit(SHA1_CTX *);
> +
> +/*
> + * Same as SHA1DCFinal, but convert collision attack case into a verbose die().
> + */
> +void git_SHA1DCFinal(unsigned char [20], SHA1_CTX *);
> +
> +/*
> + * Same as SHA1DCUpdate, but adjust types to match git's usual interface.
> + */
> +void git_SHA1DCUpdate(SHA1_CTX *ctx, const void *data, unsigned long len);
> +
> +#define platform_SHA_CTX SHA1_CTX
> +#define platform_SHA1_Init git_SHA1DCInit
> +#define platform_SHA1_Update git_SHA1DCUpdate
> +#define platform_SHA1_Final git_SHA1DCFinal
> --
> 2.13.3
>
