From: Fabian Stelzer Date: Mon, 06 Dec 2021 08:39:25 GMT Subject: Re: [PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals Message-ID: <20211206083925.tuy2w3wzlgpc36bj@fs> In-Reply-To: <70cee773-9547-e3cf-9327-ac0213d327e@mindrot.org> On 06.12.2021 10:06, Damien Miller wrote: >On Sat, 4 Dec 2021, Fabian Stelzer wrote: > >> > 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. > >Are you sure? I thought that we split principals in allowed_signers on >most whitespace, including \r. Follow: > >https://github.com/openssh/openssh-portable/blob/e9c7149/sshsig.c#L742 >https://github.com/openssh/openssh-portable/blob/e9c7149/misc.c#L452 >https://github.com/openssh/openssh-portable/blob/e9c7149/misc.c#L408 > Sorry, I should have mentioned that I quoted the principal. Within the quotes whitespace (and \r) works. Since find-principals then returns one principal per line the line ending issue can come up. >> 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? > >IMO sensible content for the principals section would be printable, non- >whitespace characters, excluding wildcards ('*', '?'). ssh-keygen mostly >assumes that the file is in good order, but maybe it could be stricter. > Ok, I think we can make sure of that when adding principals and use Junios suggested patch for trimming the \r at the line ending. >> 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 ;). > >Yeah. my mental model for the allowed_signers file is that it's similar >to ~/.ssh/authorized_keys in that it directly controls authn/authz >decisions, and if you put bad stuff in there then you're going to have >a bad day... > Thanks for your input.