Description: ------------ Although we highly recommend never running phpdocumentor as a privileged user or on a live server, this is a potential hole that should be filled: It is possible to workaround this issue by way of the -o command line option. Supply a relative path to the third part of the -o paramater; relative to the directory the templates are in for the supplied converter. It looks ugly, but it'll work. Example: The HTML:frames templates for my PEAR-installed phpDoc 1.3RC6 are in /usr/share/pear/data/PhpDocumentor/phpDocumentor/Convertors/HTML/frames/templates/ however, my templates live elsewhere. To get phpDoc to use my templates, I use: -o HTML:frames:../../../../../../../../../../path/to/templates/template Looks like a boar's behind, but it does the job. ../ should never be allowed.

I had ASSUMED that the output options ("HTML:frames:default") were actually just NAMES, not PATHED entities, and that using templates that were located elsewhere REQUIRED the use of the templatebase flag to provide the path to that other location. If my assumption is actually how the could SHOULD work, then maybe it's just a matter of checking the output arg to ensure all three pieces are names, with no path delimiters allowed in that name. Am I on the right track?

I wouldn't consider this a security issue. If you already have shell access then you could just run the PHP script directly. If your talking about the web interface on a public server, then maybe we should just add a hack me now button to punish anyone stupid enough to do it :-P. Anyhow it does make sense to clean -o just too keep people from doing odd things, and for that matter maybe some other arguments too. maybe preg_replace('/[^a-zA-Z_:]/',$dasho,''); though it might be better to clean the component parts.

Just did a quick google search, there are a couple instances of the web interface showing up but not hundreds. Lots of denial of service opportunities with that one. Maybe the web interface should have a warning on it.

Toyed with Josh's idea until I got it to work. Submitted it as sourceforge patch #1620470 (https://sourceforge.net/tracker/index.php?func=detail&aid=1620470&group_id=11194&atid=311194) That should at least handle the commandline usage... I can't test using the phpdoc webpage myself since I don't have one installed.

I've added "0-9" as well as "." to the allowed characters list in the sourceforge patch file today. Josh mentioned allowing "0-9", and I was reminded that "." must be allowed because we already have a converter with a dot in its name... HTML:frames:phpdoc.de.

be careful that you don't accidentally re-enable ../.. by putting "." back in there :)

Well, as long as no: - slashes (../default) - dollar signs ($HOME) - tildas (~) are allowed, seems like there's no way to construct a path in the string, dots or no dots.

don't forget about XML:DocBook/peardoc2:default we do need /, which is why allowing "." is dangerous. Filtering the template name for / ~ $ should be fine, but . $ ~ should be filtered from the converter name

Ok, the original code only cleans up the third part, making no effort to modify the first two parts... e.g. "HTML:frames:default" is only cleaning the "default" piece. My next patch upload will contain new code that, in addition to only allowing "." in the "default" piece, will also only allow "/" in the "frames" part, while not allowing either "/" or "." in the "HTML" part. So, any part that still needs to allow either "/" or "." will NOT allow BOTH. Although that won't disable all pathing capability (will still exist in "frames" part), it will at least prevent any pathing capability that backs out to parent directories (no part will allow "../").

Got my latest patch uploaded on 12/27 after my comment posting. Anyone have time to review it?

Your regex's all allow : which doesn't matter since your exploding on that but its not needed. Also just for readability sake can you make the single line else statements multi line.

I removed the colons from all three regexs (had them in there because they were part of your original idea), and braced out the ELSE branches (had seen many one-line IFs, and assumed it was preferred that one-line blocks were set inline... now I know better). SF patch is updated... thanks for the feedback, Josh!

Assuming the code works, i haven't checked that, I think the sf patch is good to commit

My testing on it last week showed it - handled the proper output values fine, - was able to remove extra / and . characters there weren't allowed and allow it to continue - "H/T/M/L:.frames:../../default/." became a valid "HTML:frames:default" value - and would properly crap out if an allowed / or . caused the values to be invalid - "H/T/M/L:../../frames/.:../../default/." became an invalid "HTML://frames/:....default." These are the extreme examples... I tested many permutations in between normal and these extremes. I should probably explicitly mention the patch DOES block these worrisome output values: - HTML:/home/rambo/blow-up-my/frames:default - HTML:frames:/var/log/messages/default - HTML:../frames:../../default - HTML:frames:../../../../../../../../../../default

This would be a good case for a unit test, but I think setting that up for command line parsing is a much bigger job then this bug. I'm happy with the patch.

No one's mentioned unit testing to me yet. Are we using PHPUnit?

I've put together a PHPUnit (v3.0.0) testcase that exercises the setupConverters() method inside Setup.inc.php. I had to add a very ugly $unit_testing flag to that method, as a minimum to make it testable. I may refactor that method some so I can avoid needing that flag, but you can see the updated patch along with the new testcase file on SF.

Refactored the cleanup code out of the setupConverters method, getting rid of that nasty unit_testing flag I had put in. Updated SF patch and its test case.

Committed the patch to CVS, and added the unit test to CVS also.

Reopening this... The cleaner I built does not handle the DOM templates (e.g. HTML:frames:DOM/default).

Updated this fix to allow the DOM/blahblah converter options. Changes committed to RELEASE_1_3_2_BRANCH for now in preparation for v1.3.2 release. Will later update CVS Head during v1.4.0 release prep.

Updates committed to CVS Head now.