Zend Framework

Do not commit, patch under review.

2 notes:

1.)
getMimeType() should return the MimeType which is set. When there is no way to autodetect it, due to disabled extensions, it may return the given mimetype from the client. Returning NO mimetype at all is also problematic.

A better solution would be to add a option which allows to select if a fallback should be used or not. Default to not with notice. A notice could also be rised when the clients mimetype is returned.

2.)
The detection based on the extension which can be seen within the patch is MUCH MORE UNSECURE than just returning the users mimetype. A malcious user can easily change the extension of a file by simply renaming it. But it takes much more time to change the send mimetype (which is normally detected by the browser but could also be changed).

This patch itself is really a security problem!

The described is also the reason why no where, wether within Zend_File nor within Zend_Validate_File such a solution was added.

Bah, you are right, what was I thinking.

The current suggestion is not to detect based on filename, but rather to throw an exception. Do you think a notice is better? Also, what kind of option/fallback do you envision? The same thing (but opt-in) where it will use the $_FILES['type']?

-ralph

I would change the following within my component:

*) Add a new option which sets if the clients mimetype should be returned or not... defaults to false (not returned)
*) Add another option which selects if the users mimetype should be returned instead of the real (this could be useful in combination with the validator to detect attacks)... also defaults to false
*) Any of those Options to false and no extension available then simply return NULL (empty string)

Optionally:
*) Option to true return the users mimetype but also raises a notice
*) Another option could be added which omits the notice (for those using debuggers)

In any case, this change does not only effect getMimeType() but also all other places where $_FILES['type'] is returned like getFileInfo()

I Forgot:
The first option means "return real mimetype but when not detected return 'type'. This is the fallback. Option: "fallback"

The second option means "return ONLY 'type' and do not detect the real mimetype. This can be used for an attacker detection. Option: "typedetection" or "detection"

The third option means "don't throw notices". Useful for debuggers. Option: "disablenotices" or "notices"

A few things.

First, the mimetype from $_FILES can be easily spoofed - it take little time. You can do it using Zend_Http_Client in a few short lines of code taking little time, effort, or knowledge. This is why any reliance on $_FILES is inherently insecure - it offers exactly zero assurances.

Second, the method is public which raises an expectation that it can and will be used by end users. At present it performs its detection without any warning about the $_FILES reliance if it comes down to that. The main reason it's a worry is because, at first glance, it's an simple alternative to using the mimetype validators. This means that Debian 4.0, for example, which does not package the finfo extension or mime function is insecure out of the box if this method is relied on. You can add anything else not happy with finfo's record in PECL (prior to inclusion in 5.3 as core) or the mime function's deprecated state. Returning no mimetype is far better than returning one that's easily manipulated and can lead to holes of the bad variety.

The problem with notices is that they don't clearly demonstrate to end users that the behaviour is exceptional (only that their PHP version is somehow incomplete - wrong message!). All it will result in is broken code. Throwing an exception (it is exceptional!) with an appropriate warning message offers the benefit of allowing end users fallback themselves to an exceptional solution - you can find these anywhere running from custom magic file parsers to system commands. Developers have been dealing with $_FILES for a long time.

In a public facing method, the option to return from $_FILES is also questionable (even if opt-in). Is there a valid use case? Can't the user do their own $_FILES lookup if they need it that badly? I'm not completely against it - I simply don't see any value in it other than offering yet one more way for a developer to shoot themselves in the foot. $_FILES just cannot be trusted. If the option is needed internally - then use a protected method.

I am against erasing MimeType completly like described by Padraic.

Until now I was said from the devteam that Exceptions are not acceptable. In several components we used therefor user-notices instead of exceptions when we want to note the user of something.

Looking at what I described I see to reason where or why an exception should be raised... please read my description completly.

Related to $_FILES itself there are 2 things to mention:

*) There ARE usecases where someone could be in need of the infos from $_FILES. After the upload these infos would be erased. As said these infos could be used for other ways. And as described they are switched off per default... you have to turn them on by configuration to change this behaviour.

*) It should also be noted that we are speaking of the abstract file transfer adapter. This means that the abstract adapter itself does not rely on $_FILES or HTTP. The internal array has just the same naming. Changing these internal things can add massive problems to the other adapters I am working on. Erasing the $_FILES relation should be done within the HTTP Adapter and not within the abstract class.

New patch added.

Thomas: This is a security issue, plain and simple. There are three factors we need to consider.

• ZF must be secure by default. Having options to make the code less secure is a disservice to users, and should never occur.
• Users can catch exceptions and/or extend the filter if they need something that's less secure.
• Exceptions from filters and validators are not ideal; however, they are always allowed for exceptional circumstances. This is an exceptional circumstance.

With these in mind, it makes zero sense to have options that would lead to insecure code, even if they are not enabled by default. That said, Ralph and I are in agreement with you that the exception should only be raised from the HTTP adapter of Zend_File_Transfer – not the abstract adapter. This should allow you to keep the flexibility in behavior you need for other adapters, while still eliminating the security vector. Ralph has indicated he can create a patch that does exactly this, and will be posting it shortly.

Never suggested erasing getMimeType(), Thomas. My suggestion is a reflection of how the MimeType validators operate - throwing an exception when the mimetype cannot be safely detected. The use cases on needing information from $_FILES are mitigated by the very existence of the superglobal - using an array is not a problem for developers, so the fallback opt-in isn't necessary. As I stated, I'm not completely against it, but I would prefer there was a compelling use case for needing it. The default exception behaviour could be easily adapted in its absence allowing developers manually refer to $_FILES (making it their problem) if a No-Detect form of exception was caught. What I suggested (in case it's needed for the component - not the end user) was that if $_FILES use was needed internally to the component, it could be added as a protected method leaving the public one solely exhibiting the secure behaviour for userland code. This refers only to a $_FILES fallback - not the entire method which you seem to have misinterpreted as my goal.

Fixed in trunk at r20127.
Fixed in 1.9 at r20134.
Fixed in 1.8 at r20135.

Security fix in trunk with r20141

Notes:
The provided (and also discussed/agreed) solution

• makes ZF more secure
• does not use exceptions within getMimeType() (which I said to be unnecessary there)
• does add a option to make $_FILES unsecure again (which is necessary as the original data should be able to be logged)