strcpy()
FFS.
A chap who found two serious security bugs in Git servers and clients has urged people to patch their software. The flaws are present in Git including the 2.x, 1.9 and 1.7 branches, meaning the vulnerabilities have been lurking in the open-source version control tool for years. It is possible these two programming blunders …
I'd even have said, Linus should let go another volley of insults aimed at some poor developer, but well, he himself did not notice the issues, even though he did patch that very function.
https://github.com/git/git/commit/cf2ab916afa4231f7e9db31796e7c0f712ff6ad1
I'm not expert in these matters, but how would someone go about finding this exploit if they DIDNT have access to the source code (i.e. grep -R strcpy *). To me, this seems almost impossible to figure out without source code access, but clearly people find stuff in closed source code. How?
The function looks like a pretty hard-coded way of performing what should have been a routine task — concatenating strings.
I though good coding included packaging routine tasks into reusable functions or methods, so that we can move on from debugging the umpteenth incarnation of the same code.
Exactly, there's no excuse for us to still be living in a world where C code is written like this. This is some kind of Path.join() or File.join() functionality that is handled in one place by modern languages/libraries. Rewriting this kind of crap, even with safe functions like snprintf(), is absurd in this day and age.
> This is some kind of Path.join() or File.join() functionality that is handled in one place by modern languages/libraries
Exactly. And those library function implementations may potentially suffer from the same or other vulnerabilities. Just because it's part of the language / standard lib it doesn't make it intrinsically safe. Look elsewhere for the article on Visual Basic's date handling for an example.
In this situation, the space for the final file name is computed using "strlen", so it seems to me that copying the file name into that space using "strcpy" is valid. Both go until a NUL byte is found, and so operate consistently. Am I missing something?
The issue with using signed ints is valid. However, on today's 64 bit systems you are not going to be able to allocate enough virtual memory to hit the problem. A Google search tells me that non-standard "xmalloc" will abort if it cannot allocate the needed memory, so there isn't a hole there.
Again, what am I missing here?
So far as I can tell, the allocation of (lengfh+1) bytes for the leafname is fine, as is using strcpy() for that. And yes, size_t or unsigned int should be used.
As for what you're missing: after each addition, check that the new value is greater than the previous value; if not, bail out, raise the alarm, panic, abort() or something. That said, if the lengths are guaranteed to have been checked at the point at which the pathname list was constructed, you can probably get away without that – however, belt and braces…
Stratic analysis tools would raise a bunch of issues here.
strlen returns a size_t, which is unsigned. Mixing signed and unsigned with an inequality if statement is a big red flag.
I agree with an earlier point, concating strings and general path manipulation should be a solved problem.
It turns out to be ridiculously easy to write a poor man's Git.
vi pmgit
i
#!/bin/ksh
ls ${1}*.old | sort -r > filelist
while read myfile
do
cp myfile myfile.old
done < myfile
cp $1 $1.old
(esc)
ZZ
chmod 777 pmgit
Then, as needed: pmgit [filename]
Job done.
I was going to add code to make it work in different directories but then I thought "why not just put a copy of pmgit in every directory on the computer and be done?"
Yes, the irony of it - Cygwin source code is git-hosted, but their copy of git is version 2.7.0 without any of the fixes mentioned in the article:
https://cygwin.com/packages/x86_64/git/
Gitlab's restriction to a minimum of 2.7.3 is a little harsh when a) 2.8 will contain one of the fixes (so another raise of the min version then?) and b) Cygwin git is languishing at 2.7.0.
On a similar note, CentOS 7 git is actually way back at version 1.8.3.1 (with backported fixes) - does the latest Gitlab lock out CentOS 7 users then? One trick is to rebuild the Fedora Rawhide 2.7.3 src.rpm on CentOS 7 and use it to replace 1.8.3.1 (though it means you have to track Rawhide for updates...).
This post has been deleted by its author