{"thread":{"id":"38990","subject":"[PATCH] push --signed: tighten what the receiving end can ask to sign","startedAt":"2015-04-02T22:09:15Z","lastAt":"2015-04-02T22:16:03Z","messageCount":2,"participants":["Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"258902","messageId":"xmqqlhiagp2s.fsf@gitster.dls.corp.google.com","threadId":"38990","inReplyTo":null,"subject":"[PATCH] push --signed: tighten what the receiving end can ask to sign","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-02T22:09:15Z","receivedAt":"2015-04-02T22:09:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Instead of blindly trusting the receiving side to give us a sensible\nnonce to sign, limit the length (max 256 bytes) and the alphabet\n(alnum and a few selected punctuations, enough to encode in base64)\nthat can be used in nonce.\n\nNoticed-by: Jann Horn <jann@thejh.net>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n send-pack.c | 23 +++++++++++++++++++++++\n 1 file changed, 23 insertions(+)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex 7ad1a59..2249808 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -279,6 +279,28 @@ free_return:\n \treturn update_seen;\n }\n \n+#define NONCE_LEN_LIMIT 256\n+\n+static void reject_invalid_nonce(const char *nonce, int len)\n+{\n+\tint i = 0;\n+\n+\tif (NONCE_LEN_LIMIT <= len)\n+\t\tdie(\"the receiving end asked to sign an invalid nonce <%.*s>\",\n+\t\t    len, nonce);\n+\n+\tfor (i = 0; i < len; i++) {\n+\t\tint ch = nonce[i] & 0xFF;\n+\t\tif (isalnum(ch) ||\n+\t\t    ch == '-' || ch == '.' ||\n+\t\t    ch == '/' || ch == '+' ||\n+\t\t    ch == '=' || ch == '_')\n+\t\t\tcontinue;\n+\t\tdie(\"the receiving end asked to sign an invalid nonce <%.*s>\",\n+\t\t    len, nonce);\n+\t}\n+}\n+\n int send_pack(struct send_pack_args *args,\n \t      int fd[], struct child_process *conn,\n \t      struct ref *remote_refs,\n@@ -321,6 +343,7 @@ int send_pack(struct send_pack_args *args,\n \t\tpush_cert_nonce = server_feature_value(\"push-cert\", &len);\n \t\tif (!push_cert_nonce)\n \t\t\tdie(_(\"the receiving end does not support --signed push\"));\n+\t\treject_invalid_nonce(push_cert_nonce, len);\n \t\tpush_cert_nonce = xmemdupz(push_cert_nonce, len);\n \t}\n \n-- \n2.4.0-rc1-147-g8712228\n"},{"id":"258904","messageId":"20150402221603.GA13077@peff.net","threadId":"38990","inReplyTo":"xmqqlhiagp2s.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] push --signed: tighten what the receiving end can ask to sign","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-02T22:16:03Z","receivedAt":"2015-04-02T22:16:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 02, 2015 at 03:09:15PM -0700, Junio C Hamano wrote:\n\n> +\t\tif (isalnum(ch) ||\n> +\t\t    ch == '-' || ch == '.' ||\n> +\t\t    ch == '/' || ch == '+' ||\n> +\t\t    ch == '=' || ch == '_')\n> +\t\t\tcontinue;\n\nI think this looks good. Earlier I suggested reducing the set of\nallowable punctuation characters, but if we want to allow base64, that's\n\"+/=\" right there. We use \"-\" ourselves, leaving only \".\" and \"_\" as\npossible extras. But both of those are as useful as \"-\" for separators,\nand are fairly innocuous, so I don't mind leaving them.\n\n-Peff\n"}]}