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

Commit Message

Linus Arver via GitGitGadget Sept. 28, 2020, 11:40 a.m. UTC

From: Nikita Leonov <[e-mail]>

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 <[e-mail]>
Signed-off-by: Johannes Schindelin <[e-mail]>
---
 credential.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jeff King Sept. 29, 2020, 12:42 a.m. UTC | #1

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

> From: Nikita Leonov <[e-mail]>
> 
> 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

Johannes Schindelin Oct. 2, 2020, 11:37 a.m. UTC | #2

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 <[e-mail]>
> >
> > 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

Jeff King Oct. 2, 2020, 12:01 p.m. UTC | #3

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

Johannes Schindelin Oct. 2, 2020, 12:27 p.m. UTC | #4

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

Junio C Hamano Oct. 2, 2020, 4:32 p.m. UTC | #5

Johannes Schindelin <[e-mail]> 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.

Johannes Schindelin Oct. 3, 2020, 1:28 p.m. UTC | #6

Hi Junio,

On Fri, 2 Oct 2020, Junio C Hamano wrote:

> Johannes Schindelin <[e-mail]> 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

Patch

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