/cphalcon

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XSS vulnerability inside <TextArea> elements (no escaping/htmlentities being done) #12428

Closed
hi2u opened this Issue Nov 26, 2016 · 7 comments

Comments

Assignees
No one assigned
Labels
Bug - Medium
Projects
  
Done in 4.0 Release
Milestone
  4.0.0
3 participants
@hi2u @sergeyklay @niden
@hi2u
Copy link

hi2u commented Nov 26, 2016

When using Phalcon's Phalcon\Forms\Element\TextArea element, no escaping/htmlentities is done on the value in between the real <textarea> and </textarea> tags. A malicious user simply entering </textarea> into the form allows any following client-side code to be executed when returning to the form.

Example form with malicious user input:

xss-form

Next time the form is rendered, the malicious code gets executed:

xss-free

This behaviour is inconsistent with Phalcon\Forms\Element\Text which does safely convert special characters to HTML entities.

I'm using Phalcon 3.0.1 on Ubuntu 16.04 64bit with PHP 7.0.8-0ubuntu0.16.04.3 (standard official Ubuntu repo package at present).

This appears to be where the tag is generated, "content" isn't escaped: https://github.com/phalcon/cphalcon/blob/v3.0.1/phalcon/tag.zep#L963

In the meantime, you can work around this issue by using this extended element class (instead of using Phalcon\Forms\Element\TextArea directly)...

class TextAreaSafe extends \Phalcon\Forms\Element\TextArea
{
    public function render($attributes = null)
    {
        $fieldName = $this->getName();
        $Escaper = new \Phalcon\Escaper();
        $this->getForm()->getEntity()->$fieldName = $Escaper->escapeHtml($this->getValue());
        return parent::render($attributes);
    }
}

@sergeyklay sergeyklay added the Bug - Medium label Nov 26, 2016

@sergeyklay sergeyklay added this to the 3.0.3 milestone Nov 26, 2016

@sergeyklay

This comment has been minimized.

Sign in to view
Copy link
Member

sergeyklay commented Nov 27, 2016 •
edited

@hi2u How about to use the TextArea as part of HTML editor? The escaping out of box will break such use. At least it's not a minor hotfix. In my opinion you have to escape user's input (for example in the controller's action) but not output.

In any case this is an ambiguous question and I need time to think about it.

Could you please provide minimal code which could be reproduced to understand its correct use case?

@sergeyklay sergeyklay added Need information Need script to reproduce and removed Bug - Medium labels Nov 27, 2016

@hi2u

This comment has been minimized.

Sign in to view
Copy link
Author

hi2u commented Nov 27, 2016 •
edited

How about to use the TextArea as part of HTML editor? The escaping out of box will break such use.

Sorry, I didn't really understand what you mean here?

I've done up a simple example. You can just paste this action code into a controller:

    public function xsstextareaAction()
    {
        $Form = new \Phalcon\Forms\Form();

        $e = new \Phalcon\Forms\Element\Text('demoTextField');
        $e->setDefault('this is automatically made safe <html> "doubles quotes" \'single quotes\' ');
        $Form->add($e);

        $e = new \Phalcon\Forms\Element\TextArea('demoTextAreaField');
        $e->setDefault('this is NOT automatically made safe </textarea> <html> "doubles quotes" \'single quotes\' <script>alert(\'this shouldnt happen\');</script> ');
        $Form->add($e);

        foreach ($Form as $Element) echo $Element;
    }

The resulting HTML rendered is:

<input type="text" id="demoTextField" name="demoTextField" value="this is automatically made safe &lt;html&gt; &quot;doubles quotes&quot; &#039;single quotes&#039; " />

<textarea id="demoTextAreaField" name="demoTextAreaField">this is NOT automatically made safe </textarea> <html> "doubles quotes" 'single quotes' <script>alert('this shouldnt happen');</script> </textarea>

As you can see, the regular one-line INPUT is converting special characters into HTML entities on output like it should. But the TEXTAREA does not.

Everything I've read about XSS recommends doing the escaping on output (not input), because the data in your database could be coming from anywhere, including non-web systems outside of your own control. You can't rely on your source data having already converted all its special characters to HTML entities. As that would cause all sorts of issues. And escaping is context specific, e.g. if the data is getting sent in a plain text email, you don't want it to be pre-polluted with HTML entities.

That's how we end up with stuff like this: https://www.instagram.com/p/SVfQruppEE/ :) ... I came across that on this page about XSS input vs output: http://lukeplant.me.uk/blog/posts/why-escape-on-input-is-a-bad-idea/

In either case, TextArea should be consistent with Phalcon's Text/Select/etc elements. But it really should be on output, as Text, Select etc correctly do.

If there are some specific reasons that content shouldn't be converted inside TEXTAREA tags, maybe it should just at least check for a </textarea> inside the value and just sanitize that?

Are there any downsides to just sanitizing it using the same method as Text/Select etc?

@hi2u hi2u closed this Nov 27, 2016

@hi2u hi2u reopened this Nov 27, 2016

@hi2u

This comment has been minimized.

Sign in to view
Copy link
Author

hi2u commented Nov 27, 2016

Sorry about closing the ticket before, clicked the wrong button.

I also just now tested how Phalcon behaves with special characters inside the values of <SELECT><OPTION> tags. This is also safely sanitized in the same way as Text.

e.g: if my database has an option with value:

Data with <tag> "quotes" 'quote'

\Phalcon\Forms\Element\Select is automatically rendering as:

<option value="51">Data with &lt;tag&gt; &quot;quotes&quot; &#039;quote&#039;</option>

So I'm guessing Phalcon is doing it for most other form fields too? Except for TextArea.

@sergeyklay sergeyklay added Enhancement Requires-Discussion and removed Need information Need script to reproduce labels Nov 27, 2016

@sergeyklay sergeyklay modified the milestones: 4.0.0, 3.0.3 Nov 27, 2016

@niden

This comment has been minimized.

Sign in to view
Copy link
Member

niden commented Nov 27, 2016

So

Yes this is an issue and not at the same time. Some might argue that the framework should filter things automatically (as seen in other Tag elements) while others might argue that it is the job of the developer to filter and sanitize input.

This is not a difficult fix. However, if we introduce a fix for this using htmlspecialchars right now, we will definitely break existing applications where developers escape/sanitize input as they get it from the end user. This will result in double escaping which will break existing applications.

For the above we decided to release this in 4.0.0, since it can be a breaking change.

Thanks @hi2u for reporting this.
@hi2u

This comment has been minimized.

Sign in to view
Copy link
Author

hi2u commented Nov 28, 2016 •
edited

Thanks for the feedback.

others might argue that it is the job of the developer to filter and sanitize input

A few might, but they definitely seem to be in the minority. Pretty much every XSS guide recommends doing it on output for the reasons I mentioned above and a bunch more. And this is how Phalcon itself does it for all other fields. So whichever opinion you have about input vs output, Phalcon is doing it inconsistently.

But yes I agree with what you're saying about breaking changes generally.

developers escape/sanitize input as they get it from the end user. This will result in double escaping which will break existing applications.

There's 4 possible approaches for how current users of Phalcon are handling this right now...

  1. They are escaping on input on ALL fields. If they were doing this, all of their Text fields would have the double escaping issue you mentioned, which they would find out quite quickly when entering an & symbol into text fields, which would repeat every time they save the form. So probably nobody is doing this. So this group doesn't really even exist. And if it does, they have bigger problems already.
  2. They aren't escaping on input on any fields, as it's already being handled by Phalcon on output (in forms) as far as they know. These sites are vulnerable. This was me (and probably most others).
  3. They're aware that Phalcon handles this on all other fields except TextArea, so they're doing their own escaping on input just for TextArea fields only. If they're escaping on output in non-form areas of their site, like they should be, they'd either be double escaping there, or also specifically excluding TextArea content from their regular output escaping. Seems pretty unlikely to me.
  4. They've implemented their own output escaping on the form TextAreas using code similar to what I suggested above: TextAreaSafe I also think this is fairly unlikely.

Considering that this is only noticeable if a user specifically enters </textarea> into the field (everything else is pretty much safe). My guess is this is 99% of users aren't even aware that Phalcon treats Text and TextArea differently. So I think almost everyone will be in category #2 above. I would assume not many people are in group #3 or #4, and probably zero people in #1.

Not many people would have noticed this inconsistent behaviour, because it's really only ever a serious security issue if two different people are viewing the rendered form containing the malicious code. An example of where this might be an issue is an attacking user entering malicious code into the TextArea, and then an admin user also edits that record in the same form later on. In this case the admin user's browser will execute the attacker's code. I've actually demonstrated this on the forum (see below).

It would be interesting to try this out on some other sites that we know are written in Phalcon to see what happens. If anybody reading this could please try the following on your own sites, or any others you know that use Phalcon, we'll see if we can get a few stats on this...

  1. Go to one of your forms with a TextArea field
  2. Paste this into the TextArea field

</textarea>OUTSIDE

  1. Save the form.
  2. Return to the form and see if the word "OUTSIDE" is still kept inside the TextArea field.

If my guess about most people being in group #2 above is correct, we might have a lot of vulnerable sites out there.

I just tested it on the Phalcon forum. By entering this into the TextArea of my post...

I'm just posting here to demonstrate the bug I've filed at https://github.com/phalcon/cphalcon/issues/12428

</textarea>
OUTSIDE!!!

<h1 style="font-size:50px; color:red;">i'm free!!!</h1>

<script>
document.addEventListener("DOMContentLoaded", function(event) { 
    alert('This should not happen!');
});
</script>

...then returning to the form by editing my comment, I got it to do this:

2016-11-28 13_06_22-edit discussion_ this is a demonstration of phalcon 039 s inconsistent text and

Also, funnily enough, there's an example of why XSS escaping on input is bad on that forum post itself - go there and hold your mouse over the browser tab, you'll see the title:

This is a demonstration of Phalcon&#039;s inconsistent Text and TextArea escapin

Are you guys admins on the forum? Can you edit users posts? If so, go there and edit my post here: https://forum.phalconphp.com/edit/discussion/14891 - when you open the form, you'll be executing the JavaScript code I put in there (it's actually harmless of course).

So, so far of my personal (limited) testing of my own sites + the Phalcon forum, they're all in category #2 above... vulnerable.

@hi2u hi2u referenced this issue Nov 28, 2016

Closed

XSS issue: </textarea> not escaped on comment editing form #263

@sergeyklay sergeyklay added Bug - Medium and removed Enhancement Requires-Discussion labels Dec 1, 2016

@stale

This comment has been minimized.

Sign in to view
Copy link

stale bot commented Apr 16, 2018

Thank you for contributing to this issue. As it has been 90 days since the last activity, we are automatically closing the issue. This is often because the request was already solved in some way and it just wasn't updated or it's no longer applicable. If that's not the case, please feel free to either reopen this issue or open a new one. We will be more than happy to look at it again! You can read more here: https://blog.phalconphp.com/post/github-closing-old-issues

@stale stale bot added the stale label Apr 16, 2018

@sergeyklay sergeyklay closed this Apr 17, 2018

@sergeyklay sergeyklay reopened this May 2, 2018

@stale stale bot removed the stale label May 2, 2018

@niden niden added this to In progress in 4.0 Release Nov 28, 2018

@niden niden moved this from In progress to To do in 4.0 Release Nov 28, 2018

@CameronHall CameronHall referenced this issue Nov 28, 2018

Merged

Added HTML escaping to Phalcon\Tag::textArea #13624

2 of 3 tasks complete

@niden niden moved this from To do to In progress in 4.0 Release Nov 29, 2018

@niden

This comment has been minimized.

Sign in to view
Copy link
Member

niden commented Nov 29, 2018

@hi2u This has been addressed. Thank you!

@niden niden closed this Nov 29, 2018

4.0 Release automation moved this from In progress to Done Nov 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment