Re: [PATCH 2/2] test-lib: exhaustively insert non-alnum ASCII into the TRASH_DIRECTORY name
- From
- Joachim Durchholz <jo@durchholz.org>
- Date
- Apr 10, 2017, 18:19 UTC
- Message-ID
- <d1a71326-06bc-4133-de2b-7d42f525f7a4@durchholz.org>
- In-Reply-To
- <20170410165727.4lqtit5hkcxh32ew@sigill.intra.peff.net>
Am 10.04.2017 um 18:57 schrieb Jeff King:
Show 10 quoted lines
>>> 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.
Show 10 quoted lines
> 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.
Show 10 quoted lines
>> 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.