-
Notifications
You must be signed in to change notification settings - Fork 11.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[5.4] Prevent PHP file extensions uploads by default unless explicitly allowed #20392
Conversation
return $value->getPath() != '' && | ||
(in_array($value->getMimeType(), $parameters) || | ||
in_array(explode('/', $value->getMimeType())[0].'/*', $parameters)); | ||
} | ||
|
||
/** | ||
* Check if we have explicity allowed a PHP upload, and check accordingly. | ||
* | ||
* @param string $filename |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
phpdoc is not correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks - fixed now.
@laurencei this seems great, however, what about edge cases where other than .php files are processed as PHP? Legacy systems with .php5 extensions come to mind. |
If they are that legacy then they can probably no longer run Laravel? 😄 I could add it - but I'm not sure. I'll see if Taylor likes this concept at all, and he can decide. |
I'm not sure I really see this as a framework issue if someone is calling system($file)? |
@taylorotwell - I'll send you an email with how the exploit could work. edit: email sent to [email protected] |
Would it be appropriate to include a default |
Are you going to backport this to 5.1/5.2/5.3? |
This is the "security vulnerability" I reported on e-mail about 1,5 month ago Taylor, where we couldn't come up with a good way to fix it. I'll probably still instruct my developers to always write another explicit in_array() check of the extension before manipulating files. We are working with some semi-big clients and like to be verbose rather than sorry, for the exact reason of this fix. This still applies to lots of other formats and I've always found the inspection of byte patterns in the file uploaded problematic. I can understand trying to guess the real file type and not rely on extensions, as Linux and Mac etc. kan easily have files that the user uses normally without an extension if they messed up the naming. Now we are basically building a blacklist of file extensions, but regarding security we should really be whitelisting. What if we made a list of "known" extensions of the different mimes, and threw and exception on getClientOriginalExtension() if it didn't match any of the ones the user tried to filter by? Possibly even make an getClientOriginalReallyUnsafeExtensionBeSureAboutWhatYouAreDoing() function for the original functionality. (Semi serious about the naming). |
A general approach could be that the file extension needs to match the expected MIME. i.e. if you upload a file that has a MIME of Same goes in reverse. If you upload a So its a double level protection in addition to what exists now. We dont rely on the MIME or the extension - we rely on both, and force both to match. Could be called |
@barryvdh - I had a quick look. The code would be the same, but it would need to be another PR, as the files are different (they were refactored for 5.4 I think). I'm not able to do it right now as I'm about to go out for a while - so if someone else wants to do it? (And if Taylor agrees to the backport obviously). |
I think you should probably always want that, unless you explicitly disable it. Never allowing php is a start, but perhaps there are other scripts that could be configured to execute. And not just images, but what if someone modifies a pdf to pass mime type? |
If the webserver is correctly setup - then only a That's why we explicitly block |
Yeah I was thinking about python (.py) etc, but I guess you're right that it's mostly just PHP we need to worry about, with default configs. Doesn't Symfony have the same problem with their Mime validation, or do they fix it differently? Can't we use that/ |
AFAIK - they suffer the same issue - as the problem is with MIME's themselves - not Symfony or PHP. But this is starting to get beyond my knowledge on the topic - so take that with a grain of salt... maybe someone else knows more they can join in... |
@barryvdh most Nginx configurations only check the .php extension and then pass the script to an fpm instance. Apache probably checks mime and extension when using mod_php. This makes me wonder, as I mentioned in the original thread: Wouldn't it be smarter to leave the image rule as it was and instead add better validation for mimes? I'm not sure if PHP's mime handling allows this, so maybe adding an "extension" validation rule would be a nice thing. "image" rule could then validate mimes and extensions which would were whitelisted, instead of blacklisting .php. |
Ok - backports done. |
You should also block Ideally it could be |
@vlakoff - we already do a case insensitive check with |
Not in the |
Yes - because the array part is our definition for a mime allowing php - not the users. And a correctly configured server should only run |
Yup. Some shared hostings actually run .php7 and .php5 (or other versions for whatever reason), but I think that's a borderline nonexistent minority. I would however like to hear about what people thing about my last comment, considering whitelisting instead of blacklisting (blacklisting seems like a tempfix). @taylorotwell if you'd be okay using mime+extension for validation, I'd be okay with creating the PR, the only issue that comes in mind is people uploading images with non-standard or empty extensions. |
@Numline1 - I was thinking of maybe making |
Not sure if I mentioned this before, but perhaps making an "extension" rule, which then could be used separately or aliased in "image" rule would be nice, although I'm not sure if combining rules is a good idea (how would "image" with another separate "extension" rule behave in form request if "extension" rule was already used in "image" in the framework code?). |
This is an attempt to fix #20390, #18120 and #18299
Basically during MIME validations we now block any file that ends in
.php
unless you have explicitly allowed a php MIME. This solves the image spoofing issues, where you can make a php file pass the image validation MIME.Seems like a sensible default, and if someone is brave enough to want to allow a
.php
file they still can by explicitly setting it. It doesnt solve general malicious uploads (like a badpdf
file or something) - but I think that.php
extensions are a special case, since it can result in remote execution on the server.Should be backwards compatible - on the basis it is unlikely anyone wanted to allow a
.php
file in the first place?Before:
'picture' => 'required|image
fake.php
could pass in some situations'picture' => 'required|mimes:gif
fake.php
could pass in some situations'picture' => 'required|mimes:gif,php
fake.php
will passAfter:
'picture' => 'required|image
fake.php
can no longer pass at all'picture' => 'required|mimes:gif
fake.php
can no longer pass at all'picture' => 'required|mimes:gif,php
fake.php
will passNote:
'picture' => 'required|file
fake.php
will still pass before/after.