Re: [PATCH] gpg-interface: check gpg signature for correct header
Michael J Gruber venit, vidit, dixit 14.06.2016 13:34: > Jeff King venit, vidit, dixit 14.06.2016 13:20: >> On Tue, Jun 14, 2016 at 01:11:19PM +0200, Michael J Gruber wrote: >> >>> When we create a signature, it may happen that gpg returns with >>> "success" but not with an actual detached signature on stdout. >>> >>> Check for the correct header to catch these cases better. >> >> Seems like a reasonable idea. >> >> I do worry that checking for PGP_SIGNATURE is a little fragile, though. >> We currently let you sign with gpgsm, for example, and I think this >> would break it (the verification side is not great because we don't >> recognize gpgsm headers, but this feels like a step backwards). >> >> That wouldn't be too hard to work around with a "is this a signature" >> function that checks both types. >> >>> diff --git a/gpg-interface.c b/gpg-interface.c >>> index c4b1e8c..664796f 100644 >>> --- a/gpg-interface.c >>> +++ b/gpg-interface.c >>> @@ -185,7 +185,7 @@ int sign_buffer(struct strbuf *buffer, struct strbuf >>> *signature, const char *sig >>> >>> sigchain_pop(SIGPIPE); >>> >>> - if (finish_command() || !len || len < 0) >>> + if (finish_command() || !len || len < 0 || strncmp(signature->buf, >>> PGP_SIGNATURE, strlen(PGP_SIGNATURE))) >>> return error(_("gpg failed to sign the data")); >> >> I think your strncmp is better spelled: >> >> starts_with(signature->buf, PGP_SIGNATURE); >> >> The check for "!len" is redundant now. I think you could drop "len < 0" >> as well (and in fact, drop the "len" variable entirely), as in the error >> case we'd simply have an empty signature->len. >> >> Your patch effectively swaps out "did we get any data" for "did we get >> the data we expect", which is what those "len" checks were doing. >> >> -Peff >> > > My patch actually makes several tests fail, sorry. (I did check before > that I can still create signatures...) Maybe my offset in buf is wrong. > > starts_with, yes. > > Can't check any further now, sorry. But we do check for the > PGP_SIGNATURE in our signed objects anyways. So I feel that we can either > > - tighthen the check for valid gpg signatures > > or > > - make our signature interface completely pluggable. > > We can't have it both ways, but at least things are localised in > gpg-interface.c now. > > The proposed patch is just some consistency check that does not rely on > gpg.program itself(!), or else we could simply call verify. > > Michael So, with !starts_with(signature->buf+bottom, PGP_SIGNATURE) everything is fine except our tests for RFC1991 signatures. Sigh... I'll resend a patch that uses parse_signature so that all gpg specifics are localised there. Michael -- 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] gpg-interface: check gpg signature for correct header
Jeff King venit, vidit, dixit 14.06.2016 13:20: > On Tue, Jun 14, 2016 at 01:11:19PM +0200, Michael J Gruber wrote: > >> When we create a signature, it may happen that gpg returns with >> "success" but not with an actual detached signature on stdout. >> >> Check for the correct header to catch these cases better. > > Seems like a reasonable idea. > > I do worry that checking for PGP_SIGNATURE is a little fragile, though. > We currently let you sign with gpgsm, for example, and I think this > would break it (the verification side is not great because we don't > recognize gpgsm headers, but this feels like a step backwards). > > That wouldn't be too hard to work around with a "is this a signature" > function that checks both types. > >> diff --git a/gpg-interface.c b/gpg-interface.c >> index c4b1e8c..664796f 100644 >> --- a/gpg-interface.c >> +++ b/gpg-interface.c >> @@ -185,7 +185,7 @@ int sign_buffer(struct strbuf *buffer, struct strbuf >> *signature, const char *sig >> >> sigchain_pop(SIGPIPE); >> >> -if (finish_command() || !len || len < 0) >> +if (finish_command() || !len || len < 0 || strncmp(signature->buf, >> PGP_SIGNATURE, strlen(PGP_SIGNATURE))) >> return error(_("gpg failed to sign the data")); > > I think your strncmp is better spelled: > > starts_with(signature->buf, PGP_SIGNATURE); > > The check for "!len" is redundant now. I think you could drop "len < 0" > as well (and in fact, drop the "len" variable entirely), as in the error > case we'd simply have an empty signature->len. > > Your patch effectively swaps out "did we get any data" for "did we get > the data we expect", which is what those "len" checks were doing. > > -Peff > My patch actually makes several tests fail, sorry. (I did check before that I can still create signatures...) Maybe my offset in buf is wrong. starts_with, yes. Can't check any further now, sorry. But we do check for the PGP_SIGNATURE in our signed objects anyways. So I feel that we can either - tighthen the check for valid gpg signatures or - make our signature interface completely pluggable. We can't have it both ways, but at least things are localised in gpg-interface.c now. The proposed patch is just some consistency check that does not rely on gpg.program itself(!), or else we could simply call verify. Michael -- 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] gpg-interface: check gpg signature for correct header
On Tue, Jun 14, 2016 at 01:11:19PM +0200, Michael J Gruber wrote: > When we create a signature, it may happen that gpg returns with > "success" but not with an actual detached signature on stdout. > > Check for the correct header to catch these cases better. Seems like a reasonable idea. I do worry that checking for PGP_SIGNATURE is a little fragile, though. We currently let you sign with gpgsm, for example, and I think this would break it (the verification side is not great because we don't recognize gpgsm headers, but this feels like a step backwards). That wouldn't be too hard to work around with a "is this a signature" function that checks both types. > diff --git a/gpg-interface.c b/gpg-interface.c > index c4b1e8c..664796f 100644 > --- a/gpg-interface.c > +++ b/gpg-interface.c > @@ -185,7 +185,7 @@ int sign_buffer(struct strbuf *buffer, struct strbuf > *signature, const char *sig > > sigchain_pop(SIGPIPE); > > - if (finish_command() || !len || len < 0) > + if (finish_command() || !len || len < 0 || strncmp(signature->buf, > PGP_SIGNATURE, strlen(PGP_SIGNATURE))) > return error(_("gpg failed to sign the data")); I think your strncmp is better spelled: starts_with(signature->buf, PGP_SIGNATURE); The check for "!len" is redundant now. I think you could drop "len < 0" as well (and in fact, drop the "len" variable entirely), as in the error case we'd simply have an empty signature->len. Your patch effectively swaps out "did we get any data" for "did we get the data we expect", which is what those "len" checks were doing. -Peff -- 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