| Message ID | 20191116180655.10988-2-hji@dyntopia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Limit search for primary key fingerprint | expand |
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
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.
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 <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);
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
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.
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 &&
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(-)