Re: [PATCH 1/2] gpg-interface: handle missing " with " gracefully in parse_ssh_output()
- From
Fabian Stelzer <fs@gigacodes.de>
- Date
- Nov 1, 2021, 08:40 UTC
- Message-ID
- <dedf3971-adb1-c355-0b05-4e94ad92e1f7@gigacodes.de>
- In-Reply-To
- <f6fca7c0-079c-4337-23d9-cd970c79b8ad@web.de>
On 30.10.21 19:04, René Scharfe wrote:
Show 12 quoted lines
> 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.
Show 35 quoted lines
>
> 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!