Dizkus

Hi guys,

As you might now, I'm the current maintainer of FormExpress - and I just found out, that the import function won't load with PN750rc3 (atleast using PHP-4.3.8) - but works fine with pn726 in same setup.

This is the URL that fails:
index.php?module=FormExpress&type=admin&func=import

With this message (from index.php):
Failed to load module FormExpress (at function: "import")

But I can't seem to figure out what changed in that part of index.php from pn726, that would make this happen - all clues are appreciated :(

I've tried with and without the Load legacy settings - no diff.

--
--
Regards,
Klavs Klavsen - kl at vsen.dk - http://www.vsen.dk - http://contrib.virkpaanettet.dk
Working with Unix is like wrestling a worthy opponent.
Working with windows is like attacking a small whining child who is carrying a .38.

keep in mind to get all vars via pnVarCleanFromInput() for your module - IIRC this has been introduced in .750rc3 for enhanced security and might be a reason for problems...

--
regards from germany
..::[Zikula Application Framework]::.. ..::[SEO-Blog]::.. ..::[CMS Sicherheit]::..

Klavs,

I've just checked this out and there's a problem with the logic in the module. Looking at the import function we have



The error your seeing is from the second snippet, When you set a session variable you should redirect back to somewhere in the admin interface with a pnRedirect (usually the view function). This will have always been a problem but since the rest of the code worked you won't have seen it before.

But we still have the root cause. Not using pnVarCleanFromInput here is a problem since PN no longer automatically registers every get/post/cookie etc. variable into the global namespace (for security and resource management reasons doing this is bad). Code shoudn't assume that a variable exists in the global namespace. Since pnVarCleanFromInput has been rewritten in this release it should work fine here.

Hope this helps.

-Mark

I found out that $import_file_name is not set - and thats why it dies.

I see no problem here?
$import_file_name = pnVarCleanFromInput('import_file_name');

and it is in the form (and works in pn726 - exact same code).

--
--
Regards,
Klavs Klavsen - kl at vsen.dk - http://www.vsen.dk - http://contrib.virkpaanettet.dk
Working with Unix is like wrestling a worthy opponent.
Working with windows is like attacking a small whining child who is carrying a .38.

Klavs,

In the version of the module I just downloaded you have



pnVarCleanFromInput is commented out. The code assumes that the variable import_file_name is in the globals array. While this is true for PN from .720-phoenix up to .726 this is not the case for .71x or .75+.

At .720-phoenix code was added to the top of pnAPI.php that extracted every get, post, env, session and cookie variable to a global. This was added to work around register globals being off in PHP and IMO was a mistake - better to address the problem properly than hack your way round it.

.

This code is bad for a number of reasons. Firstly it's a memory hog. Having every variable registered into the globals array is simply unnecessary. Secondly and more importantly this hasa huge potential impact on security. There are many more potential exploits in code where variables are automatically available in the global namespace. It's better that the all variables are obtained in a known, standard way - this way the developer knows what variables they need to look after.

For these reasons the global extraction code has been removed for .750 and .8x. IMO this code should never have been part of a PN release at point. Has this caused problems with modules? Yes a few but in the end it's a necessary step to creating a better, more structued and more secure application so I believe that the short term pain as modules are coded to a higher standard will be worth it in the end.

-Mark

I totally agree with you on the globals export stuff.

I tried to uncomment the line using pnVarCleanFromInput(and comment out the other) - and it still didn't work.

--
--
Regards,
Klavs Klavsen - kl at vsen.dk - http://www.vsen.dk - http://contrib.virkpaanettet.dk
Working with Unix is like wrestling a worthy opponent.
Working with windows is like attacking a small whining child who is carrying a .38.

I take it this variable is the submitted file source? If so then pnVarCleanFromInput won't work here. pnVarCleanFromInput looks at the _REQUEST superglobal. This is a combination of the get, post and cookie vars but not files. Use the _FILES superglobal and ignore the pnVarCleanFromInput and the subsequent is_array check. You may like to move this code to after you've confirmed the auth key from the form.

Note: This is mean the module will require 4.1x+ but this is no bad thing and is now a requirement for PN anyway.

-Mark

IIRC the same problem applies to the my_egallery when uploading stuff...

--
regards from germany
..::[Zikula Application Framework]::.. ..::[SEO-Blog]::.. ..::[CMS Sicherheit]::..

I've just dobule checked the old pnVarCleanFromInput and this didn't include the files either so we've not changed the behaviour of the API at all. But the question is then do we think there's a need to either include the _FILES superglobal into pnVarCleanFromInput or implement an API to work with this superglobal. Or do we let code that's dealing with file uploads handle the job itself?

-Mark

Well the input_file_name does not include the file-data - only the name of the temporary filename, PHP has saved the data in.

I then added this - for PHP-4.1+ support:
if (is_array ($import_file_name))
{
$import_file_name = $_FILES['import_file_name']['tmp_name'];
}
as it before 4.1 - returned a simple filename - but now returns an array.
I see no problem with modules just using $_FILES directly as I do - and besides it could be an image or anything, which PN could never know - and thus not "secure" in any way.

Any ideas, as to whats up here - if we could figure this out - we could fix my_egallery too :)

--
--
Regards,
Klavs Klavsen - kl at vsen.dk - http://www.vsen.dk - http://contrib.virkpaanettet.dk
Working with Unix is like wrestling a worthy opponent.
Working with windows is like attacking a small whining child who is carrying a .38.

thinking about the specific security problems with file uploads i guess it's not that bad thing right now.
if a module author wants to enable file upload *he* needs to check/fix the code...

--
regards from germany
..::[Zikula Application Framework]::.. ..::[SEO-Blog]::.. ..::[CMS Sicherheit]::..

Any ideas, why I'm not getting anything from this:
$import_file_name = pnVarCleanFromInput('import_file_name');

When it works in pn726 (it returns an array in PHP-4.3.8)?
It seems PN750rc3 handles this somewhat differently - so it doesn't work anymore :(

If you have any ideas, what I could do to debug, I'll be happy too (I printed out the var $import_file_name after this line - via trigger_error - and its empty).

--
--
Regards,
Klavs Klavsen - kl at vsen.dk - http://www.vsen.dk - http://contrib.virkpaanettet.dk
Working with Unix is like wrestling a worthy opponent.
Working with windows is like attacking a small whining child who is carrying a .38.

Klavs,

It worked in .726 by accident....by virtue of the global extraction code that was added at .720. Prior to this pnVarCleanFromInput would have only functioned at all if register_globals was on. The more I look at the original version of this API the more it strikes me as a poor implementation. The rewritten version is a lot cleaner.

What we can do is to support for the files superglobal to pnVarCleanFromInput so the API becomes



-Mark

as PN750rc3 only works with PHP-4.1+ I've simply replaced the code with this for now:
$import_file_name = $_FILES['import_file_name']['tmp_name'];

I agree that, pnvarcleanfrominput should handle the filename - but perhaps it should take care, to remove double dots (..) and other things, that might be used for mischief :) - but I'm not sure it's necessary - isn't everything in $_FILES set by PHP itself - and the file selected (on client) is automagically saved in a tmp file (which path is given in $_FILES['import_file_name']['tmp_name']) - so we can trust the vars in $_FILES - but ofcourse not the file itself - but PN can't possibly know if its suppose to be an image, Formexpress form or anything, so I can't see that PN should attempt to secure it - people handling files, should know how to handle the file they want, appropriately.

Though, this will be a lot easier, whenever someone (I know I haven't had the time yet :) puts photoshare's filehandling into PN core - as a file-storage module - so every module just can use it's functions to get/retrive (and store) files (images,documents- whatever :)

--
--
Regards,
Klavs Klavsen - kl at vsen.dk - http://www.vsen.dk - http://contrib.virkpaanettet.dk
Working with Unix is like wrestling a worthy opponent.
Working with windows is like attacking a small whining child who is carrying a .38.

That's my understanding so I don't see too much of a problem adding the files array into pnVarCleanFromInput.

@andreas - if you have no objections i'll add this support to both .7x and .8x codebases.

-Mark