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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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