[3+] com_joomlaupdate allows upload of any filetype

[PhilETaylor]

Steps to reproduce the issue

In Joomla 3+ go to com_joomlaupdate ->Upload & Update tab

Select a PNG file, or XML file or any-other-file-type

Expected result

Validation error - its not a zip file.

Actual result

The file is uploaded to the tmp path, renamed and prefixed with 'ju

The file extension and mime type is not validated first.

Then you get a login screen.

Additional comments

Tested Joomla 3.9.19 and Joomla 4 beta 1
Cant think of anyway to exploit this so posting publicly.

[richard67]

How far shall the validation go? Shall it just check for the extension, e.g. ".zip"? I ask because one could be so funny and rename the PNG file or whatever he or she tries to upload to a ZIP file, i.e. with chagning the extension.

[richard67]

And I have an answer (42) and have forgotten the problem for which it was ;-)

[brianteeman]

this should be reported to @joomla/security

[SniperSister]

I agree with @PhilETaylor that I can hardly see an attack vector here, however, some proper error handling (at least validating the file type) makes sense to provide better feedback for users.

[richard67]

@SniperSister What do you mean with file type? The file name extension? You know it’s not the same. I think we only can check for the extension.

[richard67]

Does the extensions installer have the same problem? I can’t check that now because I’m on the road. Maybe someone else can do that faster?

[mbabker]

File type detection should NEVER rely on the given file name. You always analyze the file data.

[richard67]

Right ... and I found out meanwhile that it can be done.

If someone wants to work on it, let us know here so we are not working in parallel.

I'n not sure if I can do something, but if so, then not before weekend.

[richard67]

@mbabker I assume you mean with "analyze the file data" a MIME type check with functions like "mime_content_type" or "finfo_open" and "finfo_file", is that right? And do you really think it is necessary for the issue here? As far as I understand it, it is not a security issue here because it needs a super admin anyway to be logged in to backend. I understand it more as a check being needed to prevent uploading of an accidently selected wrong file.

@PhilETaylor Is my understanding right that it's not a security but an UX issue?

@SniperSister @zero-24 What's your opinion? Does it need the mime type check, too, or is a check of the file name before the upload in JS sufficient? I'd implement the check at least in J4 in the same way as I did for the maximum upload size with PR #27570 , i.e. as soon as the user selects a wrong file, a warning box is shown, and when he or she tries to upload it anyway, an alert is shown, and upload is rejected.

[zero-24]

I personally (not speaking in any official name of Production, maintainers or JSST) would implement an mine type check given that IIRC only zip files can be used anyway at this place. What we might have to check is whether there can be issues with mine type detection on different hosting configurations.

[richard67]

I personally (not speaking in any official name of Production, maintainers or JSST) would implement an mine type check given that IIRC only zip files can be used anyway at this place.

Your personal opinion is welcome.

What we might have to check is whether there can be issues with mine type detection on different hosting configurations.

In the MediaHelper we use for things which are not pictures either "mime_content_type" or "finfo_open" and "finfo_file", depending on what's available, and we check it in this order. "mime_content_type" should be available at PHP version >= 4.3, including 7.x, and doesn't require an extra module, while the "finfo_..." functions are available at PHP versions >= 5.3, including 7.x, and they require PECL fileinfo. The "mime_content_type" is not marked as deprecated in PHP docs, but I did read something somwehere that this might be soon the case. So I am almost but not 100% sure it should work everywhere. But if it fails due to missing functions, and we decide to refuse the upload in that case, then it would block people behind firewalls who can't use the Live Update from updating forever.

[zero-24]

I personally (not speaking in any official name of Production, maintainers or JSST) would implement an mine type check given that IIRC only zip files can be used anyway at this place.

Your personal opinion is welcome.

Sure just wanted to make this clear as at some places my opinions has been interpreted as opinion of any of my positions or teams that i'm involved in ;-)

What we might have to check is whether there can be issues with mine type detection on different hosting configurations.

In the MediaHelper we use for things which are not pictures either "mime_content_type" or "finfo_open" and "finfo_file", depending on what's available, and we check it in this order. "mime_content_type" should be available at PHP version >= 4.3, including 7.x, and doesn't require an extra module, while the "finfo_..." functions are available at PHP versions >= 5.3, including 7.x, and they require PECL fileinfo. The "mime_content_type" is not marked as deprecated in PHP docs, but I did read something somwehere that this might be soon the case. So I am almost but not 100% sure it should work everywhere. But if it fails due to missing functions, and we decide to refuse the upload in that case, then it would block people behind firewalls who can't use the Live Update from updating forever.

That is exactly the case i mean. In some setups you might also have the issue that the mine type is not correctly interpreted. So we might add a switch to disable the mime type checking?

As this is about com_joomlaupdate i would suggest to backport that to 3.10 too.

[richard67]

That is exactly the case i mean. In some setups you might also have the issue that the mine type is not correctly interpreted. So we might add a switch to disable the mime type checking?

You mean a configuration switch in com_joomlaupdate's options? Or maybe we should just silently skip the test if none of the functions is available?

As this is about com_joomlaupdate i would suggest to backport that to 3.10 too.

The issue is even for 3.9. Should it be solved there, too, or only in 3.10 and 4?

[zero-24]

That is exactly the case i mean. In some setups you might also have the issue that the mine type is not correctly interpreted. So we might add a switch to disable the mime type checking?

You mean a configuration switch in com_joomlaupdate's options? Or maybe we should just silently skip the test if none of the functions is available?

Hmm again my personal opinion when nothing is there we should issue an error and stop uploading. Yes a switch to turn it off in com_joomlaupdate. We might do a similiar thing in com_installer.

As for com_joomlaupdate please fork the method in the com_joomlaupdate model so we do not depend on that method to be there on the systems where we send a updated com_joomlaupdate. In the installer we can use the media helper.

As this is about com_joomlaupdate i would suggest to backport that to 3.10 too.

The issue is even for 3.9. Should it be solved there, too, or only in 3.10 and 4?

Up to @HLeithner to decide on. When it is implemented in 4 I'm happy to merge it into 3.10 for sure.

[richard67]

As for com_joomlaupdate please fork the method in the com_joomlaupdate model so we do not depend on that method to be there on the systems where we send a updated com_joomlaupdate. In the installer we can use the media helper.

@zero-24 I don't get it what you mean with fork, and which method. And the installer, would that be for extensions then? Or shall I put all that studff into the installer, and com_joomlaupdate shall use the installer functions? I'm available on Glip, too, so you can explain me there in detail in German.

[richard67]

@HLeithner Shall I fix this issue here in staging? And shall I backport the change from #27570 , too (which would make sense because both touch the same code)? And if both, shall I do it in 1 or in 2 separate PRs?

2 separate would theoretically be better because later merge up into J4, but practically the JS has changes so much between 3 and 4 that it would be better not to merge it up but solve this issue here with 2 separate PRs in staging and J4 and later solve the merge conflicts.

@wilsonge What would you prefer? In J3 the JS is hard-coded in the PH file "default_upload.php", in J4 it's a separate JS source, so there is no simple upmerge for that.

[HLeithner]

@richard67 to make it pulled proof (because is a critical part of the cms) I would use the same function used by the update it self to uncompress the ZIP file to validate if it's a zip file. (and if we only support zip files). Every mime checker (or better guesser) could stop users from upgrading without a good reason. And in my opinion I would only check the .zip extension because there is no security risk only a ux problem.

[richard67]

To make it pulled proof (because is a critical part of the cms) I would use the same function used by the update it self to uncompress the ZIP file to validate if it's a zip file. (and if we only support zip files).

And in my opinion I would only check the .zip extension because there is no security risk only a ux problem.

@HLeithner Am confused now. Shall I make it bullet proof, or shall I do the check of the file name extension only?

[HLeithner]

bullet proof as in "it should never fail because the detection is wrong"

[richard67]

Am still confused. Shall I do a mime type check or shall I not do it?

[richard67]

Am working one one or several fixes, stay tuned. It may take a bit time due to private stuff keeping me a bit busy.

[mbabker]

@mbabker I assume you mean with "analyze the file data" a MIME type check with functions like "mime_content_type" or "finfo_open" and "finfo_file", is that right? And do you really think it is necessary for the issue here? As far as I understand it, it is not a security issue here because it needs a super admin anyway to be logged in to backend. I understand it more as a check being needed to prevent uploading of an accidently selected wrong file.

File upload validation should be done right regardless of whether you’re supporting public uploads or the upload can only be done by a super user. Which means using dedicated filesystem functions (like those you listed) to handle validation. Any form of “does this uploaded file have X extension” check is unreliable and a borderline joke, file names and extensions are very easily changed and this is a very common exploit pattern used for attempting to upload not allowed content. I have cleaned JPG and PNG files off of servers way too often whose file contents were PHP scripts, with proper upload validation those files would have never made it to the upload destination; this is just one example of why filename validation is to me even worse than no validation at all.

[richard67]

@mbabker Thanks for pointing me to the right direction with my other PRs. I've closed them and will make new ones.

[richard67]

@mbabker For the client side (adding attribute "accept" to the input element), see PR #29877 : Was this what you had in mind, leaving the user the freedom to disable the filter on particular browser and upload another kind of file anyway? Or did you have in mind to keep the additional check in JS and the messages from my other, meanwhile closed PR, and just make that check use the "accept" attribute of the input element instead of another hard-coded regex?

For the server side (MIME type check): Could you check if PR #29877 #29884 is going into the right direction? And I would be really thankful if you could check the 3 "RFC" items in the PR's description and leave your opinion in a comment.

[richard67]

So what is the status here?

• I've provided a user friendly solution with appropriate error messages in case of accidently selecting a wrong file, [4.0] Add upload file name extension check to com_joomlaupdate's Upload & Update function #29865 for J4 and Add pre-upload file checks to com_joomlaupdate's Upload & Update function #29869 for J3, but it was rejected.
• So I've provided PR Add "accept" attribute to the upload file input element of com_joomlaupdate's Upload & Update tab #29877 to have at least something to avoid such wrong selection, but it leaves it to the browser what to do with it. This has been merged into staging meanwhile and will find its way up to J4.
• Because I was pushed into that direction here, I have added a server side MIME type check, but this was rejected, too, see PR [RFC] [WiP] Add MIME type check for upload files to com_joomlaupdate #29884 . I had a change ready to make it possible to switch it off, but that was not enough, it seems.

So I did what I can to solve this issue.

@PhilETaylor If you think that PR #29877 is sufficient to solve this issue here, please close it, otherwise leave it open. In the latter case your opinion on how it should be solved would be welcome.

[brianteeman]

There is a cross project initiative looking at signed packages

[zero-24]

Anyone with access to com_joomlaupdate can upload any file and have it saved as a tmp file, or any zip file containing any file content and have it partially extracted to the root of the site bypassing all other admin security measures (like those implemented in the Media Manager when it comes to media). Including creation of file, overwriting of existing files without additional warning.

Anyone with access to com_joomlaupdate is an SU anyway right?

Even the most basic hash checking that is in Joomla 4 - is currently only a warning and doesnt abort the installation.. /facepalm

When you check back this is intentional and will be changed in 4.x to an error.

Cryptography is hard to get right. Lets leave it to the experts.

https://theupdateframework.io/ - Done (we dont even try to reinvent the crypto here)

Lets not digress into fantasy land - In the history of Joomla, Nothing has every been delivered by "cross project initiatives" that I can think of.

Well I can say it is working out good so far with the Drupal and TYPO3 people we work with on that topic.

[zero-24]

Lets not digress into fantasy land - In the history of Joomla, Nothing has every been delivered by "cross project initiatives" that I can think of.

https://github.com/joomla/joomla-cms/tree/staging/libraries/vendor/typo3/phar-stream-wrapper

Jup it is a small one but that is used in Drupal, TYPO3 and IIRC WP too :)

[zero-24]

Does that mean we should forego basic security principles just because they are a super admin?

No as mention above I would have added it only Harald did not want it to be shipped with 3.9

A framework Joomla doesnt implement. Not a single open source PHP CMS has adopted this framework.

Right that is the thing we are working on :D it is not ready yet. As mention in the last JSST reports when you follow them.

I checked, its still a todo.

Yes. IIRC @alikon was about to check that and do a PR.

Im done. This issue is closed as a won't fix

You closed that issue here. ;)

[zero-24]

A meeting of three people... out of a team of 11+ ?

Do we discuess now how many people attend an optional meeting or do we talk about on the subject of this PR?

About this new meeting format for the JSST Meeting
This meeting is intended to be an informal meeting only so the JSST leadership can update the team on ongoing things and current reports can be discussed. As well as the JSST Members can bring in topics they want to discuss with the team. This informal meeting is a fully optional meeting held using Google Meet. Depending on the topics (whether we are allowed to share them to the public or not) there will be irregular reports. For all official meetings where we have votings or motions or similar things to take there will always be a meeting report. But due to the nature of this team such official meetings will be very irregular as we usually act on issues reported to us or proactive work on security improvements in the public tracker.

https://volunteers.joomla.org/teams/security-strike-team/reports/1245-first-informal-jsst-meeting-2020-2020-01

[zero-24]

But yet you only report on the "unofficial meetings" and not report on the "official meetings"....

What are you talking about? There is no unofficial meetings just optional meetings meaning not requiring anyone to join on any meeting and to share updates with the community.

So can we please go back on topic? Feel free to reach out to me by Glip or Mail when you have any questions on JSST topics or meeting reports. So we can keep that issue here on topic.

[alikon]

just to be clear #19792 have been followed by a POC #20073 closed by me cause of lack of interest sorry if i'm talking from the past ie 2018 iirc for the https://github.com//issues/29695 it is in my to do list..... just for history the original pr's that introduced the hash check .... which were originally developed as a GSOC project of too much year's ago.... they were submitted with error if hash check fail.... someone decided to have a more soft approach

[zero-24]

This conflicts with your quote. "so the JSST leadership can update the team"

Well the process is that we update the team and the community with the reports published internaly and than as public reports. That meeting would allow than questions or discussiones when required. Some of that is also handled via the Glip chat and GH issues etc.

There have been no meeting reports since May 2019
But no reports for over a year...

Yes as the only official meeting voting we had where the vote on the TL. As mention in that exact meeting report.

informal implies unofficial/non-formal.

You are the native person in here so I will let the definition on you but this is what it is intended. An open meeting where we can meet and optionaly discuss stuff.

But again can we please stay on topic of this issue here?