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
