Thomas Schoepf wrote: > I've uploaded an update to version 4.4-2 of unzoo which is already in sarge > that fixes the recently discovered directory traversal bug. > > My patch can be found in Bugreport #306164 ( > http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=306164 ). The patch has > been sent to the bts about half an hour ago, there's one older patch that is > irrelevant now. > > What actions need to be taken to get unzoo_4.4-3 into sarge? > Where can I help?
Your post is sufficient to get it reviewed by a member of the release
team.
+ char patu [sizeof(Entry.diru) + sizeof(Entry.namu) + 1];
+ strcpy( patu, Entry.diru );
+ if ( strlen(patu) && patu[strlen(patu)-1] != '/') strcat( patu,
"/" );
+ strcat( patu, (Entry.lnamu ? Entry.namu : Entry.nams) );
This has an off-by-one error; I don't know if it's exploitable or even a
problem, but if Entry.diru does not end with a trailing slash,the "+ 1"
does not add enough space for both the trailing slash and terminating NULL.
Also, if Entry.nams can be longer than Entry.namu, then the last line will
overflow the buffer.
+ q = patu;
+
+ while ( !strncmp(q, "/../", 4) ) {
+ q += 3;
+ }
This seems to be buggy. The code is supposed to be finding the first
occurance of "/../" in patu, to remove it. But follow through the while
loop with a patu of "foo/bar/../baz" and see what happens to q:
q = "foo/bar/../baz"
q = "/bar/../baz" /* loop */
q = "r/../baz" /* loop */
q = "./baz" /* loop */
So it's prone to failure if the /../ is not positioned starting at a
multiple of 3 characters into patu. I don't see the point of coding
the check like this at all; it should just use strstr.
+ if (q[0] == '/') q++;
+
+ while ((p = strstr( q, "/../" )) != NULL) {
If q is "/../" it increments q and then the while loop fails.
+ /* patu contains the sanitized 'universal' path, i.e. */
+ /* separated by '/' characters, including the file name. */
+
+ char *f = strrchr( patu, '/' );
+ *f++ = '\0';
+ /* Now, patu points to the directory part, f to the file */
+
+ strcpy( Entry.diru, patu );
+ strcpy( (Entry.lnamu ? Entry.namu : Entry.nams), f );
I haven't bothered to find a way to exploit it, but this strikes
me as extremely dangerous. patu has been subject to various tranformations
in the earlier code, that try to sanitise it, and now we blithely lop
off a directory and a filename part and just assume that they're
both smaller or equal size than they were in the beginning. If this
or any of the earlier code gets it wrong, there's a buffer overrun.
--
see shy jo
signature.asc
Description: Digital signature
