Fabian Stelzer <fs@xxxxxxxxxxxx> writes: > 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 ;). 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 */
