Re: [PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals

From: Jeff King <peff@peff.net>
Date: Dec 3, 2021, 15:58 UTC
Message-ID: <Yao+l0ckDWZNf4AE@coredump.intra.peff.net>
In-Reply-To: <pull.1090.git.1638538276608.gitgitgadget@gmail.com>

On Fri, Dec 03, 2021 at 01:31:16PM +0000, Johannes Schindelin via GitGitGadget wrote:

Show 12 quoted lines

> We need to trim \r from the output of 'ssh-keygen -Y find-principals' on
> Windows, or we end up calling 'ssh-keygen -Y verify' with a bogus signer
> identity. ssh-keygen.c:2841 contains a call to puts(3), which confirms this
> hypothesis. Signature verification passes with the fix.
> [...]
> @@ -497,7 +497,7 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,
>  			if (!*line)
>  				break;
>  
> -			trust_size = strcspn(line, "\n");
> +			trust_size = strcspn(line, "\r\n");
>  			principal = xmemdupz(line, trust_size);

Just playing devil's advocate for a moment: this parsing is kind of loose. Is there any chance that I could smuggle a CR into my principal name, and make "a principal\rthat is fake" now get parsed as "a principal"? Our strcspn() here would cut off at the first CR.

I'm guessing probably not, but when it comes to something with security implications like this, it pays to be extra careful. I'm hoping somebody familiar with the ssh-keygen side and how the rest of the parsing works (like Fabian) can verify that this is OK.

-Peff