| Message ID | f6fca7c0-079c-4337-23d9-cd970c79b8ad@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] gpg-interface: handle missing " with " gracefully in parse_ssh_output() | expand |
René Scharfe <l.s.r@web.de> writes: > If the output of ssh-keygen starts with "Good \"git\" signature for ", > but is not followed by " with " for some reason, then parse_ssh_output() > uses -1 as the len parameter of xmemdupz(), which in turn will end the > program. Reject the signature and carry on instead in that case. > > Signed-off-by: René Scharfe <l.s.r@web.de> > --- > This code was added after v2.33.0. > Patch formatted with --inter-hunk-context=2 for easier review. Nice spotting. > Silly bonus question: What's the purpose of the "+ 1" and "- 1", which > seem to cancel each other out? Fabian, we are in -rc phase where we concentrate on fixing bugs in the new code in 'master'. A quick ack, "here is a better way to fix it", or "no, that won't be needed because..." is very much appreciated. Thanks. > > gpg-interface.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/gpg-interface.c b/gpg-interface.c > index 800d8caa67..62d340e78a 100644 > --- a/gpg-interface.c > +++ b/gpg-interface.c > @@ -387,17 +387,19 @@ static void parse_ssh_output(struct signature_check *sigc) > line = to_free = xmemdupz(sigc->output, strcspn(sigc->output, "\n")); > > if (skip_prefix(line, "Good \"git\" signature for ", &line)) { > - /* Valid signature and known principal */ > - sigc->result = 'G'; > - sigc->trust_level = TRUST_FULLY; > - > /* Search for the last "with" to get the full principal */ > principal = line; > do { > search = strstr(line, " with "); > if (search) > line = search + 1; > } while (search != NULL); > + if (line == principal) > + goto cleanup; > + > + /* Valid signature and known principal */ > + sigc->result = 'G'; > + sigc->trust_level = TRUST_FULLY; > sigc->signer = xmemdupz(principal, line - principal - 1); > } else if (skip_prefix(line, "Good \"git\" signature with ", &line)) { > /* Valid signature, but key unknown */ > -- > 2.33.1
On 30.10.21 19:04, René Scharfe wrote: > If the output of ssh-keygen starts with "Good \"git\" signature for ", > but is not followed by " with " for some reason, then parse_ssh_output() > uses -1 as the len parameter of xmemdupz(), which in turn will end the > program. Reject the signature and carry on instead in that case. > > Signed-off-by: René Scharfe <l.s.r@web.de> > --- > This code was added after v2.33.0. > Patch formatted with --inter-hunk-context=2 for easier review. > > Silly bonus question: What's the purpose of the "+ 1" and "- 1", which > seem to cancel each other out? They do. But only for the xmemdupz. It is important for the strstr() to skip over already found strings (" with " could be in the principal name as well - multiple times even). And doing strstr(line +1, ...) can be problematic for the first iteration. > > gpg-interface.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/gpg-interface.c b/gpg-interface.c > index 800d8caa67..62d340e78a 100644 > --- a/gpg-interface.c > +++ b/gpg-interface.c > @@ -387,17 +387,19 @@ static void parse_ssh_output(struct signature_check *sigc) > line = to_free = xmemdupz(sigc->output, strcspn(sigc->output, "\n")); > > if (skip_prefix(line, "Good \"git\" signature for ", &line)) { > - /* Valid signature and known principal */ > - sigc->result = 'G'; > - sigc->trust_level = TRUST_FULLY; > - > /* Search for the last "with" to get the full principal */ > principal = line; > do { > search = strstr(line, " with "); > if (search) > line = search + 1; > } while (search != NULL); > + if (line == principal) > + goto cleanup; > + > + /* Valid signature and known principal */ > + sigc->result = 'G'; > + sigc->trust_level = TRUST_FULLY; > sigc->signer = xmemdupz(principal, line - principal - 1); > } else if (skip_prefix(line, "Good \"git\" signature with ", &line)) { > /* Valid signature, but key unknown */ > -- > 2.33.1 > The fix looks good otherwise. Thanks!
Am 01.11.21 um 09:40 schrieb Fabian Stelzer: > On 30.10.21 19:04, René Scharfe wrote: >> Silly bonus question: What's the purpose of the "+ 1" and "- 1", which >> seem to cancel each other out? > > They do. But only for the xmemdupz. It is important for the strstr() to > skip over already found strings (" with " could be in the principal name > as well - multiple times even). And doing strstr(line +1, ...) can be > problematic for the first iteration. Ah, right. >> do { >> search = strstr(line, " with "); >> if (search) >> line = search + 1; >> } while (search != NULL); This can be shortened to: while ((search = strstr(line, " with "))) line = search + 1; I still would have tripped over the +1 / -1, though. Calling an rfind() or strrstr() or find_last() function that deals with it internally would have helped me avoid that, I think, but we don't have use for such a thing anywhere else. René
diff --git a/gpg-interface.c b/gpg-interface.c index 800d8caa67..62d340e78a 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -387,17 +387,19 @@ static void parse_ssh_output(struct signature_check *sigc) line = to_free = xmemdupz(sigc->output, strcspn(sigc->output, "\n")); if (skip_prefix(line, "Good \"git\" signature for ", &line)) { - /* Valid signature and known principal */ - sigc->result = 'G'; - sigc->trust_level = TRUST_FULLY; - /* Search for the last "with" to get the full principal */ principal = line; do { search = strstr(line, " with "); if (search) line = search + 1; } while (search != NULL); + if (line == principal) + goto cleanup; + + /* Valid signature and known principal */ + sigc->result = 'G'; + sigc->trust_level = TRUST_FULLY; sigc->signer = xmemdupz(principal, line - principal - 1); } else if (skip_prefix(line, "Good \"git\" signature with ", &line)) { /* Valid signature, but key unknown */
If the output of ssh-keygen starts with "Good \"git\" signature for ", but is not followed by " with " for some reason, then parse_ssh_output() uses -1 as the len parameter of xmemdupz(), which in turn will end the program. Reject the signature and carry on instead in that case. Signed-off-by: René Scharfe <l.s.r@web.de> --- This code was added after v2.33.0. Patch formatted with --inter-hunk-context=2 for easier review. Silly bonus question: What's the purpose of the "+ 1" and "- 1", which seem to cancel each other out? gpg-interface.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) -- 2.33.1