[PATCH] Receive-pack: include entire SHA1 in nonce

[PATCH] Receive-pack: include entire SHA1 in nonce

[Brian Gernhardt]

clang gives the following warning:

builtin/receive-pack.c:327:35: error: sizeof on array function
parameter will return size of 'unsigned char *' instead of 'unsigned
char [20]' [-Werror,-Wsizeof-array-argument]
        git_SHA1_Update(&ctx, out, sizeof(out));
                                         ^
builtin/receive-pack.c:292:37: note: declared here
static void hmac_sha1(unsigned char out[20],
                                    ^
---

 I dislike changing sizeof to a magic constant, but clang informs me that
 sizeof is doing the wrong thing.  Perhaps there's an appropriate constant
 #defined in the code somewhere?

 builtin/receive-pack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index aab3df7..92388e5 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -324,7 +324,7 @@ static void hmac_sha1(unsigned char out[20],
 	/* RFC 2104 2. (6) & (7) */
 	git_SHA1_Init(&ctx);
 	git_SHA1_Update(&ctx, k_opad, sizeof(k_opad));
-	git_SHA1_Update(&ctx, out, sizeof(out));
+	git_SHA1_Update(&ctx, out, 20);
 	git_SHA1_Final(out, &ctx);
 }
 
-- 
2.1.1.445.gb8dfbef.dirty

Re: [PATCH] Receive-pack: include entire SHA1 in nonce

[Junio C Hamano]

Brian Gernhardt <brian@gernhardtsoftware.com> writes:

> clang gives the following warning:
>
> builtin/receive-pack.c:327:35: error: sizeof on array function
> parameter will return size of 'unsigned char *' instead of 'unsigned
> char [20]' [-Werror,-Wsizeof-array-argument]
>         git_SHA1_Update(&ctx, out, sizeof(out));
>                                          ^
> builtin/receive-pack.c:292:37: note: declared here
> static void hmac_sha1(unsigned char out[20],
>                                     ^
> ---
>
>  I dislike changing sizeof to a magic constant, but clang informs me that
>  sizeof is doing the wrong thing.

Thanks. I knew the code was wrong when I wrote it but somehow it
ended up as I wrote X-<.  Let me find a brown paper bag ;-)

Re: [PATCH] Receive-pack: include entire SHA1 in nonce

[Junio C Hamano]

Brian Gernhardt <brian@gernhardtsoftware.com> writes:

> clang gives the following warning:
>
> builtin/receive-pack.c:327:35: error: sizeof on array function
> parameter will return size of 'unsigned char *' instead of 'unsigned
> char [20]' [-Werror,-Wsizeof-array-argument]
>         git_SHA1_Update(&ctx, out, sizeof(out));
>                                          ^
> builtin/receive-pack.c:292:37: note: declared here
> static void hmac_sha1(unsigned char out[20],
>                                     ^
> ---
>
>  I dislike changing sizeof to a magic constant, but clang informs me that
>  sizeof is doing the wrong thing.  Perhaps there's an appropriate constant
>  #defined in the code somewhere?

By the way, the title is very misleading, as truncating the HMAC
when creating nonce is done deliberately and it sounds as if the
patch is breaking that part of the system.

We could pass "how many bytes of output do we want" as another
parameter to hmac_sha1() or define that as a constant, and copy out
only that many from here.

And then use the same constant when deciding to truncate the result
of sha1_to_hex() in the caller.

I am not happy with this version, either, though, because now we
have an uninitialized piece of memory at the end of sha1[20] of the
caller, which is given to sha1_to_hex() to produce garbage.  It is
discarded by %.*s format so there is no negative net effect, but I
suspect that the compiler would not see that through.


 builtin/receive-pack.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index efb13b1..93fc39d 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -287,8 +287,9 @@ static int copy_to_sideband(int in, int out, void *arg)
 }
 
 #define HMAC_BLOCK_SIZE 64
+#define HMAC_TRUNCATE 10 /* bytes */
 
-static void hmac_sha1(unsigned char out[20],
+static void hmac_sha1(unsigned char *out,
 		      const char *key_in, size_t key_len,
 		      const char *text, size_t text_len)
 {
@@ -323,7 +324,7 @@ static void hmac_sha1(unsigned char out[20],
 	/* RFC 2104 2. (6) & (7) */
 	git_SHA1_Init(&ctx);
 	git_SHA1_Update(&ctx, k_opad, sizeof(k_opad));
-	git_SHA1_Update(&ctx, out, sizeof(out));
+	git_SHA1_Update(&ctx, out, HMAC_TRUNCATE);
 	git_SHA1_Final(out, &ctx);
 }
 
@@ -337,7 +338,8 @@ static char *prepare_push_cert_nonce(const char *path, unsigned long stamp)
 	strbuf_release(&buf);
 
 	/* RFC 2104 5. HMAC-SHA1-80 */
-	strbuf_addf(&buf, "%lu-%.*s", stamp, 20, sha1_to_hex(sha1));
+	strbuf_addf(&buf, "%lu-%.*s", stamp,
+		    2 * HMAC_TRUNCATE, sha1_to_hex(sha1));
 	return strbuf_detach(&buf, NULL);
 }
 

Re: [PATCH] Receive-pack: include entire SHA1 in nonce

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I am not happy with this version, either, though, because now we
> have an uninitialized piece of memory at the end of sha1[20] of the
> caller, which is given to sha1_to_hex() to produce garbage.  It is
> discarded by %.*s format so there is no negative net effect, but I
> suspect that the compiler would not see that through.

... and if we want to fix that, we would end up with a set of
changes, somewhat ugly like this.

Which might be an improvement, but let's start with your "sizeof(arg)
is the size of a pointer, even when the definition of arg[] is
spelled with bra-ket, a dummy maintainer!" fix.

I'd like to have your sign-off.  I'd also prefer to retitle it as
something like "hmac_sha1: copy the entire SHA-1 hash out", as it is
deliberate that we do not include the entire SHA-1 in nonce.
    
Thanks.

---


 builtin/receive-pack.c | 13 ++++++++-----
 cache.h                |  1 +
 hex.c                  | 24 ++++++++++++++----------
 3 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index efb13b1..e0e7c75 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -287,8 +287,9 @@ static int copy_to_sideband(int in, int out, void *arg)
 }
 
 #define HMAC_BLOCK_SIZE 64
+#define HMAC_TRUNCATE 10 /* in bytes */
 
-static void hmac_sha1(unsigned char out[20],
+static void hmac_sha1(unsigned char *out,
 		      const char *key_in, size_t key_len,
 		      const char *text, size_t text_len)
 {
@@ -323,21 +324,23 @@ static void hmac_sha1(unsigned char out[20],
 	/* RFC 2104 2. (6) & (7) */
 	git_SHA1_Init(&ctx);
 	git_SHA1_Update(&ctx, k_opad, sizeof(k_opad));
-	git_SHA1_Update(&ctx, out, sizeof(out));
+	git_SHA1_Update(&ctx, out, HMAC_TRUNCATE);
 	git_SHA1_Final(out, &ctx);
 }
 
 static char *prepare_push_cert_nonce(const char *path, unsigned long stamp)
 {
 	struct strbuf buf = STRBUF_INIT;
-	unsigned char sha1[20];
+	unsigned char hmac[HMAC_TRUNCATE];
+	char hmac_trunc[HMAC_TRUNCATE * 2 + 1];
 
 	strbuf_addf(&buf, "%s:%lu", path, stamp);
-	hmac_sha1(sha1, buf.buf, buf.len, cert_nonce_seed, strlen(cert_nonce_seed));;
+	hmac_sha1(hmac, buf.buf, buf.len, cert_nonce_seed, strlen(cert_nonce_seed));;
 	strbuf_release(&buf);
 
 	/* RFC 2104 5. HMAC-SHA1-80 */
-	strbuf_addf(&buf, "%lu-%.*s", stamp, 20, sha1_to_hex(sha1));
+	bin_to_hex(hmac, HMAC_TRUNCATE, hmac_trunc);
+	strbuf_addf(&buf, "%lu-%s", stamp, hmac_trunc);
 	return strbuf_detach(&buf, NULL);
 }
 
diff --git a/cache.h b/cache.h
index fcb511d..bf508a2 100644
--- a/cache.h
+++ b/cache.h
@@ -965,6 +965,7 @@ extern int for_each_abbrev(const char *prefix, each_abbrev_fn, void *);
  */
 extern int get_sha1_hex(const char *hex, unsigned char *sha1);
 
+extern void bin_to_hex(const unsigned char *bin, int, char *hexout);
 extern char *sha1_to_hex(const unsigned char *sha1);	/* static buffer result! */
 extern int read_ref_full(const char *refname, unsigned char *sha1,
 			 int reading, int *flags);
diff --git a/hex.c b/hex.c
index 9ebc050..1b30e6e 100644
--- a/hex.c
+++ b/hex.c
@@ -56,20 +56,24 @@ int get_sha1_hex(const char *hex, unsigned char *sha1)
 	return 0;
 }
 
-char *sha1_to_hex(const unsigned char *sha1)
+void bin_to_hex(const unsigned char *bin, int len, char *hexout)
 {
-	static int bufno;
-	static char hexbuffer[4][50];
 	static const char hex[] = "0123456789abcdef";
-	char *buffer = hexbuffer[3 & ++bufno], *buf = buffer;
-	int i;
 
-	for (i = 0; i < 20; i++) {
-		unsigned int val = *sha1++;
-		*buf++ = hex[val >> 4];
-		*buf++ = hex[val & 0xf];
+	while (0 < len--) {
+		unsigned int val = *bin++;
+		*hexout++ = hex[val >> 4];
+		*hexout++ = hex[val & 0xf];
 	}
-	*buf = '\0';
+	*hexout = '\0';
+}
+
+char *sha1_to_hex(const unsigned char *sha1)
+{
+	static int bufno;
+	static char hexbuffer[4][50];
+	char *buffer = hexbuffer[3 & ++bufno];
 
+	bin_to_hex(sha1, 20, buffer);
 	return buffer;
 }

Re: [PATCH] Receive-pack: include entire SHA1 in nonce

[Brian Gernhardt]

On Sep 25, 2014, at 1:54 PM, Junio C Hamano <gitster@pobox.com> wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
>> I am not happy with this version, either, though, because now we
>> have an uninitialized piece of memory at the end of sha1[20] of the
>> caller, which is given to sha1_to_hex() to produce garbage.  It is
>> discarded by %.*s format so there is no negative net effect, but I
>> suspect that the compiler would not see that through.
> 
> ... and if we want to fix that, we would end up with a set of
> changes, somewhat ugly like this.
> 
> Which might be an improvement, but let's start with your "sizeof(arg)
> is the size of a pointer, even when the definition of arg[] is
> spelled with bra-ket, a dummy maintainer!" fix.
> 
> I'd like to have your sign-off.  I'd also prefer to retitle it as
> something like "hmac_sha1: copy the entire SHA-1 hash out", as it is
> deliberate that we do not include the entire SHA-1 in nonce.

It's been long enough since I've done any crypto, so I didn't really know what the algorithm should look like.  Mostly I remember "doing it right is hard", so don't feel too bad.  Making the commit message accurate is perfectly fine, and all the patches you've posted look right at first glance (and to make test as well), so I'm fine with a 

Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>

attached to whatever commit is actually appropriate instead of the minimum to make my compiler happy.  :-)

~~ Brian