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.
