gpg-interface: prefer check_signature() for GPG verification
This commit refactors the use of verify_signed_buffer() outside of gpg-interface.c to use check_signature() instead. It also turns verify_signed_buffer() into a file-local function since it's now only invoked internally by check_signature(). There were previously two globally scoped functions used in different parts of Git to perform GPG signature verification: verify_signed_buffer() and check_signature(). Now only check_signature() is used. The verify_signed_buffer() function doesn't guard against duplicate signatures as described by Michał Górny [1]. Instead it only ensures a non-erroneous exit code from GPG and the presence of at least one GOODSIG status field. This stands in contrast with check_signature() that returns an error if more than one signature is encountered. The lower degree of verification makes the use of verify_signed_buffer() problematic if callers don't parse and validate the various parts of the GPG status message themselves. And processing these messages seems like a task that should be reserved to gpg-interface.c with the function check_signature(). Furthermore, the use of verify_signed_buffer() makes it difficult to introduce new functionality that relies on the content of the GPG status lines. Now all operations that does signature verification share a single entry point to gpg-interface.c. This makes it easier to propagate changed or additional functionality in GPG signature verification to all parts of Git, without having odd edge-cases that don't perform the same degree of verification. [1] https://dev.gentoo.org/~mgorny/articles/attack-on-git-signature-verification.html Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Hans Jerry Illikainen committed
Mar 4, 2020 at 11:48 UTC
67948981983b336eab2fa7e6a0e125d529391dfc
4 files changed
+73
-78
builtin/fmt-merge-msg.c
+7
-4
@@ -494,6 +494,7 @@ static void fmt_merge_msg_sigs(struct strbuf *out)
494
enum object_type type;
495
unsigned long size, len;
496
char *buf = read_object_file(oid, &type, &size);
497
+ struct signature_check sigc = { 0 };
498
struct strbuf sig = STRBUF_INIT;
499
500
if (!buf || type != OBJ_TAG)
@@ -502,10 +503,12 @@ static void fmt_merge_msg_sigs(struct strbuf *out)
503
504
if (size == len)
505
; /* merely annotated */
505
- else if (verify_signed_buffer(buf, len, buf + len, size - len, &sig, NULL)) {
506
- if (!sig.len)
507
- strbuf_addstr(&sig, "gpg verification failed.\n");
508
- }
506
+ else if (check_signature(buf, len, buf + len, size - len, &sigc) &&
507
+ !sigc.gpg_output)
508
+ strbuf_addstr(&sig, "gpg verification failed.\n");
509
+ else
510
+ strbuf_addstr(&sig, sigc.gpg_output);
511
+ signature_check_clear(&sigc);
512
513
if (!tag_number++) {
514
fmt_tag_signature(&tagbuf, &sig, buf, len);
gpg-interface.c
+49
-48
@@ -256,6 +256,55 @@ error:
256
FREE_AND_NULL(sigc->key);
257
}
258
259
+static int verify_signed_buffer(const char *payload, size_t payload_size,
260
+ const char *signature, size_t signature_size,
261
+ struct strbuf *gpg_output,
262
+ struct strbuf *gpg_status)
263
+{
264
+ struct child_process gpg = CHILD_PROCESS_INIT;
265
+ struct gpg_format *fmt;
266
+ struct tempfile *temp;
267
+ int ret;
268
+ struct strbuf buf = STRBUF_INIT;
269
+
270
+ temp = mks_tempfile_t(".git_vtag_tmpXXXXXX");
271
+ if (!temp)
272
+ return error_errno(_("could not create temporary file"));
273
+ if (write_in_full(temp->fd, signature, signature_size) < 0 ||
274
+ close_tempfile_gently(temp) < 0) {
275
+ error_errno(_("failed writing detached signature to '%s'"),
276
+ temp->filename.buf);
277
+ delete_tempfile(&temp);
278
+ return -1;
279
+ }
280
+
281
+ fmt = get_format_by_sig(signature);
282
+ if (!fmt)
283
+ BUG("bad signature '%s'", signature);
284
+
285
+ argv_array_push(&gpg.args, fmt->program);
286
+ argv_array_pushv(&gpg.args, fmt->verify_args);
287
+ argv_array_pushl(&gpg.args,
288
+ "--status-fd=1",
289
+ "--verify", temp->filename.buf, "-",
290
+ NULL);
291
+
292
+ if (!gpg_status)
293
+ gpg_status = &buf;
294
+
295
+ sigchain_push(SIGPIPE, SIG_IGN);
296
+ ret = pipe_command(&gpg, payload, payload_size,
297
+ gpg_status, 0, gpg_output, 0);
298
+ sigchain_pop(SIGPIPE);
299
+
300
+ delete_tempfile(&temp);
301
+
302
+ ret |= !strstr(gpg_status->buf, "\n[GNUPG:] GOODSIG ");
303
+ strbuf_release(&buf); /* no matter it was used or not */
304
+
305
+ return ret;
306
+}
307
+
308
int check_signature(const char *payload, size_t plen, const char *signature,
309
size_t slen, struct signature_check *sigc)
310
{
@@ -418,51 +467,3 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
467
468
return 0;
469
}
421
-
422
-int verify_signed_buffer(const char *payload, size_t payload_size,
423
- const char *signature, size_t signature_size,
424
- struct strbuf *gpg_output, struct strbuf *gpg_status)
425
-{
426
- struct child_process gpg = CHILD_PROCESS_INIT;
427
- struct gpg_format *fmt;
428
- struct tempfile *temp;
429
- int ret;
430
- struct strbuf buf = STRBUF_INIT;
431
-
432
- temp = mks_tempfile_t(".git_vtag_tmpXXXXXX");
433
- if (!temp)
434
- return error_errno(_("could not create temporary file"));
435
- if (write_in_full(temp->fd, signature, signature_size) < 0 ||
436
- close_tempfile_gently(temp) < 0) {
437
- error_errno(_("failed writing detached signature to '%s'"),
438
- temp->filename.buf);
439
- delete_tempfile(&temp);
440
- return -1;
441
- }
442
-
443
- fmt = get_format_by_sig(signature);
444
- if (!fmt)
445
- BUG("bad signature '%s'", signature);
446
-
447
- argv_array_push(&gpg.args, fmt->program);
448
- argv_array_pushv(&gpg.args, fmt->verify_args);
449
- argv_array_pushl(&gpg.args,
450
- "--status-fd=1",
451
- "--verify", temp->filename.buf, "-",
452
- NULL);
453
-
454
- if (!gpg_status)
455
- gpg_status = &buf;
456
-
457
- sigchain_push(SIGPIPE, SIG_IGN);
458
- ret = pipe_command(&gpg, payload, payload_size,
459
- gpg_status, 0, gpg_output, 0);
460
- sigchain_pop(SIGPIPE);
461
-
462
- delete_tempfile(&temp);
463
-
464
- ret |= !strstr(gpg_status->buf, "\n[GNUPG:] GOODSIG ");
465
- strbuf_release(&buf); /* no matter it was used or not */
466
-
467
- return ret;
468
-}
gpg-interface.h
-9
@@ -54,15 +54,6 @@ size_t parse_signature(const char *buf, size_t size);
54
int sign_buffer(struct strbuf *buffer, struct strbuf *signature,
55
const char *signing_key);
56
57
-/*
58
- * Run "gpg" to see if the payload matches the detached signature.
59
- * gpg_output, when set, receives the diagnostic output from GPG.
60
- * gpg_status, when set, receives the status output from GPG.
61
- */
62
-int verify_signed_buffer(const char *payload, size_t payload_size,
63
- const char *signature, size_t signature_size,
64
- struct strbuf *gpg_output, struct strbuf *gpg_status);
65
-
57
int git_gpg_config(const char *, const char *, void *);
58
void set_signing_key(const char *);
59
const char *get_signing_key(void);
log-tree.c
+17
-17
@@ -449,22 +449,21 @@ static void show_signature(struct rev_info *opt, struct commit *commit)
449
{
450
struct strbuf payload = STRBUF_INIT;
451
struct strbuf signature = STRBUF_INIT;
452
- struct strbuf gpg_output = STRBUF_INIT;
452
+ struct signature_check sigc = { 0 };
453
int status;
454
455
if (parse_signed_commit(commit, &payload, &signature) <= 0)
456
goto out;
457
458
- status = verify_signed_buffer(payload.buf, payload.len,
459
- signature.buf, signature.len,
460
- &gpg_output, NULL);
461
- if (status && !gpg_output.len)
462
- strbuf_addstr(&gpg_output, "No signature\n");
463
-
464
- show_sig_lines(opt, status, gpg_output.buf);
458
+ status = check_signature(payload.buf, payload.len, signature.buf,
459
+ signature.len, &sigc);
460
+ if (status && !sigc.gpg_output)
461
+ show_sig_lines(opt, status, "No signature\n");
462
+ else
463
+ show_sig_lines(opt, status, sigc.gpg_output);
464
+ signature_check_clear(&sigc);
465
466
out:
467
- strbuf_release(&gpg_output);
467
strbuf_release(&payload);
468
strbuf_release(&signature);
469
}
@@ -497,8 +496,9 @@ static int show_one_mergetag(struct commit *commit,
496
struct object_id oid;
497
struct tag *tag;
498
struct strbuf verify_message;
499
+ struct signature_check sigc = { 0 };
500
int status, nth;
501
- size_t payload_size, gpg_message_offset;
501
+ size_t payload_size;
502
503
hash_object_file(the_hash_algo, extra->value, extra->len,
504
type_name(OBJ_TAG), &oid);
@@ -520,19 +520,19 @@ static int show_one_mergetag(struct commit *commit,
520
else
521
strbuf_addf(&verify_message,
522
"parent #%d, tagged '%s'\n", nth + 1, tag->tag);
523
- gpg_message_offset = verify_message.len;
523
524
payload_size = parse_signature(extra->value, extra->len);
525
status = -1;
526
if (extra->len > payload_size) {
527
/* could have a good signature */
529
- if (!verify_signed_buffer(extra->value, payload_size,
530
- extra->value + payload_size,
531
- extra->len - payload_size,
532
- &verify_message, NULL))
533
- status = 0; /* good */
534
- else if (verify_message.len <= gpg_message_offset)
528
+ status = check_signature(extra->value, payload_size,
529
+ extra->value + payload_size,
530
+ extra->len - payload_size, &sigc);
531
+ if (sigc.gpg_output)
532
+ strbuf_addstr(&verify_message, sigc.gpg_output);
533
+ else
534
strbuf_addstr(&verify_message, "No signature\n");
535
+ signature_check_clear(&sigc);
536
/* otherwise we couldn't verify, which is shown as bad */
537
}
538