git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "brian m. carlson" <sandals@crustytoothpaste.net>
To: Matthew DeVore <matvore@google.com>
Cc: git@vger.kernel.org, jeffhost@microsoft.com, l.s.r@web.de,
	gitster@pobox.com, spearce@spearce.org, jrn@google.com
Subject: Re: [PATCH 2/2] url: do not allow %00 to represent NULL in URLs
Date: Tue, 4 Jun 2019 01:02:43 +0000	[thread overview]
Message-ID: <20190604010243.GR8616@genre.crustytoothpaste.net> (raw)
In-Reply-To: <20190603204526.7723-3-matvore@google.com>

[-- Attachment #1: Type: text/plain, Size: 1433 bytes --]

On 2019-06-03 at 20:45:26, Matthew DeVore wrote:
> There is no reason to allow %00 to terminate a string, so do not allow it.
> Otherwise, we end up returning arbitrary content in the string (that which is
> after the %00) which is effectively hidden from callers and can escape sanity
> checks and validation, and possible be used in tandem with a security
> vulnerability to introduce a payload.

So I think the reason you've stated is good and I agree that we
shouldn't decode data we're not going to use. However, I'm also
interested in the cases in which we decode data and don't want to allow
NULs, because we should, in general, allow bizarre URLs as long as
they're URL-encoded.

It looks like several of the places we do this are in the credential
manager code, and I think I can agree that usernames and passwords
should not contain NUL characters (for Basic auth, RFC 7617 prohibits
it). It also seems that the credential code decodes the path parameter
before passing it on, which is unfortunate, but can't be changed for
backward compatibility reasons.

And then the other instances are a file: URL in remote-testsvn.c and
query parameters that have no reason to contain NULs in http-backend.c.

So I think overall this is fine, although we probably want to change the
commit summary to say "NUL" instead of "NULL".
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

  reply	other threads:[~2019-06-04  1:02 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-03 20:45 [PATCH 0/2] Harden url.c URL-decoding logic Matthew DeVore
2019-06-03 20:45 ` [PATCH 1/2] url: do not read past end of buffer Matthew DeVore
2019-06-04  5:00   ` René Scharfe
2019-06-04 17:22     ` Matthew DeVore
2019-06-03 20:45 ` [PATCH 2/2] url: do not allow %00 to represent NULL in URLs Matthew DeVore
2019-06-04  1:02   ` brian m. carlson [this message]
2019-06-04 17:38     ` Matthew DeVore
2019-06-04  5:01   ` René Scharfe
2019-06-04 17:23     ` Matthew DeVore

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

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to=20190604010243.GR8616@genre.crustytoothpaste.net \
    --to=sandals@crustytoothpaste.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jeffhost@microsoft.com \
    --cc=jrn@google.com \
    --cc=l.s.r@web.de \
    --cc=matvore@google.com \
    --cc=spearce@spearce.org \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

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 read-only IMAP folder(s) and NNTP newsgroup(s).