Re: svn commit: r826196 - /ofbiz/trunk/applications/content/src/org/ofbiz/content/survey/SurveyWrapper.java

[Adam Heath-2]

[hidden email] 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.

[Scott Gray-2]

On 2/12/2009, at 9:23 PM, Adam Heath wrote:

> [hidden email] 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.

[Adam Heath-2]

Scott Gray wrote:

> On 2/12/2009, at 9:23 PM, Adam Heath wrote:
>
>> [hidden email] 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.

[Scott Gray-2]

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:
>>
>>> [hidden email] 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.

[Adam Heath-2]

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?

[Scott Gray-2]

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

[Adam Heath-2]

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.

[Adam Heath-2]

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.