From: Fabian Stelzer <fs@gigacodes.de>
Date: Sat, 04 Dec 2021 13:11:49 GMT
Subject: Re: [PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals
Message-ID: <20211204131149.cvyu7dvf6p66dotq@fs>
In-Reply-To: <Yao+l0ckDWZNf4AE@coredump.intra.peff.net>

On 03.12.2021 10:58, Jeff King wrote:
>On Fri, Dec 03, 2021 at 01:31:16PM +0000, Johannes Schindelin via GitGitGadget wrote:
>
>> 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 ;).

Thanks