cphalcon icon indicating copy to clipboard operation
cphalcon copied to clipboard

XSS: Select element does not escape values

Open zacek opened this issue 1 year ago • 1 comments

Phalcon's Phalcon\Forms\Element\Select element is not escaping the values between the tags. This allows malicious user to break the html code and execute javascript code.

This is very similar to the closed issue #12428 and brings similar backward compatibility problems (double escaping for users who escape it before injecting data to the form). But it clearly can generate invalid html code as demonstrated below and should be escaped by default. Anyone using this element has to do it anyway.

To Reproduce

$form = new \Phalcon\Forms\Form();
$data = [
    1 => 'entry with malicious javascript <script>alert("I could do some dirty job instead");</script>',
    2 => 'entry with malicious html </option></select>This appears outside the select box',
];
$select = new \Phalcon\Forms\Element\Select('selector', $data);
$form->add($select);

foreach ($form as $element) {
    echo $element;
}

This is the rendered result:

<select id="selector" name="selector">
	<option value="1">entry with malicious javascript <script>alert("I could do some dirty job instead");</script></option>
	<option value="2">entry with malicious html </option></select>This appears outside the select box</option>
</select>

Expected behavior Currently, the workaround fix is to escape the data manually

$form = new \Phalcon\Forms\Form();
$escaper = new \Phalcon\Html\Escaper;
$data = [
    1 => $escaper->escapeHtml('entry with malicious javascript <script>alert("I could do some dirty job instead");</script>'),
    2 => $escaper->escapeHtml('entry with malicious html </option></select>This appears outside the select box'),
];
$select = new \Phalcon\Forms\Element\Select('selector', $data);
$form->add($select);

foreach ($form as $element) {
    echo $element;
}

with rendered html code

<select id="selector" name="selector">
	<option value="1">entry with malicious javascript &lt;script&gt;alert(&quot;I could do some dirty job instead&quot;);&lt;/script&gt;</option>
	<option value="2">entry with malicious html &lt;/option&gt;&lt;/select&gt;This appears outside the select box</option>
</select>

Details

  • Phalcon version: 5.7.0
  • PHP Version: 8.1.29
  • Operating System: Rocky Linux 9
  • Installation type: installing via package manager
  • Zephir version 0.18.0:
  • Server: Apache

I know there is newer phalcon and newer PHP but the problem will be he same as it was not reported yet.

zacek avatar Oct 24 '24 08:10 zacek

Ok, this is something we can argue about.

Escaping should happen where user controlled data is used. A select is 99% of times developer controlled. #12428 is talking about textarea, were content is nearly 100% user controlled and a standard escaping makes sense.

In my opinion, solution 2 is the correct one. If you provide user controlled data, it is your job to escape it (or not).

Now the part to argue: If we apply escaping to the select, then we should apply escaping to all elements and all options. Malicious code is not only provided in texts, it could also provided in class, style, title attribute etc. I think it is clear, that this will have a lot of side effects. That's why I would say, solution 2 is the correct one. The developers knows best where they apply user controlled data and should escape there.

noone-silent avatar May 16 '25 02:05 noone-silent