Web lists-archives.com

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




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.