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

From: pedro martelletto <[email protected]>

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 <[email protected]>

Signed-off-by Johannes Schindelin <[email protected]>

---
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(-)

gpg-interface.c +1 -1

index 3e7255a2a91..85e26882782 100644
@@ -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

Signature · 1 line

gitgitgadget

gpg-interface.c static int verify_ssh_signed_buffer(struct signature_check *sigc, · line 501

Quoted 12 lines · Johannes Schindelin via GitGitGadget

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.
[...]

gpg-interface.c +1 -1

@@ -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.

Quoted 20 lines · Jeff King

gpg-interface.c static int verify_ssh_signed_buffer(struct signature_check *sigc, · line 501

Quoted 12 lines · Johannes Schindelin via GitGitGadget

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.
[...]

gpg-interface.c +1 -1

@@ -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.

Quoted 5 lines

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 ;).

Quoted 5 lines

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?

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 ;).

Quoted 10 lines · Fabian Stelzer

Quoted 5 lines

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

Quoted 4 lines

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.

Quoted 6 lines

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...

Quoted 3 lines · Junio C Hamano

So instead of the posted patch, we should do something along this line instead?

Quoted 6 lines

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:

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.

Quoted 25 lines · Fabian Stelzer

Quoted 5 lines · Pedro Martelletto

Quoted 3 lines · Junio C Hamano

So instead of the posted patch, we should do something along this line instead?

Quoted 9 lines

Quoted 6 lines

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;

Quoted 3 lines

        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".

Quoted 25 lines · Fabian Stelzer

Quoted 5 lines · Pedro Martelletto

Quoted 3 lines · Junio C Hamano

So instead of the posted patch, we should do something along this line instead?

Quoted 9 lines

Quoted 6 lines

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;

Quoted 3 lines

        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".

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 <[email protected]>

Signed-off-by Fabian Stelzer <[email protected]>

---

Quoted 10 lines · 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 <[email protected]>

Signed-off-by Fabian Stelzer <[email protected]>

---

gpg-interface.c static int verify_ssh_signed_buffer(struct signature_check *sigc, · line 512

Quoted 5 lines

gpg-interface.c +4 -1

@@ -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--;

Quoted 12 lines · Eric Sunshine

gpg-interface.c static int verify_ssh_signed_buffer(struct signature_check *sigc, · line 515

Quoted 10 lines · 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.
---

gpg-interface.c +4 -1

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

Quoted 5 lines

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.

Quoted 19 lines · Junio C Hamano

Quoted 12 lines · Eric Sunshine

gpg-interface.c static int verify_ssh_signed_buffer(struct signature_check *sigc, · line 515

Quoted 10 lines · 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.
---

gpg-interface.c +4 -1

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

Quoted 5 lines

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

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

Quoted 6 lines

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).

Quoted 22 lines · Eric Sunshine

Quoted 14 lines · Junio C Hamano

Quoted 7 lines · Eric Sunshine

gpg-interface.c static int verify_ssh_signed_buffer(struct signature_check *sigc, · line 515

Quoted 5 lines · Fabian Stelzer

gpg-interface.c +4 -1

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

Quoted 5 lines

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

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

Quoted 6 lines

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.

Quoted 4 lines

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.

Quoted 28 lines · Junio C Hamano

Quoted 22 lines · Eric Sunshine

Quoted 14 lines · Junio C Hamano

Quoted 7 lines · Eric Sunshine

gpg-interface.c static int verify_ssh_signed_buffer(struct signature_check *sigc, · line 515

Quoted 5 lines · Fabian Stelzer

gpg-interface.c +4 -1

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

Quoted 5 lines

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

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

Quoted 6 lines

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.

Quoted 4 lines

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).

Quoted 4 lines

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).

Quoted 14 lines · Fabian Stelzer

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.

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.

Quoted 16 lines · Junio C Hamano

Quoted 14 lines · Fabian Stelzer

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);

Quoted 3 lines

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.

Quoted 6 lines

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.

Quoted 3 lines

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(?).

Quoted 16 lines

        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.

Quoted 18 lines

                /* 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);
                ...
        }

Quoted 3 lines

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 ?

Quoted 18 lines · Fabian Stelzer

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');

Quoted 7 lines

	/* 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. ... ... Since the gpg stuff is in place for a long time and no one complained we can probably leave it as is.

Quoted 6 lines · Eric Sunshine

Quoted 5 lines

Quoted 5 lines

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

Quoted 4 lines

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.

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 <[email protected]>

Signed-off-by Fabian Stelzer <[email protected]>

---

gpg-interface.c +14 -0

@@ -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--;
+

Quoted 7 lines · Fabian Stelzer

Quoted 6 lines · Eric Sunshine

Quoted 5 lines

Quoted 5 lines

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

Quoted 8 lines

Quoted 4 lines

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...

Quoted 7 lines · 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 <[email protected]>

Signed-off-by Fabian Stelzer <[email protected]>

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

[1]: https://lore.kernel.org/git/[email protected]/

Quoted 17 lines

---

gpg-interface.c +14 -0

@@ -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--;
+

gpg-interface.c static int verify_ssh_signed_buffer(struct signature_check *sigc, · line 524

Quoted 10 lines

gpg-interface.c +10 -0

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

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.

Quoted 6 lines · Eric Sunshine

Quoted 5 lines · 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.

Quoted 5 lines

Quoted 2 lines

Helped-by Pedro Martelletto <[email protected]>

Signed-off-by Fabian Stelzer <[email protected]>

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).