Skip to content
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

J3.4.0 cannot install certain extensions #6173

Closed
sakiss opened this issue Feb 25, 2015 · 22 comments
Closed

J3.4.0 cannot install certain extensions #6173

sakiss opened this issue Feb 25, 2015 · 22 comments

Comments

@sakiss
Copy link
Contributor

sakiss commented Feb 25, 2015

Hi

It seems that changes were done to the installer and also to the JInputFiles class in J3.4.0.
After trying to install several of my extensions i am getting the following message
"No file selected.
Unable to find install package"

After extensive debugging i found out that the problem is caused by the the function JInputFiles::isSafeFile which checks the contents of the files if they contain the <?php tag

lines 475-478
if ($options['php_tag_in_content'] && strstr($buffer, '<?php'))
{
return false;
}

I cannot figure out how a php extension cannot contain this tag or if it is something related with my code

@Bakual
Copy link
Contributor

Bakual commented Feb 25, 2015

"isSafeFile" is meant to check if an uploaded file is safe and can't contain malicious code. Obviously, PHP files are not safe to be uploaded. This method is new in 3.4.0.
If your extension needs to allow uploading unsafe files for some reason, you can set the fourth argument of JFile::upload() to true. See https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/filesystem/file.php#L452

@Bakual
Copy link
Contributor

Bakual commented Feb 25, 2015

And yes. The Installer classes were rewritten a bit. @mbabker may be able to tell you more there.

@sakiss
Copy link
Contributor Author

sakiss commented Feb 25, 2015

If it is something from my side i can fix it.
But what you mean with "PHP files are not safe to be uploaded"?
It is a zip which of course contains php files

@mbabker
Copy link
Contributor

mbabker commented Feb 25, 2015

The UploadShield changes have nothing to do with the JInstaller refactoring.

@sakiss
Copy link
Contributor Author

sakiss commented Feb 25, 2015

Does the isSafeFile checks all the files inside the zip?
If yes how can i avoid getting that error, since most of the files are php files?

@wilsonge
Copy link
Contributor

So can you isolate the differences between your extensions. I assume by the fact it is only targeting some of your extensions that there must be others that contain php code that are being sucessfully installed?

@OctavianC
Copy link
Contributor

Can confirm this: today we've had a customer with this issue when installing one of our extensions (RSDirectory!). Couldn't replicate it locally, so I asked him for credentials but he sorted it out by downgrading to 3.3.6, installing and then updating to 3.4.0, so I couldn't test further.

Looking at the code, it should show an 'JLIB_FILESYSTEM_ERROR_WARNFS_ERR03' error, but it doesn't. I'm going to do some more testing tomorrow.

@nikosdion
Copy link
Contributor

Do you have uncompressed .php files in your ZIP files? That would explain why UploadShield is being triggered (the raw <?php open tag would be present). Also, if it's triggered it should raise the JLIB_FILESYSTEM_ERROR_WARNFS_ERR03 error message.

Frankly, this is a bug in com_installer. It should NOT be using UploadShield since we do expect to upload files containing executable code. Apparently none of the packages the very limited group of people we tested the patch had uncompressed files triggering it and nobody noticed. I'll send a PR to address this issue in a few minutes.

@nikosdion
Copy link
Contributor

I just sent patch gh-6180. @sakiss and @OctavianC can you please test it with the affected servers / packages? I think I have nailed it, but it's hard to be sure with a blind fix without someone confirming it under real world conditions.

@sakiss
Copy link
Contributor Author

sakiss commented Feb 25, 2015

@nikosdion It did not helped. It seems that the files are checked with JFilterInput::isSafeFile once the JApplication->input->files is generated. Hence gives a null var, when the file is called at the 1st lines of the InstallerModelInstall::_getPackageFromUpload

$userfile = $input->files->get('install_package', null, 'array');

@Kubik-Rubik
Copy link
Member

@sakiss Why don't you use compression in your packages? Can you please create a new zip archive with compression of your extension and test it again. Does it then work properly?

@sakiss
Copy link
Contributor Author

sakiss commented Feb 25, 2015

A workaraound is to close some open <?php tags in the files. But does not seem like a viable solution

@nikosdion
Copy link
Contributor

What does THAT have to do with anything at all?! It's entirely irrelevant.

@nikosdion
Copy link
Contributor

The one thing you're right about is that JInputFiles is using isSafeFile which doesn't make any sense. I am investigating when and why this change was applied.

@nikosdion
Copy link
Contributor

OK, I have updated the patch.

For history's sake, the bug is two-fold.

Part 1: com_installer's use of JFile::upload. It was missing the fourth parameter to instruct upload() to ignore UploadShield scanning of uploaded package files. This was present in my original PR two years ago, it was lost when we were rebasing the code for inclusion in Joomla! 3.4. That was fixed earlier in my patch gh-6180.

Part 2: JInputFiles::get will NOT return files marked as unsafe by UploadShield unless the otherwise ignored filter type is set to 'raw'. com_installer used the filter type 'array' and I completely missed this when I was writing the original UploadShield PR two years ago. I just fixed it no in patch gh-6180.

So, we have a bug in two parts. I take responsibility for part 2. It was my fault. Nobody else picked it up and it ended up in 3.4. The other half is just what happens when a PR is filed August 10, 2013 and merged February 24th, 2015. Oh well, what the heck...

@sakiss
Copy link
Contributor Author

sakiss commented Feb 25, 2015

Tested and works

@OctavianC
Copy link
Contributor

I'm sorry but I could not replicate this (unless I actually upload a .php file through com_installer, this is how I noticed the error isn't showing). My archive is using the highest ZIP compression so there shouldn't be any <?php tags showing inside the archive (like I said, customer said he couldn't install it, my tests on the other hand were successful). Since I'm out of the office right now I'll make some tests tomorrow in the morning.

@nikosdion
Copy link
Contributor

@OctavianC Was your client using Mac OS X and Safari by any chance? By default that combination would uncompress your ZIP file automatically and he'd have to recompress it with the built-in archiver. That would leave some small files uncompressed.

@OctavianC
Copy link
Contributor

Nope, according to the submitted ticket he's using: "Mozilla/5.0 (Windows NT 6.1; WOW64; Trident/7.0; rv:11.0) like Gecko" so that would be Windows 7 64-bit with Internet Explorer 11.

@OctavianC
Copy link
Contributor

Bump, just had a customer using a Mac having issues. "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/40.0.2214.115 Safari/537.36" Hopefully he'll allow me to debug his install.

@nikosdion
Copy link
Contributor

Yes, please. Try the patch (it's basically two lines in con_installer's InstallerModelInstall class).

@brianteeman
Copy link
Contributor

Closed as we have a PR #6180 so we can try to keep the conversation in one place.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/6173.

Kubik-Rubik added a commit that referenced this issue Feb 26, 2015
…taller

Executable PHP code in installation packages - Fix #6173
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants