You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@click.apache.org by Lorenzo Simionato <lo...@simionato.org> on 2010/11/20 12:43:22 UTC
Request Parameter Autobinding
Hi, i've a couple of questions about page "Request Parameter Auto Binding" (section 2.3 of the documentation).
1-Is it possible to disable the feature that: "binds automatically any request parameter values to public Page fields with the same name"?
I see that it is possible for page autobinding but not for request parameters. I find this feature very subtle, makes the code less clear and is
possibly dangerous (class fields can be set by an attacker in a way that is not evident and it is easy to make mistakes).
2-According to the documentation: "When binding these values Click will also attempt to convert them to the correct type". However, if the
conversion is not successful is the intended behavior to throw an exception?
Say i have a page:
public class MyPage extends Page {
@Bindable
protected Integer customerId;
}
and the following request is made: mypage.htm?customerId=xxx
In this case an exception is thrown.
3-Why the @Bindable annotation is used both for request parameters and for page autobinding (if autobinding for pages is enabled of course)?
This makes it very confusing. It is not clear if @Bindable is used to get a parameter or put something on the page.
In addition, it could lead to security problems.
For example, consider the page:
MyPage.java:
public class MyPage extends Page {
@Bindable
protected String welcomeMessage = "Welcome to my web site";
}
MyPage.htm:
$welcomeMessage
and an attacker makes the following request: mypage.htm?welcomeMessage=Fake message
So in this case we have a possible XSS attack.
Thanks,
Lorenzo Simionato
Re: Request Parameter Autobinding
Posted by Bob Schellink <sa...@gmail.com>.
Just a bit of history on binding. In Click 1.4 binding was introduced because people wanted a
shortcut for binding and converting input params. This was in the Java 1.4 days, pre annotation. So
public fields was used as binding. With Java 1.5 Bindable was introduced but backward compatibility
was kept in place. (If you want to use public fields in your pages that should not be bound, you can
set autobinding="annotation" and public fields won't be touched.)
So the intention wasn't to support both public fields and @Bindable. It was for preserving backwards
compatibility.
I understand the argument of splitting @Bindable into two behaviors -> @InputParameter (which sets
and coerce the value vs @AddToModel which adds the value to the model. The Bindable design also
raised these issues:
https://cwiki.apache.org/confluence/display/CLICK/Java+5+Support
but this work was never completed.
Kind regards
Bob
On 21/11/2010 01:49, Lorenzo Simionato wrote:
> On Nov 20, 2010, at 14:35 , Bob Schellink wrote:
>
> Here what i meant is that if you declare a field as @Bindable you are clearly aware that it can be set in some way by the user.
> If you have a public field (ok, it's rare) this is not that obvious.
>
> Here the XSS was just an example. The fact that one can set a value that i intended only for output is disturbing.
> As a couple of other examples:
> -suppose the welcomeMessage is the title of the page. It's not nice that one can put an arbitrary title on the page, even if it is escaped properly.
>
> -suppose one modifies the RequestTypeConverter as explained in the documentation to dynamically load customers from the db.
> In a page one would like to do something with a customer object and then print the details, so we could have something like:
> MyPage.class
> public class MyPage extends Page {
> @Bindable protected Customer customer = loadCustomer(3);
>
> pubic void action() {
> customer.set.....();
> }
>
> MyPage.html
> $customer.name
>
> a different customer can be loaded with a request like mypage.htm?customer=56
> (this example is a little weird but is just to get the idea)
>
>
> These are just examples and maybe if all is handled very carefully by the programmer there would not be any problems.
> However, they demonstrate that it is easy to make something that does not work as intended.
> As a last example consider SQL Injections: if you escape the input properly you do not have the problem. On the other hand,
> to prevent the problem even if you are not that careful PHP has introduced magic quotes and in Java we have preparedStatements (yes i'm simplifying a lot the things here!). The concept is that this double role of public field and ones annotated with @Bindable (as parameters and variables added to the page) it does not seem a good idea to me.
>
> --
> Lorenzo
Re: Request Parameter Autobinding
Posted by Lorenzo Simionato <lo...@simionato.org>.
On Nov 20, 2010, at 14:35 , Bob Schellink wrote:
> Hi,
>
>
> On 20/11/2010 22:43, Lorenzo Simionato wrote:
>>
>> 1-Is it possible to disable the feature that: "binds automatically any request parameter values to public Page fields with the same name"?
>
>
> Yep, just set autobinding="none"
OK. From the user guide it seemed that autobinding="none" was only for Page Autobinding and not for Request Parameters.
>
> Binding request parameters will only be done for "public" or Bindable fields. Java devs generally
> delare fields as private or protected so unlikely to be an issue.
>
>> I see that it is possible for page autobinding but not for request parameters. I find this feature very subtle, makes the code less clear and is
>
> I don't like the feature much either. One thing I'd like to do for a future release is revamping the
> click-examples to not use autobinding.
>
>> possibly dangerous (class fields can be set by an attacker in a way that is not evident and it is easy to make mistakes).
>
> If you declare the field as @Bindable how is it more dangerous than using a Form or link? An
> attacker can tamper with any value.
Here what i meant is that if you declare a field as @Bindable you are clearly aware that it can be set in some way by the user.
If you have a public field (ok, it's rare) this is not that obvious.
>>
>> 2-According to the documentation: "When binding these values Click will also attempt to convert them to the correct type". However, if the
>> conversion is not successful is the intended behavior to throw an exception?
>> Say i have a page:
>> public class MyPage extends Page {
>> @Bindable
>> protected Integer customerId;
>> }
>> and the following request is made: mypage.htm?customerId=xxx
>>
>> In this case an exception is thrown.
>
> Why is this a problem? If your an attacker is tampering with data what should your application do?
Maybe set the field to null, but the exception is okay. Just asking if this was the indented behavior.
>
>
>> 3-Why the @Bindable annotation is used both for request parameters and for page autobinding (if autobinding for pages is enabled of course)?
>> This makes it very confusing. It is not clear if @Bindable is used to get a parameter or put something on the page.
>
> Yeah I don't like it either. Ideally Bindable should only work for request parameters not Controls.
> However this will be difficult to add as majority of existing apps will break. First thing is to
> revamp the examples so people aren't encouraged to use this pattern.
A possibility could be to introduce a new annotation like @InputParameter or something like this to clearly separate the two things.
Backward compatibility can be possibly maintained by adding an option into click.xml (e.g. a new value for the aubinding property)
>
>> In addition, it could lead to security problems.
>> For example, consider the page:
>> MyPage.java:
>> public class MyPage extends Page {
>> @Bindable
>> protected String welcomeMessage = "Welcome to my web site";
>
>
> The security problem doesn't come from Bindable but from rendering without escaping. The Velocity
> template model can be populated from anywhere including forms, arbitrary request params, web
> services, external systems etc. For example:
>
> addModel("msg", getContext().getRequestParameter("welcomeMessage"));
>
> If you are rendering output that could be dangerous you should escape it:
>
> $format.html($msg);
>
> In our app we've gone even further, specifying a Velocity ReferenceInsertionEventHandler.
>
> eventhandler.referenceinsertion.class=org.apache.velocity.app.event.implement.EscapeHtmlReference
> eventhandler.escape.html.match = /_.*/
>
> So any variable in the template that starts with _ is escaped. In our border template we overrode
> addModel to ensure all non control objects added are prefixed with an underscore. Since Controls
> escape their values and attributes we had to exclude them otherwise they are doubly escaped. The net
> effect is that when people rendered Velocity model objects they had to use:
>
> $_customer
>
> Not that user-friendly, but safe.
>
> Kind regards
>
> Bob
Here the XSS was just an example. The fact that one can set a value that i intended only for output is disturbing.
As a couple of other examples:
-suppose the welcomeMessage is the title of the page. It's not nice that one can put an arbitrary title on the page, even if it is escaped properly.
-suppose one modifies the RequestTypeConverter as explained in the documentation to dynamically load customers from the db.
In a page one would like to do something with a customer object and then print the details, so we could have something like:
MyPage.class
public class MyPage extends Page {
@Bindable protected Customer customer = loadCustomer(3);
pubic void action() {
customer.set.....();
}
MyPage.html
$customer.name
a different customer can be loaded with a request like mypage.htm?customer=56
(this example is a little weird but is just to get the idea)
These are just examples and maybe if all is handled very carefully by the programmer there would not be any problems.
However, they demonstrate that it is easy to make something that does not work as intended.
As a last example consider SQL Injections: if you escape the input properly you do not have the problem. On the other hand,
to prevent the problem even if you are not that careful PHP has introduced magic quotes and in Java we have preparedStatements (yes i'm simplifying a lot the things here!). The concept is that this double role of public field and ones annotated with @Bindable (as parameters and variables added to the page) it does not seem a good idea to me.
--
Lorenzo
Re: Request Parameter Autobinding
Posted by Bob Schellink <sa...@gmail.com>.
Hi,
On 20/11/2010 22:43, Lorenzo Simionato wrote:
>
> 1-Is it possible to disable the feature that: "binds automatically any request parameter values to public Page fields with the same name"?
Yep, just set autobinding="none"
Binding request parameters will only be done for "public" or Bindable fields. Java devs generally
delare fields as private or protected so unlikely to be an issue.
> I see that it is possible for page autobinding but not for request parameters. I find this feature very subtle, makes the code less clear and is
I don't like the feature much either. One thing I'd like to do for a future release is revamping the
click-examples to not use autobinding.
> possibly dangerous (class fields can be set by an attacker in a way that is not evident and it is easy to make mistakes).
If you declare the field as @Bindable how is it more dangerous than using a Form or link? An
attacker can tamper with any value.
>
> 2-According to the documentation: "When binding these values Click will also attempt to convert them to the correct type". However, if the
> conversion is not successful is the intended behavior to throw an exception?
> Say i have a page:
> public class MyPage extends Page {
> @Bindable
> protected Integer customerId;
> }
> and the following request is made: mypage.htm?customerId=xxx
>
> In this case an exception is thrown.
Why is this a problem? If your an attacker is tampering with data what should your application do?
> 3-Why the @Bindable annotation is used both for request parameters and for page autobinding (if autobinding for pages is enabled of course)?
> This makes it very confusing. It is not clear if @Bindable is used to get a parameter or put something on the page.
Yeah I don't like it either. Ideally Bindable should only work for request parameters not Controls.
However this will be difficult to add as majority of existing apps will break. First thing is to
revamp the examples so people aren't encouraged to use this pattern.
> In addition, it could lead to security problems.
> For example, consider the page:
> MyPage.java:
> public class MyPage extends Page {
> @Bindable
> protected String welcomeMessage = "Welcome to my web site";
The security problem doesn't come from Bindable but from rendering without escaping. The Velocity
template model can be populated from anywhere including forms, arbitrary request params, web
services, external systems etc. For example:
addModel("msg", getContext().getRequestParameter("welcomeMessage"));
If you are rendering output that could be dangerous you should escape it:
$format.html($msg);
In our app we've gone even further, specifying a Velocity ReferenceInsertionEventHandler.
eventhandler.referenceinsertion.class=org.apache.velocity.app.event.implement.EscapeHtmlReference
eventhandler.escape.html.match = /_.*/
So any variable in the template that starts with _ is escaped. In our border template we overrode
addModel to ensure all non control objects added are prefixed with an underscore. Since Controls
escape their values and attributes we had to exclude them otherwise they are doubly escaped. The net
effect is that when people rendered Velocity model objects they had to use:
$_customer
Not that user-friendly, but safe.
Kind regards
Bob