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

From

Junio C Hamano <gitster@pobox.com>

Date

Sep 25, 2014, 17:54 UTC

Message-ID

<xmqqa95nbn7g.fsf@gitster.dls.corp.google.com>

In-Reply-To

<xmqqk34rbqu0.fsf@gitster.dls.corp.google.com>

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.

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;
 }