You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@ofbiz.apache.org by le...@apache.org on 2009/10/17 10:40:17 UTC
svn commit: r826196 -
/ofbiz/trunk/applications/content/src/org/ofbiz/content/survey/SurveyWrapper.java
Author: lektran
Date: Sat Oct 17 08:40:17 2009
New Revision: 826196
URL: http://svn.apache.org/viewvc?rev=826196&view=rev
Log:
Fix security issue reported by Alexandre Mazari - OFBIZ-2747
Request parameters were being made available to surveys which in turn were sending them straight back to the browser creating an XSS vulnerability.
This isn't a true fix because I've simply disabled the functionality but at least the security hole is plugged.
Modified:
ofbiz/trunk/applications/content/src/org/ofbiz/content/survey/SurveyWrapper.java
Modified: ofbiz/trunk/applications/content/src/org/ofbiz/content/survey/SurveyWrapper.java
URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/content/src/org/ofbiz/content/survey/SurveyWrapper.java?rev=826196&r1=826195&r2=826196&view=diff
==============================================================================
--- ofbiz/trunk/applications/content/src/org/ofbiz/content/survey/SurveyWrapper.java (original)
+++ ofbiz/trunk/applications/content/src/org/ofbiz/content/survey/SurveyWrapper.java Sat Oct 17 08:40:17 2009
@@ -101,10 +101,13 @@
* @param passThru
*/
public void setPassThru(Map passThru) {
+ /* Creates an XSS vulnerability, by passing incoming parameters straight back out to the browser
+ * commented until someone decides either cleanse the parameters or find an alternative solution
if (passThru != null) {
this.passThru = FastMap.newInstance();
this.passThru.putAll(passThru);
}
+ */
}
/**
Re: svn commit: r826196 - /ofbiz/trunk/applications/content/src/org/ofbiz/content/survey/SurveyWrapper.java
Posted by Adam Heath <do...@brainfood.com>.
Adam Heath wrote:
> Scott Gray wrote:
>> Sorry, yes I have had a look, my initial hope was to explicitly set the
>> parameters that we wanted to be passed through the survey form rather
>> than blindly including the entire map of incoming parameters. This is
>> unfeasible though due to the number of potential parameters coming in
>> when adding a product to the cart. I'm thinking we'll need to come up
>> with a new approach whereby the parameters are temporarily stored in the
>> session and then reloaded as attributes once the survey response has
>> been persisted, unfortunately this is as far as I've had time to go so
>> far. I'll try and get something in place later on today.
>
> It's not a hugely big deal for us at Brainfood. trunk is broken, and
> maybe 09.04, but we created a local ofbiz branch before you did this
> commit, so it's not really holding us back.
Just confirmed that additemsurvey + product stuff is broken in 09.04.
This time, I tried financial account activation.
Re: svn commit: r826196 - /ofbiz/trunk/applications/content/src/org/ofbiz/content/survey/SurveyWrapper.java
Posted by Adam Heath <do...@brainfood.com>.
Scott Gray wrote:
> Sorry, yes I have had a look, my initial hope was to explicitly set the
> parameters that we wanted to be passed through the survey form rather
> than blindly including the entire map of incoming parameters. This is
> unfeasible though due to the number of potential parameters coming in
> when adding a product to the cart. I'm thinking we'll need to come up
> with a new approach whereby the parameters are temporarily stored in the
> session and then reloaded as attributes once the survey response has
> been persisted, unfortunately this is as far as I've had time to go so
> far. I'll try and get something in place later on today.
It's not a hugely big deal for us at Brainfood. trunk is broken, and
maybe 09.04, but we created a local ofbiz branch before you did this
commit, so it's not really holding us back.
Re: svn commit: r826196 - /ofbiz/trunk/applications/content/src/org/ofbiz/content/survey/SurveyWrapper.java
Posted by Scott Gray <sc...@hotwaxmedia.com>.
On 9/12/2009, at 9:26 AM, Adam Heath wrote:
> Scott Gray wrote:
>>>>> This change breaks purchase of gift cards. Go to /ecommerce,
>>>>> select
>>>>> gift card, $100 variant, classic type, add to cart, fill out
>>>>> survey,
>>>>> and then it fails to add to the cart.
>>>>>
>>>>> I would suggest reverting this commit, as having broken
>>>>> functionality
>>>>> is worse than having a security hole.
>>>>>
>>>>> ps: I was able to find this by making use of 'git bisect'. I just
>>>>> love having a copy of all previous ofbiz history.
>>>>
>>>> It's a pretty big security hole, I'd be interested in hearing the
>>>> opinions of others before reverting. Blindly passing all incoming
>>>> parameters back out to the resulting page is pretty bad form and
>>>> needs
>>>> to be fixed.
>>>
>>> These seems to happen when chained requests occur,
>>> /control/additemsurvey/addproduct, or some such. The previous step
>>> then sends in all parameters needed by both requests.
>>>
>>> I wouldn't know how to fix this, as we use our own frontend, not the
>>> controller, widget or minilang systems.
>>
>> I'll try and have a look at it tomorrow, I didn't have a use case for
>> the pass through parameters at the time so I couldn't easily see how
>> they were being used. Now that I have one, it should be easier to
>> come
>> up with an alternate solution.
>
> Have you had a chance to look into this yet?
Sorry, yes I have had a look, my initial hope was to explicitly set
the parameters that we wanted to be passed through the survey form
rather than blindly including the entire map of incoming parameters.
This is unfeasible though due to the number of potential parameters
coming in when adding a product to the cart. I'm thinking we'll need
to come up with a new approach whereby the parameters are temporarily
stored in the session and then reloaded as attributes once the survey
response has been persisted, unfortunately this is as far as I've had
time to go so far. I'll try and get something in place later on today.
Regards
Scott
Re: svn commit: r826196 - /ofbiz/trunk/applications/content/src/org/ofbiz/content/survey/SurveyWrapper.java
Posted by Adam Heath <do...@brainfood.com>.
Scott Gray wrote:
>>>> This change breaks purchase of gift cards. Go to /ecommerce, select
>>>> gift card, $100 variant, classic type, add to cart, fill out survey,
>>>> and then it fails to add to the cart.
>>>>
>>>> I would suggest reverting this commit, as having broken functionality
>>>> is worse than having a security hole.
>>>>
>>>> ps: I was able to find this by making use of 'git bisect'. I just
>>>> love having a copy of all previous ofbiz history.
>>>
>>> It's a pretty big security hole, I'd be interested in hearing the
>>> opinions of others before reverting. Blindly passing all incoming
>>> parameters back out to the resulting page is pretty bad form and needs
>>> to be fixed.
>>
>> These seems to happen when chained requests occur,
>> /control/additemsurvey/addproduct, or some such. The previous step
>> then sends in all parameters needed by both requests.
>>
>> I wouldn't know how to fix this, as we use our own frontend, not the
>> controller, widget or minilang systems.
>
> I'll try and have a look at it tomorrow, I didn't have a use case for
> the pass through parameters at the time so I couldn't easily see how
> they were being used. Now that I have one, it should be easier to come
> up with an alternate solution.
Have you had a chance to look into this yet?
Re: svn commit: r826196 - /ofbiz/trunk/applications/content/src/org/ofbiz/content/survey/SurveyWrapper.java
Posted by Scott Gray <sc...@hotwaxmedia.com>.
On 2/12/2009, at 9:35 PM, Adam Heath wrote:
> Scott Gray wrote:
>> On 2/12/2009, at 9:23 PM, Adam Heath wrote:
>>
>>> lektran@apache.org wrote:
>>>> Author: lektran
>>>> Date: Sat Oct 17 08:40:17 2009
>>>> New Revision: 826196
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=826196&view=rev
>>>> Log:
>>>> Fix security issue reported by Alexandre Mazari - OFBIZ-2747
>>>> Request parameters were being made available to surveys which in
>>>> turn
>>>> were sending them straight back to the browser creating an XSS
>>>> vulnerability.
>>>> This isn't a true fix because I've simply disabled the
>>>> functionality
>>>> but at least the security hole is plugged.
>>>>
>>>> Modified:
>>>>
>>>> ofbiz/trunk/applications/content/src/org/ofbiz/content/survey/
>>>> SurveyWrapper.java
>>>>
>>>
>>> This change breaks purchase of gift cards. Go to /ecommerce, select
>>> gift card, $100 variant, classic type, add to cart, fill out survey,
>>> and then it fails to add to the cart.
>>>
>>> I would suggest reverting this commit, as having broken
>>> functionality
>>> is worse than having a security hole.
>>>
>>> ps: I was able to find this by making use of 'git bisect'. I just
>>> love having a copy of all previous ofbiz history.
>>
>> It's a pretty big security hole, I'd be interested in hearing the
>> opinions of others before reverting. Blindly passing all incoming
>> parameters back out to the resulting page is pretty bad form and
>> needs
>> to be fixed.
>
> These seems to happen when chained requests occur,
> /control/additemsurvey/addproduct, or some such. The previous step
> then sends in all parameters needed by both requests.
>
> I wouldn't know how to fix this, as we use our own frontend, not the
> controller, widget or minilang systems.
I'll try and have a look at it tomorrow, I didn't have a use case for
the pass through parameters at the time so I couldn't easily see how
they were being used. Now that I have one, it should be easier to
come up with an alternate solution.
Re: svn commit: r826196 - /ofbiz/trunk/applications/content/src/org/ofbiz/content/survey/SurveyWrapper.java
Posted by Adam Heath <do...@brainfood.com>.
Scott Gray wrote:
> On 2/12/2009, at 9:23 PM, Adam Heath wrote:
>
>> lektran@apache.org wrote:
>>> Author: lektran
>>> Date: Sat Oct 17 08:40:17 2009
>>> New Revision: 826196
>>>
>>> URL: http://svn.apache.org/viewvc?rev=826196&view=rev
>>> Log:
>>> Fix security issue reported by Alexandre Mazari - OFBIZ-2747
>>> Request parameters were being made available to surveys which in turn
>>> were sending them straight back to the browser creating an XSS
>>> vulnerability.
>>> This isn't a true fix because I've simply disabled the functionality
>>> but at least the security hole is plugged.
>>>
>>> Modified:
>>>
>>> ofbiz/trunk/applications/content/src/org/ofbiz/content/survey/SurveyWrapper.java
>>>
>>
>> This change breaks purchase of gift cards. Go to /ecommerce, select
>> gift card, $100 variant, classic type, add to cart, fill out survey,
>> and then it fails to add to the cart.
>>
>> I would suggest reverting this commit, as having broken functionality
>> is worse than having a security hole.
>>
>> ps: I was able to find this by making use of 'git bisect'. I just
>> love having a copy of all previous ofbiz history.
>
> It's a pretty big security hole, I'd be interested in hearing the
> opinions of others before reverting. Blindly passing all incoming
> parameters back out to the resulting page is pretty bad form and needs
> to be fixed.
These seems to happen when chained requests occur,
/control/additemsurvey/addproduct, or some such. The previous step
then sends in all parameters needed by both requests.
I wouldn't know how to fix this, as we use our own frontend, not the
controller, widget or minilang systems.
Re: svn commit: r826196 - /ofbiz/trunk/applications/content/src/org/ofbiz/content/survey/SurveyWrapper.java
Posted by Scott Gray <sc...@hotwaxmedia.com>.
On 2/12/2009, at 9:23 PM, Adam Heath wrote:
> lektran@apache.org wrote:
>> Author: lektran
>> Date: Sat Oct 17 08:40:17 2009
>> New Revision: 826196
>>
>> URL: http://svn.apache.org/viewvc?rev=826196&view=rev
>> Log:
>> Fix security issue reported by Alexandre Mazari - OFBIZ-2747
>> Request parameters were being made available to surveys which in
>> turn were sending them straight back to the browser creating an XSS
>> vulnerability.
>> This isn't a true fix because I've simply disabled the
>> functionality but at least the security hole is plugged.
>>
>> Modified:
>> ofbiz/trunk/applications/content/src/org/ofbiz/content/survey/
>> SurveyWrapper.java
>
> This change breaks purchase of gift cards. Go to /ecommerce, select
> gift card, $100 variant, classic type, add to cart, fill out survey,
> and then it fails to add to the cart.
>
> I would suggest reverting this commit, as having broken functionality
> is worse than having a security hole.
>
> ps: I was able to find this by making use of 'git bisect'. I just
> love having a copy of all previous ofbiz history.
It's a pretty big security hole, I'd be interested in hearing the
opinions of others before reverting. Blindly passing all incoming
parameters back out to the resulting page is pretty bad form and needs
to be fixed.
Re: svn commit: r826196 - /ofbiz/trunk/applications/content/src/org/ofbiz/content/survey/SurveyWrapper.java
Posted by Adam Heath <do...@brainfood.com>.
lektran@apache.org wrote:
> Author: lektran
> Date: Sat Oct 17 08:40:17 2009
> New Revision: 826196
>
> URL: http://svn.apache.org/viewvc?rev=826196&view=rev
> Log:
> Fix security issue reported by Alexandre Mazari - OFBIZ-2747
> Request parameters were being made available to surveys which in turn were sending them straight back to the browser creating an XSS vulnerability.
> This isn't a true fix because I've simply disabled the functionality but at least the security hole is plugged.
>
> Modified:
> ofbiz/trunk/applications/content/src/org/ofbiz/content/survey/SurveyWrapper.java
This change breaks purchase of gift cards. Go to /ecommerce, select
gift card, $100 variant, classic type, add to cart, fill out survey,
and then it fails to add to the cart.
I would suggest reverting this commit, as having broken functionality
is worse than having a security hole.
ps: I was able to find this by making use of 'git bisect'. I just
love having a copy of all previous ofbiz history.