[PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals

[PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals

[Johannes Schindelin via GitGitGadget]

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.

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

Re: [PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals

[Fabian Stelzer]

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

Re: [PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals

[Jeff King]

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

Re: [PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals

[Fabian Stelzer]

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

Re: [PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals

[Junio C Hamano]

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 */

Re: [PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals

[Fabian Stelzer]

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.

Re: [PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals

[Fabian Stelzer]

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

Re: [PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals

[Fabian Stelzer]

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?

Re: [PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals

[Damien Miller]

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

Re: [PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals

[Fabian Stelzer]

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.

[PATCH v2] gpg-interface: trim CR from ssh-keygen

[Fabian Stelzer]

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.

Helped-by: Pedro Martelletto <pedro@yubico.com>
Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
---
 gpg-interface.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index b52eb0e2e0..d5eca417e8 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -509,7 +509,10 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,
 			if (!*line)
 				break;
 
-			trust_size = strcspn(line, "\n");
+			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 */
 			principal = xmemdupz(line, trust_size);
 
 			child_process_init(&ssh_keygen);
-- 
2.33.1

Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen

[Eric Sunshine]

On Mon, Jan 3, 2022 at 9:24 AM Fabian Stelzer <fs@gigacodes.de> 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.
>
> Helped-by: Pedro Martelletto <pedro@yubico.com>
> Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
> ---
> diff --git a/gpg-interface.c b/gpg-interface.c
> @@ -509,7 +509,10 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,
> -                       trust_size = strcspn(line, "\n");
> +                       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 */

I may be misunderstanding, but isn't the strlen() unnecessary?

    if (trust_size && line[trust_size] &&
        line[trust_size - 1] == '\r')
            trust_size--;

Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Mon, Jan 3, 2022 at 9:24 AM Fabian Stelzer <fs@gigacodes.de> 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.
>>
>> Helped-by: Pedro Martelletto <pedro@yubico.com>
>> Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
>> ---
>> diff --git a/gpg-interface.c b/gpg-interface.c
>> @@ -509,7 +509,10 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,
>> -                       trust_size = strcspn(line, "\n");
>> +                       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 */
>
> I may be misunderstanding, but isn't the strlen() unnecessary?
>
>     if (trust_size && line[trust_size] &&
>         line[trust_size - 1] == '\r')
>             trust_size--;

That changes behaviour when "line" has more than one lines in it.
strcspn() finds the first LF, and the posted patch ignores CRLF not
at the end of line[].  Your variant feels more correct if the
objective is to find the end of the first line (regardless of the
choice of the end-of-line convention, either LF or CRLF) and omit
the line terminator.

Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen

[Eric Sunshine]

On Mon, Jan 3, 2022 at 6:34 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > On Mon, Jan 3, 2022 at 9:24 AM Fabian Stelzer <fs@gigacodes.de> 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.
> >> ---
> >> -                       trust_size = strcspn(line, "\n");
> >> +                       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 */
> >
> > I may be misunderstanding, but isn't the strlen() unnecessary?
> >
> >     if (trust_size && line[trust_size] &&
> >         line[trust_size - 1] == '\r')
> >             trust_size--;
>
> That changes behaviour when "line" has more than one lines in it.
> strcspn() finds the first LF, and the posted patch ignores CRLF not
> at the end of line[].  Your variant feels more correct if the
> objective is to find the end of the first line (regardless of the
> choice of the end-of-line convention, either LF or CRLF) and omit
> the line terminator.

Okay, that makes sense if that's the intention of the patch. Perhaps
the commit message should mention that `line` might contain multiple
lines and that it's only interested in the very last LF (unless it's
already obvious to everyone else, even though it wasn't to me). I
think it can still be done without strlen(), but it gets uglier and
less obvious[*], so strlen() is probably the way to go, and I presume
this isn't a hot path, so no big reason to avoid strlen().

[*] Like this, for instance, which is safe because there must be at
least one character after the '\n' since this is a NUL-terminated
string:

    if (trust_size && line[trust_size] == '\n'
        line[true_size + 1] == '\0' &&
        line[trust_size - 1] == '\r')

Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Mon, Jan 3, 2022 at 6:34 PM Junio C Hamano <gitster@pobox.com> wrote:
>> Eric Sunshine <sunshine@sunshineco.com> writes:
>> > On Mon, Jan 3, 2022 at 9:24 AM Fabian Stelzer <fs@gigacodes.de> 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.
>> >> ---
>> >> -                       trust_size = strcspn(line, "\n");
>> >> +                       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 */
>> >
>> > I may be misunderstanding, but isn't the strlen() unnecessary?
>> >
>> >     if (trust_size && line[trust_size] &&
>> >         line[trust_size - 1] == '\r')
>> >             trust_size--;
>>
>> That changes behaviour when "line" has more than one lines in it.
>> strcspn() finds the first LF, and the posted patch ignores CRLF not
>> at the end of line[].  Your variant feels more correct if the
>> objective is to find the end of the first line (regardless of the
>> choice of the end-of-line convention, either LF or CRLF) and omit
>> the line terminator.
>
> Okay, that makes sense if that's the intention of the patch. Perhaps
> the commit message should mention that `line` might contain multiple
> lines and that it's only interested in the very last LF (unless it's
> already obvious to everyone else, even though it wasn't to me).

I do not think that is the case.  strcspn(line, "\n") will stop at
the first one, so unless it is guaranteed that "line" has only one
line in it, the patch as posted is not correct.  Your variant
without strlen() feels more correct, as I said.

Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen

[Eric Sunshine]

On Mon, Jan 3, 2022 at 8:19 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > On Mon, Jan 3, 2022 at 6:34 PM Junio C Hamano <gitster@pobox.com> wrote:
> >> Eric Sunshine <sunshine@sunshineco.com> writes:
> >> > On Mon, Jan 3, 2022 at 9:24 AM Fabian Stelzer <fs@gigacodes.de> wrote:
> >> >> -                       trust_size = strcspn(line, "\n");
> >> >> +                       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 */
> >> >
> >> > I may be misunderstanding, but isn't the strlen() unnecessary?
> >> >
> >> >     if (trust_size && line[trust_size] &&
> >> >         line[trust_size - 1] == '\r')
> >> >             trust_size--;
> >>
> >> That changes behaviour when "line" has more than one lines in it.
> >> strcspn() finds the first LF, and the posted patch ignores CRLF not
> >> at the end of line[].  Your variant feels more correct if the
> >> objective is to find the end of the first line (regardless of the
> >> choice of the end-of-line convention, either LF or CRLF) and omit
> >> the line terminator.
> >
> > Okay, that makes sense if that's the intention of the patch. Perhaps
> > the commit message should mention that `line` might contain multiple
> > lines and that it's only interested in the very last LF (unless it's
> > already obvious to everyone else, even though it wasn't to me).
>
> I do not think that is the case.  strcspn(line, "\n") will stop at
> the first one, so unless it is guaranteed that "line" has only one
> line in it, the patch as posted is not correct.  Your variant
> without strlen() feels more correct, as I said.

Okay, sorry for my unclear thinking. The existing code (before this
patch) does indeed seem to be interested only in the first line of
`line`, in which case I agree that the patch's use of strlen() does
not appear to be correct if `line` could ever contain more than one
line.

Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen

[Fabian Stelzer]

On 03.01.2022 22:06, Eric Sunshine wrote:
>On Mon, Jan 3, 2022 at 8:19 PM Junio C Hamano <gitster@pobox.com> wrote:
>> Eric Sunshine <sunshine@sunshineco.com> writes:
>> > On Mon, Jan 3, 2022 at 6:34 PM Junio C Hamano <gitster@pobox.com> wrote:
>> >> Eric Sunshine <sunshine@sunshineco.com> writes:
>> >> > On Mon, Jan 3, 2022 at 9:24 AM Fabian Stelzer <fs@gigacodes.de> wrote:
>> >> >> -                       trust_size = strcspn(line, "\n");
>> >> >> +                       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 */
>> >> >
>> >> > I may be misunderstanding, but isn't the strlen() unnecessary?
>> >> >
>> >> >     if (trust_size && line[trust_size] &&
>> >> >         line[trust_size - 1] == '\r')
>> >> >             trust_size--;
>> >>
>> >> That changes behaviour when "line" has more than one lines in it.
>> >> strcspn() finds the first LF, and the posted patch ignores CRLF not
>> >> at the end of line[].  Your variant feels more correct if the
>> >> objective is to find the end of the first line (regardless of the
>> >> choice of the end-of-line convention, either LF or CRLF) and omit
>> >> the line terminator.
>> >
>> > Okay, that makes sense if that's the intention of the patch. Perhaps
>> > the commit message should mention that `line` might contain multiple
>> > lines and that it's only interested in the very last LF (unless it's
>> > already obvious to everyone else, even though it wasn't to me).
>>
>> I do not think that is the case.  strcspn(line, "\n") will stop at
>> the first one, so unless it is guaranteed that "line" has only one
>> line in it, the patch as posted is not correct.  Your variant
>> without strlen() feels more correct, as I said.
>
>Okay, sorry for my unclear thinking. The existing code (before this
>patch) does indeed seem to be interested only in the first line of
>`line`, in which case I agree that the patch's use of strlen() does
>not appear to be correct if `line` could ever contain more than one
>line.

I guess we need a bit more context for this patch to make sense:

for (line = ssh_principals_out.buf; *line;
      line = strchrnul(line + 1, '\n')) {
	while (*line == '\n')
		line++;
	if (!*line)
		break;

	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 */
	principal = xmemdupz(line, trust_size);

ssh_principals_out contains the result of the find-principals call which 
contains one found principal per line (normally LF, CRLF in some cygwin 
setup).

A principal can contain CR as a valid character. This is problematic if CR 
is the last char of the principal since we have no way of knowing then if we 
are in cygwin with CRLF line endings or another platform using LF and the CR 
is the last character of the principal.
Lets leave this rather weird edge case aside for now.

So what we want to do is split the buffer by line, no matter which line 
endings are used, and copy the principal without any line ending characters.

The `trust_size != strlen(line)` check was supposed to guard against `line` 
having no LF at all and ending with a CR. I think a
`line[trust_size + 1] != '\0'` would work as well.

But since this whole thing is already hard enough to follow i guess it's 
better we simply remove it instead of adding checks for the unlikely case we 
encounter a broken ssh-keygen. Especially since the effect would only be a 
failed signature validation.
We could even remove the `if (trust_size)` condition since this only happens 
when `line` begins with LF which is already skipped over a few lines before.  
But it's probably better to leave this in just in case the code changes.

Generally I think this is a common enough problem that there should be a 
function to split a strbuf by line no matter if LF or CRLF is used. Similar 
to strbuf_getline() but to read from a strbuf or maybe even handle this 
within pipe_command() when filling the strbuf. Maybe git even has better 
code to handle this but i haven't found it yet?

Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen

[Junio C Hamano]

Fabian Stelzer <fs@gigacodes.de> writes:

> I guess we need a bit more context for this patch to make sense:
>
> for (line = ssh_principals_out.buf; *line;
>      line = strchrnul(line + 1, '\n')) {
> 	while (*line == '\n')
> 		line++;
> 	if (!*line)
> 		break;
>
> 	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 */
> 	principal = xmemdupz(line, trust_size);
>
> ssh_principals_out contains the result of the find-principals call
> which contains one found principal per line (normally LF, CRLF in some
> cygwin setup).

Ahh, OK.  Sorry for being ultra lazy for not visiting the actual
source but just responding after reading only somebody else's
comments.

So, the code skips over one or more LFs (but users of platforms that
use CRLF line termination are screwed here already) to find the
beginning of a non-empty line.  Then it wants to find the end of
that non-empty line (if there is still LF there in the buffer).
Since strcspn() may not find any LF (i.e. it is an incomplete line),
strlen(line) is used to see if we found a LF or if we hit the
terminating NUL.  If the line ended with CR, we do not want to strip
it.

OK, so I was completely missing the idea.  And I agree that it may
be a good idea to check how strcspn() returned to deal with an
incomplete line, although as you hint later in the message I am
responding to, checking line[trust_size] would be a more obvious
implementation.

In any case, I think the earlier part of the loop is more confusing,
and I think fixing that would naturally fix the trust_size
computation.  For example, wouldn't this easier to grok?

	const char *next;

	for (line = ssh_principals_out.buf;
	     *line;
	     line = next) {
		const char *end_of_text;

                /* Find the terminating LF */
               	next = end_of_text = strchrnul(line, '\n');

		/* Did we find a LF, and did we have CR before it? */
		if (*end_of_text &&
                    line < end_of_text &&
		    end_of_text[-1] == '\r')
			end_of_text--;

		/* Unless we hit NUL, skip over the LF we found */
		if (*next)
			next++;

		/* Not all lines are data.  Skip empty ones */
		if (line == end_of_text)
			/* 
                         * You may want to allow skipping more than just
			 * lines with 0-byte on them (e.g. comments?)
			 * depending on the format you are reading.
			 */
			continue;

		/* We now know we have an non-empty line. Process it */
		principal = xmemdupz(line, end_of_text - line);
		...
	}
		
The idea is to make sure that the place where the line ending
convention is taken care of is very isolated at the beginning of the
loop.

Hmm?

Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen

[Eric Sunshine]

On Tue, Jan 4, 2022 at 2:33 PM Junio C Hamano <gitster@pobox.com> wrote:
> Fabian Stelzer <fs@gigacodes.de> writes:
> > I guess we need a bit more context for this patch to make sense:
> >
> > for (line = ssh_principals_out.buf; *line;
> >      line = strchrnul(line + 1, '\n')) {
> >       while (*line == '\n')
> >               line++;
> >       if (!*line)
> >               break;
> >
> >       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 */
> >       principal = xmemdupz(line, trust_size);
>
> Ahh, OK.  Sorry for being ultra lazy for not visiting the actual
> source but just responding after reading only somebody else's
> comments.

I'm also guilty of being lazy and not consulting the actual source. Sorry.

Fabian, thanks for all the extra context information.

> OK, so I was completely missing the idea.  And I agree that it may
> be a good idea to check how strcspn() returned to deal with an
> incomplete line, although as you hint later in the message I am
> responding to, checking line[trust_size] would be a more obvious
> implementation.
>
> In any case, I think the earlier part of the loop is more confusing,
> and I think fixing that would naturally fix the trust_size
> computation.  For example, wouldn't this easier to grok?

Indeed, the existing code is confusing me. I've been staring at it for
several minutes and I think I'm still failing to understand the
purpose of the +1 in the strchrnul() call. Perhaps I'm missing
something obvious(?).

>         const char *next;
>
>         for (line = ssh_principals_out.buf;
>              *line;
>              line = next) {
>                 const char *end_of_text;
>
>                 /* Find the terminating LF */
>                 next = end_of_text = strchrnul(line, '\n');
>
>                 /* Did we find a LF, and did we have CR before it? */
>                 if (*end_of_text &&
>                     line < end_of_text &&
>                     end_of_text[-1] == '\r')
>                         end_of_text--;

It took several seconds for me to convince myself that the -1 array
index was safe. Had the `line < end_of_text` condition been written
`end_of_text > line`, I think it would have been immediately obvious,
but it's subjective, of course.

>                 /* Unless we hit NUL, skip over the LF we found */
>                 if (*next)
>                         next++;
>
>                 /* Not all lines are data.  Skip empty ones */
>                 if (line == end_of_text)
>                         /*
>                          * You may want to allow skipping more than just
>                          * lines with 0-byte on them (e.g. comments?)
>                          * depending on the format you are reading.
>                          */
>                         continue;
>
>                 /* We now know we have an non-empty line. Process it */
>                 principal = xmemdupz(line, end_of_text - line);
>                 ...
>         }
>
> The idea is to make sure that the place where the line ending
> convention is taken care of is very isolated at the beginning of the
> loop.

Yes, this may be an improvement, though the cognitive load is still
somewhat high. Using one of the `split` functions from strbuf.h or
string-list.h might reduce the cognitive load significantly, even if
this code still needs to handle CR removal manually since none of the
`split` functions are LF/CRLF agnostic. (Adding such a function might
be useful but could be outside the scope of this bug fix patch.)

Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen

[Fabian Stelzer]

On 05.01.2022 02:09, Eric Sunshine wrote:
>On Tue, Jan 4, 2022 at 2:33 PM Junio C Hamano <gitster@pobox.com> wrote:
>> Fabian Stelzer <fs@gigacodes.de> writes:
>> > I guess we need a bit more context for this patch to make sense:
>> >
>> > for (line = ssh_principals_out.buf; *line;
>> >      line = strchrnul(line + 1, '\n')) {
>> >       while (*line == '\n')
>> >               line++;
>> >       if (!*line)
>> >               break;
>> >
>> >       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 */
>> >       principal = xmemdupz(line, trust_size);
>>
>> Ahh, OK.  Sorry for being ultra lazy for not visiting the actual
>> source but just responding after reading only somebody else's
>> comments.
>
>I'm also guilty of being lazy and not consulting the actual source. Sorry.
>
>Fabian, thanks for all the extra context information.
>
>> OK, so I was completely missing the idea.  And I agree that it may
>> be a good idea to check how strcspn() returned to deal with an
>> incomplete line, although as you hint later in the message I am
>> responding to, checking line[trust_size] would be a more obvious
>> implementation.
>>
>> In any case, I think the earlier part of the loop is more confusing,
>> and I think fixing that would naturally fix the trust_size
>> computation.  For example, wouldn't this easier to grok?
>
>Indeed, the existing code is confusing me. I've been staring at it for
>several minutes and I think I'm still failing to understand the
>purpose of the +1 in the strchrnul() call. Perhaps I'm missing
>something obvious(?).

This whole loop was basically copied from parse_gpg_output() above. Without 
the +1 this would always find the same line in the buffer. The +1 skips over 
the previously found LF.

>
>>         const char *next;
>>
>>         for (line = ssh_principals_out.buf;
>>              *line;
>>              line = next) {
>>                 const char *end_of_text;
>>
>>                 /* Find the terminating LF */
>>                 next = end_of_text = strchrnul(line, '\n');
>>
>>                 /* Did we find a LF, and did we have CR before it? */
>>                 if (*end_of_text &&
>>                     line < end_of_text &&
>>                     end_of_text[-1] == '\r')
>>                         end_of_text--;
>
>It took several seconds for me to convince myself that the -1 array
>index was safe. Had the `line < end_of_text` condition been written
>`end_of_text > line`, I think it would have been immediately obvious,
>but it's subjective, of course.
>
>>                 /* Unless we hit NUL, skip over the LF we found */
>>                 if (*next)
>>                         next++;
>>
>>                 /* Not all lines are data.  Skip empty ones */
>>                 if (line == end_of_text)
>>                         /*
>>                          * You may want to allow skipping more than just
>>                          * lines with 0-byte on them (e.g. comments?)
>>                          * depending on the format you are reading.
>>                          */
>>                         continue;
>>
>>                 /* We now know we have an non-empty line. Process it */
>>                 principal = xmemdupz(line, end_of_text - line);
>>                 ...
>>         }
>>
>> The idea is to make sure that the place where the line ending
>> convention is taken care of is very isolated at the beginning of the
>> loop.
>
>Yes, this may be an improvement, though the cognitive load is still
>somewhat high. Using one of the `split` functions from strbuf.h or
>string-list.h might reduce the cognitive load significantly, even if
>this code still needs to handle CR removal manually since none of the
>`split` functions are LF/CRLF agnostic. (Adding such a function might
>be useful but could be outside the scope of this bug fix patch.)

How about something like this:

int string_find_line(char **line, size_t *len) {
	const char *eol = NULL;

	if (*len > 0) {
		*line = *line + *len;
		if (**line && **line == '\r')
			(*line)++;
		if (**line && **line == '\n')
			(*line)++;
	}

	if (!**line)
		return 0;

	eol = strchrnul(*line, '\n');

	/* Trim trailing CR from length */
	if (eol > *line && eol[-1] == '\r')
		eol--;

	*len = eol - *line;
	return 1;
}

Its use would then simply be:

char *line = strbuf.buf;
size_t len = 0;
while(string_find_line(&line,&len)) {
	if (!len)
		continue; /* Skip over empty lines */
	principal = xmemdupz(line, len);
}

Not sure about the name though.
Maybe string_find_line() / _iterate_line / foreach_line ?

Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen

[Junio C Hamano]

Fabian Stelzer <fs@gigacodes.de> writes:

> How about something like this:
>
> int string_find_line(char **line, size_t *len) {
> 	const char *eol = NULL;
>
> 	if (*len > 0) {
> 		*line = *line + *len;
> 		if (**line && **line == '\r')
> 			(*line)++;
> 		if (**line && **line == '\n')
> 			(*line)++;
> 	}
>
> 	if (!**line)
> 		return 0;
>
> 	eol = strchrnul(*line, '\n');
>
> 	/* Trim trailing CR from length */
> 	if (eol > *line && eol[-1] == '\r')
> 		eol--;
>
> 	*len = eol - *line;
> 	return 1;
> }

It is a confusing piece of "we handle one line at a time" helper.
It is not obvious what the loop invariants are.

It would be most natural to readers if *line points at the very
beginning of the buffer, i.e. the beginning of the first line,
and *len points at the very first character of that line, i.e. 0.

But then the first thing this function worries about is a case where
*len is not 0.  I obviously am biased, but sorry, I find what I gave
you 100 times simpler to understand.

>
> Its use would then simply be:
>
> char *line = strbuf.buf;
> size_t len = 0;
> while(string_find_line(&line,&len)) {
> 	if (!len)
> 		continue; /* Skip over empty lines */
> 	principal = xmemdupz(line, len);
> }
>
> Not sure about the name though.
> Maybe string_find_line() / _iterate_line / foreach_line ?

Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen

[Fabian Stelzer]

On 05.01.2022 12:40, Junio C Hamano wrote:
>Fabian Stelzer <fs@gigacodes.de> writes:
>
>> How about something like this:
>>
>> int string_find_line(char **line, size_t *len) {
>> 	const char *eol = NULL;
>>
>> 	if (*len > 0) {
>> 		*line = *line + *len;
>> 		if (**line && **line == '\r')
>> 			(*line)++;
>> 		if (**line && **line == '\n')
>> 			(*line)++;
>> 	}
>>
>> 	if (!**line)
>> 		return 0;
>>
>> 	eol = strchrnul(*line, '\n');
>>
>> 	/* Trim trailing CR from length */
>> 	if (eol > *line && eol[-1] == '\r')
>> 		eol--;
>>
>> 	*len = eol - *line;
>> 	return 1;
>> }
>
>It is a confusing piece of "we handle one line at a time" helper.
>It is not obvious what the loop invariants are.
>
>It would be most natural to readers if *line points at the very
>beginning of the buffer, i.e. the beginning of the first line,
>and *len points at the very first character of that line, i.e. 0.
>
>But then the first thing this function worries about is a case where
>*len is not 0.  I obviously am biased, but sorry, I find what I gave
>you 100 times simpler to understand.
>

There are a few more places where the same thing happens and text is just 
split by LF, ignoring CR. The gpg parsing where this code originated being 
the most prominent example. However those just parse some parts from the 
output and the worst that seems to happen is a trailing CR in some log 
outputs.
If we are ok with this then your version is indeed the better one. If we 
want to correct the parsing at the other sites then I think a more 
generalized function would be better. Since the gpg stuff is in place for a 
long time and no one complained we can probably leave it as is. I'll prepare 
a new patch.

Thanks

Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen

[Junio C Hamano]

Fabian Stelzer <fs@gigacodes.de> writes:

> There are a few more places where the same thing happens and text
> is just split by LF, ignoring CR. The gpg parsing where this code
> originated being the most prominent example. ...  ... Since the
> gpg stuff is in place for a long time and no one complained we can
> probably leave it as is.

Yeah, that is the conclusion I was hoping for.
Thanks.

Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen

[Eric Sunshine]

On Wed, Jan 5, 2022 at 5:36 AM Fabian Stelzer <fs@gigacodes.de> wrote:
> On 05.01.2022 02:09, Eric Sunshine wrote:
> >> >      line = strchrnul(line + 1, '\n')) {
> >> >       while (*line == '\n')
> >> >               line++;
> >> >       if (!*line)
> >> >               break;
> >
> >Indeed, the existing code is confusing me. I've been staring at it for
> >several minutes and I think I'm still failing to understand the
> >purpose of the +1 in the strchrnul() call. Perhaps I'm missing
> >something obvious(?).
>
> This whole loop was basically copied from parse_gpg_output() above. Without
> the +1 this would always find the same line in the buffer. The +1 skips over
> the previously found LF.

I still don't see the point of +1 in the strchrnul() call. After:

    line = strchrnul(line + 1, '\n'))

`line` is going to point either at '\n' or at NUL. Then:

    while (*line == '\n')
        line++;

skips over the '\n' if present. So, by the time the next loop
iteration starts, `line` will already be pointing past the '\n' we
just found, thus the +1 seems pointless (and maybe even buggy).

But perhaps I have a blind spot and am missing something obvious...

Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen

[Fabian Stelzer]

On 09.01.2022 15:49, Eric Sunshine wrote:
>On Wed, Jan 5, 2022 at 5:36 AM Fabian Stelzer <fs@gigacodes.de> wrote:
>> On 05.01.2022 02:09, Eric Sunshine wrote:
>> >> >      line = strchrnul(line + 1, '\n')) {
>> >> >       while (*line == '\n')
>> >> >               line++;
>> >> >       if (!*line)
>> >> >               break;
>> >
>> >Indeed, the existing code is confusing me. I've been staring at it for
>> >several minutes and I think I'm still failing to understand the
>> >purpose of the +1 in the strchrnul() call. Perhaps I'm missing
>> >something obvious(?).
>>
>> This whole loop was basically copied from parse_gpg_output() above. Without
>> the +1 this would always find the same line in the buffer. The +1 skips over
>> the previously found LF.
>
>I still don't see the point of +1 in the strchrnul() call. After:
>
>    line = strchrnul(line + 1, '\n'))
>
>`line` is going to point either at '\n' or at NUL. Then:
>
>    while (*line == '\n')
>        line++;
>
>skips over the '\n' if present. So, by the time the next loop
>iteration starts, `line` will already be pointing past the '\n' we
>just found, thus the +1 seems pointless (and maybe even buggy).
>
>But perhaps I have a blind spot and am missing something obvious...

Hm, yeah. I think you are correct. The while below should make the +1 
unnecessary. I think this never mattered to parse_gpg_output() since it is 
only looking for the [GNUPG:] status line which probably comes first anyway.  
If it does not then I think the loop will skip over it. Same thing with ssh 
(but we are changing this whole loop anyway)

[PATCH v3] gpg-interface: trim CR from ssh-keygen

[Fabian Stelzer]

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.

Helped-by: Pedro Martelletto <pedro@yubico.com>
Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
---
 gpg-interface.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index b52eb0e2e0..17b1e44baa 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -433,7 +433,6 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,
 	struct tempfile *buffer_file;
 	int ret = -1;
 	const char *line;
-	size_t trust_size;
 	char *principal;
 	struct strbuf ssh_principals_out = STRBUF_INIT;
 	struct strbuf ssh_principals_err = STRBUF_INIT;
@@ -502,15 +501,30 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,
 		ret = -1;
 	} else {
 		/* Check every principal we found (one per line) */
-		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);
+		const char *next;
+		for (line = ssh_principals_out.buf;
+		     *line;
+		     line = next) {
+			const char *end_of_text;
+
+			next = end_of_text = strchrnul(line, '\n');
+
+			 /* Did we find a LF, and did we have CR before it? */
+			if (*end_of_text &&
+			    line < end_of_text &&
+			    end_of_text[-1] == '\r')
+				end_of_text--;
+
+			/* Unless we hit NUL, skip over the LF we found */
+			if (*next)
+				next++;
+
+			/* Not all lines are data.  Skip empty ones */
+			if (line == end_of_text)
+				continue;
+
+			/* We now know we have an non-empty line. Process it */
+			principal = xmemdupz(line, end_of_text - line);
 
 			child_process_init(&ssh_keygen);
 			strbuf_release(&ssh_keygen_out);
-- 
2.33.1

Re: [PATCH v3] gpg-interface: trim CR from ssh-keygen

[Eric Sunshine]

On Fri, Jan 07, 2022 at 10:07:35AM +0100, Fabian Stelzer 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.
>
> Helped-by: Pedro Martelletto <pedro@yubico.com>
> Signed-off-by: Fabian Stelzer <fs@gigacodes.de>

Should this also have a "Helped-by: Junio" since this code was heavily
inspired by his suggestion[1]?

[1]: https://lore.kernel.org/git/xmqqo84rcn3j.fsf@gitster.g/

> ---
> diff --git a/gpg-interface.c b/gpg-interface.c
> @@ -502,15 +501,30 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,
> +		const char *next;
> +		for (line = ssh_principals_out.buf;
> +		     *line;
> +		     line = next) {
> +			const char *end_of_text;
> +
> +			next = end_of_text = strchrnul(line, '\n');
> +
> +			 /* Did we find a LF, and did we have CR before it? */
> +			if (*end_of_text &&
> +			    line < end_of_text &&
> +			    end_of_text[-1] == '\r')
> +				end_of_text--;
> +
> +			/* Unless we hit NUL, skip over the LF we found */
> +			if (*next)
> +				next++;
> +
> +			/* Not all lines are data.  Skip empty ones */
> +			if (line == end_of_text)
> +				continue;
> +
> +			/* We now know we have an non-empty line. Process it */
> +			principal = xmemdupz(line, end_of_text - line);

Considering that this code makes a copy of the line _anyhow_ which it
assigns to `principal`, it still seems like it would be simpler and
far easier to understand at-a-glance to instead take advantage of one
of the existing string-splitting functions. For instance, something
like this:

    struct strbuf **line, **to_free;
    line = to_free = strbuf_split(&ssh_principals_out, '\n');
    for (; *line; line++) {
        strbuf_trim_trailing_newline(*line);
        if (!(*line)->len)
            continue;
        principal = (*line)->buf;

keeping in mind that strbuf_trim_trailing_newline() takes care of
CR/LF, and with appropriate cleanup at the end of the loop:

        strbuf_list_free(to_free);

(and removal of `FREE_AND_NULL(principal)` which is no longer needed).

Something similar can be done with string_list_split(), as well.

Re: [PATCH v3] gpg-interface: trim CR from ssh-keygen

[Fabian Stelzer]

On 09.01.2022 16:37, Eric Sunshine wrote:
>On Fri, Jan 07, 2022 at 10:07:35AM +0100, Fabian Stelzer 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.
>>
>> Helped-by: Pedro Martelletto <pedro@yubico.com>
>> Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
>
>Should this also have a "Helped-by: Junio" since this code was heavily
>inspired by his suggestion[1]?

Yeah, this should have a "Written-by: Junio" ^^
I'm never sure when to add these headers (except the signed-off).

>
>[1]: https://lore.kernel.org/git/xmqqo84rcn3j.fsf@gitster.g/
>
>> ---
>> diff --git a/gpg-interface.c b/gpg-interface.c
>> @@ -502,15 +501,30 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,
>> +		const char *next;
>> +		for (line = ssh_principals_out.buf;
>> +		     *line;
>> +		     line = next) {
>> +			const char *end_of_text;
>> +
>> +			next = end_of_text = strchrnul(line, '\n');
>> +
>> +			 /* Did we find a LF, and did we have CR before it? */
>> +			if (*end_of_text &&
>> +			    line < end_of_text &&
>> +			    end_of_text[-1] == '\r')
>> +				end_of_text--;
>> +
>> +			/* Unless we hit NUL, skip over the LF we found */
>> +			if (*next)
>> +				next++;
>> +
>> +			/* Not all lines are data.  Skip empty ones */
>> +			if (line == end_of_text)
>> +				continue;
>> +
>> +			/* We now know we have an non-empty line. Process it */
>> +			principal = xmemdupz(line, end_of_text - line);
>
>Considering that this code makes a copy of the line _anyhow_ which it
>assigns to `principal`, it still seems like it would be simpler and
>far easier to understand at-a-glance to instead take advantage of one
>of the existing string-splitting functions. For instance, something
>like this:
>
>    struct strbuf **line, **to_free;
>    line = to_free = strbuf_split(&ssh_principals_out, '\n');
>    for (; *line; line++) {
>        strbuf_trim_trailing_newline(*line);
>        if (!(*line)->len)
>            continue;
>        principal = (*line)->buf;
>
>keeping in mind that strbuf_trim_trailing_newline() takes care of
>CR/LF, and with appropriate cleanup at the end of the loop:
>
>        strbuf_list_free(to_free);
>
>(and removal of `FREE_AND_NULL(principal)` which is no longer needed).
>
>Something similar can be done with string_list_split(), as well.

I agree that this is the most readable of the variants (and it works just as 
well). Since in most cases there even will only ever be a single line of 
output the extra work/allocation we might be doing with it is quite minimal.

I have done something quite similar in get_default_ssh_signing_key() and got 
a bit of negative feedback for it (being overkill for retrieving a single 
line) but ended up using it anyway.

Re: [PATCH v3] gpg-interface: trim CR from ssh-keygen

[Junio C Hamano]

Fabian Stelzer <fs@gigacodes.de> writes:

> On 09.01.2022 16:37, Eric Sunshine wrote:
>>On Fri, Jan 07, 2022 at 10:07:35AM +0100, Fabian Stelzer 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.
>>>
>>> Helped-by: Pedro Martelletto <pedro@yubico.com>
>>> Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
>>
>>Should this also have a "Helped-by: Junio" since this code was heavily
>>inspired by his suggestion[1]?
>
> Yeah, this should have a "Written-by: Junio" ^^
> I'm never sure when to add these headers (except the signed-off).

Heh, helped-by might be OK but I certainly didn't write it.  I
merely translated what you wrote without knowing exactly what's
on these lines (and what I knew, like the lines are the unit of
processing and empty lines are to be skipped, I all learned from
your original without knowing why) ;-)

So if anything, Helped-by: is good enough, but I do not need more
credit or blame ;-)

Re: [PATCH v3] gpg-interface: trim CR from ssh-keygen

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> of the existing string-splitting functions. For instance, something
> like this:
>
>     struct strbuf **line, **to_free;
>     line = to_free = strbuf_split(&ssh_principals_out, '\n');
>     for (; *line; line++) {
>         strbuf_trim_trailing_newline(*line);
>         if (!(*line)->len)
>             continue;
>         principal = (*line)->buf;
>
> keeping in mind that strbuf_trim_trailing_newline() takes care of
> CR/LF, and with appropriate cleanup at the end of the loop:
>
>         strbuf_list_free(to_free);
>
> (and removal of `FREE_AND_NULL(principal)` which is no longer needed).
>
> Something similar can be done with string_list_split(), as well.

Unless you are writing an interactive text editor, an array of
lines, each of which can individually be manupulated cheaply when
inserting or deleting a span of chars, is a way too ugly and overly
expensive data structure to keep your data in the long haul.  In
short, strbuf_split() was a mistaken piece of API that does not
belong to this project ;-)

The cycles spent by crypto before getting to this point in the code
is expensive enough that the extra cycles to separately scan to
split them into lines and another scan from the end of the each line
to trim may not matter, so I'd stop at saying "I'd rather not to see
the above code" instead of my usual "Please don't", from performance
perspective in this case.

But from code cleanliness perspective, well, let me just say that
this is not Python or Java but a C project.