Michał Górny <mgo...@gentoo.org> writes:
> GnuPG supports creating signatures consisting of multiple signature
> packets. If such a signature is verified, it outputs all the status
> messages for each signature separately. However, git currently does not
> account for such scenario and gets terribly confused over getting
> multiple *SIG statuses.
>
> For example, if a malicious party alters a signed commit and appends
> a new untrusted signature, git is going to ignore the original bad
> signature and report untrusted commit instead. However, %GK and %GS
> format strings may still expand to the data corresponding
> to the original signature, potentially tricking the scripts into
> trusting the malicious commit.
>
> Given that the use of multiple signatures is quite rare, git does not
> support creating them without jumping through a few hoops, and finally
> supporting them properly would require extensive API improvement, it
> seems reasonable to just reject them at the moment.
>
> Signed-off-by: Michał Górny <mgo...@gentoo.org>
> ---
> gpg-interface.c | 90 +++++++++++++++++++++++++++-------------
> t/t7510-signed-commit.sh | 26 ++++++++++++
> 2 files changed, 87 insertions(+), 29 deletions(-)
>
> Changes in v4:
> * switched to using skip_prefix(),
> * renamed the variable to seen_exclusive_status,
> * made the loop terminate early on first duplicate status seen.
Thanks for sticking to the topic and polishing it further. Looks
very good.
Will replace.
> + int seen_exclusive_status = 0;
> +
> + /* Iterate over all lines */
> + for (line = buf; *line; line = strchrnul(line+1, '\n')) {
> + while (*line == '\n')
> + line++;
> + /* Skip lines that don't start with GNUPG status */
> + if (!skip_prefix(line, "[GNUPG:] ", &line))
> + continue;
> +
> + /* Iterate over all search strings */
> + for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {
> + if (skip_prefix(line, sigcheck_gpg_status[i].check,
> &line)) {
> + if (sigcheck_gpg_status[i].flags &
> GPG_STATUS_EXCLUSIVE) {
> + if (++seen_exclusive_status > 1)
> + goto found_duplicate_status;
Very minor point but by not using pre-increment, i.e.
if (seen_exclusive_status++)
goto found_duplicate_status;
you can use the expression as a "have we already seen?" boolean,
whic may probably be more idiomatic.
The patch is good in the way written as-is, and this is so minor
that it is not worth rerolling to only update this part.
Thanks.
