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

Fabian Stelzer [off-list ref] writes:

On 03.12.2021 10:58, Jeff King wrote:

quoted

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

quoted

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.

A good point. I just tested this and CR is a valid character to use in
a principal name in the allowed signers file and as of now the
principal will be passed to the verify call `as is` and everything
works just fine. When we introduce the patch above a principal with a
CR in it will fail to verify.

I've added Damien Miller to this thread. He knows more about what the
expected behaviour for the principal would/should be. I think at the
moment almost everything except \n or \0 goes. Maybe restricting \r as
well would make life easier for other uses too?

From a security perspective I don't think this is problem. The
principal does not come from any user input but is actually looked up
in the allowed signers file using the signatures public key (with
ssh-keygen -Y find-principals).  If I could manipulate this file I
could change the key as well.

If we add `trust on first use` in a future series I would assume we
use the email address from the commit/tag author ident when adding a
new principal to the file. Can the ident contain a CR?
Even if it did, I would only allow a list of allowed alphanumeric
chars to be added anyway since a principal can contain wildcards which
we obviously don't want to trust on first use ;).

So instead of the posted patch, we should do something along this
line instead?

	trust_size = strcspn(line, "\n"); /* truncate at LF */
	if (trust_size && line[trust_size - 1] == '\r')
		trust_size--; /* the LF was part of CRLF at the end */