Re: [PATCH 2/2] test-lib: exhaustively insert non-alnum ASCII into the TRASH_DIRECTORY name

[Joachim Durchholz <jo@durchholz.org>]

Am 10.04.2017 um 18:57 schrieb Jeff King:
>>> If there are security bugs where a malicious input can cause us
>>> to do something bad, that's something to care about. But that's very
>>> different than asking "do these tests run to completion with a funny
>>> input".
>>
>> If the tests do not complete, git is doing something unexpected.
>
> I very much disagree with that. Git's test operate under a set of
> assumptions, and if you violate those assumptions, then the failures are
> not meaningful.

In that case the tests do not validate that git can properly work with 
special characters.
That's a pretty big coverage gap.

> Take alternates, for instance. The on-disk storage format cannot
> represent paths with newlines in them. If a test does:
>
>   git clone -s "$(pwd)" parent.git child &&
>   test -d child
>
> then that test is going to fail if the test directory has a newline in
> it. But that doesn't tell us anything meaningful. Maybe there is a bug
> and maybe there isn't, but we cannot know because the thing being tested
> cannot possibly work under the test environment given.

Sure. Not all tests are meaningful in all environments.
That doesn't mean that the tests are generally meaningless.

Also, we're talking about two pretty different things here: gits 
interaction with the file system, and git's interaction with whatever 
shell its scripts are using.
In an ideal world, these two aspects would be orthogonal and could be 
tested independently of each other. Since in practice we do have 
correlations and dependencies, this isn't always possible, but it's what 
we should aim for.

> You can rewrite all the tests to say "skip this test if there's a
> newline in the test directory". But to what end? It's work to write and
> to maintain, and we haven't gained new information.

Not on that "alternates" thing (whatever that is), but we have a test 
that will work and provide information on systems that do allow newlines.

>> That in itself is not a security hole, but there's a pretty good chance that
>> at least one of these ~230 unexpected things can be turned into one, given
>> enough time and motivation. The risk multiplies as this is shell scripting,
>> where the path from "string is misinterpreted" to "string is run as a
>> command" is considerably shorter than in other languages.
>
> Sure, and I'd encourage people who are interested to dig through the
> results and see if they can find a real problem. I looked at several and
> didn't find anything that wasn't an example of the "test assumptions"
> thing above.

Don't assume that there's no risk just because you didn't find anything.

Also, git might not be the actual hole, but other software that relies 
on git not doing anything awkward might fail that assumption and expose 
the actual breach of security.
You could argue that it's not a problem in git, but it is. Unless and 
until the git docs clearly state that things may break if "funny 
characters" are being used, and where.

> But again, I'm happy to be proven wrong.

With security, you need to be confident about the absence of /any/ type 
of hole, not the absence of a /specific/ type of hole such as a shell 
injection.
So the way forward isn't proving you wrong by providing a specific 
exploit, it's making sure that no exploit with URLs and file names can 
possibly exist.

Now if I'm reading things like "heuristics" and "git uses URL-specific 
code even after it has determined it's not a URL"... well, that's the 
exact opposite of reassuring messages.

 > I just don't think
> plastering control characters into the test directory names all the time
> is a good way of finding those problems (and doesn't balance out the
> cost).

Fair enough.