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.

I dont have answers only problems :)

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

this should be reported to @joomla/security

It requires super admin login before the upload form is shown - historically the JSST are not interested if a super admin can hack themselves... but bat signal for @SniperSister anyway...

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.

@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.

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?

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

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.

@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.

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.

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.

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.

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?

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.

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.

@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.

@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.

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?

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

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

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 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.

@mbabker I am working exactly on that, mime type verification using the mentioned functions, but seperately from my current PRs for making a first check of the file name before the upload. That first check is only to show a better error message and prevent the unnecessary upload. I've separated these 2 steps because it will be easier to reject the PR for one of them if it is wrong, like it may turn out with those I have created. Stay tuned for the future PRs for the mime type checks.

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

@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.

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, #29865 for J4 and #29869 for J3, but it was rejected.
• So I've provided PR #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 #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.

@richard67 Sorry to hear your PRs have been rejected. This is the problem with the PR system, spending hours and hours researching fixing and proposing a complete fix only for someone to come along and say they dont like it and refuse to implement it. Like you said, and I agree, this is why its best to get consensus from more than one person (and preferably someone with a title) before putting in any considerable work.

The fact is, whether people like to accept it or not, this issue is a security issue and also a conceptual issue that unfiltered, unvalidated, file uploads should not be allowed.

Some quotes:

@brianteeman

this should be reported to @joomla/security

It was. The "official" response was:

@SniperSister speaking in his role as lead of the JSST.

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.

**It is a security issue. We make a big song and dance about validating files uploaded in the Media Manager, yet we bypass all validation using this upload method. **

Take this as an example:

Take a zip file full of fake images, use the Joomla update upload form and you will see that at least the first image is extracted to the root of the site with zero validation.** Thus bypassing all the media checks in the Media Manager and allowing media upload..

Next take hacked.php and zip it up, use the Joomla update form to upload this zip, hacked.php is then found in the root of the site despite the extraction ultimately leading to an error.

I did not want to reveal the full extent of the knock on effects of this issue and spell it out, as I thought people would "get it" but it seems they dont.

@HLeithner

Yes I already noticed this, my simple message is please don't break the updater because of a cosmetic thing.

/faceplam This is not a "cosmetic thing", its a "security thing"

To make it short I don't want to have this in j3.9

Well I guess that's it. Game Over. The boss has spoken.

@zero-24

When it is implemented in 4 I'm happy to merge it into 3.10 for sure.

@brianteeman

all of this seems like a LOT of work to address a fairly non-existent problem. Definitely more important things to work on

Yes there probably are more important things to work on, but this is not a non-existent problem.

@richard67

If you think that PR #29877 is sufficient to solve this issue here, please close it, otherwise leave it open

Client side validation is never the solution to security issues.

In the latter case your opinion on how it should be solved would be welcome.

My opinion counts for nothing as has been repeatedly proved. I can only report the issues I see.

In a perfect world, as this upload form should only ever accept official versions of Joomla distribution files, it should validate the hash of those files against an official source provided by Joomla - that would fix the security issue but. ensuring the uploaded file matches a cryptographic hash of the expected file content, however we live in an imperfect world and the whole point of this file upload screen is when a Joomla Site is " behind a firewall or otherwise unable to contact the update servers" thus making a call out to a trusted list of hashes impossible anyway

So where does all this leave us... back where we started:

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.

Zero client side validation and Zero server side validation before actions are taken up the user supplied file upload.

That, to me, doesnt sound like a good place to be in.

The only thing that saves the day here is that you have to be a Super Admin (Or someone with ACL to access com_joomlaupdate) to achieve this unrestricted file upload...

@mbabker

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.

There is a cross project initiative looking at signed packages

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.

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

File validation and basic file upload checks we can do today.

As for signed packages... its not going to happen in Joomla without better management and leadership. We simply go around and around aka the comments in #19792 which ultimately was closed without implementation.

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

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.

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 :)

ok lets digress further then....

Anyone with access to com_joomlaupdate is an SU anyway right?

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

phar-stream-wrapper

Thats a library.. just because its used by everyone doesnt mean it was designed, researched and developed as a "cross project initiative".

https://theupdateframework.io/

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

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

I checked, its still a todo.

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

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. ;)

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

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

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.

to share updates with the community.

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

This meeting is intended to be an informal meeting only

informal implies unofficial/non-formal.

For all official meetings where we have votings or motions or similar things to take there will always be a meeting report.

There have been no meeting reports since May 2019

But due to the nature of this team such official meetings will be very irregular

But no reports for over a year...

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?

Please reopen as this is still not resolved as @richard67 proposed fixes were not merged and this remains an issue to be resolved.

#34138 was reported to the JSST team today and @HLeithner stated once again that the JSST Is not responsible for Joomla 4 yet. I had totally forgotten about this issue being logged almost a year ago...

Lack of interest in resolving this security issue...