Re: [PATCH v2 1/3] credential.c: fix credential reading with regards to CR/LF

From: Jeff King <peff@peff.net>
Date: Sep 29, 2020, 00:42 UTC
Message-ID: <20200929004220.GC898702@coredump.intra.peff.net>
In-Reply-To: <27f6400a21412d762b290a34a78ebe7296d36bf3.1601293224.git.gitgitgadget@gmail.com>

On Mon, Sep 28, 2020 at 11:40:22AM +0000, Nikita Leonov via GitGitGadget wrote:

Show 13 quoted lines

> From: Nikita Leonov <nykyta.leonov@gmail.com>
> 
> 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.

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:

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

diff --git a/credential.c b/credential.c
index efc29dc5e1..73143c5ed0 100644
--- a/credential.c
+++ b/credential.c
@@ -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.

-Peff