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