{"thread":{"id":"37630","subject":"[PATCH] Receive-pack: include entire SHA1 in nonce","startedAt":"2014-09-25T15:02:20Z","lastAt":"2014-09-25T18:03:32Z","messageCount":5,"participants":["Brian Gernhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"249846","messageId":"1411657340-62950-1-git-send-email-brian@gernhardtsoftware.com","threadId":"37630","inReplyTo":null,"subject":"[PATCH] Receive-pack: include entire SHA1 in nonce","fromName":"Brian Gernhardt","fromEmail":"brian@gernhardtsoftware.com","sentAt":"2014-09-25T15:02:20Z","receivedAt":"2014-09-25T15:02:20Z","isPatch":true,"sender":{"key":"brian@gernhardtsoftware.com","avatar":"https://avatars.githubusercontent.com/u/133455?v=4"},"body":"clang gives the following warning:\n\nbuiltin/receive-pack.c:327:35: error: sizeof on array function\nparameter will return size of 'unsigned char *' instead of 'unsigned\nchar [20]' [-Werror,-Wsizeof-array-argument]\n git_SHA1_Update(&ctx, out, sizeof(out));\n ^\nbuiltin/receive-pack.c:292:37: note: declared here\nstatic void hmac_sha1(unsigned char out[20],\n ^\n---\n\n I dislike changing sizeof to a magic constant, but clang informs me that\n sizeof is doing the wrong thing. Perhaps there's an appropriate constant\n #defined in the code somewhere?\n\n builtin/receive-pack.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex aab3df7..92388e5 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -324,7 +324,7 @@ static void hmac_sha1(unsigned char out[20],\n \t/* RFC 2104 2. (6) & (7) */\n \tgit_SHA1_Init(&ctx);\n \tgit_SHA1_Update(&ctx, k_opad, sizeof(k_opad));\n-\tgit_SHA1_Update(&ctx, out, sizeof(out));\n+\tgit_SHA1_Update(&ctx, out, 20);\n \tgit_SHA1_Final(out, &ctx);\n }\n \n-- \n2.1.1.445.gb8dfbef.dirty\n"},{"id":"249850","messageId":"xmqqoau3brf1.fsf@gitster.dls.corp.google.com","threadId":"37630","inReplyTo":"1411657340-62950-1-git-send-email-brian@gernhardtsoftware.com","subject":"Re: [PATCH] Receive-pack: include entire SHA1 in nonce","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-25T16:23:14Z","receivedAt":"2014-09-25T16:23:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brian Gernhardt writes:\n\n> clang gives the following warning:\n>\n> builtin/receive-pack.c:327:35: error: sizeof on array function\n> parameter will return size of 'unsigned char *' instead of 'unsigned\n> char [20]' [-Werror,-Wsizeof-array-argument]\n> git_SHA1_Update(&ctx, out, sizeof(out));\n> ^\n> builtin/receive-pack.c:292:37: note: declared here\n> static void hmac_sha1(unsigned char out[20],\n> ^\n> ---\n>\n> I dislike changing sizeof to a magic constant, but clang informs me that\n> sizeof is doing the wrong thing.\n\nThanks. I knew the code was wrong when I wrote it but somehow it\nended up as I wrote X-<. Let me find a brown paper bag ;-)\n"},{"id":"249851","messageId":"xmqqk34rbqu0.fsf@gitster.dls.corp.google.com","threadId":"37630","inReplyTo":"1411657340-62950-1-git-send-email-brian@gernhardtsoftware.com","subject":"Re: [PATCH] Receive-pack: include entire SHA1 in nonce","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-25T16:35:51Z","receivedAt":"2014-09-25T16:35:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brian Gernhardt writes:\n\n> clang gives the following warning:\n>\n> builtin/receive-pack.c:327:35: error: sizeof on array function\n> parameter will return size of 'unsigned char *' instead of 'unsigned\n> char [20]' [-Werror,-Wsizeof-array-argument]\n> git_SHA1_Update(&ctx, out, sizeof(out));\n> ^\n> builtin/receive-pack.c:292:37: note: declared here\n> static void hmac_sha1(unsigned char out[20],\n> ^\n> ---\n>\n> I dislike changing sizeof to a magic constant, but clang informs me that\n> sizeof is doing the wrong thing. Perhaps there's an appropriate constant\n> #defined in the code somewhere?\n\nBy the way, the title is very misleading, as truncating the HMAC\nwhen creating nonce is done deliberately and it sounds as if the\npatch is breaking that part of the system.\n\nWe could pass \"how many bytes of output do we want\" as another\nparameter to hmac_sha1() or define that as a constant, and copy out\nonly that many from here.\n\nAnd then use the same constant when deciding to truncate the result\nof sha1_to_hex() in the caller.\n\nI am not happy with this version, either, though, because now we\nhave an uninitialized piece of memory at the end of sha1[20] of the\ncaller, which is given to sha1_to_hex() to produce garbage. It is\ndiscarded by %.*s format so there is no negative net effect, but I\nsuspect that the compiler would not see that through.\n\n\n builtin/receive-pack.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex efb13b1..93fc39d 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -287,8 +287,9 @@ static int copy_to_sideband(int in, int out, void *arg)\n }\n \n #define HMAC_BLOCK_SIZE 64\n+#define HMAC_TRUNCATE 10 /* bytes */\n \n-static void hmac_sha1(unsigned char out[20],\n+static void hmac_sha1(unsigned char *out,\n \t\t const char *key_in, size_t key_len,\n \t\t const char *text, size_t text_len)\n {\n@@ -323,7 +324,7 @@ static void hmac_sha1(unsigned char out[20],\n \t/* RFC 2104 2. (6) & (7) */\n \tgit_SHA1_Init(&ctx);\n \tgit_SHA1_Update(&ctx, k_opad, sizeof(k_opad));\n-\tgit_SHA1_Update(&ctx, out, sizeof(out));\n+\tgit_SHA1_Update(&ctx, out, HMAC_TRUNCATE);\n \tgit_SHA1_Final(out, &ctx);\n }\n \n@@ -337,7 +338,8 @@ static char *prepare_push_cert_nonce(const char *path, unsigned long stamp)\n \tstrbuf_release(&buf);\n \n \t/* RFC 2104 5. HMAC-SHA1-80 */\n-\tstrbuf_addf(&buf, \"%lu-%.*s\", stamp, 20, sha1_to_hex(sha1));\n+\tstrbuf_addf(&buf, \"%lu-%.*s\", stamp,\n+\t\t 2 * HMAC_TRUNCATE, sha1_to_hex(sha1));\n \treturn strbuf_detach(&buf, NULL);\n }\n \n"},{"id":"249853","messageId":"xmqqa95nbn7g.fsf@gitster.dls.corp.google.com","threadId":"37630","inReplyTo":"xmqqk34rbqu0.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] Receive-pack: include entire SHA1 in nonce","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-25T17:54:11Z","receivedAt":"2014-09-25T17:54:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I am not happy with this version, either, though, because now we\n> have an uninitialized piece of memory at the end of sha1[20] of the\n> caller, which is given to sha1_to_hex() to produce garbage. It is\n> discarded by %.*s format so there is no negative net effect, but I\n> suspect that the compiler would not see that through.\n\n... and if we want to fix that, we would end up with a set of\nchanges, somewhat ugly like this.\n\nWhich might be an improvement, but let's start with your \"sizeof(arg)\nis the size of a pointer, even when the definition of arg[] is\nspelled with bra-ket, a dummy maintainer!\" fix.\n\nI'd like to have your sign-off. I'd also prefer to retitle it as\nsomething like \"hmac_sha1: copy the entire SHA-1 hash out\", as it is\ndeliberate that we do not include the entire SHA-1 in nonce.\n \nThanks.\n\n---\n\n\n builtin/receive-pack.c | 13 ++++++++-----\n cache.h | 1 +\n hex.c | 24 ++++++++++++++----------\n 3 files changed, 23 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex efb13b1..e0e7c75 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -287,8 +287,9 @@ static int copy_to_sideband(int in, int out, void *arg)\n }\n \n #define HMAC_BLOCK_SIZE 64\n+#define HMAC_TRUNCATE 10 /* in bytes */\n \n-static void hmac_sha1(unsigned char out[20],\n+static void hmac_sha1(unsigned char *out,\n \t\t const char *key_in, size_t key_len,\n \t\t const char *text, size_t text_len)\n {\n@@ -323,21 +324,23 @@ static void hmac_sha1(unsigned char out[20],\n \t/* RFC 2104 2. (6) & (7) */\n \tgit_SHA1_Init(&ctx);\n \tgit_SHA1_Update(&ctx, k_opad, sizeof(k_opad));\n-\tgit_SHA1_Update(&ctx, out, sizeof(out));\n+\tgit_SHA1_Update(&ctx, out, HMAC_TRUNCATE);\n \tgit_SHA1_Final(out, &ctx);\n }\n \n static char *prepare_push_cert_nonce(const char *path, unsigned long stamp)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n-\tunsigned char sha1[20];\n+\tunsigned char hmac[HMAC_TRUNCATE];\n+\tchar hmac_trunc[HMAC_TRUNCATE * 2 + 1];\n \n \tstrbuf_addf(&buf, \"%s:%lu\", path, stamp);\n-\thmac_sha1(sha1, buf.buf, buf.len, cert_nonce_seed, strlen(cert_nonce_seed));;\n+\thmac_sha1(hmac, buf.buf, buf.len, cert_nonce_seed, strlen(cert_nonce_seed));;\n \tstrbuf_release(&buf);\n \n \t/* RFC 2104 5. HMAC-SHA1-80 */\n-\tstrbuf_addf(&buf, \"%lu-%.*s\", stamp, 20, sha1_to_hex(sha1));\n+\tbin_to_hex(hmac, HMAC_TRUNCATE, hmac_trunc);\n+\tstrbuf_addf(&buf, \"%lu-%s\", stamp, hmac_trunc);\n \treturn strbuf_detach(&buf, NULL);\n }\n \ndiff --git a/cache.h b/cache.h\nindex fcb511d..bf508a2 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -965,6 +965,7 @@ extern int for_each_abbrev(const char *prefix, each_abbrev_fn, void *);\n */\n extern int get_sha1_hex(const char *hex, unsigned char *sha1);\n \n+extern void bin_to_hex(const unsigned char *bin, int, char *hexout);\n extern char *sha1_to_hex(const unsigned char *sha1);\t/* static buffer result! */\n extern int read_ref_full(const char *refname, unsigned char *sha1,\n \t\t\t int reading, int *flags);\ndiff --git a/hex.c b/hex.c\nindex 9ebc050..1b30e6e 100644\n--- a/hex.c\n+++ b/hex.c\n@@ -56,20 +56,24 @@ int get_sha1_hex(const char *hex, unsigned char *sha1)\n \treturn 0;\n }\n \n-char *sha1_to_hex(const unsigned char *sha1)\n+void bin_to_hex(const unsigned char *bin, int len, char *hexout)\n {\n-\tstatic int bufno;\n-\tstatic char hexbuffer[4][50];\n \tstatic const char hex[] = \"0123456789abcdef\";\n-\tchar *buffer = hexbuffer[3 & ++bufno], *buf = buffer;\n-\tint i;\n \n-\tfor (i = 0; i < 20; i++) {\n-\t\tunsigned int val = *sha1++;\n-\t\t*buf++ = hex[val >> 4];\n-\t\t*buf++ = hex[val & 0xf];\n+\twhile (0 < len--) {\n+\t\tunsigned int val = *bin++;\n+\t\t*hexout++ = hex[val >> 4];\n+\t\t*hexout++ = hex[val & 0xf];\n \t}\n-\t*buf = '\\0';\n+\t*hexout = '\\0';\n+}\n+\n+char *sha1_to_hex(const unsigned char *sha1)\n+{\n+\tstatic int bufno;\n+\tstatic char hexbuffer[4][50];\n+\tchar *buffer = hexbuffer[3 & ++bufno];\n \n+\tbin_to_hex(sha1, 20, buffer);\n \treturn buffer;\n }\n"},{"id":"249856","messageId":"84433534-D6A9-4FD3-BA53-DCD610B64251@gernhardtsoftware.com","threadId":"37630","inReplyTo":"xmqqa95nbn7g.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] Receive-pack: include entire SHA1 in nonce","fromName":"Brian Gernhardt","fromEmail":"brian@gernhardtsoftware.com","sentAt":"2014-09-25T18:03:32Z","receivedAt":"2014-09-25T18:03:32Z","isPatch":true,"sender":{"key":"brian@gernhardtsoftware.com","avatar":"https://avatars.githubusercontent.com/u/133455?v=4"},"body":"On Sep 25, 2014, at 1:54 PM, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n>> I am not happy with this version, either, though, because now we\n>> have an uninitialized piece of memory at the end of sha1[20] of the\n>> caller, which is given to sha1_to_hex() to produce garbage. It is\n>> discarded by %.*s format so there is no negative net effect, but I\n>> suspect that the compiler would not see that through.\n> \n> ... and if we want to fix that, we would end up with a set of\n> changes, somewhat ugly like this.\n> \n> Which might be an improvement, but let's start with your \"sizeof(arg)\n> is the size of a pointer, even when the definition of arg[] is\n> spelled with bra-ket, a dummy maintainer!\" fix.\n> \n> I'd like to have your sign-off. I'd also prefer to retitle it as\n> something like \"hmac_sha1: copy the entire SHA-1 hash out\", as it is\n> deliberate that we do not include the entire SHA-1 in nonce.\n\nIt'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 \n\nSigned-off-by: Brian Gernhardt \n\nattached to whatever commit is actually appropriate instead of the minimum to make my compiler happy. :-)\n\n~~ Brian\n"}]}