Cross-site scripting issue in TForm

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Use IE to access http://www.pradosoft.com/?"><script>alert(1);</script>
(I have tried with IE 7.)

What is the expected output? What do you see instead?
PRADO outputs the URL inside of TForm's action attribute without escaping
to HTML, causing the JavaScript code to be executed.
IE doesn't further encode URLs typed in the location bar (while Firefox
does, so the problem doesn't occur in Firefox).

What version of the product are you using? On what operating system?
Problem verified in PRADO 3.1.7r2783 source.

Here's a working patch to be applied on Web/UI/TForm.php within function
addAttributesToRender.

51c51
<              
$writer->addAttribute('action',str_replace('&','&amp;',str_replace('&amp;','&',$
uri)));

---
>              
$writer->addAttribute('action',htmlspecialchars(str_replace('&amp;','&',$uri)));

However I wonder if the problem should really be fixed within THtmlWriter
which currently outputs attributes without HTML-encoding them. Doing so
would probably cause other problems though.
The above fix should be safe, as it simply encodes all HTML characters
instead of just "&".

Original issue reported on code.google.com by pbe...@gmail.com on 6 Apr 2010 at 3:22

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

"However I wonder if the problem should really be fixed within THtmlWriter
which currently outputs attributes without HTML-encoding them."
Actually, that's not true, because THtmlWriter DOES encode certain attributes 
(see 
THtmlWriter::_attrEncode array). The problem is that obviously "action" isn't 
listed 
there. So the fix should be rather adding "action" to that array, than touching 
the 
TForm code. Also there are a lot of attributes missing from there, as 
practically 
every and all attributes could be added to that list, except those, which are 
ment 
to contain HTML text (in which case "&" and co. can be valid escapers on their 
own, 
to denote named entities) - but even then '<' and '>' should be _ALWAYS_ 
encoded 
(which unfortunately isn't the case now), because '<' and '>' characters inside 
tags 
(in attribute values/names) are NEVER allowed in HTML, under no circumstances. 
The 
latter security measure alone would have thwarted the attack in the example, 
even 
without "action" being on the to-be-htmlescaped attribute list.

Original comment by google...@pcforum.hu on 7 Apr 2010 at 7:13

[GoogleCodeExporter]

You're right. In addition to '<' and '>', the double-quote '"' should also be 
encoded.

Original comment by pbe...@gmail.com on 8 Apr 2010 at 12:54

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Someone plz commit the fix for this issue. The report is already more than a 
year old and is about a severe issue.

Simply all

  isset(self::$_attrEncode[$name])?THttpUtility::htmlEncode($value):$value

references should be changed to

  THttpUtility::htmlEncode($value)

in THtmlWriter.php as all attribute names and values MUST have all '<', '>' and 
'"' characters encoded in them, regardless of the actual purpose of the 
attribute. Also attribute names MUST NOT be allowed to have those characters in 
them in the first place - so they should be eliminated from them.

Failing to do so only paves the way for severe XSS vulnerabilites in the 
codebase. 

Btw. the same applies to style attribute names and values too.

Original comment by google...@pcforum.hu on 25 Jun 2011 at 11:16

[GoogleCodeExporter]

I've created a test patch and committed in r3005. I don't fully get the reason 
why ampersands are not encoded/stripped, too; that would make us rely on 
htmlspecialchars() instead of our implementation. I've tracked down the change 
to prado 3.0.0rc1 => rc2: http://code.google.com/p/prado3/source/detail?r=890
Comments?

Original comment by ctrlal...@gmail.com on 26 Jun 2011 at 10:02

• Changed state: Started

[GoogleCodeExporter]

Ampersands DO NOT need to be encoded at those places, so it's fine. The only 
characters needing encoding there are <, > and ".

Ampersands need to be encoded in attributes only where the framework stores 
non-HTML-encoded property values in attributes. For ex. if a component/control 
has an URL property, that usually should store the value in a non-HTML-encoded 
form, as outside the markup it makes no sense to have an URL html-encoded. 
However, when rendering that property to HTML code the value must be 
html-encoded first.

On the other side with properties like Caption or Text it makes sense to store 
the values already in HTML-encoded form - thus allowing the programmer to pass 
named entities as their values (so for ex. he can include special marks like 
'»' in navigation elements or crumbs). In this case the properties' value 
obviously MUST NOT be html-encoded when rendering, as that would lead to 
"double-quoting" (and for ex. instead of displaying the double arrow '»' would 
literally render as such - ie. as '»' - in the page).

So, htmlencoding always shall be left to the component code, and can't be done 
automatically, especially not at the html-writer level. At least not in current 
Prado implementations, as any kind of auto-escaping said properties would break 
compatibility with existing code.

Original comment by google...@pcforum.hu on 27 Jun 2011 at 5:49

[GoogleCodeExporter]

One final note: not enforcing property htmlencoding (but encoding < > and " on 
our own) can lead to no security risk, as even though with those 
directly-passed-over-to-markup properties that are expected to be encoded by 
the application programmer already when setting the properties (like for ex. 
Caption and Text in the above example) if the programmer is careless, unencoded 
properties/texts or wrongly encoded properties can "leak" thru, since an 
invalid/incomplete named-entity doesn't affect/disturb parsing of the html code 
and can't lead to premature ending of an attibute name/value or the tag itself, 
it can't be used to inject non-wanted markup/code into the page, neither by 
accident, nor on purpose. 

Original comment by google...@pcforum.hu on 27 Jun 2011 at 5:55

[GoogleCodeExporter]

Original comment by ctrlal...@gmail.com on 27 Jun 2011 at 7:34

• Changed state: Fixed