Opened 14 years ago

Closed 14 years ago

#417 closed defect (fixed)

Some changes to HtmlTidy

Reported by: Michael Hosse <michael.hosse@…> Owned by: gogo
Priority: normal Milestone:
Component: Plugin_HtmlTidy Version: trunk
Severity: normal Keywords:
Cc:

Description

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.

Attachments (2)

HtmlTidy.patch (1.8 KB) - added by Michael Hosse <michael.hosse@…> 14 years ago.
html-tidy-logic.php.patch (3.9 KB) - added by Michael Hosse <michael.hosse@…> 14 years ago.
new patch file

Download all attachments as: .zip

Change History (8)

Changed 14 years ago by Michael Hosse <michael.hosse@…>

comment:1 Changed 14 years ago by didier Belot

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

comment:2 Changed 14 years ago by 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>

comment:3 Changed 14 years ago by 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.

comment:4 Changed 14 years ago by 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.

Changed 14 years ago by Michael Hosse <michael.hosse@…>

new patch file

comment:5 Changed 14 years ago by Michael Hosse <michael.hosse@…>

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

comment:6 Changed 14 years ago by gogo

  • Resolution set to fixed
  • Status changed from new to closed

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.

Note: See TracTickets for help on using tickets.