On 2005/01/22, at 0:02, Ilia Alshanetsky wrote:
Uhm yes. The purpose of the code is to trim characters from the
filename that could be interpreted as a path consequently leading to
all sorts of interesting vulnerabilities. Given that we have a
function for this very purpose called php_basename() it would only
seem logic to use it, instead of duplicating it's functionality
(incorrectly I might add).
What is really confusing around this stuff, is that php_basename() is
all but designed to work on the filesystem where the script is running,
while the filename coming from the outside isn't necessarily given in
the same scheme. For example, php_basename() is unlikely to handle a
filename encoded in a different encoding than the system's. In fact,
'\\' (0x5c) appears in the second byte of a Shift_JIS sequence, which
is the most commonly used multibyte encoding in Japan.
It's not just about a reinvention of the automobiles :)
Moriyoshi
Ilia
Moriyoshi Koizumi wrote:
On 2005/01/21, at 2:57, Ilia Alshanetsky wrote:
iliaa Thu Jan 20 12:57:42 2005 EDT
Modified files: (Branch: PHP_4_3)
/php-src NEWS
/php-src/main rfc1867.c
/php-src/ext/standard string.c
Log:
MFH: Fixed bug #31398 (When magic_guotes_gpc are enabled filenames
with '
get cutoff).
http://cvs.php.net/diff.php/php-src/NEWS?
r1=1.1247.2.810&r2=1.1247.2.811&ty=u
Index: php-src/NEWS
diff -u php-src/NEWS:1.1247.2.810 php-src/NEWS:1.1247.2.811
--- php-src/NEWS:1.1247.2.810 Wed Jan 19 20:43:19 2005
+++ php-src/NEWS Thu Jan 20 12:57:40 2005
@@ -19,6 +19,8 @@
- Fixed bug #31174 (compile warning in url.c). (Ilia, lukem at
NetBSD dot org)
- Fixed bug #31159 (COM object access is not working). (Wez)
- Fixed bug #31142 (imap_mail_compose() fails to generate correct
output). (Ilia)
+- Fixed bug #31398 (When magic_guotes_gpc are enabled filenames
with ' get cutoff).
+ (Ilia)
- Fixed bug #31120 (mssql_query returns false on successfull
inserts and
stored procedures). (Frank)
- Fixed bugs #31107, #31110, #31111 (Compile failure of
zend_strtod.c). (Jani)
http://cvs.php.net/diff.php/php-src/main/rfc1867.c?
r1=1.122.2.28&r2=1.122.2.29&ty=u
Index: php-src/main/rfc1867.c
diff -u php-src/main/rfc1867.c:1.122.2.28
php-src/main/rfc1867.c:1.122.2.29
--- php-src/main/rfc1867.c:1.122.2.28 Sat Nov 20 15:16:44 2004
+++ php-src/main/rfc1867.c Thu Jan 20 12:57:41 2005
@@ -16,7 +16,7 @@
| Jani Taskinen <[EMAIL PROTECTED]>
|
+--------------------------------------------------------------------
-- +
*/
-/* $Id: rfc1867.c,v 1.122.2.28 2004/11/20 20:16:44 sesser Exp $ */
+/* $Id: rfc1867.c,v 1.122.2.29 2005/01/20 17:57:41 iliaa Exp $ */
/*
* This product includes software developed by the Apache Group
@@ -31,6 +31,7 @@
#include "php_globals.h"
#include "php_variables.h"
#include "rfc1867.h"
+#include "ext/standard/php_string.h"
#undef DEBUG_FILE_UPLOAD
@@ -842,7 +843,7 @@
while (!multipart_buffer_eof(mbuff TSRMLS_CC))
{
char buff[FILLUNIT];
- char *cd=NULL,*param=NULL,*filename=NULL, *tmp=NULL;
+ char *cd=NULL,*param=NULL,*filename=NULL;
int blen=0, wlen=0;
zend_llist_clean(&header);
@@ -1064,30 +1065,13 @@
str_len = strlen(filename);
php_mb_gpc_encoding_converter(&filename,
&str_len, 1, NULL, NULL TSRMLS_CC);
}
- s = php_mb_strrchr(filename, '＼＼' TSRMLS_CC);
- if ((tmp = php_mb_strrchr(filename, '/' TSRMLS_CC))
> s) {
- s = tmp;
- }
num_vars--;
- } else {
- s = strrchr(filename, '＼＼');
- if ((tmp = strrchr(filename, '/')) > s) {
- s = tmp;
- }
- }
-#else
- s = strrchr(filename, '＼＼');
- if ((tmp = strrchr(filename, '/')) > s) {
- s = tmp;
}
#endif
- if (PG(magic_quotes_gpc)) {
- s = s ? s : filename;
- tmp = strrchr(s, '＼'');
- s = tmp > s ? tmp : s;
- tmp = strrchr(s, '"');
- s = tmp > s ? tmp : s;
- }
+ /* ensure that the uploaded file name only contains the
path */
+ s = php_basename(filename, strlen(filename), NULL, 0);
+ efree(filename);
+ filename = s;
Are you really sure how the removed part of code would work?
Obviously you broke something. Please clarify.
Moriyoshi
--
PHP CVS Mailing List (http://www.php.net/)
To unsubscribe, visit: http://www.php.net/unsub.php
