git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Nikita Leonov via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org,
	Johannes Schindelin <johannes.schindelin@gmx.de>,
	Nikita Leonov <nykyta.leonov@gmail.com>
Subject: Re: [PATCH v2 1/3] credential.c: fix credential reading with regards to CR/LF
Date: Mon, 28 Sep 2020 20:42:20 -0400	[thread overview]
Message-ID: <20200929004220.GC898702@coredump.intra.peff.net> (raw)
In-Reply-To: <27f6400a21412d762b290a34a78ebe7296d36bf3.1601293224.git.gitgitgadget@gmail.com>

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

  reply	other threads:[~2020-09-29  0:42 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-14 13:49 [PATCH] " Johannes Schindelin via GitGitGadget
2020-02-14 17:55 ` Junio C Hamano
2020-02-14 18:32 ` Jeff King
2020-09-28 11:40 ` [PATCH v2 0/3] Prepare git credential to read input with DOS line endings Johannes Schindelin via GitGitGadget
2020-09-28 11:40   ` [PATCH v2 1/3] credential.c: fix credential reading with regards to CR/LF Nikita Leonov via GitGitGadget
2020-09-29  0:42     ` Jeff King [this message]
2020-10-02 11:37       ` Johannes Schindelin
2020-10-02 12:01         ` Jeff King
2020-10-02 12:27           ` Johannes Schindelin
2020-10-02 16:32         ` Junio C Hamano
2020-10-03 13:28           ` Johannes Schindelin
2020-09-28 11:40   ` [PATCH v2 2/3] credentials: make line reading Windows compatible Nikita Leonov via GitGitGadget
2020-09-28 20:58     ` Junio C Hamano
2020-09-29  0:35       ` Jeff King
2020-09-30 22:33         ` Junio C Hamano
2020-10-02  7:53           ` Johannes Schindelin
2020-09-28 23:26     ` Carlo Arenas
2020-09-28 23:41       ` Junio C Hamano
2020-09-29  0:30       ` Jeff King
2020-09-29  0:41         ` Junio C Hamano
2020-09-29  0:44           ` Jeff King
2020-09-29  0:54             ` Junio C Hamano
2020-09-29  3:00               ` Jeff King
2020-09-30 22:25                 ` Junio C Hamano
2020-09-30 22:39                   ` Jeff King
2020-09-30 22:56                     ` Junio C Hamano
2020-10-01 13:54                       ` Jeff King
2020-10-01 15:42                         ` Junio C Hamano
2020-10-02  8:07                           ` Johannes Schindelin
2020-09-28 11:40   ` [PATCH v2 3/3] docs: make notes regarding credential line reading Nikita Leonov via GitGitGadget
2020-09-28 20:31   ` [PATCH v2 0/3] Prepare git credential to read input with DOS line endings Junio C Hamano
2020-10-03 13:29   ` [PATCH v3] credential: treat CR/LF as line endings in the credential protocol Johannes Schindelin via GitGitGadget

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200929004220.GC898702@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=johannes.schindelin@gmx.de \
    --cc=nykyta.leonov@gmail.com \
    --subject='Re: [PATCH v2 1/3] credential.c: fix credential reading with regards to CR/LF' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).