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
Nov 27, 2019 at 17:48 UTC
72b006f4bfd30b7c5037c163efaf279ab65bea9c
4 files changed
+72
-75
builtin/fmt-merge-msg.c
+7
-4
@@ -495,6 +495,7 @@ static void fmt_merge_msg_sigs(struct strbuf *out)
495
enum object_type type;
496
unsigned long size, len;
497
char *buf = read_object_file(oid, &type, &size);
498
+ struct signature_check sigc = { 0 };
499
struct strbuf sig = STRBUF_INIT;
500
501
if (!buf || type != OBJ_TAG)
@@ -503,10 +504,12 @@ static void fmt_merge_msg_sigs(struct strbuf *out)
504
505
if (size == len)
506
; /* merely annotated */
506
- else if (verify_signed_buffer(buf, len, buf + len, size - len, &sig, NULL)) {
507
- if (!sig.len)
508
- strbuf_addstr(&sig, "gpg verification failed.\n");
509
- }
507
+ else if (!check_signature(buf, len, buf + len, size - len,
508
+ &sigc)) {
509
+ strbuf_addstr(&sig, sigc.gpg_output);
510
+ signature_check_clear(&sigc);
511
+ } else
512
+ strbuf_addstr(&sig, "gpg verification failed.\n");
513
514
if (!tag_number++) {
515
fmt_tag_signature(&tagbuf, &sig, buf, len);
gpg-interface.c
+49
-48
@@ -207,6 +207,55 @@ found_duplicate_status:
207
FREE_AND_NULL(sigc->key);
208
}
209
210
+static int verify_signed_buffer(const char *payload, size_t payload_size,
211
+ const char *signature, size_t signature_size,
212
+ struct strbuf *gpg_output,
213
+ struct strbuf *gpg_status)
214
+{
215
+ struct child_process gpg = CHILD_PROCESS_INIT;
216
+ struct gpg_format *fmt;
217
+ struct tempfile *temp;
218
+ int ret;
219
+ struct strbuf buf = STRBUF_INIT;
220
+
221
+ temp = mks_tempfile_t(".git_vtag_tmpXXXXXX");
222
+ if (!temp)
223
+ return error_errno(_("could not create temporary file"));
224
+ if (write_in_full(temp->fd, signature, signature_size) < 0 ||
225
+ close_tempfile_gently(temp) < 0) {
226
+ error_errno(_("failed writing detached signature to '%s'"),
227
+ temp->filename.buf);
228
+ delete_tempfile(&temp);
229
+ return -1;
230
+ }
231
+
232
+ fmt = get_format_by_sig(signature);
233
+ if (!fmt)
234
+ BUG("bad signature '%s'", signature);
235
+
236
+ argv_array_push(&gpg.args, fmt->program);
237
+ argv_array_pushv(&gpg.args, fmt->verify_args);
238
+ argv_array_pushl(&gpg.args,
239
+ "--status-fd=1",
240
+ "--verify", temp->filename.buf, "-",
241
+ NULL);
242
+
243
+ if (!gpg_status)
244
+ gpg_status = &buf;
245
+
246
+ sigchain_push(SIGPIPE, SIG_IGN);
247
+ ret = pipe_command(&gpg, payload, payload_size,
248
+ gpg_status, 0, gpg_output, 0);
249
+ sigchain_pop(SIGPIPE);
250
+
251
+ delete_tempfile(&temp);
252
+
253
+ ret |= !strstr(gpg_status->buf, "\n[GNUPG:] GOODSIG ");
254
+ strbuf_release(&buf); /* no matter it was used or not */
255
+
256
+ return ret;
257
+}
258
+
259
int check_signature(const char *payload, size_t plen, const char *signature,
260
size_t slen, struct signature_check *sigc)
261
{
@@ -351,51 +400,3 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
400
401
return 0;
402
}
354
-
355
-int verify_signed_buffer(const char *payload, size_t payload_size,
356
- const char *signature, size_t signature_size,
357
- struct strbuf *gpg_output, struct strbuf *gpg_status)
358
-{
359
- struct child_process gpg = CHILD_PROCESS_INIT;
360
- struct gpg_format *fmt;
361
- struct tempfile *temp;
362
- int ret;
363
- struct strbuf buf = STRBUF_INIT;
364
-
365
- temp = mks_tempfile_t(".git_vtag_tmpXXXXXX");
366
- if (!temp)
367
- return error_errno(_("could not create temporary file"));
368
- if (write_in_full(temp->fd, signature, signature_size) < 0 ||
369
- close_tempfile_gently(temp) < 0) {
370
- error_errno(_("failed writing detached signature to '%s'"),
371
- temp->filename.buf);
372
- delete_tempfile(&temp);
373
- return -1;
374
- }
375
-
376
- fmt = get_format_by_sig(signature);
377
- if (!fmt)
378
- BUG("bad signature '%s'", signature);
379
-
380
- argv_array_push(&gpg.args, fmt->program);
381
- argv_array_pushv(&gpg.args, fmt->verify_args);
382
- argv_array_pushl(&gpg.args,
383
- "--status-fd=1",
384
- "--verify", temp->filename.buf, "-",
385
- NULL);
386
-
387
- if (!gpg_status)
388
- gpg_status = &buf;
389
-
390
- sigchain_push(SIGPIPE, SIG_IGN);
391
- ret = pipe_command(&gpg, payload, payload_size,
392
- gpg_status, 0, gpg_output, 0);
393
- sigchain_pop(SIGPIPE);
394
-
395
- delete_tempfile(&temp);
396
-
397
- ret |= !strstr(gpg_status->buf, "\n[GNUPG:] GOODSIG ");
398
- strbuf_release(&buf); /* no matter it was used or not */
399
-
400
- return ret;
401
-}
gpg-interface.h
-9
@@ -46,15 +46,6 @@ size_t parse_signature(const char *buf, size_t size);
46
int sign_buffer(struct strbuf *buffer, struct strbuf *signature,
47
const char *signing_key);
48
49
-/*
50
- * Run "gpg" to see if the payload matches the detached signature.
51
- * gpg_output, when set, receives the diagnostic output from GPG.
52
- * gpg_status, when set, receives the status output from GPG.
53
- */
54
-int verify_signed_buffer(const char *payload, size_t payload_size,
55
- const char *signature, size_t signature_size,
56
- struct strbuf *gpg_output, struct strbuf *gpg_status);
57
-
49
int git_gpg_config(const char *, const char *, void *);
50
void set_signing_key(const char *);
51
const char *get_signing_key(void);
log-tree.c
+16
-14
@@ -448,22 +448,22 @@ static void show_signature(struct rev_info *opt, struct commit *commit)
448
{
449
struct strbuf payload = STRBUF_INIT;
450
struct strbuf signature = STRBUF_INIT;
451
- struct strbuf gpg_output = STRBUF_INIT;
451
+ struct signature_check sigc = { 0 };
452
int status;
453
454
if (parse_signed_commit(commit, &payload, &signature) <= 0)
455
goto out;
456
457
- status = verify_signed_buffer(payload.buf, payload.len,
458
- signature.buf, signature.len,
459
- &gpg_output, NULL);
460
- if (status && !gpg_output.len)
461
- strbuf_addstr(&gpg_output, "No signature\n");
462
-
463
- show_sig_lines(opt, status, gpg_output.buf);
457
+ status = check_signature(payload.buf, payload.len, signature.buf,
458
+ signature.len, &sigc);
459
+ if (status && sigc.result == 'N')
460
+ show_sig_lines(opt, status, "No signature\n");
461
+ else {
462
+ show_sig_lines(opt, status, sigc.gpg_output);
463
+ signature_check_clear(&sigc);
464
+ }
465
466
out:
466
- strbuf_release(&gpg_output);
467
strbuf_release(&payload);
468
strbuf_release(&signature);
469
}
@@ -496,6 +496,7 @@ 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;
502
@@ -524,12 +525,13 @@ static int show_one_mergetag(struct commit *commit,
525
status = -1;
526
if (extra->len > payload_size) {
527
/* could have a good signature */
527
- if (!verify_signed_buffer(extra->value, payload_size,
528
- extra->value + payload_size,
529
- extra->len - payload_size,
530
- &verify_message, NULL))
528
+ if (!check_signature(extra->value, payload_size,
529
+ extra->value + payload_size,
530
+ extra->len - payload_size, &sigc)) {
531
+ strbuf_addstr(&verify_message, sigc.gpg_output);
532
+ signature_check_clear(&sigc);
533
status = 0; /* good */
532
- else if (verify_message.len <= gpg_message_offset)
534
+ } else if (verify_message.len <= gpg_message_offset)
535
strbuf_addstr(&verify_message, "No signature\n");
536
/* otherwise we couldn't verify, which is shown as bad */
537
}