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:
signature.asc
Description: PGP signature
