Re: [PATCH v2 19/19] signed push: fortify against replay attacks

[Eric Sunshine <sunshine@sunshineco.com>]

On Fri, Aug 22, 2014 at 4:30 PM, Junio C Hamano <gitster@pobox.com> wrote:
> In order to prevent a valid push certificate for pushing into an
> repository from getting replayed to push to an unrelated one, send a
> nonce string from the receive-pack process and have the signer
> include it in the push certificate.  The original nonce is exported
> as GIT_PUSH_CERT_NONCE for the hooks to examine and match against
> the value on the "nonce" header in the certificate to notice a replay.
>
> Because the built-in nonce generation may not be suitable for all
> situations, allow the server to invoke receive-pack with pregenerated
> nonce from the command line argument.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 991e417..8ad4d9b 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -1226,12 +1232,28 @@ static int delete_only(struct command *commands)
>         return 1;
>  }
>
> +static char *prepare_push_cert_nonce(const char *sitename, const char *dir)
> +{
> +       struct strbuf buf = STRBUF_INIT;
> +       unsigned char sha1[20];
> +
> +       if (!sitename) {
> +               static char buf[1024];

Potentially confusing 'buf' shadows 'buf' in outer scope.

> +               gethostname(buf, sizeof(buf));
> +               sitename = buf;
> +       }
> +       strbuf_addf(&buf, "%s:%s:%lu", sitename, dir, time(NULL));
> +       hash_sha1_file(buf.buf, buf.len, "blob", sha1);

strbuf_release(&buf);

> +       return xstrdup(sha1_to_hex(sha1));
> +}
> +
>  int cmd_receive_pack(int argc, const char **argv, const char *prefix)
>  {
>         int advertise_refs = 0;
>         int stateless_rpc = 0;
>         int i;
>         const char *dir = NULL;
> +       const char *sitename = NULL;
>         struct command *commands;
>         struct sha1_array shallow = SHA1_ARRAY_INIT;
>         struct sha1_array ref = SHA1_ARRAY_INIT;
> @@ -1261,6 +1283,13 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
>                                 fix_thin = 0;
>                                 continue;
>                         }
> +                       if (skip_prefix(arg, "--sitename=", &sitename)) {
> +                               continue;
> +                       }
> +                       if (skip_prefix(arg, "--push-cert-nonce=", &push_cert_nonce)) {
> +                               push_cert_nonce = xstrdup(push_cert_nonce);
> +                               continue;
> +                       }
>
>                         usage(receive_pack_usage);
>                 }
> @@ -1277,6 +1306,8 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
>                 die("'%s' does not appear to be a git repository", dir);
>
>         git_config(receive_pack_config, NULL);
> +       if (!push_cert_nonce)
> +               push_cert_nonce = prepare_push_cert_nonce(sitename, dir);
>
>         if (0 <= transfer_unpack_limit)
>                 unpack_limit = transfer_unpack_limit;
> @@ -1321,5 +1352,6 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
>                 packet_flush(1);
>         sha1_array_clear(&shallow);
>         sha1_array_clear(&ref);
> +       free((void *)push_cert_nonce);
>         return 0;
>  }
> diff --git a/send-pack.c b/send-pack.c
> index 61f321d..349393a 100644
> --- a/send-pack.c
> +++ b/send-pack.c
> @@ -228,7 +228,8 @@ static const char *next_line(const char *line, size_t len)
>  static int generate_push_cert(struct strbuf *req_buf,
>                               const struct ref *remote_refs,
>                               struct send_pack_args *args,
> -                             const char *cap_string)
> +                             const char *cap_string,
> +                             const char *push_cert_nonce)
>  {
>         const struct ref *ref;
>         char stamp[60];
> @@ -240,6 +241,8 @@ static int generate_push_cert(struct strbuf *req_buf,
>         datestamp(stamp, sizeof(stamp));
>         strbuf_addf(&cert, "certificate version 0.1\n");
>         strbuf_addf(&cert, "pusher %s %s\n", signing_key, stamp);
> +       if (push_cert_nonce[0])
> +               strbuf_addf(&cert, "nonce %s\n", push_cert_nonce);
>         strbuf_addstr(&cert, "\n");
>
>         for (ref = remote_refs; ref; ref = ref->next) {
> @@ -290,6 +293,8 @@ int send_pack(struct send_pack_args *args,
>         unsigned cmds_sent = 0;
>         int ret;
>         struct async demux;
> +       const char *push_cert_nonce = NULL;
> +
>
>         /* Does the other end support the reporting? */
>         if (server_supports("report-status"))
> @@ -306,8 +311,14 @@ int send_pack(struct send_pack_args *args,
>                 agent_supported = 1;
>         if (server_supports("no-thin"))
>                 args->use_thin_pack = 0;
> -       if (args->push_cert && !server_supports("push-cert"))
> -               die(_("the receiving end does not support --signed push"));
> +       if (args->push_cert) {
> +               int len;
> +
> +               push_cert_nonce = server_feature_value("push-cert", &len);
> +               if (!push_cert_nonce)
> +                       die(_("the receiving end does not support --signed push"));
> +               push_cert_nonce = xmemdupz(push_cert_nonce, len);
> +       }
>
>         if (!remote_refs) {
>                 fprintf(stderr, "No refs in common and none specified; doing nothing.\n"
> @@ -338,7 +349,7 @@ int send_pack(struct send_pack_args *args,
>
>         if (!args->dry_run && args->push_cert)
>                 cmds_sent = generate_push_cert(&req_buf, remote_refs, args,
> -                                              cap_buf.buf);
> +                                              cap_buf.buf, push_cert_nonce);
>
>         /*
>          * Clear the status for each ref and see if we need to send
> diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh
> index 659bca0..6db59ce 100755
> --- a/t/t5534-push-signed.sh
> +++ b/t/t5534-push-signed.sh
> @@ -58,17 +58,22 @@ test_expect_success GPG 'signed push sends push certificate' '
>         SIGNER=${GIT_PUSH_CERT_SIGNER-nobody}
>         KEY=${GIT_PUSH_CERT_KEY-nokey}
>         STATUS=${GIT_PUSH_CERT_STATUS-nostatus}
> +       NONCE=${GIT_PUSH_CERT_NONCE-nononce}
>         E_O_F
>
>         EOF
>
> -       cat >expect <<-\EOF &&
> -       SIGNER=C O Mitter <committer@example.com>
> -       KEY=13B6F51ECDDE430D
> -       STATUS=G
> -       EOF
> -
>         git push --signed dst noop ff +noff &&
> +
> +       (
> +               cat <<-\EOF &&
> +               SIGNER=C O Mitter <committer@example.com>
> +               KEY=13B6F51ECDDE430D
> +               STATUS=G
> +               EOF
> +               sed -n -e "s/^nonce /NONCE=/p" -e "/^$/q" dst/push-cert
> +       ) >expect &&
> +
>         grep "$(git rev-parse noop ff) refs/heads/ff" dst/push-cert &&
>         grep "$(git rev-parse noop noff) refs/heads/noff" dst/push-cert &&
>         test_cmp expect dst/push-cert-status
> --
> 2.1.0-304-g950f846
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html