PEAK XOOPS - Protector protects from a legitimate upload. in englishin japanese

Protector protects from a legitimate upload.

  • You cannot open a new topic into this forum
  • Guests cannot post into this forum
Previous post - Next post | Parent - Children.1 .2 .3 .4 .5 .6 .7 .8 .9 | Posted on 2007/8/27 0:46 | Last modified
vaughan  ¾åÅùʼ   Posts: 37
in response to this post on smartfactory

when users are uploading files with wf-downloads that contain multiple periods(.) in the filename, protector module stops the submission.

i modified wfd uploader to change the periods(.) into underscores upon submission.

in protector 3.01, the above change worked and allowed the submissions to continue.

in protector 3.13 though, this does not work and protector module is stopping the download.

is there any solution to this at all?
Votes:42 Average:7.38
Previous post - Next post | Parent - No child | Posted on 2007/8/28 12:35
GIJOE  ÀèǤ·³Áâ   Posts: 4110
hi vaughan.

Quote:

i modified wfd uploader to change the periods(.) into underscores upon submission.

in protector 3.01, the above change worked and allowed the submissions to continue.

in protector 3.13 though, this does not work and protector module is stopping the download.
I cannot believe that.
There are no differences between 3.01 and 3.13 about checking "upload ($_FILES)".

And I believe protector never stops "download".

Anyway, If you have a confidence treating $_FILES, define

'PROTECTOR_SKIP_FILESCHECKER'

before including mainfile.php.



If you use this definition, promise it.

- NEVER judge by $_FILES[]['type']
- NEVER use $_FILES[]['name']
Votes:18 Average:5.56
Previous post - Next post | Parent - Children.1 | Posted on 2007/8/29 3:48
vaughan  ¾åÅùʼ   Posts: 37
sorry, what i meant to say was protector stops the submission of the download when the file with multiple dots is uploaded.

3.01 stopped the upload until i modified the uploader class to change dots to underscores.

but when i upgraded to 3.13, protector stops the upload, also have a report that 3.04 prevents the upload too. I don't know why 3.01 worked after modifiying uploader class but later versions don't.

what do i define 'PROTECTOR_SKIP_FILESCHECKER' with? I am a little confused by your wording.

do i define('PROTECTOR_SKIP_FILESCHECKER', true); or define('PROTECTOR_SKIP_FILESCHECKER', 'wfdownloads');

the uploaded files are all given unique filenames on upload & can be stored outside the webroot.
Votes:19 Average:5.79
Previous post - Next post | Parent - Children.1 .2 | Posted on 2007/8/29 4:30 | Last modified
Dave_L  ¾åÅùʼ From: Virginia, USA  Posts: 35
Are you talking about filenames with consecutive periods (../foo/bar.txt) or non-consecutive periods (foo.tar.gz)?

I think the latter (non-consecutive periods) is blocked if the "Exit if bad files are uploaded" setting in the Protector module preferences is enabled. This is implemented in Protector::check_uploaded_files() in trust_path/modules/protector/class/protector.php.

I have this setting disabled.

The former (consecutive periods) is blocked if the setting "Protection from Directroy Traversals" is enabled. This is implemented in function protector_prepare() in trust_path/modules/protector/include/precheck.inc.php.

This applies to Protector 3.04. I haven't upgraded to 3.1 yet.
Votes:23 Average:4.35
Previous post - Next post | Parent - No child | Posted on 2007/8/29 5:32 | Last modified
GIJOE  ÀèǤ·³Áâ   Posts: 4110
which do you mean "you" ?

Perhaps, "directory traversal" would be irrelevant with this issue.

Quote:

Dave_L wrotes:
Are you talking about filenames with consecutive periods (../foo/bar.txt) or non-consecutive periods (foo.tar.gz)?
"tar.gz" is the only exception. (treated as .tgz innerly)
You can upload foo.tar.gz with the setting on.

Quote:
I think the latter (non-consecutive periods) is blocked if the "Exit if bad files are uploaded" setting in the Protector module preferences is enabled. This is implemented in Protector::check_uploaded_files() in trust_path/modules/protector/class/protector.php.

I have this setting disabled.

I don't recommend "disable the setting".
There are some modules with the vulnerability about multiple dot. (Just checking by a simple black list...)

You know these files can be treated as parsable PHP with apache.

foo.php.bar
foo.php.en.anonymous

Thus, Protector inhibits such extensions in $_FILES[]['name']

Of course, all modules should ignore $_FILES[]['name'] if the file is placed under DocumentRoot.
And you can turn this off when all modules are free from such vulnerability.

However, when I read some major modules released from xoops.org some months ago, there are NO modules without the vulnerability...
Votes:12 Average:4.17
Previous post - Next post | Parent - No child | Posted on 2007/8/29 5:51
GIJOE  ÀèǤ·³Áâ   Posts: 4110
Quote:

vaughan wrotes:
but when i upgraded to 3.13, protector stops the upload, also have a report that 3.04 prevents the upload too. I don't know why 3.01 worked after modifiying uploader class but later versions don't.
Though I cannnot understand it too, This can be just a "setting" issue.

Quote:
what do i define 'PROTECTOR_SKIP_FILESCHECKER' with? I am a little confused by your wording.

the head of controllers:
define( 'PROTECTOR_SKIP_FILESCHECKER' , true ) ;
require '../../mainfile.php' ;

Quote:
the uploaded files are all given unique filenames on upload & can be stored outside the webroot.
It sounds good!
Votes:14 Average:5.00
Previous post - Next post | Parent - Children.1 | Posted on 2007/8/29 8:44
vaughan  ¾åÅùʼ   Posts: 37
Quote:
Are you talking about filenames with consecutive periods (../foo/bar.txt) or non-consecutive periods (foo.tar.gz)?

no i mean filenames with more than 1 dot.

ie. v.a.u.g.h.a.n.zip

a bit extreme example.

after modifying uploader, the filename is changed to wfd_<uniqid hash>--v_a_u_g_h_a_n.zip

then when the file is retrieved the filename is restored to just v_a_u_g_h_a_n.zip before it's sent to the browser, so the filename on the server is never known or can be seen by users, altho if you select the upload location outside of webroot, it's even securer.


with regards to $_FILES[]['name'];

the submit form uses $_FILES['userfile']['name'] & $_FILES['userfile']['type']

however if you think that is an insecure method, how could we get this done a different way without using $_FILES['userfile]['name']

i am not a fully competent php coder, i only know basics, but can follow the logic somewhat in most cases.

@gijoe, thanks for the clarification with the define value. could use this in the meantime, but if we can make the upload process more secure & better then that would be the ultimate goal.
Votes:13 Average:6.15
Previous post - Next post | Parent - No child | Posted on 2007/8/31 4:30
GIJOE  ÀèǤ·³Áâ   Posts: 4110
Quote:

vaughan wrotes:
the submit form uses $_FILES['userfile']['name'] & $_FILES['userfile']['type']
You should know $_FILES[]['type'] has non-sense at all.

And if you dare to use $_FILES[]['name'], filter it by white list like
preg_replace( '/[^0-9a-zA-Z_-]/' , '' , $_FILES[]['name'] ) ;

And its extension should be set by file contents (check by getimagesize()).

If the file is not a image, don't place it under DocumentRoot.
Votes:1 Average:10.00
Previous post - Next post | Parent - No child | Posted on 2007/9/1 4:15
suico  ÆóÅùʼ   Posts: 7
Hi GIJOE, how do you recomend to upload files ? Checking if there are dots in its name? making a new name ? NO way to make it secure? Can you gimme a hint, I am making a module wich uses xoops upload object. Is it secure to use it? Do I have to check onlymimetypes? IS it secure to check the extensions and the mymetype ?

Sorry for all these questions but I am still learning.
Votes:1 Average:10.00
Previous post - Next post | Parent - Children.1 | Posted on 2007/9/4 17:59
Dave_L  ¾åÅùʼ From: Virginia, USA  Posts: 35
Here's how I use the XOOPS upload object:

Only allow known safe file extensions. The mimetype is not really checked; XOOPS uses the mimetype only as a way of keeping track of extensions, and for use by the downloader to provide a content-type header.

Store uploaded files in a subdirectory (or subdirectories) that is not accessible by a web browser, either outside the web root or protected by an .htaccess file.

Store the uploaded file using a generated numeric filename (1.doc, 2.pdf, etc.). The original filename is stored in the database for use by the downloader.
Votes:1 Average:10.00
Previous post - Next post | Parent - No child | Posted on 2007/9/4 20:11
suico  ÆóÅùʼ   Posts: 7
Hi Dave. I am working with images , and I have made some tests here. The upload does check something as if I rename a gif and try to send when I state it is a jpg mimetype it returns me saying it is a suspicious file. I also tryed with txt renamed to jpg and also returned me an error. My question is: is that enough? The upload checks also for multiple extensions as I could see in the method sanitizeMultipleExtensions wich is called when we call the upload method. Are those precautions enough?
Thanks .
Votes:1 Average:10.00
Previous post - Next post | Parent - No child | Posted on 2007/9/5 12:34 | Last modified
GIJOE  ÀèǤ·³Áâ   Posts: 4110
When I had read the class of media uploader in the core from xoops.org 1~2 years ago, it didnot look well-designed.

Checking MIME is non-sense at all.
It's just a member of user's post.

Even image files you have to check their contents.
(by getimagesize() etc.)

There is a major and bad browser named "Microsoft Internet Explorer".
Since the idiot browser ignores the header of "Content-Type", storing HTML as gif makes XSS.

Of course, attackers can post HTML file as ".gif" and "image/gif".
Votes:17 Average:10.00
Previous post - Next post | Parent - Children.1 | Posted on 2007/9/6 2:44
suico  ÆóÅùʼ   Posts: 7
OK GIJOE , and what about the php extension fileinfo? Is it safe enough? Of course I'll need to verify in my module if the server has it to use and if it hasn't don't check this way. But can a user con me with this?

Thanks for the explanations
Votes:1 Average:10.00
Previous post - Next post | Parent - No child | Posted on 2007/9/6 4:03
GIJOE  ÀèǤ·³Áâ   Posts: 4110
Easy to answer.
Just store files with allowed extensions inside DocumentRoot.
And never allow multiple dots.

Blacklist will be non-sense.
Only Both checking by whitelist of extensions(.gif/.png/.jpg/.jpeg) and their contents are meaningful.

.xls, .doc, .pdf...
They should not be stored inside DocumentRoot.
Because there are no means to checking the contents.

If you want to put a php source file, use outside of DocumentRoot or make file name hash like pukiwiki.


XOOPS_TRUST_PATH must be useful such uploading situation.

XOOPS_TRUST_PATH/uploads/
Votes:1 Average:10.00
Previous post - Next post | Parent - Children.1 | Posted on 2007/9/21 3:41
vaughan  ¾åÅùʼ   Posts: 37
sorry, been busy so only just got back to this thread.

Quote:
You should know $_FILES[]['type'] has non-sense at all.

And if you dare to use $_FILES[]['name'], filter it by white list like
preg_replace( '/[^0-9a-zA-Z_-]/' , '' , $_FILES[]['name'] ) ;

And its extension should be set by file contents (check by getimagesize()).

If the file is not a image, don't place it under DocumentRoot.

I understand, and from looking at our uploader class (we don't use xoops uploader class, we use a different class for wfdownloads) the $_FILES['filename']['name'] is sanitized with MyTextSanitizer::stripSlashesGPC($_FILES[$media_name]['name'])

which I presume is safe(r);

we leave the choice of whether the user wants to store the uploads outside of the DocumentRoot to the user, but the option is there for the user to upload to whatever path they choose whether it is inside the DocumentRoot or outside the DocumentRoot, they just have to set the path in preferences.

the filename on the server is stored using uniqid. for example:

$this->mediaName = MyTextSanitizer::stripSlashesGPC($_FILES[$media_name]['name'][$index]);
            if ($this->randomfilename) {
                $unique = uniqid(time());
                $this->mediaName = 'wfd_'.$unique.'--'.$this->mediaName;
            }

and on user choosing the download, the filename is changed to it's proper name before it is sent to the browser, so both the filename & the upload path are never shown to the user at all.

the file is also checked with getimagesize() so i think we are thinking along the same lines? i hope.
Votes:2 Average:5.00
Previous post - Next post | Parent - No child | Posted on 2007/9/22 17:34
GIJOE  ÀèǤ·³Áâ   Posts: 4110
Quote:

vaughan wrotes:
I understand, and from looking at our uploader class (we don't use xoops uploader class, we use a different class for wfdownloads) the $_FILES['filename']['name'] is sanitized with MyTextSanitizer::stripSlashesGPC($_FILES[$media_name]['name'])

which I presume is safe(r);
Is this a joke?
The code just removes wrong effect from magic_quote_gpc.
It's far from "Sanitizing".

Quote:
we leave the choice of whether the user wants to store the uploads outside of the DocumentRoot to the user, but the option is there for the user to upload to whatever path they choose whether it is inside the DocumentRoot or outside the DocumentRoot, they just have to set the path in preferences.
XOOPS_TRUST_PATH/uploads/ is.


Anyway, I cannot check the class because I'll never use it.
(I don't have such spare time)
Votes:1 Average:10.00
Previous post - Next post | Parent - Children.1 | Posted on 2007/9/28 1:38 | Last modified
vaughan  ¾åÅùʼ   Posts: 37
ok so from the information you have given so far.

i'm presuming this should be better than method used at moment >

$index = intval($index);
$this->mediaName = MyTextSanitizer::stripSlashesGPC($_FILES[$media_name]['name'][$index]);
$this->mediaName = preg_replace( '/[^0-9a-zA-Z_-]/' , '' , $this->mediaName);
Votes:1 Average:10.00
Previous post - Next post | Parent - No child | Posted on 2007/9/28 3:09
GIJOE  ÀèǤ·³Áâ   Posts: 4110
OK.
This looks good code.
Votes:1 Average:10.00

  Advanced search


Login
Username or e-mail:

Password:

Remember Me

Lost Password?

Register now!