[1/1] gpg-interface: limit search for primary key fingerprint
diff mbox series

Message ID 20191116180655.10988-2-hji@dyntopia.com
State New
Headers show
Series
  • Limit search for primary key fingerprint
Related show

Commit Message

Hans Jerry Illikainen Nov. 16, 2019, 6:06 p.m. UTC
The VALIDSIG status line from GnuPG with --status-fd has a field that
specifies the fingerprint of the primary key that made the signature.
However, that field is only available for OpenPGP signatures; not for
CMS/X.509.

An unbounded search for a non-existent primary key fingerprint for X509
signatures results in the following status line being interpreted as the
fingerprint.

Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com>
---
 gpg-interface.c | 20 +++++++++++++++-----
 t/t4202-log.sh  |  6 ++++++
 2 files changed, 21 insertions(+), 5 deletions(-)

Comments

Junio C Hamano Nov. 18, 2019, 5:40 a.m. UTC | #1
Hans Jerry Illikainen <hji@dyntopia.com> writes:

> The VALIDSIG status line from GnuPG with --status-fd has a field that
> specifies the fingerprint of the primary key that made the signature.
> However, that field is only available for OpenPGP signatures; not for
> CMS/X.509.
>
> An unbounded search for a non-existent primary key fingerprint for X509
> signatures results in the following status line being interpreted as the
> fingerprint.

The above two paragraphs give us an excellent explanation of what
happens in today's code.  What needs to follow is a description of
the approach taken to solve the problem in such a way that helps
readers to agree or disagree with the patch.  It needs to convince
them of at least two things:

 - The change is necessary to avoid a wrong line from getting
   mistaken as the fingerprint with CMS/X.509 sigs, and instead we
   say "there is no fingerprint" on X.509 sig (or whatever happens
   with this change---I cannot tell what ramifications lack of the
   fingerprint has to the callers of this function offhand, or how
   the lack of fingerprint is reported back to the callers, but the
   proposed log message must talk about it).

 - The change safely keeps identifying the right fingerprint with
   OpenPGP sigs because the primary fingerprint, if shown, must be
   on the same line (or whatever reason why it makes it safe---I do
   not offhand know if this is guaranteed how and by whom [*1*], but
   you must have researched it before sending this patch, so please
   write it down to help readers).

>  				/* Do we have fingerprint? */
>  				if (sigcheck_gpg_status[i].flags & GPG_STATUS_FINGERPRINT) {
> +					const char *limit;
> +

I wonder if it is simpler to define it next to 'next'.  Yes, this
new variable is used only within this block, but it also gets used
only in conjunction with that other variable.

>  					next = strchrnul(line, ' ');
>  					free(sigc->fingerprint);
>  					sigc->fingerprint = xmemdupz(line, next - line);

So, we skipped "VALIDSIG " and grabbed the first field <fingerprint>
in sigc->fingerprint.  Then we used to unconditionally skip 9 SP
separated fields.  But there may only be 8 fields on the line, which
is why this patch is needed.

> -					/* Skip interim fields */
> +					/* Skip interim fields.  The search is
> +					 * limited to the same line since only
> +					 * OpenPGP signatures has a field with
> +					 * the primary fingerprint. */

	/*
	 * A multi-line comment of ours looks like this; the
	 * slash-asterisk that begins it, and the asterisk-slash
	 * that ends it, are on their own lines, without anything
	 * else but the indentation.
	 */

> +					limit = strchrnul(line, '\n');
>  					for (j = 9; j > 0; j--) {
> -						if (!*next)
> +						if (!*next || next >= limit)
>  							break;

This makes sure that a premature exit (i.e. "0 < j") means we ran
out of the fields.  

I'd prefer to write it "limit <= next" to help visualizing the two
values (on a single line, lower values come left, higher ones come
right), by the way.  That is a minor point.

A bigger question is, when this happens, what value do we want to
leave in sigc->primary_key_fingerprint?  As we can see from the
original code that makes sure the old value in the field will not
leak by first free()ing, it seems that it is possible in this code
that the field may not be NULL, but we just saw that on _our_
signature verification system, the primary key is not available.
Shouldn't we be nulling it out, after free()ing possibly leftover
value in the field?

>  						line = next + 1;
>  						next = strchrnul(line, ' ');
>  					}
>  
> -					next = strchrnul(line, '\n');
> -					free(sigc->primary_key_fingerprint);
> -					sigc->primary_key_fingerprint = xmemdupz(line, next - line);
> +					if (j == 0) {
> +						next = strchrnul(line, '\n');
> +						free(sigc->primary_key_fingerprint);
> +						sigc->primary_key_fingerprint =
> +							xmemdupz(line,
> +								 next - line);
> +					}

Avoid such an unnatural line-wrapping that makes the result harder
to read.  A short helper

	static void replace_cstring(const char **field,
				    const char *line, const char *next)
	{
		free(*field);
		if (line && next)
			*field = xmemdupz(line, next - line);
		else
			*field = NULL;
	}

may have quite a lot of uses in this function, not only for this
field.

This is a tangent, but I think "skip 9 fields" loop by itself
deserves to become a small static helper function.

With such a helper, it would become

		if (!j) {
			next = strchrnul(line, '\n');
			replace_cstring(&sigc->primary_key_fingerprint, line, next);
		} else {
			replace_cstring(&sigc->primary_key_fingerprint,	NULL, NULL);
		}

or if you do not like the overlong lines (I don't), perhaps

		field = &sigc->primary_key_fingerprint;
		if (!j)
			replace_cstring(field, line, strchrnul(line, '\n'));
		else
			replace_cstring(field, NULL, NULL);

Note that sigc->key, sigc->signer, sigc->fingerprint fields all
share the same pattern and will benefit from the use of such a
helper function.

Thanks.

[Reference]

*1* Perhaps this one?  https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=doc/DETAILS;h=dea9d426351e043f872007696e841afb22fe9689;hb=591523ec94b6279b8b39a01501d78cf980de8722#l480
Hans Jerry Illikainen Nov. 21, 2019, 11:19 p.m. UTC | #2
On Mon, Nov 18 2019, Junio C Hamano wrote:
> I wonder if it is simpler to define it next to 'next'.  Yes, this
> new variable is used only within this block, but it also gets used
> only in conjunction with that other variable.

Done in v3 (sorry for the messed up versioning going from v0 to v2!).

> A bigger question is, when this happens, what value do we want to
> leave in sigc->primary_key_fingerprint?  As we can see from the
> original code that makes sure the old value in the field will not
> leak by first free()ing, it seems that it is possible in this code
> that the field may not be NULL, but we just saw that on _our_
> signature verification system, the primary key is not available.
> Shouldn't we be nulling it out, after free()ing possibly leftover
> value in the field?

I investigated the code paths to `primary_key_fingerprint` and deduced
that it's only ever touched when GPG_STATUS_FINGERPRINT is encountered
and a primary fingerprint is extracted.  However, v3 will NULL it even
when no primary fingerprint is found.

>> +							xmemdupz(line,
>> +								 next - line);
>> +					}
>
> Avoid such an unnatural line-wrapping that makes the result harder
> to read.

Sorry about that!  I figured that some projects prefer to always trust
in the code formatter; so I just left it be.  Now I know that human
decisions are allowed :)

> A short helper
>
> 	static void replace_cstring(const char **field,
> 				    const char *line, const char *next)
> 	{
> 		free(*field);
> 		if (line && next)
> 			*field = xmemdupz(line, next - line);
> 		else
> 			*field = NULL;
> 	}
>
> may have quite a lot of uses in this function, not only for this
> field.

Implemented.  I wasn't sure whether to do it in a separate commit or
not, but #git-devel suggested that I do; so that's what I did.
Junio C Hamano Nov. 22, 2019, 2:39 a.m. UTC | #3
Hans Jerry Illikainen <hji@dyntopia.com> writes:

> On Mon, Nov 18 2019, Junio C Hamano wrote:
> ...
>> A short helper
>>
>> 	static void replace_cstring(const char **field,
>> 				    const char *line, const char *next)
>> 	{
>> 		free(*field);
>> 		if (line && next)
>> 			*field = xmemdupz(line, next - line);
>> 		else
>> 			*field = NULL;
>> 	}
>>
>> may have quite a lot of uses in this function, not only for this
>> field.
>
> Implemented.  I wasn't sure whether to do it in a separate commit or
> not, but #git-devel suggested that I do; so that's what I did.

If such a refactoring helped the readability of _existing_ code that
can also use this helper, then I agree it is the right approach to
make that into a separate prelimimary commit.

Thanks for working on this.
Junio C Hamano Nov. 22, 2019, 3:44 a.m. UTC | #4
Junio C Hamano <gitster@pobox.com> writes:

> Hans Jerry Illikainen <hji@dyntopia.com> writes:
>
>> On Mon, Nov 18 2019, Junio C Hamano wrote:
>> ...
>>> A short helper
>>>
>>> 	static void replace_cstring(const char **field,
>>> 				    const char *line, const char *next)
>>> 	{
>>> 		free(*field);
>>> 		if (line && next)
>>> 			*field = xmemdupz(line, next - line);
>>> 		else
>>> 			*field = NULL;
>>> 	}
>>>
>>> may have quite a lot of uses in this function, not only for this
>>> field.
>>
>> Implemented.  I wasn't sure whether to do it in a separate commit or
>> not, but #git-devel suggested that I do; so that's what I did.
>
> If such a refactoring helped the readability of _existing_ code that
> can also use this helper, then I agree it is the right approach to
> make that into a separate prelimimary commit.

I did not expect the "how about doing it along this line...?"
suggestion written in my MUA without even compilation testing would
work well, and acually I do not think the above would work without
further tweaks on the types.  Wouldn't some of the fields this
helper works on be of type "char *"?

Perhaps something like this squashed in...

 gpg-interface.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index 4269937b83..b481b0811b 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -105,7 +105,7 @@ static struct {
 	{ 0, "VALIDSIG ", GPG_STATUS_FINGERPRINT },
 };
 
-static void replace_cstring(const char **field, const char *line,
+static void replace_cstring(char **field, const char *line,
 			    const char *next)
 {
 	free(*field);
@@ -120,7 +120,6 @@ static void parse_gpg_output(struct signature_check *sigc)
 {
 	const char *buf = sigc->gpg_status;
 	const char *line, *next, *limit;
-	const char **field;
 	int i, j;
 	int seen_exclusive_status = 0;
 
@@ -158,6 +157,8 @@ static void parse_gpg_output(struct signature_check *sigc)
 				}
 				/* Do we have fingerprint? */
 				if (sigcheck_gpg_status[i].flags & GPG_STATUS_FINGERPRINT) {
+					char **field;
+
 					next = strchrnul(line, ' ');
 					replace_cstring(&sigc->fingerprint, line, next);
Hans Jerry Illikainen Nov. 22, 2019, 8:23 p.m. UTC | #5
On Fri, Nov 22 2019, Junio C Hamano wrote:
> Wouldn't some of the fields this helper works on be of type "char *"?

Wow, that's embarrassing.  I completely messed that one up after a
looong day.  Gah!  Fixed and re-built with DEVELOPER=1 and re-ran the
test suite for both commits in an attempt to avoid further fuckups.

I also fixed the criticism on 2/2 (even though you mentioned that
there's no need for that) and sent it as v4 because I'm not sure what
the right approach is for changing only 1/2.

For future reference; how does the project prefer fixups for a single
commit on a multi-patch submission?

--
hji
Junio C Hamano Nov. 23, 2019, 12:18 a.m. UTC | #6
Hans Jerry Illikainen <hji@dyntopia.com> writes:

> On Fri, Nov 22 2019, Junio C Hamano wrote:
>> Wouldn't some of the fields this helper works on be of type "char *"?
>
> Wow, that's embarrassing.  I completely messed that one up after a
> looong day.  Gah!  Fixed and re-built with DEVELOPER=1 and re-ran the
> test suite for both commits in an attempt to avoid further fuckups.

The embarrassment is mine ;-)

> I also fixed the criticism on 2/2 (even though you mentioned that
> there's no need for that) and sent it as v4 because I'm not sure what
> the right approach is for changing only 1/2.
>
> For future reference; how does the project prefer fixups for a single
> commit on a multi-patch submission?

Unless the series is insanely loooooooooooooooooooooooooooooooong,
resending the whole thing, optionally with summary of the changes
since the previous iteration for each step after the three-dash
lines (i.e. this allows readers to notice "unchanged since v3"
and skip individual ones marked as such while reviewing v4), would
be a good way to help both reviewers who saw the previous round and
those who have skipped the previous round.

Thanks.

Patch
diff mbox series

diff --git a/gpg-interface.c b/gpg-interface.c
index d60115ca40..01c7ef42d4 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -148,21 +148,31 @@  static void parse_gpg_output(struct signature_check *sigc)
 				}
 				/* Do we have fingerprint? */
 				if (sigcheck_gpg_status[i].flags & GPG_STATUS_FINGERPRINT) {
+					const char *limit;
+
 					next = strchrnul(line, ' ');
 					free(sigc->fingerprint);
 					sigc->fingerprint = xmemdupz(line, next - line);
 
-					/* Skip interim fields */
+					/* Skip interim fields.  The search is
+					 * limited to the same line since only
+					 * OpenPGP signatures has a field with
+					 * the primary fingerprint. */
+					limit = strchrnul(line, '\n');
 					for (j = 9; j > 0; j--) {
-						if (!*next)
+						if (!*next || next >= limit)
 							break;
 						line = next + 1;
 						next = strchrnul(line, ' ');
 					}
 
-					next = strchrnul(line, '\n');
-					free(sigc->primary_key_fingerprint);
-					sigc->primary_key_fingerprint = xmemdupz(line, next - line);
+					if (j == 0) {
+						next = strchrnul(line, '\n');
+						free(sigc->primary_key_fingerprint);
+						sigc->primary_key_fingerprint =
+							xmemdupz(line,
+								 next - line);
+					}
 				}
 
 				break;
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index e803ba402e..5d893b3137 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -1580,6 +1580,12 @@  test_expect_success GPGSM 'setup signed branch x509' '
 	git commit -S -m signed_commit
 '
 
+test_expect_success GPGSM 'log x509 fingerprint' '
+	echo "F8BF62E0693D0694816377099909C779FA23FD65 | " >expect &&
+	git log -n1 --format="%GF | %GP" signed-x509 >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success GPG 'log --graph --show-signature' '
 	git log --graph --show-signature -n1 signed >actual &&
 	grep "^| gpg: Signature made" actual &&