[#36695] - Updated MediaHelper::canUpload function
- Closed
- 15 Jan 2022
- Medium
- Build: 4.1-dev
- # 36695
- Diff
- akshitrattan:Fixed-36674
User tests: Successful: Unsuccessful:
Pull Request for Issue #36674.
Summary of Changes
In libraries/src/Helper/MediaHelper
changed
if (count($allowedExecutables)) { $executable = array_diff($executables, $allowedExecutables); }
to
if (count($allowedExecutables)) { $executables = array_diff($executables, $allowedExecutables); }
to allow only some permitted executable file type to be uploaded in the media tab
Testing Instructions
In the media tab, try uploading executable file types that are not permitted by the cms
for reference,
/** * A special list of blocked executable extensions, skipping executables that are * typically executable in the webserver context as those are fetched from * Joomla\CMS\Filter\InputFilter * * @var string[] * @since 4.0.0 */ public const EXECUTABLES = array( 'js', 'exe', 'dll', 'go', 'ade', 'adp', 'bat', 'chm', 'cmd', 'com', 'cpl', 'hta', 'ins', 'isp', 'jse', 'lib', 'mde', 'msc', 'msp', 'mst', 'pif', 'scr', 'sct', 'shb', 'sys', 'vb', 'vbe', 'vbs', 'vxd', 'wsc', 'wsf', 'wsh', 'html', 'htm', 'msi' );
if you are unable to execute/run the files, the test is successful.
Actual result BEFORE applying this Pull Request
Expected result AFTER applying this Pull Request
Documentation Changes Required
No major Changes
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Libraries |
if you are unable to do so, the test is successful.
thats not a valid test of the function
if you are unable to do so, the test is successful.
thats not a valid test of the function
I editted the comment, could you cross-check??
if you are unable to do so, the test is successful.
thats not a valid test of the function
I editted the comment, could you cross-check??
@akshitrattan Now it's even more wrong because it is not about executing the uploaded files.
if you are unable to do so, the test is successful.
thats not a valid test of the function
I editted the comment, could you cross-check??
@akshitrattan Now it's even more wrong because it is not about executing the uploaded files.
I'm so sorry, could you pls help me in editting it??
@richard67 this really should not be discussed on the public tracker. If its a valid issue (I'm not sure it is) then it absolutely must be handled by the security team.
I could explain more about this but the public tracker is not the place. The code comments should explain enough.
@brianteeman Agree, we are already discussing.
@brianteeman Regarding security we are safe here because the error makes the allowed executables feature fail so that they are still not allowed.
- I can't find any values for $allowedExecutables
- From the original PR I would have thought it would have been $allowedExtensions
- Have you actually verified that a bug even exists
- I can't find any values for $allowedExecutables
The $allowedExecutables can be passed as optional parameters to the checkFileExtension and the canUpload functions. Obviously in the CMS core that's not used yet, and unfortunately it seems that this parameter has been forgotten in the doc block of these functions.
- From the original PR I would have thought it would have been $allowedExtensions
@brianteeman No, the $allowedExtensions is something else, and if we do an array diff with those, we will make the check for the executables break.
- Have you actually verified that a bug even exists
Just check the code of the complete file. There is nowhere any use of the $executable (singular) variable, so it MUST be a typo.
And compare the usage of the parameter in the 2 mentioned functions.
For a test one could add some PHP code with a call to the canUpload function to the index.php of Atum or Cassiopeia with the parameter being used, and verify the results without and with the patch.
Because security spent some time figuring out what was going on here (and determined it to not be an issue). I've chosen to directly merge this on review for the RC anyhow - because it's clearly a correct typo fix I've agreed with them to merge this for 4.0.6 as I was building the final packages as we speak.
So this has been merged to 4.0-dev with 4e4f596 thankyou @akshitrattan !
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2022-01-15 16:59:28 |
| Closed_By | ⇒ | wilsonge | |
| Labels |
Added:
?
|
||
Thank you
Thank you everyone.
@bembelimen The error exists already in the 4.0-dev branch. Shall we go on with this PR here for 4.1-dev, or shall it be fixed in 4.0-dev and then be merged up into 4.10-dev?