Message ID | 27f6400a21412d762b290a34a78ebe7296d36bf3.1601293224.git.gitgitgadget@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Prepare git credential to read input with DOS line endings | expand |
On Mon, Sep 28, 2020 at 11:40:22AM +0000, Nikita Leonov via GitGitGadget wrote: > 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
Hi Peff, On Mon, 28 Sep 2020, Jeff King wrote: > On Mon, Sep 28, 2020 at 11:40:22AM +0000, Nikita Leonov via GitGitGadget wrote: > > > 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. Indeed. > 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. True. It's a problem with all of those URLs that end in Carriage Returns ;-) > 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). Indeed. > 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. I checked again with github.com/git-for-windows/git/pull/2516, where the patch originally entered the public eye, but could not find any background information. 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: 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? Ciao, Dscho
On Fri, Oct 02, 2020 at 01:37:23PM +0200, Johannes Schindelin wrote: > 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? 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: > 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$// -Peff
Hi Peff, On Fri, 2 Oct 2020, Jeff King wrote: > On Fri, Oct 02, 2020 at 01:37:23PM +0200, Johannes Schindelin wrote: > > > 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? > > 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: > > > 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$// Thanks, fixed! Ciao, Dscho
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > 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? 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. Thanks.
Hi Junio, On Fri, 2 Oct 2020, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > > 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? > > 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. Okay, I incorporated a comment talking about `quit\r` and will submit a new iteration right now. Thanks, Dscho
diff --git a/credential.c b/credential.c index efc29dc5e1..e5202fbef2 100644 --- a/credential.c +++ b/credential.c @@ -202,7 +202,7 @@ int credential_read(struct credential *c, FILE *fp) { struct strbuf line = STRBUF_INIT; - while (strbuf_getline_lf(&line, fp) != EOF) { + while (strbuf_getline(&line, fp) != EOF) { char *key = line.buf; char *value = strchr(key, '=');