# CVE-2008-5658 unfixed or new problem with Zip::extractTo in 5.2.x? - Thread: https://externals.io/message/42758 - Original archive: https://news-web.php.net/php.internals/42758 - Started by sean finney on 2009-01-21T21:57:50+00:00 - Messages: 6 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #42758 — sean finney — 2009-01-21T21:57:50+00:00 URL: https://externals.io/message/42758#42758 hi everyone, i'm looking for a sanity check here, as i've already lost more time than i'd like chasing ghosts on my treasure hunt through {bugs,lists,cvs}.php.net :( afaict, CVE-2008-5658[1] is only half-fixed on 5.2.8, while it was supposed to be fixed in 5.2.7. while the zip library no longer blindly extracts files such as "../../../var/www/index.php", it now seems to segfault on any files that have a leading "..". I've put some sample code illustrating my problem at[2]. am i on crack? a backtrace points to virtual_file_ex() returning an unchecked error in php_zip_extract_file(). it looks like there *might* have been a fix in the 5.3 branch, but it was surrounded by so much other noise that i'm not sure. i guess someone here knows better than me what's going on. it doesn't seem exploitable for more than a DoS at first glance, but i'll defer to the experts on that as well. sean [1] http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2008-5658 [2] http://people.debian.org/~seanius/php/security/ziptest.tgz --- ## #42760 — Pierre Joye — 2009-01-21T22:25:21+00:00 URL: https://externals.io/message/42758#42760 · In reply to: #42758 hi, > hi everyone, > > i'm looking for a sanity check here, as i've already lost more time than > i'd like chasing ghosts on my treasure hunt through {bugs,lists,cvs}.php.net :( > > afaict, CVE-2008-5658[1] is only half-fixed on 5.2.8, while it was supposed > to be fixed in 5.2.7. it is fixed in 5.2.7RC2 or RC3, see: http://cvs.php.net/viewvc.cgi/php-src/ext/zip/php_zip.c?r1=1.1.2.43&r2=1.1.2.44 > while the zip library no longer blindly extracts files such as > "../../../var/www/index.php", it now seems to segfault on any files > that have a leading "..". I've put some sample code illustrating my > problem at[2]. am i on crack? No idea, can you open a bug and post the backtrace, a zip data to reproduce the problem and a simple script please? Simply post the links you gave here. I will take a look at them as soon as possible. Thanks for the report! Cheers, -- Pierre http://blog.thepimp.net | http://www.libgd.org --- ## #42762 — Pierre Joye — 2009-01-21T23:54:38+00:00 URL: https://externals.io/message/42758#42762 · In reply to: #42760 re, I ran a quick test to solve this problem sooner rather than later (using only the crash.zip part): pierre@ubuntu:~/cvs/php53/bld$ ./sapi/cli/php ./ziptest.php opening 'bad' zipfile...ok. extracted. C:\Users\pierre\Documents\php-sdk\php53\vc9\x86\php53clean>Debug\php.exe ziptest.php opening 'bad' zipfile...ok. extracted. But it crashes in 5.2, it seems to be a problem in virtual_file_ex, it return an empty string instead of the expected path. Can you try the attached patch please? against 5.2. I backported the necessary functions from TSRM and removed what we do not use. It should fix the problem. Cheers, > hi, > > > > […] > > it is fixed in 5.2.7RC2 or RC3, see: > http://cvs.php.net/viewvc.cgi/php-src/ext/zip/php_zip.c?r1=1.1.2.43&r2=1.1.2.44 > > > […] > > No idea, can you open a bug and post the backtrace, a zip data to > reproduce the problem and a simple script please? Simply post the > links you gave here. I will take a look at them as soon as possible. > > Thanks for the report! > > Cheers, > -- > Pierre > > http://blog.thepimp.net | http://www.libgd.org > -- Pierre http://blog.thepimp.net | http://www.libgd.org --- ## #42765 — sean finney — 2009-01-22T07:12:09+00:00 URL: https://externals.io/message/42758#42765 · In reply to: #42760 hi pierre sorry, was already asleep when you came looking for me on IRC :) > it is fixed in 5.2.7RC2 or RC3, see: > http://cvs.php.net/viewvc.cgi/php-src/ext/zip/php_zip.c?r1=1.1.2.43&r2=1.1.2.44 FSVO "fixed" that includes segfaulting, anyway :) > No idea, can you open a bug and post the backtrace, a zip data to > reproduce the problem and a simple script please? Simply post the > links you gave here. I will take a look at them as soon as possible. http://bugs.php.net/47188 > But it crashes in 5.2, it seems to be a problem in virtual_file_ex, it > return an empty string instead of the expected path. /* Resolve path relatively to state and put the real path into state */ /* returns 0 for ok, 1 for error */ and it's returning 1 in this case, so it's an unhandled error, which is then also unhandled in php_zip_extract_file, as previously suggested. > Can you try the attached patch please? against 5.2. I backported the > necessary functions from TSRM and removed what we do not use. It > should fix the problem. sadly, i think there's been too much change in TSRM etc between 5.2<->5.3, so more functions would need to be backported afaict. maybe it'd be better to try and figure out why the existing virtual_file_ex doesn't like this filename, since it might affect other codepaths too? rangda[/home/sean/Desktop/php-5.2.8] make :) ... /bin/sh /home/sean/Desktop/php-5.2.8/libtool --silent --preserve-dup-deps --mode=compile gcc -Iext/zip/ -I/home/sean/Desktop/php-5.2.8/ext/zip/ -DPHP_ATOM_INC -I/home/sean/Desktop/php-5.2.8/include -I/home/sean/Desktop/php-5.2.8/main -I/home/sean/Desktop/php-5.2.8 -I/usr/include/libxml2 -I/home/sean/Desktop/php-5.2.8/ext/date/lib -I/home/sean/Desktop/php-5.2.8/TSRM -I/home/sean/Desktop/php-5.2.8/Zend -g -O0 -c /home/sean/Desktop/php-5.2.8/ext/zip/php_zip.c -o ext/zip/php_zip.lo /home/sean/Desktop/php-5.2.8/ext/zip/php_zip.c:175:39: error: macro "tsrm_do_alloca" passed 2 arguments, but takes just 1 /home/sean/Desktop/php-5.2.8/ext/zip/php_zip.c: In function 'php_zip_realpath_r': /home/sean/Desktop/php-5.2.8/ext/zip/php_zip.c:175: error: 'tsrm_do_alloca' undeclared (first use in this function) /home/sean/Desktop/php-5.2.8/ext/zip/php_zip.c:175: error: (Each undeclared identifier is reported only once /home/sean/Desktop/php-5.2.8/ext/zip/php_zip.c:175: error: for each function it appears in.) /home/sean/Desktop/php-5.2.8/ext/zip/php_zip.c:207:35: error: macro "tsrm_free_alloca" passed 2 arguments, but takes just 1 /home/sean/Desktop/php-5.2.8/ext/zip/php_zip.c:207: error: 'tsrm_free_alloca' undeclared (first use in this function) /home/sean/Desktop/php-5.2.8/ext/zip/php_zip.c:215:33: error: macro "tsrm_free_alloca" passed 2 arguments, but takes just 1 /home/sean/Desktop/php-5.2.8/ext/zip/php_zip.c: In function 'php_zip_extract_file': /home/sean/Desktop/php-5.2.8/ext/zip/php_zip.c:487: warning: passing argument 6 of 'php_basename' from incompatible pointer type make: *** [ext/zip/php_zip.lo] Error 1 rangda[/home/sean/Desktop/php-5.2.8] [2] :( sean --- ## #42796 — sean finney — 2009-01-23T07:23:59+00:00 URL: https://externals.io/message/42758#42796 · In reply to: #42762 hi pierre, i've tested the patch that you proposed via IRC (attached) and it seems to work for me against 5.2.8. passes valgrind too, without any detected errors or leaks. it's unfortunate that there isn't a more surgical fix (301 insertions!), but i'll take your word for it that it would be too complicated/dangerous to try and modify virtual_file_ex() directly. thanks! sean --- ## #42797 — sean finney — 2009-01-23T08:06:28+00:00 URL: https://externals.io/message/42758#42797 · In reply to: #42796 hi again, > it's unfortunate that there isn't a more surgical fix (301 insertions!), > but i'll take your word for it that it would be too complicated/dangerous > to try and modify virtual_file_ex() directly. actually, i think i've found a slightly more graceful workaround :) since virtual_file_ex is to fragile to be changed, here's a patch that does the following as a workaround: - take a temporary copy of the filename - replace all instances of "^../", "/../", and "/..$" with "///". - pass this mangled filename to virtual_file_ex for normalization it seems virtual_file_ex can handle such a filename without problem, and with proper formatting the current patch only inserts 22 lines to php_zip.c. someone should probably double check this code for early-morning coding errors though :) what do you think? sean