Skip to content

Commit 0256189

Browse files
gitstergitster
committed
Merge branch 'mg/gpg-parse-tighten'
Detect and reject a signature block that has more than one GPG signature. * mg/gpg-parse-tighten: gpg-interface.c: detect and reject multiple signatures on commits
2 parents ff8e25e + da6cf1b commit 0256189

File tree

2 files changed

+87
-29
lines changed

2 files changed

+87
-29
lines changed

‎gpg-interface.c

Lines changed: 61 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -75,48 +75,80 @@ void signature_check_clear(struct signature_check *sigc)
7575
FREE_AND_NULL(sigc->key);
7676
}
7777

78+
/* An exclusive status -- only one of them can appear in output */
79+
#define GPG_STATUS_EXCLUSIVE (1<<0)
80+
7881
static struct {
7982
char result;
8083
const char *check;
84+
unsigned int flags;
8185
} sigcheck_gpg_status[] = {
82-
{ 'G', "\n[GNUPG:] GOODSIG " },
83-
{ 'B', "\n[GNUPG:] BADSIG " },
84-
{ 'U', "\n[GNUPG:] TRUST_NEVER" },
85-
{ 'U', "\n[GNUPG:] TRUST_UNDEFINED" },
86-
{ 'E', "\n[GNUPG:] ERRSIG "},
87-
{ 'X', "\n[GNUPG:] EXPSIG "},
88-
{ 'Y', "\n[GNUPG:] EXPKEYSIG "},
89-
{ 'R', "\n[GNUPG:] REVKEYSIG "},
86+
{ 'G', "GOODSIG ", GPG_STATUS_EXCLUSIVE },
87+
{ 'B', "BADSIG ", GPG_STATUS_EXCLUSIVE },
88+
{ 'U', "TRUST_NEVER", 0 },
89+
{ 'U', "TRUST_UNDEFINED", 0 },
90+
{ 'E', "ERRSIG ", GPG_STATUS_EXCLUSIVE },
91+
{ 'X', "EXPSIG ", GPG_STATUS_EXCLUSIVE },
92+
{ 'Y', "EXPKEYSIG ", GPG_STATUS_EXCLUSIVE },
93+
{ 'R', "REVKEYSIG ", GPG_STATUS_EXCLUSIVE },
9094
};
9195

9296
static void parse_gpg_output(struct signature_check *sigc)
9397
{
9498
const char *buf = sigc->gpg_status;
99+
const char *line, *next;
95100
int i;
96-
97-
/* Iterate over all search strings */
98-
for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {
99-
const char *found, *next;
100-
101-
if (!skip_prefix(buf, sigcheck_gpg_status[i].check + 1, &found)) {
102-
found = strstr(buf, sigcheck_gpg_status[i].check);
103-
if (!found)
104-
continue;
105-
found += strlen(sigcheck_gpg_status[i].check);
106-
}
107-
sigc->result = sigcheck_gpg_status[i].result;
108-
/* The trust messages are not followed by key/signer information */
109-
if (sigc->result != 'U') {
110-
next = strchrnul(found, ' ');
111-
sigc->key = xmemdupz(found, next - found);
112-
/* The ERRSIG message is not followed by signer information */
113-
if (*next && sigc-> result != 'E') {
114-
found = next + 1;
115-
next = strchrnul(found, '\n');
116-
sigc->signer = xmemdupz(found, next - found);
101+
int seen_exclusive_status = 0;
102+
103+
/* Iterate over all lines */
104+
for (line = buf; *line; line = strchrnul(line+1, '\n')) {
105+
while (*line == '\n')
106+
line++;
107+
/* Skip lines that don't start with GNUPG status */
108+
if (!skip_prefix(line, "[GNUPG:] ", &line))
109+
continue;
110+
111+
/* Iterate over all search strings */
112+
for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {
113+
if (skip_prefix(line, sigcheck_gpg_status[i].check, &line)) {
114+
if (sigcheck_gpg_status[i].flags & GPG_STATUS_EXCLUSIVE) {
115+
if (seen_exclusive_status++)
116+
goto found_duplicate_status;
117+
}
118+
119+
sigc->result = sigcheck_gpg_status[i].result;
120+
/* The trust messages are not followed by key/signer information */
121+
if (sigc->result != 'U') {
122+
next = strchrnul(line, ' ');
123+
free(sigc->key);
124+
sigc->key = xmemdupz(line, next - line);
125+
/* The ERRSIG message is not followed by signer information */
126+
if (*next && sigc->result != 'E') {
127+
line = next + 1;
128+
next = strchrnul(line, '\n');
129+
free(sigc->signer);
130+
sigc->signer = xmemdupz(line, next - line);
131+
}
132+
}
133+
134+
break;
117135
}
118136
}
119137
}
138+
return;
139+
140+
found_duplicate_status:
141+
/*
142+
* GOODSIG, BADSIG etc. can occur only once for each signature.
143+
* Therefore, if we had more than one then we're dealing with multiple
144+
* signatures. We don't support them currently, and they're rather
145+
* hard to create, so something is likely fishy and we should reject
146+
* them altogether.
147+
*/
148+
sigc->result = 'E';
149+
/* Clear partial data to avoid confusion */
150+
FREE_AND_NULL(sigc->signer);
151+
FREE_AND_NULL(sigc->key);
120152
}
121153

122154
int check_signature(const char *payload, size_t plen, const char *signature,

‎t/t7510-signed-commit.sh

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -234,4 +234,30 @@ test_expect_success GPG 'check config gpg.format values' '
234234
test_must_fail git commit -S --amend -m "fail"
235235
'
236236

237+
test_expect_success GPG 'detect fudged commit with double signature' '
238+
sed -e "/gpgsig/,/END PGP/d" forged1 >double-base &&
239+
sed -n -e "/gpgsig/,/END PGP/p" forged1 | \
240+
sed -e "s/^gpgsig//;s/^ //" | gpg --dearmor >double-sig1.sig &&
241+
gpg -o double-sig2.sig -u 29472784 --detach-sign double-base &&
242+
cat double-sig1.sig double-sig2.sig | gpg --enarmor >double-combined.asc &&
243+
sed -e "s/^\(-.*\)ARMORED FILE/\1SIGNATURE/;1s/^/gpgsig /;2,\$s/^/ /" \
244+
double-combined.asc > double-gpgsig &&
245+
sed -e "/committer/r double-gpgsig" double-base >double-commit &&
246+
git hash-object -w -t commit double-commit >double-commit.commit &&
247+
test_must_fail git verify-commit $(cat double-commit.commit) &&
248+
git show --pretty=short --show-signature $(cat double-commit.commit) >double-actual &&
249+
grep "BAD signature from" double-actual &&
250+
grep "Good signature from" double-actual
251+
'
252+
253+
test_expect_success GPG 'show double signature with custom format' '
254+
cat >expect <<-\EOF &&
255+
E
256+
257+
258+
EOF
259+
git log -1 --format="%G?%n%GK%n%GS" $(cat double-commit.commit) >actual &&
260+
test_cmp expect actual
261+
'
262+
237263
test_done

0 commit comments

Comments
 (0)