Re: [PATCH v5 4/5] merge/pull Check for untrusted good GPG signatures

[John Keeping <john@keeping.me.uk>]

On Sun, Mar 31, 2013 at 02:27:20PM +0200, Sebastian Götte wrote:
> On 03/31/2013 02:16 PM, Thomas Rast wrote:
> > "Sebastian Götte" <jaseg@physik.tu-berlin.de> wrote:
> > 
> >> expecting success: 
> >> test_must_fail git merge --ff-only --verify-signatures side-untrusted
> >> 2>mergeerror &&
> >>        test_i18ngrep "has a good, untrusted GPG signature" mergeerror
> >>
> >> ==1430== Conditional jump or move depends on uninitialised value(s)
> >> ==1430==    at 0x4C26B5C: strchrnul (mc_replace_strmem.c:711)
> >> ==1430==    by 0x47B90B: check_commit_signature (commit.c:1057)
> >> ==1430==    by 0x444212: cmd_merge (merge.c:1245)
> >> ==1430==    by 0x4050E6: handle_internal_command (git.c:281)
> >> ==1430==    by 0x40530C: main (git.c:489)
> >>
> >> Though I also can't see the problem. strchrnul gets passed a char* that
> >> is just fine.
> > 
> > Usually that means that the string *contents* are uninitialized,
> > usually because you scanned past the end of the string...
>
> I checked for that, everything looks fine to me. The pointer should point to a valid, 0-terminated string.

It looks like the "found" pointer has wandered off the end of the
string.  In the test case here, the gpg_status is:

-- >8 --
[GNUPG:] SIG_ID rzX3GbdzQyxB4Jdm1uD0CzL4B4Y 2013-03-31 1364735152
[GNUPG:] GOODSIG 61092E85B7227189 Eris Discordia <discord@example.net>
[GNUPG:] VALIDSIG D4BE22311AD3131E5EDA29A461092E85B7227189 2013-03-31
1364735152 0 4 0 1 2 00 D4BE22311AD3131E5EDA29A461092E85B7227189
[GNUPG:] TRUST_UNDEFINED
-- 8< --

But the parse_signature_lines code assumes that after reading a
signature it can fill in the key from the next 16 bytes and then look
for a newline after that.  In this case it clearly needs to only read
the signature if it's a GOODSIG or BADSIG line.

Wrapping a "signature_check[i].result != 'U'" condition around the lines
that extract the key and advance the "found" pointer after doing so
fixes this for me.


John