| Message ID | pull.1090.git.1638538276608.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | gpg-interface: trim CR from ssh-keygen -Y find-principals | expand |
On 03.12.2021 13:31, Johannes Schindelin via GitGitGadget wrote: >From: pedro martelletto <pedro@yubico.com> > >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. This fix is obviously fine. But I'm a unsure if this is the only place where we would need to account for windows line endings. There are at least two similar uses in gpg-interface.c. parse_ssh_output() might include a trailing \r in the fingerprint. So I think we should do the same thing here: diff --git a/gpg-interface.c b/gpg-interface.c index 330cfc5845..92cd0f0ebd 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -383,7 +383,7 @@ static void parse_ssh_output(struct signature_check *sigc) sigc->result = 'B'; sigc->trust_level = TRUST_NEVER; - line = to_free = xmemdupz(sigc->output, strcspn(sigc->output, "\n")); + line = to_free = xmemdupz(sigc->output, strcspn(sigc->output, "\r\n")); if (skip_prefix(line, "Good \"git\" signature for ", &line)) { /* Search for the last "with" to get the full principal */ get_default_ssh_signing_key() also splits the defaultKeyCommand output by \n but only puts the result into a file for ssh to use which should be able to deal with it. However the whole parse_gpg_output() also assumes "\n" everywhere. So either GPG behaves differently under windows than ssh or a similar bug could be in there (and if, then probably is for a long time). I'm not familiar with the windows details (like what MSYS2 is / whats different here) and don't really have the means to test it. > >Signed-off-by: pedro martelletto <pedro@yubico.com> >Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> >--- > Allow for CR in the output of ssh-keygen > > This came in via https://github.com/git-for-windows/git/pull/3561. It > affects current Windows versions of OpenSSH (but apparently not the > MSYS2 version included in Git for Windows). > >Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1090%2Fdscho%2Fallow-cr-from-ssh-keygen-v1 >Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1090/dscho/allow-cr-from-ssh-keygen-v1 >Pull-Request: https://github.com/gitgitgadget/git/pull/1090 > > gpg-interface.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > >diff --git a/gpg-interface.c b/gpg-interface.c >index 3e7255a2a91..85e26882782 100644 >--- a/gpg-interface.c >+++ b/gpg-interface.c >@@ -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); > > child_process_init(&ssh_keygen); > >base-commit: abe6bb3905392d5eb6b01fa6e54d7e784e0522aa >-- >gitgitgadget
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. -Peff
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
Fabian Stelzer <fs@gigacodes.de> 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 */
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 > 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. > 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... -d
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.
On 06.12.2021 10:06, Pedro Martelletto wrote: >On Sun, Dec 5, 2021 at 6:50 AM Junio C Hamano <gitster@pobox.com> wrote: > >> 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 */ >> >> >> >I agree that's a more consistent fix. A minor nit: if the intention is to >only trim CR as part of a CRLF sequence, we need to ensure a LF is found: > This shouldn't be necessary as we split/loop by LF just above. for (line = ssh_principals_out.buf; *line; line = strchrnul(line + 1, '\n')) { while (*line == '\n') line++; if (!*line) break; trust_size = strcspn(line, "\n"); principal = xmemdupz(line, trust_size); - Fabian >trust_size = strcspn(line, "\n"); /* truncate at LF */ >if (trust_size && trust_size != strlen(line) && line[trust_size - 1] == >'\r') > trust_size--; /* the LF was part of CRLF at the end */ > >-p.
On 09.12.2021 17:58, Pedro Martelletto wrote: >On Thu, Dec 9, 2021 at 5:33 PM Fabian Stelzer <fs@gigacodes.de> wrote: > >> On 06.12.2021 10:06, Pedro Martelletto wrote: >> >On Sun, Dec 5, 2021 at 6:50 AM Junio C Hamano <gitster@pobox.com> wrote: >> > >> >> 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 */ >> >> >> >> >> >> >> >I agree that's a more consistent fix. A minor nit: if the intention is to >> >only trim CR as part of a CRLF sequence, we need to ensure a LF is found: >> > >> >> This shouldn't be necessary as we split/loop by LF just above. >> >> for (line = ssh_principals_out.buf; *line; >> line = strchrnul(line + 1, '\n')) { >> while (*line == '\n') >> line++; >> if (!*line) >> break; >> >> trust_size = strcspn(line, "\n"); >> principal = xmemdupz(line, trust_size); >> > >The loop ensures that 'line' points to the first character of >ssh_principals_out.buf or to a non-NUL character after a '\n'. It does not >ensure that that 'line' contains a '\n', e.g: >"principalA\nprincipalB\nprincipalC\r" or just "principalA\r". > Oops, yep. You are of course right. I still dislike how we have to consider this in various places and i guess there might be more bugs hidden on the windows platform with things like this. I kinda wish we could strip \r\n -> \n within pipe_command and then have the rest of the code not have to deal with it :/ Especially since writing a test for this would involve mirroring at leas parts oft the ssh-keygen api. But that would be a much bigger/riskier change so for this i think your latest version is fine. - Fabian
On 09.12.2021 17:58, Pedro Martelletto wrote: >On Thu, Dec 9, 2021 at 5:33 PM Fabian Stelzer <fs@gigacodes.de> wrote: > >> On 06.12.2021 10:06, Pedro Martelletto wrote: >> >On Sun, Dec 5, 2021 at 6:50 AM Junio C Hamano <gitster@pobox.com> wrote: >> > >> >> 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 */ >> >> >> >> >> >> >> >I agree that's a more consistent fix. A minor nit: if the intention is to >> >only trim CR as part of a CRLF sequence, we need to ensure a LF is found: >> > >> >> This shouldn't be necessary as we split/loop by LF just above. >> >> for (line = ssh_principals_out.buf; *line; >> line = strchrnul(line + 1, '\n')) { >> while (*line == '\n') >> line++; >> if (!*line) >> break; >> >> trust_size = strcspn(line, "\n"); >> principal = xmemdupz(line, trust_size); >> > >The loop ensures that 'line' points to the first character of >ssh_principals_out.buf or to a non-NUL character after a '\n'. It does not >ensure that that 'line' contains a '\n', e.g: >"principalA\nprincipalB\nprincipalC\r" or just "principalA\r". > Just saw that this is still open. @pedro: do you want to send an updated version of your patch or would you like me to pick this up and send one?
diff --git a/gpg-interface.c b/gpg-interface.c index 3e7255a2a91..85e26882782 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -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); child_process_init(&ssh_keygen);