[Gallery 3] #697: XSS - forge does not escape form input element values

[Andy Staudacher-3]

See: http://sourceforge.net/apps/trac/gallery/ticket/697

 $form = new Forge(...);
 $form->group(..)->input(...)->value($xss_payload)

 $xss_payload (e.g. $item->title or $tag->name) could be anything from user
 input.
 $xss_payload is not escaped / sanitized by forge.
 The generated HTML is just ... value="$xss_payload", thus allowing the
 attacker to break out.

--

What do you think about running all parameters to forge (label, value, ...) through html::clean_attribute()?

I guess I'd patch the forge and form classes directly.

So developers could type:

 $form->group(..)->input(...)

  ->label(t("Foo"))

  ->value($xss_payload)

The alternative is that we expect developers to do things like:

$form->group(..)->input(...)

  ->label(html::clean_attribute(t("Foo")))

  ->value(html::clean_attribute($xss_payload))

Opinions?

 - Andy

---------- Forwarded message ----------
From: gallery - trac <noreply@...>
Date: Mon, Aug 31, 2009 at 1:24 AM
Subject: [Gallery 3] #697: XSS - forge does not escape form input element values
To:


#697: XSS - forge does not escape form input element values
-----------------------+----------------------------------------------------
Reporter:  andy_st     |       Owner:
   Type:  defect      |      Status:  new
Priority:  critical    |   Milestone:  3.0 Beta 3
 Version:  3.0 Beta 2  |    Keywords:  security
-----------------------+----------------------------------------------------
 $form = new Forge(...);
 $form->group(..)->input(...)->value($xss_payload)

 $xss_payload (e.g. $item->title or $tag->name) could be anything from user
 input.
 $xss_payload is not escaped / sanitized by forge.
 The generated HTML is just ... value="$xss_payload", thus allowing the
 attacker to break out.

 Example from the tags module (but it's a pattern used all over G3):

 Here, $tag->name is something that is clearly user input.
 But even t("Save") should be escaped for use as submit button value.

  static function get_rename_form($tag) {
    $form = new Forge("admin/tags/rename/$tag->id", "", "post", array("id"
 => "gRenameTagForm"));
    $group = $form->group("rename_tag")->label(t("Rename Tag"));
    $group->input("name")->label(t("Tag
 name"))->value($tag->name)->rules("required|length[1,64]");
    $group->inputs["name"]->error_messages("in_use", t("There is already a
 tag with that name"));
    $group->submit("")->value(t("Save"));
    return $form;
  }

--
Ticket URL: <http://sourceforge.net/apps/trac/gallery/ticket/697>
Gallery 3 <http://sourceforge.net/projects/gallery/>
Gallery 3

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
__[ g a l l e r y - d e v e l ]_________________________

[ list info/archive --> http://gallery.sf.net/lists.php ]
[ gallery info/FAQ/download --> http://gallery.sf.net ]

[Bharat Mediratta]

Have you verified this in all cases?  I seem to recall that all the
stuff that went through the form helper got escaped, but maybe it's not
using a sufficiently advanced escaping?

Patching Forge directly kind of sucks because:
a) it's magical
b) it's a 3rd party library
c) we're going to replace it

I know that it's more work, but I think I'd prefer to do our cleaning
explicitly, and have a test which verifies that anything that goes into
a Forge object is cleaned.

-Bharat

Andy Staudacher wrote:

> See: http://sourceforge.net/apps/trac/gallery/ticket/697
>
>  $form = new Forge(...);
>  $form->group(..)->input(...)->value($xss_payload)
>
>  $xss_payload (e.g. $item->title or $tag->name) could be anything from user
>  input.
>  $xss_payload is not escaped / sanitized by forge.
>  The generated HTML is just ... value="$xss_payload", thus allowing the
>  attacker to break out.
>
> --
>
> What do you think about running all parameters to forge (label, value,
> ...) through html::clean_attribute()?
> I guess I'd patch the forge and form classes directly.
> So developers could type:
>
>  $form->group(..)->input(...)
>   ->label(t("Foo"))
>   ->value($xss_payload)
>
> The alternative is that we expect developers to do things like:
> $form->group(..)->input(...)
>   ->label(html::clean_attribute(t("Foo")))
>   ->value(html::clean_attribute($xss_payload))
>
> Opinions?
>  - Andy
>
> ---------- Forwarded message ----------
> From: *gallery - trac* <noreply@...
> <mailto:noreply@...>>
> Date: Mon, Aug 31, 2009 at 1:24 AM
> Subject: [Gallery 3] #697: XSS - forge does not escape form input
> element values
> To:
>
>
> #697: XSS - forge does not escape form input element values
> -----------------------+----------------------------------------------------
> Reporter:  andy_st     |       Owner:
>    Type:  defect      |      Status:  new
> Priority:  critical    |   Milestone:  3.0 Beta 3
>  Version:  3.0 Beta 2  |    Keywords:  security
> -----------------------+----------------------------------------------------
>  $form = new Forge(...);
>  $form->group(..)->input(...)->value($xss_payload)
>
>  $xss_payload (e.g. $item->title or $tag->name) could be anything from user
>  input.
>  $xss_payload is not escaped / sanitized by forge.
>  The generated HTML is just ... value="$xss_payload", thus allowing the
>  attacker to break out.
>
>  Example from the tags module (but it's a pattern used all over G3):
>
>  Here, $tag->name is something that is clearly user input.
>  But even t("Save") should be escaped for use as submit button value.
>
>   static function get_rename_form($tag) {
>     $form = new Forge("admin/tags/rename/$tag->id", "", "post", array("id"
>  => "gRenameTagForm"));
>     $group = $form->group("rename_tag")->label(t("Rename Tag"));
>     $group->input("name")->label(t("Tag
>  name"))->value($tag->name)->rules("required|length[1,64]");
>     $group->inputs["name"]->error_messages("in_use", t("There is already a
>  tag with that name"));
>     $group->submit("")->value(t("Save"));
>     return $form;
>   }
>
> --
> Ticket URL: <http://sourceforge.net/apps/trac/gallery/ticket/697>
> Gallery 3 <http://sourceforge.net/projects/gallery/>
> Gallery 3
>
>
> ------------------------------------------------------------------------
>
> ------------------------------------------------------------------------------
> Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
> trial. Simplify your report design, integration and deployment - and focus on
> what you do best, core application coding. Discover what's new with
> Crystal Reports now.  http://p.sf.net/sfu/bobj-july
>
>
> ------------------------------------------------------------------------
>
> __[ g a l l e r y - d e v e l ]_________________________
>
> [ list info/archive --> http://gallery.sf.net/lists.php ]
> [ gallery info/FAQ/download --> http://gallery.sf.net ]

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
__[ g a l l e r y - d e v e l ]_________________________

[ list info/archive --> http://gallery.sf.net/lists.php ]
[ gallery info/FAQ/download --> http://gallery.sf.net ]

[Andy Staudacher-3]

On Mon, Aug 31, 2009 at 11:30 AM, Bharat Mediratta <bharat@...> wrote:

Have you verified this in all cases?

Verified it for forge. Haven't verified it for using form:: directly.

 I seem to recall that all the
stuff that went through the form helper got escaped, but maybe it's not
using a sufficiently advanced escaping?

Patching Forge directly kind of sucks because:
a) it's magical
b) it's a 3rd party library
c) we're going to replace it

I know that it's more work, but I think I'd prefer to do our cleaning
explicitly, and have a test which verifies that anything that goes into
a Forge object is cleaned.

From a developers point of view, I'd prefer if $form_input->label($foo)  was smart enough to escape double quotes in foo before generating the HTML.

There's not much magic to that.

Same goes for ->value($foo), it's not too much asked to expect that the API generates HTML that meets my expectations, which is that $foo won't be able to escape the values="$foo" field.

So I'm definitely in favor of ensuring that these expectations hold.

From a personal point of view, I'm all but keen to write another XSS parser for non view code, looking at form and forge uses.

 - Andy

-Bharat

Andy Staudacher wrote:
> See: http://sourceforge.net/apps/trac/gallery/ticket/697
>
>  $form = new Forge(...);
>  $form->group(..)->input(...)->value($xss_payload)
>
>  $xss_payload (e.g. $item->title or $tag->name) could be anything from user
>  input.
>  $xss_payload is not escaped / sanitized by forge.
>  The generated HTML is just ... value="$xss_payload", thus allowing the
>  attacker to break out.
>
> --
>
> What do you think about running all parameters to forge (label, value,
> ...) through html::clean_attribute()?
> I guess I'd patch the forge and form classes directly.
> So developers could type:
>
>  $form->group(..)->input(...)
>   ->label(t("Foo"))
>   ->value($xss_payload)
>
> The alternative is that we expect developers to do things like:
> $form->group(..)->input(...)
>   ->label(html::clean_attribute(t("Foo")))
>   ->value(html::clean_attribute($xss_payload))
>
> Opinions?
>  - Andy
>
> ---------- Forwarded message ----------
> From: *gallery - trac* <noreply@...

> <mailto:noreply@...>>
> Date: Mon, Aug 31, 2009 at 1:24 AM
> Subject: [Gallery 3] #697: XSS - forge does not escape form input
> element values
> To:
>
>
> #697: XSS - forge does not escape form input element values
> -----------------------+----------------------------------------------------
> Reporter:  andy_st     |       Owner:
>    Type:  defect      |      Status:  new
> Priority:  critical    |   Milestone:  3.0 Beta 3
>  Version:  3.0 Beta 2  |    Keywords:  security
> -----------------------+----------------------------------------------------
>  $form = new Forge(...);
>  $form->group(..)->input(...)->value($xss_payload)
>
>  $xss_payload (e.g. $item->title or $tag->name) could be anything from user
>  input.
>  $xss_payload is not escaped / sanitized by forge.
>  The generated HTML is just ... value="$xss_payload", thus allowing the
>  attacker to break out.
>
>  Example from the tags module (but it's a pattern used all over G3):
>
>  Here, $tag->name is something that is clearly user input.
>  But even t("Save") should be escaped for use as submit button value.
>
>   static function get_rename_form($tag) {
>     $form = new Forge("admin/tags/rename/$tag->id", "", "post", array("id"
>  => "gRenameTagForm"));
>     $group = $form->group("rename_tag")->label(t("Rename Tag"));
>     $group->input("name")->label(t("Tag
>  name"))->value($tag->name)->rules("required|length[1,64]");
>     $group->inputs["name"]->error_messages("in_use", t("There is already a
>  tag with that name"));
>     $group->submit("")->value(t("Save"));
>     return $form;
>   }
>
> --
> Ticket URL: <http://sourceforge.net/apps/trac/gallery/ticket/697>
> Gallery 3 <http://sourceforge.net/projects/gallery/>
> Gallery 3
>
>

> ------------------------------------------------------------------------
>
> ------------------------------------------------------------------------------
> Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
> trial. Simplify your report design, integration and deployment - and focus on
> what you do best, core application coding. Discover what's new with
> Crystal Reports now.  http://p.sf.net/sfu/bobj-july
>
>
> ------------------------------------------------------------------------
>
> __[ g a l l e r y - d e v e l ]_________________________
>
> [ list info/archive --> http://gallery.sf.net/lists.php ]
> [ gallery info/FAQ/download --> http://gallery.sf.net ]

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
__[ g a l l e r y - d e v e l ]_________________________

[ list info/archive --> http://gallery.sf.net/lists.php ]
[ gallery info/FAQ/download --> http://gallery.sf.net ]

[Bharat Mediratta]

Andy Staudacher wrote:

>  From a developers point of view, I'd prefer if $form_input->label($foo)
>  was smart enough to escape double quotes in foo before generating the HTML.
> There's not much magic to that.
> Same goes for ->value($foo), it's not too much asked to expect that the
> API generates HTML that meets my expectations, which is that $foo won't
> be able to escape the values="$foo" field.
>
> So I'm definitely in favor of ensuring that these expectations hold.
>
>  From a personal point of view, I'm all but keen to write another XSS
> parser for non view code, looking at form and forge uses.

Understood.

I think that you can achieve most of this effect by creating
MY_Form_Input and injecting yourself into the __call() method.  If we
can do this without affecting the upstream code it'll save us some grief.

Alternative: we just fork Forge now and be done with it, expecting that
we're going to write it off in 3.1 and replace it with Formo.  I think
that for clarity, I'd prefer to avoid the fork if we can.

-Bharat

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
__[ g a l l e r y - d e v e l ]_________________________

[ list info/archive --> http://gallery.sf.net/lists.php ]
[ gallery info/FAQ/download --> http://gallery.sf.net ]

[Andy Staudacher-3]

On Mon, Aug 31, 2009 at 12:35 PM, Bharat Mediratta <bharat@...> wrote:

Andy Staudacher wrote:

 From a developers point of view, I'd prefer if $form_input->label($foo)  was smart enough to escape double quotes in foo before generating the HTML.
There's not much magic to that.
Same goes for ->value($foo), it's not too much asked to expect that the API generates HTML that meets my expectations, which is that $foo won't be able to escape the values="$foo" field.

So I'm definitely in favor of ensuring that these expectations hold.

 From a personal point of view, I'm all but keen to write another XSS parser for non view code, looking at form and forge uses.

Understood.

I think that you can achieve most of this effect by creating MY_Form_Input and injecting yourself into the __call() method.  If we can do this without affecting the upstream code it'll save us some grief.

Alternative: we just fork Forge now and be done with it, expecting that we're going to write it off in 3.1 and replace it with Formo.  I think that for clarity, I'd prefer to avoid the fork if we can.

You were right in that the form helper did escaping in most places already. It had a few holes though.

Forge uses form, and where it doesn't, it adds a few holes.

For now, I've fixed it in system/helper/form.php and modules/forge/libraries/Form_*.php.

And the actual bug (tags module) was a JS bug in the tags module, which is fixed now as well.

I'd say I'm done now with XSS security stuff.

Which is good timing. I need to take a break from G3 for a few days, got some family visit arriving tomorrow.

 - Andy

-Bharat

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
__[ g a l l e r y - d e v e l ]_________________________

[ list info/archive --> http://gallery.sf.net/lists.php ]
[ gallery info/FAQ/download --> http://gallery.sf.net ]

[Bharat Mediratta]

Andy Staudacher wrote:
> You were right in that the form helper did escaping in most places
> already. It had a few holes though.
> Forge uses form, and where it doesn't, it adds a few holes.
>
> For now, I've fixed it in system/helper/form.php and
> modules/forge/libraries/Form_*.php.

Doing it this way is an undocumented and incompatible change to an
upstream module.  This essentially means that we're permanently forking
Forge.  That's the least attractive of all options here from my
perspective because it means that we're stuck with the change and have
to deal with it properly moving forward (no matter which direction we go
in).

Is there an open ticket to resolve this incompatibility in some way?
I'd say that we're not done until this approach is fully defined for the
long term.

-Bharat


------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
__[ g a l l e r y - d e v e l ]_________________________

[ list info/archive --> http://gallery.sf.net/lists.php ]
[ gallery info/FAQ/download --> http://gallery.sf.net ]

[Andy Staudacher-3]

On Tue, Sep 1, 2009 at 9:16 AM, Bharat Mediratta <bharat@...> wrote:

Andy Staudacher wrote:

You were right in that the form helper did escaping in most places already. It had a few holes though.
Forge uses form, and where it doesn't, it adds a few holes.

For now, I've fixed it in system/helper/form.php and modules/forge/libraries/Form_*.php.

Doing it this way is an undocumented and incompatible change to an upstream module.  This essentially means that we're permanently forking Forge.  That's the least attractive of all options here from my perspective because it means that we're stuck with the change and have to deal with it properly moving forward (no matter which direction we go in).

Is there an open ticket to resolve this incompatibility in some way? I'd say that we're not done until this approach is fully defined for the long term.

#1 concern is to resolve security issues, and from that perspective, we're done.

But I fully agree that we need to make this maintainable. I've created a ticket to track it.

See: https://sourceforge.net/apps/trac/gallery/ticket/707

 - Andy

-Bharat

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
__[ g a l l e r y - d e v e l ]_________________________

[ list info/archive --> http://gallery.sf.net/lists.php ]
[ gallery info/FAQ/download --> http://gallery.sf.net ]