Some changes to HtmlTidy

[Michael Hosse <michael.hosse@…>]

There are some disadvantages and bugs in this Plugin: 1. the call to tidy ist hard-coded. I changed this to use a global variable _tidy. This variable is sent to html-tidy-logic.php 2. html-tidy-logic.php is not able to find html-tidy-config.cfg in every case. I added $cwd to ensure it. 3. Splitting source into an array is now done with a regular expression. The old method had problems with empty lines. 4. <?php $jsMakeSrc; ?> does not work. There is a echo missing.

I applied a patch-file to this ticket. Maybe you can use it.

[didier Belot]

When you resolve this one, mark also #412 as resolved.

[gogo]

Your patch introduces a major security vulnerability. There is nothing to stop anybody who can make an HTML post to the logic php from passing in arbitrary commands as the tidy command - eg

<form action="http://vulnerablesite.com/xinha/plugins/HtmlTidy/html-tidy-logic.php" method="POST">
  <input type="text" name="tidy_prg" value="cat /etc/passwd " />
  <input type="submit" value="Attack!" />
</form>

[jkronika@…]

Wouldn't something like this fix the security flaw?

- $tidy = $_POST['tidy_prg'];
+ $tidy = escapeshellcmd($_POST['tidy_prg']);

See the escapeshellcmd function in the PHP Manual.

[Michael Hosse <michael.hosse@…>]

gogo you are right. I did some further changes to the code, because escapeshellcmd does not work for this.

The substantial change are just some lines:

$tidy = $_POST['tidy_prg'];

will go to:

	list($tidy,$dummy) = explode(" ",$_POST['tidy_prg'],2);
	if (!is_executable($tidy) && substr($tidy,-4) != "tidy")
    echo "alert(HTMLArea._lc('Tidy not found. Check your configuration.', 'HtmlTidy'));\n";
	else {

Because of this change the indentation of the whole file changes too, so i added a new patch file.

[Michael Hosse <michael.hosse@…>]

new patch file

[Michael Hosse <michael.hosse@…>]

I just reviewed the code once more. You should change the "&&" in the if construct to "".

[gogo]

I'm not satisfied that this modification is sensible. Even with removing arguments (the explode call) it is still possible to call any arbitrary command on the system, chances are you can do no harm, but it's still not a good idea IMHO.

I've applied the parts not related to the security problem in changeset:410 but I think we'll leave the tidy command hard coded without path for now. It's not too much to expect the command to be called "tidy" and in the binary search path somewhere.