Re: [PATCH v5 4/5] merge/pull Check for untrusted good GPG signatures
Sebastian Götte ja...@physik.tu-berlin.de writes:
When --verify-signatures is specified, abort the merge in case a good
GPG signature from an untrusted key is encountered.
[...]
+test_expect_success GPG 'merge commit with untrusted signature with
verification' '
^
`.
Nit: you have a pointless(?) double space here-´
+ test_must_fail git merge --ff-only --verify-signatures side-untrusted
2mergeerror
+ test_i18ngrep from an untrusted key mergeerror
+'
This test gives me the following:
==26527== Conditional jump or move depends on uninitialised value(s)
==26527==at 0x4C2D8BC: strchrnul (mc_replace_strmem.c:1084)
==26527==by 0x4989CC: parse_signature_lines (commit.c:1074)
==26527==by 0x498B33: check_commit_signature (commit.c:1100)
==26527==by 0x453719: cmd_merge (merge.c:1246)
==26527==by 0x4057B6: run_builtin (git.c:282)
==26527==by 0x405949: handle_internal_command (git.c:444)
==26527==by 0x405A63: run_argv (git.c:490)
==26527==by 0x405BF2: main (git.c:565)
though I currently cannot see what's wrong, probably because I don't
know the format that parse_signature_lines gives. Can you look into it?
--
Thomas Rast
trast@{inf,student}.ethz.ch
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 4/5] merge/pull Check for untrusted good GPG signatures
On 03/31/2013 10:32 AM, Thomas Rast wrote: Sebastian Götte ja...@physik.tu-berlin.de writes: When --verify-signatures is specified, abort the merge in case a good GPG signature from an untrusted key is encountered. [...] +test_expect_success GPG 'merge commit with untrusted signature with verification' ' ^ `. Nit: you have a pointless(?) double space here-´ Will fix that in the next revision ;) +test_must_fail git merge --ff-only --verify-signatures side-untrusted 2mergeerror +test_i18ngrep from an untrusted key mergeerror +' This test gives me the following: ==26527== Conditional jump or move depends on uninitialised value(s) ==26527==at 0x4C2D8BC: strchrnul (mc_replace_strmem.c:1084) ==26527==by 0x4989CC: parse_signature_lines (commit.c:1074) ==26527==by 0x498B33: check_commit_signature (commit.c:1100) ==26527==by 0x453719: cmd_merge (merge.c:1246) ==26527==by 0x4057B6: run_builtin (git.c:282) ==26527==by 0x405949: handle_internal_command (git.c:444) ==26527==by 0x405A63: run_argv (git.c:490) ==26527==by 0x405BF2: main (git.c:565) though I currently cannot see what's wrong, probably because I don't know the format that parse_signature_lines gives. Can you look into it? Against what version/combination of the patches are you running the test? parse_signature_lines is called parse_gpg_output in v5. Perhaps just try again with v6 of the patch, the difference between v5 and v6 is parse_gpg_output (Junio did not like the v5 variant). Thanks Sebastian -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 4/5] merge/pull Check for untrusted good GPG signatures
Sebastian Götte ja...@physik.tu-berlin.de wrote: On 03/31/2013 10:32 AM, Thomas Rast wrote: + test_must_fail git merge --ff-only --verify-signatures side-untrusted 2mergeerror + test_i18ngrep from an untrusted key mergeerror +' This test gives me the following: ==26527== Conditional jump or move depends on uninitialised value(s) ==26527==at 0x4C2D8BC: strchrnul (mc_replace_strmem.c:1084) ==26527==by 0x4989CC: parse_signature_lines (commit.c:1074) ==26527==by 0x498B33: check_commit_signature (commit.c:1100) ==26527==by 0x453719: cmd_merge (merge.c:1246) ==26527==by 0x4057B6: run_builtin (git.c:282) ==26527==by 0x405949: handle_internal_command (git.c:444) ==26527==by 0x405A63: run_argv (git.c:490) ==26527==by 0x405BF2: main (git.c:565) though I currently cannot see what's wrong, probably because I don't know the format that parse_signature_lines gives. Can you look into it? Against what version/combination of the patches are you running the test? parse_signature_lines is called parse_gpg_output in v5. Perhaps just try again with v6 of the patch, the difference between v5 and v6 is parse_gpg_output (Junio did not like the v5 variant). Oh, my bad then. I used the version in pu. I just ran all tests under valgrind and this cropped up. If you have valgrind installed locally, you can also test yourself ;-) just pass --valgrind to the test script. -- http://code.google.com/p/k9mail -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 4/5] merge/pull Check for untrusted good GPG signatures
On 03/31/2013 01:38 PM, Thomas Rast wrote: Sebastian Götte ja...@physik.tu-berlin.de wrote: On 03/31/2013 10:32 AM, Thomas Rast wrote: + test_must_fail git merge --ff-only --verify-signatures side-untrusted 2mergeerror + test_i18ngrep from an untrusted key mergeerror +' This test gives me the following: ==26527== Conditional jump or move depends on uninitialised value(s) ==26527==at 0x4C2D8BC: strchrnul (mc_replace_strmem.c:1084) ==26527==by 0x4989CC: parse_signature_lines (commit.c:1074) ==26527==by 0x498B33: check_commit_signature (commit.c:1100) ==26527==by 0x453719: cmd_merge (merge.c:1246) ==26527==by 0x4057B6: run_builtin (git.c:282) ==26527==by 0x405949: handle_internal_command (git.c:444) ==26527==by 0x405A63: run_argv (git.c:490) ==26527==by 0x405BF2: main (git.c:565) [...] If you have valgrind installed locally, you can also test yourself ;-) just pass --valgrind to the test script. Ok, I can reproduce this with v6 of the patch: expecting success: test_must_fail git merge --ff-only --verify-signatures side-untrusted 2mergeerror 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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 4/5] merge/pull Check for untrusted good GPG signatures
Sebastian Götte ja...@physik.tu-berlin.de wrote: expecting success: test_must_fail git merge --ff-only --verify-signatures side-untrusted 2mergeerror 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... -- http://code.google.com/p/k9mail -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 4/5] merge/pull Check for untrusted good GPG signatures
On 03/31/2013 02:16 PM, Thomas Rast wrote: Sebastian Götte ja...@physik.tu-berlin.de wrote: expecting success: test_must_fail git merge --ff-only --verify-signatures side-untrusted 2mergeerror 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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 4/5] merge/pull Check for untrusted good GPG signatures
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 ja...@physik.tu-berlin.de wrote: expecting success: test_must_fail git merge --ff-only --verify-signatures side-untrusted 2mergeerror 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 disc...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
