credential.c: fix credential reading with regards to CR/LF

From: Nikita Leonov <[email protected]>

This fix makes using Git credentials more friendly to Windows users. In previous version it was unable to finish input correctly without configuration changes (tested in PowerShell, CMD, Cygwin).

We know credential filling should be finished by empty input, but the current implementation does not take into account CR/LF ending, and hence instead of the empty string we get '\r', which is interpreted as an incorrect string.

So this commit changes default reading function to a more Windows compatible reading function.

Signed-off-by Nikita Leonov <[email protected]>

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

---

 credential.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

credential.c +0 -0

index 62be651b03..65989dfa4d 100644

From: Nikita Leonov <[email protected]>

This fix makes using Git credentials more friendly to Windows users. In previous version it was unable to finish input correctly without configuration changes (tested in PowerShell, CMD, Cygwin).

We know credential filling should be finished by empty input, but the current implementation does not take into account CR/LF ending, and hence instead of the empty string we get '\r', which is interpreted as an incorrect string.

So this commit changes default reading function to a more Windows compatible reading function.

This contribution came in via Git for Windows
[https://github.com/git-for-windows/git/pull/2516].

Sadly, I did not find the time to go through all the changes of 8f309aeb ("strbuf: introduce strbuf_getline_{lf,nul}()", 2016-01-13) (as Junio asked
[https://public-inbox.org/git/[email protected]]).
Rather than delaying this patch indefinitely, I admit defeat on that angle.

Changes since v1:

* Added a commit to adjust credential-daemon and credential-store in the same manner.

From: Nikita Leonov <[email protected]>

This commit makes reading process regarding credentials compatible with 'CR/LF' line ending. It makes using git more convenient on systems like Windows.

Signed-off-by Nikita Leonov <[email protected]>

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

---
 builtin/credential-cache--daemon.c |  4 ++--
 builtin/credential-store.c         |  2 +-
 t/t0302-credential-store.sh        | 16 ++++++----------
 3 files changed, 9 insertions(+), 13 deletions(-)

builtin/credential-cache--daemon.c +2 -2

index c61f123a3b..17664bb0d5 100644
@@ -99,12 +99,12 @@ static int read_request(FILE *fh, struct credential *c,
 	static struct strbuf item = STRBUF_INIT;
 	const char *p;
 
-	strbuf_getline_lf(&item, fh);
+	strbuf_getline(&item, fh);
 	if (!skip_prefix(item.buf, "action=", &p))
 		return error("client sent bogus action line: %s", item.buf);
 	strbuf_addstr(action, p);
 
-	strbuf_getline_lf(&item, fh);
+	strbuf_getline(&item, fh);
 	if (!skip_prefix(item.buf, "timeout=", &p))
 		return error("client sent bogus timeout line: %s", item.buf);
 	*timeout = atoi(p);

builtin/credential-store.c +1 -1

index 5331ab151a..d4e90b68df 100644
@@ -23,7 +23,7 @@ static int parse_credential_file(const char *fn,
 		return found_credential;
 	}
 
-	while (strbuf_getline_lf(&line, fh) != EOF) {
+	while (strbuf_getline(&line, fh) != EOF) {
 		if (!credential_from_url_gently(&entry, line.buf, 1) &&
 		    entry.username && entry.password &&
 		    credential_match(c, &entry)) {

t/t0302-credential-store.sh +6 -10

index 716bf1af9f..f2c672e4b6 100755
@@ -142,7 +142,7 @@ invalid_credential_test "scheme" ://user:[email protected]
 invalid_credential_test "valid host/path" https://user:pass@
 invalid_credential_test "username/password" https://[email protected]
 
-test_expect_success 'get: credentials with DOS line endings are invalid' '
+test_expect_success 'get: credentials with DOS line endings are valid' '
 	printf "https://user:[email protected]\r\n" >"$HOME/.git-credentials" &&
 	check fill store <<-\EOF
 	protocol=https

@@ -150,11 +150,9 @@ test_expect_success 'get: credentials with DOS line endings are invalid' '
 	--
 	protocol=https
 	host=example.com
-	username=askpass-username
-	password=askpass-password
+	username=user
+	password=pass
 	--
-	askpass: Username for '\''https://example.com'\'':
-	askpass: Password for '\''https://[email protected]'\'':
 	--
 	EOF
 '

@@ -172,7 +170,7 @@ test_expect_success 'get: credentials with path and DOS line endings are valid'
 	EOF
 '
 
-test_expect_success 'get: credentials with DOS line endings are invalid if path is relevant' '
+test_expect_success 'get: credentials with DOS line endings are valid if path is relevant' '
 	printf "https://user:[email protected]/repo.git\r\n" >"$HOME/.git-credentials" &&
 	test_config credential.useHttpPath true &&
 	check fill store <<-\EOF

@@ -181,11 +179,9 @@ test_expect_success 'get: credentials with DOS line endings are invalid if path
 	protocol=https
 	host=example.com
 	path=repo.git
-	username=askpass-username
-	password=askpass-password
+	username=user
+	password=pass
 	--
-	askpass: Username for '\''https://example.com/repo.git'\'':
-	askpass: Password for '\''https://[email protected]/repo.git'\'':
 	--
 	EOF
 '

t/t0302-credential-store.sh +3 -5

index 716bf1af9f..f2c672e4b6 100755
@@ -142,7 +142,7 @@ invalid_credential_test "scheme" ://user:[email protected]
 invalid_credential_test "valid host/path" https://user:pass@
 invalid_credential_test "username/password" https://[email protected]
 
-test_expect_success 'get: credentials with DOS line endings are invalid' '
+test_expect_success 'get: credentials with DOS line endings are valid' '
        printf "https://user:[email protected]\r\n" >"$HOME/.git-credentials" &&
        check fill store <<-\EOF
        protocol=https

@@ -150,11 +150,9 @@ test_expect_success 'get: credentials with DOS line endings are invalid' '
        --
        protocol=https
        host=example.com
-       username=askpass-username
-       password=askpass-password
+       username=user
+       password=pass
        --
-       askpass: Username for '\''https://example.com'\'':
-       askpass: Password for '\''https://[email protected]'\'':
        --
        EOF
 '

t/t0302-credential-store.sh test_expect_success 'get: credentials with path and DOS line endings are valid' · line 173

Quoted 2 lines

t/t0302-credential-store.sh +1 -1

-test_expect_success 'get: credentials with DOS line endings are invalid if path is relevant' '
+test_expect_success 'get: credentials with DOS line endings are valid if path is relevant' '

note that this test was put in place to protect users from regressions like the one we got after the release of 2.26.1 where users that had '\r' as part of their credentials were getting an error[1]

while I am sympathetic to the change (indeed I proposed something similar, but was reminded by Peff that while it looks like a text file it was designed to be considered opaque and therefore should use UNIX LF as record terminator by specification), I am concerned this could result in a similar regression since we know they are still users out there that had modified this file manually (something that was not recommended) and are currently relying on the fact that these lines are invalid and therefore silently ignored.

Carlo

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

t/t0302-credential-store.sh test_expect_success 'get: credentials with path and DOS line endings are valid' · line 173

Quoted 2 lines

t/t0302-credential-store.sh +1 -1

-test_expect_success 'get: credentials with DOS line endings are invalid if path is relevant' '
+test_expect_success 'get: credentials with DOS line endings are valid if path is relevant' '

note that this test was put in place to protect users from regressions like the one we got after the release of 2.26.1 where users that had '\r' as part of their credentials were getting an error[1]

while I am sympathetic to the change (indeed I proposed something similar, but was reminded by Peff that while it looks like a text file it was designed to be considered opaque and therefore should use UNIX LF as record terminator by specification), I am concerned this could result in a similar regression since we know they are still users out there that had modified this file manually (something that was not recommended) and are currently relying on the fact that these lines are invalid and therefore silently ignored.

Quoted 5 lines · Nikita Leonov via GitGitGadget

From: Nikita Leonov <[email protected]>

This commit makes reading process regarding credentials compatible with 'CR/LF' line ending. It makes using git more convenient on systems like Windows.

I can see why this is a good thing for "store" and the two updated pieces of the test script demonstrate it very well.

But it is unclear why and how cache-daemon benefits from this change. That needs to be justified.

I think that:

- we'd never write a raw CR ourselves, as we'd urlencode the character

- if somebody did put in a raw CR manually like:

https://example.com\r\n

then we'd currently fail to match "example.com". Which is probably not what they wanted. I suspect that \r in a hostname is bogus anyway (certainly curl will complain about it).

From: Nikita Leonov <[email protected]>

This fix makes using Git credentials more friendly to Windows users. In previous version it was unable to finish input correctly without configuration changes (tested in PowerShell, CMD, Cygwin).

We know credential filling should be finished by empty input, but the current implementation does not take into account CR/LF ending, and hence instead of the empty string we get '\r', which is interpreted as an incorrect string.

So this commit changes default reading function to a more Windows compatible reading function.

We know credential filling should be finished by empty input, but the current implementation does not take into account CR/LF ending, and hence instead of the empty string we get '\r', which is interpreted as an incorrect string.

Quoted 11 lines · Jeff King

I think that:

- we'd never write a raw CR ourselves, as we'd urlencode the character

- if somebody did put in a raw CR manually like:

https://example.com\r\n

then we'd currently fail to match "example.com". Which is probably not what they wanted. I suspect that \r in a hostname is bogus anyway (certainly curl will complain about it).

I may be misremembering, but an argument I recall against the kind of change we are dicussing now was that we ignore such an entry right now, and the user may have added an entry for the host anew, possibly with a more recent password. Changing the parsing to ignore CR would silently resurrect such a stale entry that the user has written off as unused, and depending on the order of entries in the file, a site that used to work may start failing suddenly.

I still don't see why we need to touch the cache-daemon, though.

Yeah, that is probably what would happen. I have to admit that it's such an obscure case that I'm not sure I really care. It's unlikely in practice, and if somebody did report such a case, I think my first response would be "well, why did you have a broken entry stuck in your file?".

Quoted 5 lines · Jeff King

Yeah, that is probably what would happen. I have to admit that it's such an obscure case that I'm not sure I really care. It's unlikely in practice, and if somebody did report such a case, I think my first response would be "well, why did you have a broken entry stuck in your file?".

I think we know the likely answer. "I once used Windows to edit the file manually".

After which the file looks broken, so the user may have re-added via the credential API (with LF line ending) a new entry for the host and have been happily using the result.

Quoted 11 lines · Junio C Hamano

Quoted 5 lines · Jeff King

Yeah, that is probably what would happen. I have to admit that it's such an obscure case that I'm not sure I really care. It's unlikely in practice, and if somebody did report such a case, I think my first response would be "well, why did you have a broken entry stuck in your file?".

I think we know the likely answer. "I once used Windows to edit the file manually".

Quoted 3 lines

After which the file looks broken, so the user may have re-added via the credential API (with LF line ending) a new entry for the host and have been happily using the result.

I wrote something less charitable towards the user at first, and then toned it down. But I think I toned it down too much. Maybe my response would be "garbage in, garbage out; it was lucky that it worked before".

Quoted 8 lines · Junio C Hamano

Quoted 5 lines · Nikita Leonov via GitGitGadget

From: Nikita Leonov <[email protected]>

This commit makes reading process regarding credentials compatible with 'CR/LF' line ending. It makes using git more convenient on systems like Windows.

Quoted 5 lines

I can see why this is a good thing for "store" and the two updated pieces of the test script demonstrate it very well.

But it is unclear why and how cache-daemon benefits from this change. That needs to be justified.

I suspect it doesn't need touched, because it is internal to git-daemon. But it does handle CRLF for some lines, because the first patch modified credential_read(), which the daemon builds on (and which _is_ user-facing via git-credential). So there's perhaps an argument that these calls should just be made consistent, even though the only one who would write them is our matching client. If that is the argument to be made, I think it would make sense to do so in a separate patch, since there's no functional change.

(I'm also slightly puzzled that anybody on Windows would care about credential-cache, since it require unix sockets. But maybe in a world of WSL people are actually able to mix the two. I confess I haven't kept up with the state of things in Windows).

OK, so what's the final verdict from us? This is good enough to move forward as-is?

Quoted 2 lines · Junio C Hamano

OK, so what's the final verdict from us? This is good enough to move forward as-is?

I'd prefer to see the credential-cache--daemon changes either dropped, or split out into a separate patch with the justification of: this probably doesn't matter in practice, but it makes the whole protocol between client and server treat CRLF consistently.

I had also raised a question in:

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

about whether they just care about the empty line, or about CRLF on data lines. If the former (which is what the commit message claims), then I think we can do something much simpler. But I suspect it is the latter, and it is simply the commit message that is a bit misleading.

Other than those nits, I think the series is OK.

Quoted 1 line

Other than those nits, I think the series is OK.

Sure. But credential-store side is also iffy; it is not like they want CRLF on data lines (if they want CR in data, that needs to be encoded). The only reason I can think of that the change to "-store" makes any difference is when people edit it, but the file is not designed to be manually edited, so even that part of the series needs a better justification. It's not like "We want to be compatible" without "why it is better to be compatible" is a good rationale, when we define the file format not to be manually edited in the first place.

Yeah, I agree that just teaching git-credential's stdin to handle CR would be an OK stopping point. I don't have a strong opinion on credential-store's on disk format. At least allowing CRLF there is _plausibly_ useful, unlike credential-cache--daemon's pipe. And I doubt that making the change would hurt anybody.

Quoted 25 lines · Jeff King

Quoted 8 lines · Junio C Hamano

Quoted 5 lines · Nikita Leonov via GitGitGadget

From: Nikita Leonov <[email protected]>

This commit makes reading process regarding credentials compatible with 'CR/LF' line ending. It makes using git more convenient on systems like Windows.

Quoted 5 lines

I can see why this is a good thing for "store" and the two updated pieces of the test script demonstrate it very well.

But it is unclear why and how cache-daemon benefits from this change. That needs to be justified.

I suspect it doesn't need touched, because it is internal to git-daemon. But it does handle CRLF for some lines, because the first patch modified credential_read(), which the daemon builds on (and which _is_ user-facing via git-credential). So there's perhaps an argument that these calls should just be made consistent, even though the only one who would write them is our matching client. If that is the argument to be made, I think it would make sense to do so in a separate patch, since there's no functional change.

Quoted 4 lines

(I'm also slightly puzzled that anybody on Windows would care about credential-cache, since it require unix sockets. But maybe in a world of WSL people are actually able to mix the two. I confess I haven't kept up with the state of things in Windows).

True, so some, if not all, parts of these changes start to look more and more like "I change LF to CRLF purely for political correctness, even I know nobody sends CRLF in these cases (or "even I do not know if anybody sends CRLF in these cases", which essentially amounts to the same thing but may be worse)", needless code churns.

Quoted 5 lines · Jeff King

Yeah, I agree that just teaching git-credential's stdin to handle CR would be an OK stopping point. I don't have a strong opinion on credential-store's on disk format. At least allowing CRLF there is _plausibly_ useful, unlike credential-cache--daemon's pipe. And I doubt that making the change would hurt anybody.

I agree 100% with all of the above, including the part that says it is also OK to let credential-store read from its files with line ending converted to CRLF.

Quoted 11 lines · Nikita Leonov via GitGitGadget

From: Nikita Leonov <[email protected]>

This fix makes using Git credentials more friendly to Windows users. In previous version it was unable to finish input correctly without configuration changes (tested in PowerShell, CMD, Cygwin).

We know credential filling should be finished by empty input, but the current implementation does not take into account CR/LF ending, and hence instead of the empty string we get '\r', which is interpreted as an incorrect string.

Quoted 2 lines

So this commit changes default reading function to a more Windows compatible reading function.

Unlike the credential-store file case, where we expect the data to be URL-encoded anyway (and so any true "\r" in the data would not be found in raw form), this means that the credential protocol can no longer represent "\r" at the end of a value.

And we'd match "example.com\r" and "example.com" as the same (unlikely, since carriage returns aren't allowed in hostnames, and curl will complain about this). We'd also match "cert://some/path\r" and "cert://some/path". Or "https://example.com/path\r" and its match, if you have credential.useHTTPPath set.

That may be acceptable if it makes things more convenient. Those are all pretty obscure cases, and I find it hard to believe an attacker could hijack credentials using this (it implies that the only difference between their malicious url and a known-good one is a trailing CR).

This part of the commit message confused me a little:

Quoted 4 lines

We know credential filling should be finished by empty input, but the current implementation does not take into account CR/LF ending, and hence instead of the empty string we get '\r', which is interpreted as an incorrect string.

If all we care about is the empty line, and not data lines, then we could do this:

credential.c +1 -1

index efc29dc5e1..73143c5ed0 100644
@@ -206,7 +206,7 @@ int credential_read(struct credential *c, FILE *fp)
 		char *key = line.buf;
 		char *value = strchr(key, '=');
 
-		if (!line.len)
+		if (!line.len || (line.len == 1 && line.buf[0] == '\r'))
 			break;
 
 		if (!value) {

without impacting the ability to send raw CR in the lines with actual data. But I imagine that a trailing CR in all of the data would also cause problems.

But I would highly doubt that the empty lines were the biggest problem:
Sure, we would fail to recognize an empty line with CR/LF line endings when reading with strbuf_getline_lf(), but we would totally misunderstand the entire rest of the lines, too. For example, we would mistake quit\r for an unknown command, and hence simply ignore it.

I do agree, however, that your confusion validly points out a flaw in the commit message: the "empty line" comment is a red herring.

Therefore, I spent some time pouring over the commit message. This is my current version:
[...]
What do you think?

While we do this for the credential helper protocol, we do _not_ do adjust git credential-cache--daemon (which won't work on Windows,

Quoted 6 lines · Johannes Schindelin

But I would highly doubt that the empty lines were the biggest problem:
Sure, we would fail to recognize an empty line with CR/LF line endings when reading with strbuf_getline_lf(), but we would totally misunderstand the entire rest of the lines, too. For example, we would mistake quit\r for an unknown command, and hence simply ignore it.

Quoted 7 lines

I do agree, however, that your confusion validly points out a flaw in the commit message: the "empty line" comment is a red herring.

Therefore, I spent some time pouring over the commit message. This is my current version:
[...]
What do you think?

I think we are on the same page, and this revision does a good job of fixing my complaint about the commit message. Thanks. One minor typo:

Quoted 2 lines

While we do this for the credential helper protocol, we do _not_ do adjust git credential-cache--daemon (which won't work on Windows,

s/do$//

Therefore, I spent some time pouring over the commit message. This is my current version:

credential: treat CR/LF as line endings in the credential protocol

This fix makes using Git credentials more friendly to Windows users: it allows a credential helper to communicate using CR/LF line endings ("DOS line endings" commonly found on Windows) instead of LF-only line endings ("Unix line endings").

Note that this changes the behavior a bit: if a credential helper produces, say, a password with a trailing Carriage Return character, that will now be culled even when the rest of the lines end only in Line Feed characters, indicating that the Carriage Return was not meant to be part of the line ending.

In practice, it seems _very_ unlikely that something like this happens. Passwords usually need to consist of non-control characters, URLs need to have special characters URL-encoded, and user names, well, are names.

So let's change the credential machinery to accept both CR/LF and LF line endings.

While we do this for the credential helper protocol, we do _not_ do adjust git credential-cache--daemon (which won't work on Windows, anyway, because it requires Unix sockets) nor git credential-store (which writes the file ~/.git-credentials which we consider an implementation detail that should be opaque to the user, read: we do expect users _not_ to edit this file manually).

What do you think?

Quoted 30 lines · Johannes Schindelin

Therefore, I spent some time pouring over the commit message. This is my current version:

credential: treat CR/LF as line endings in the credential protocol

This fix makes using Git credentials more friendly to Windows users: it allows a credential helper to communicate using CR/LF line endings ("DOS line endings" commonly found on Windows) instead of LF-only line endings ("Unix line endings").

Note that this changes the behavior a bit: if a credential helper produces, say, a password with a trailing Carriage Return character, that will now be culled even when the rest of the lines end only in Line Feed characters, indicating that the Carriage Return was not meant to be part of the line ending.

In practice, it seems _very_ unlikely that something like this happens. Passwords usually need to consist of non-control characters, URLs need to have special characters URL-encoded, and user names, well, are names.

So let's change the credential machinery to accept both CR/LF and LF line endings.

While we do this for the credential helper protocol, we do _not_ do adjust git credential-cache--daemon (which won't work on Windows, anyway, because it requires Unix sockets) nor git credential-store (which writes the file ~/.git-credentials which we consider an implementation detail that should be opaque to the user, read: we do expect users _not_ to edit this file manually).

Quoted 1 line

What do you think?

I am not Peff, but I was also drawn into the same confusion by the "we never see an empty line" red herring.

There are some micronits, but the above made a lot easier to understand (I think you could even add "quit\r" bit to make it even easier to understand) than the original description.