[PATCH] push --signed: tighten what the receiving end can ask to sign

Junio C Hamano· Apr 2, 2015, 22:09 UTC · lore

Instead of blindly trusting the receiving side to give us a sensible nonce to sign, limit the length (max 256 bytes) and the alphabet (alnum and a few selected punctuations, enough to encode in base64) that can be used in nonce.

Noticed-by: Jann Horn <jann@thejh.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 send-pack.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

Show changes to send-pack.c +23 −0

diff --git a/send-pack.c b/send-pack.c
index 7ad1a59..2249808 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -279,6 +279,28 @@ free_return:
 	return update_seen;
 }
 
+#define NONCE_LEN_LIMIT 256
+
+static void reject_invalid_nonce(const char *nonce, int len)
+{
+	int i = 0;
+
+	if (NONCE_LEN_LIMIT <= len)
+		die("the receiving end asked to sign an invalid nonce <%.*s>",
+		    len, nonce);
+
+	for (i = 0; i < len; i++) {
+		int ch = nonce[i] & 0xFF;
+		if (isalnum(ch) ||
+		    ch == '-' || ch == '.' ||
+		    ch == '/' || ch == '+' ||
+		    ch == '=' || ch == '_')
+			continue;
+		die("the receiving end asked to sign an invalid nonce <%.*s>",
+		    len, nonce);
+	}
+}
+
 int send_pack(struct send_pack_args *args,
 	      int fd[], struct child_process *conn,
 	      struct ref *remote_refs,
@@ -321,6 +343,7 @@ int send_pack(struct send_pack_args *args,
 		push_cert_nonce = server_feature_value("push-cert", &len);
 		if (!push_cert_nonce)
 			die(_("the receiving end does not support --signed push"));
+		reject_invalid_nonce(push_cert_nonce, len);
 		push_cert_nonce = xmemdupz(push_cert_nonce, len);
 	}

-- 
2.4.0-rc1-147-g8712228

Jeff King· Apr 2, 2015, 22:16 UTC · re: Junio C Hamano · lore

Re: [PATCH] push --signed: tighten what the receiving end can ask to sign

On Thu, Apr 02, 2015 at 03:09:15PM -0700, Junio C Hamano wrote:

Show 5 quoted lines

> +		if (isalnum(ch) ||
> +		    ch == '-' || ch == '.' ||
> +		    ch == '/' || ch == '+' ||
> +		    ch == '=' || ch == '_')
> +			continue;

I think this looks good. Earlier I suggested reducing the set of allowable punctuation characters, but if we want to allow base64, that's "+/=" right there. We use "-" ourselves, leaving only "." and "_" as possible extras. But both of those are as useful as "-" for separators, and are fairly innocuous, so I don't mind leaving them.

-Peff

patchpush --signed: tighten what the receiving end can ask to sign