question about ServiceHandler.checkSecureParameter

[Samuel-2]

Hi,

recently I run against this check method which throw me an error to
prevent me accessing url parameters from a service. Error message
mentions a security reason to forbid accessing url parameters but I
definitely don't get this, so could you explain to me the "security"
reason ? or could we simply remove this check ?

Samuel

PS: I've also checked mentionned jira issue
https://issues.apache.org/jira/browse/OFBIZ-2330, but this didn't help
me understanding the "security" reason

[Samuel-2]

oups it's not about ServiceHandler class but ServiceEventHandler class

On 18/10/2019 10:08, Samuel wrote:

> Hi,
>
> recently I run against this check method which throw me an error to
> prevent me accessing url parameters from a service. Error message
> mentions a security reason to forbid accessing url parameters but I
> definitely don't get this, so could you explain to me the "security"
> reason ? or could we simply remove this check ?
>
> Samuel
>
> PS: I've also checked mentionned jira issue
> https://issues.apache.org/jira/browse/OFBIZ-2330, but this didn't help
> me understanding the "security" reason

[Jacques Le Roux]

Hi Samuel,

It started with http://svn.apache.org/viewvc?view=revision&revision=764286

Then I did http://svn.apache.org/viewvc?view=revision&revision=767688

Then I created OFBIZ-2330 after OFBIZ-2332, OFBIZ-2260, OFBIZ-2256

About removing, there are still few cases popping up. What is your case? Is it relevant?

You are not the 1st one to question the security aspect, I commented that here:  https://s.apache.org/4z2bt

Thanks

Jacques

Le 18/10/2019 à 10:08, Samuel a écrit :

> Hi,
>
> recently I run against this check method which throw me an error to prevent me accessing url parameters from a service. Error message mentions a
> security reason to forbid accessing url parameters but I definitely don't get this, so could you explain to me the "security" reason ? or could we
> simply remove this check ?
>
> Samuel
>
> PS: I've also checked mentionned jira issue https://issues.apache.org/jira/browse/OFBIZ-2330, but this didn't help me understanding the "security"
> reason
>

[Samuel-2]

Hi Jacques,

thanks for your detail and quick answer !
I still can't see the point with this check :s What kind of attack this
check is protecting us ? could you describe an attack scenario where
this check is a good protection ?

My use case is to be able to access url parameters in an event service,
I see that I can bypass the check with
`service.http.parameters.require.encrypted` property but still I really
want to understand the point with this check ;)

Samuel

On 18/10/2019 10:48, Jacques Le Roux wrote:

> Hi Samuel,
>
> It started with http://svn.apache.org/viewvc?view=revision&revision=764286
>
> Then I did http://svn.apache.org/viewvc?view=revision&revision=767688
>
> Then I created OFBIZ-2330 after OFBIZ-2332, OFBIZ-2260, OFBIZ-2256
>
> About removing, there are still few cases popping up. What is your case?
> Is it relevant?
>
> You are not the 1st one to question the security aspect, I commented
> that here:  https://s.apache.org/4z2bt
>
> Thanks
>
> Jacques
>
> Le 18/10/2019 à 10:08, Samuel a écrit :
>> Hi,
>>
>> recently I run against this check method which throw me an error to
>> prevent me accessing url parameters from a service. Error message
>> mentions a security reason to forbid accessing url parameters but I
>> definitely don't get this, so could you explain to me the "security"
>> reason ? or could we simply remove this check ?
>>
>> Samuel
>>
>> PS: I've also checked mentionned jira issue
>> https://issues.apache.org/jira/browse/OFBIZ-2330, but this didn't help
>> me understanding the "security" reason
>>

[Jacques Le Roux]

Samuel,

This was initiated by David (the founder of the project). It was not much discussed at the time. I guess he had a possible attack scenario in head, or
faced one.

As he commented in ServiceEventHandler::checkSecureParameter

    // special case for security: if this is a request-map defined as secure in controller.xml then only accept body parameters coming in, ie don't allow the insecure URL parameters
    // NOTTODO: may want to allow parameters that map to entity PK fields to be in the URL, but that might be a big security hole since there are certain security sensitive entities that are made of only PK fields, or that only need PK fields to function (like  UserLoginSecurityGroup)
    // NOTTODO: we could allow URL parameters when it is not a POST (ie when !request.getMethod().equalsIgnoreCase("POST")), but that would open a security hole where sensitive parameters can be passed on the URL in a GET/etc and bypass this security constraint

This article is related: https://www.owasp.org/index.php/Information_exposure_through_query_strings_in_url

Here is an use case https://www.fullcontact.com/blog/never-put-secrets-urls-query-parameters/

We could consider a CSRF attack. An attacker could create an email similar to the one we use to allow users to change passwords. In the fake email an
underneath URL could be

.../partymgr/control/ProfileAddUserLoginToSecurityGroup?userLoginId=ExistingEcommerceUser&partyId=ExistingEcommerceUser&AddUserLoginSecurityGroup=FULLADMIN

I let you imagine more :)

Jacques

Le 18/10/2019 à 11:28, Samuel a écrit :

> Hi Jacques,
>
> thanks for your detail and quick answer !
> I still can't see the point with this check :s What kind of attack this check is protecting us ? could you describe an attack scenario where this
> check is a good protection ?
>
> My use case is to be able to access url parameters in an event service, I see that I can bypass the check with
> `service.http.parameters.require.encrypted` property but still I really want to understand the point with this check ;)
>
> Samuel
>
> On 18/10/2019 10:48, Jacques Le Roux wrote:
>> Hi Samuel,
>>
>> It started with http://svn.apache.org/viewvc?view=revision&revision=764286
>>
>> Then I did http://svn.apache.org/viewvc?view=revision&revision=767688
>>
>> Then I created OFBIZ-2330 after OFBIZ-2332, OFBIZ-2260, OFBIZ-2256
>>
>> About removing, there are still few cases popping up. What is your case? Is it relevant?
>>
>> You are not the 1st one to question the security aspect, I commented that here:  https://s.apache.org/4z2bt
>>
>> Thanks
>>
>> Jacques
>>
>> Le 18/10/2019 à 10:08, Samuel a écrit :
>>> Hi,
>>>
>>> recently I run against this check method which throw me an error to prevent me accessing url parameters from a service. Error message mentions a
>>> security reason to forbid accessing url parameters but I definitely don't get this, so could you explain to me the "security" reason ? or could we
>>> simply remove this check ?
>>>
>>> Samuel
>>>
>>> PS: I've also checked mentionned jira issue https://issues.apache.org/jira/browse/OFBIZ-2330, but this didn't help me understanding the "security"
>>> reason
>>>
>

[Samuel-2]

Jacques,

I agree that sending sensible data in url is definitely not a good
practice, but I think it is overkill to forbid all url parameters to
achieve this security goal.

Moreover if you don't use a service event in your request map you can
access whatever url parameter you want, so I cannot see why service
event is so particular in this regards.

Again my use case is to access url parameters in a service like
accessing view_size, or view_index which is definitely not sensible
information.


Samuel

On 18/10/2019 16:21, Jacques Le Roux wrote:

> Samuel,
>
> This was initiated by David (the founder of the project). It was not
> much discussed at the time. I guess he had a possible attack scenario in
> head, or faced one.
>
> As he commented in ServiceEventHandler::checkSecureParameter
>
>     // special case for security: if this is a request-map defined as
> secure in controller.xml then only accept body parameters coming in, ie
> don't allow the insecure URL parameters
>     // NOTTODO: may want to allow parameters that map to entity PK
> fields to be in the URL, but that might be a big security hole since
> there are certain security sensitive entities that are made of only PK
> fields, or that only need PK fields to function (like  
> UserLoginSecurityGroup)
>     // NOTTODO: we could allow URL parameters when it is not a POST (ie
> when !request.getMethod().equalsIgnoreCase("POST")), but that would open
> a security hole where sensitive parameters can be passed on the URL in a
> GET/etc and bypass this security constraint
>
> This article is related:
> https://www.owasp.org/index.php/Information_exposure_through_query_strings_in_url 
>
>
> Here is an use case
> https://www.fullcontact.com/blog/never-put-secrets-urls-query-parameters/
>
> We could consider a CSRF attack. An attacker could create an email
> similar to the one we use to allow users to change passwords. In the
> fake email an underneath URL could be
>
> .../partymgr/control/ProfileAddUserLoginToSecurityGroup?userLoginId=ExistingEcommerceUser&partyId=ExistingEcommerceUser&AddUserLoginSecurityGroup=FULLADMIN
>
>
> I let you imagine more :)
>
> Jacques
>
> Le 18/10/2019 à 11:28, Samuel a écrit :
>> Hi Jacques,
>>
>> thanks for your detail and quick answer !
>> I still can't see the point with this check :s What kind of attack
>> this check is protecting us ? could you describe an attack scenario
>> where this check is a good protection ?
>>
>> My use case is to be able to access url parameters in an event
>> service, I see that I can bypass the check with
>> `service.http.parameters.require.encrypted` property but still I
>> really want to understand the point with this check ;)
>>
>> Samuel
>>
>> On 18/10/2019 10:48, Jacques Le Roux wrote:
>>> Hi Samuel,
>>>
>>> It started with
>>> http://svn.apache.org/viewvc?view=revision&revision=764286
>>>
>>> Then I did http://svn.apache.org/viewvc?view=revision&revision=767688
>>>
>>> Then I created OFBIZ-2330 after OFBIZ-2332, OFBIZ-2260, OFBIZ-2256
>>>
>>> About removing, there are still few cases popping up. What is your
>>> case? Is it relevant?
>>>
>>> You are not the 1st one to question the security aspect, I commented
>>> that here:  https://s.apache.org/4z2bt
>>>
>>> Thanks
>>>
>>> Jacques
>>>
>>> Le 18/10/2019 à 10:08, Samuel a écrit :
>>>> Hi,
>>>>
>>>> recently I run against this check method which throw me an error to
>>>> prevent me accessing url parameters from a service. Error message
>>>> mentions a security reason to forbid accessing url parameters but I
>>>> definitely don't get this, so could you explain to me the "security"
>>>> reason ? or could we simply remove this check ?
>>>>
>>>> Samuel
>>>>
>>>> PS: I've also checked mentionned jira issue
>>>> https://issues.apache.org/jira/browse/OFBIZ-2330, but this didn't
>>>> help me understanding the "security" reason
>>>>
>>
>

[Jacques Le Roux]

Hi Samuel,

I agree that it's an overkill. I guess because services give you more power so need to be more secure.

The same person that, like you, doubted about the reason of checkSecureParameter() spoke also about the possibility to "inject stuff using parameters"

Here is what he said (actually this was reported to me by a 3rd person but I much trust them both)

    "The other case I reported has more to do with people being able to inject stuff using parameters. Because they are not always escaped. In
    particular the case for the VIEW_INDEX parameter and alike  view_size, view_index often in combination with screen widget but not only"

Anyway what would you suggest? You know you can set service.http.parameters.require.encrypted=N, is that not enough?

Jacques

Le 18/10/2019 à 16:48, Samuel a écrit :

> Jacques,
>
> I agree that sending sensible data in url is definitely not a good practice, but I think it is overkill to forbid all url parameters to achieve this
> security goal.
>
> Moreover if you don't use a service event in your request map you can access whatever url parameter you want, so I cannot see why service event is
> so particular in this regards.
>
> Again my use case is to access url parameters in a service like accessing view_size, or view_index which is definitely not sensible information.
>
>
> Samuel
>
> On 18/10/2019 16:21, Jacques Le Roux wrote:
>> Samuel,
>>
>> This was initiated by David (the founder of the project). It was not much discussed at the time. I guess he had a possible attack scenario in head,
>> or faced one.
>>
>> As he commented in ServiceEventHandler::checkSecureParameter
>>
>>     // special case for security: if this is a request-map defined as secure in controller.xml then only accept body parameters coming in, ie don't
>> allow the insecure URL parameters
>>     // NOTTODO: may want to allow parameters that map to entity PK fields to be in the URL, but that might be a big security hole since there are
>> certain security sensitive entities that are made of only PK fields, or that only need PK fields to function (like  UserLoginSecurityGroup)
>>     // NOTTODO: we could allow URL parameters when it is not a POST (ie when !request.getMethod().equalsIgnoreCase("POST")), but that would open a
>> security hole where sensitive parameters can be passed on the URL in a GET/etc and bypass this security constraint
>>
>> This article is related: https://www.owasp.org/index.php/Information_exposure_through_query_strings_in_url
>>
>> Here is an use case https://www.fullcontact.com/blog/never-put-secrets-urls-query-parameters/
>>
>> We could consider a CSRF attack. An attacker could create an email similar to the one we use to allow users to change passwords. In the fake email
>> an underneath URL could be
>>
>> .../partymgr/control/ProfileAddUserLoginToSecurityGroup?userLoginId=ExistingEcommerceUser&partyId=ExistingEcommerceUser&AddUserLoginSecurityGroup=FULLADMIN
>>
>>
>> I let you imagine more :)
>>
>> Jacques
>>
>> Le 18/10/2019 à 11:28, Samuel a écrit :
>>> Hi Jacques,
>>>
>>> thanks for your detail and quick answer !
>>> I still can't see the point with this check :s What kind of attack this check is protecting us ? could you describe an attack scenario where this
>>> check is a good protection ?
>>>
>>> My use case is to be able to access url parameters in an event service, I see that I can bypass the check with
>>> `service.http.parameters.require.encrypted` property but still I really want to understand the point with this check ;)
>>>
>>> Samuel
>>>
>>> On 18/10/2019 10:48, Jacques Le Roux wrote:
>>>> Hi Samuel,
>>>>
>>>> It started with http://svn.apache.org/viewvc?view=revision&revision=764286
>>>>
>>>> Then I did http://svn.apache.org/viewvc?view=revision&revision=767688
>>>>
>>>> Then I created OFBIZ-2330 after OFBIZ-2332, OFBIZ-2260, OFBIZ-2256
>>>>
>>>> About removing, there are still few cases popping up. What is your case? Is it relevant?
>>>>
>>>> You are not the 1st one to question the security aspect, I commented that here:  https://s.apache.org/4z2bt
>>>>
>>>> Thanks
>>>>
>>>> Jacques
>>>>
>>>> Le 18/10/2019 à 10:08, Samuel a écrit :
>>>>> Hi,
>>>>>
>>>>> recently I run against this check method which throw me an error to prevent me accessing url parameters from a service. Error message mentions a
>>>>> security reason to forbid accessing url parameters but I definitely don't get this, so could you explain to me the "security" reason ? or could
>>>>> we simply remove this check ?
>>>>>
>>>>> Samuel
>>>>>
>>>>> PS: I've also checked mentionned jira issue https://issues.apache.org/jira/browse/OFBIZ-2330, but this didn't help me understanding the
>>>>> "security" reason
>>>>>
>>>
>>
>

[Mathieu Lirzin]

Hello,

Samuel <[hidden email]> writes:

> Moreover if you don't use a service event in your request map you can
> access whatever url parameter you want, so I cannot see why service
> event is so particular in this regards.

Indeed if the issue is about forbidding URI parameters because of
security reason, this check should be hardcoded in the RequestHandler
not by individual EventHandler implementations.

Otherwise this is just absurd because every administrator/integrator can
then implement an ad-hoc Java/Groovy event handler invoking a service
without being warned about a “known” security issue.

Jacques Le Roux <[hidden email]> writes:

> I agree that it's an overkill. I guess because services give you more
> power so need to be more secure.
>
> The same person that, like you, doubted about the reason of
> checkSecureParameter() spoke also about the possibility to "inject
> stuff using parameters"
>
> Here is what he said (actually this was reported to me by a 3rd person
> but I much trust them both)
>
>    "The other case I reported has more to do with people being able to
>    inject stuff using parameters. Because they are not always
>    escaped. In particular the case for the VIEW_INDEX parameter and
>    alike  view_size, view_index often in combination with screen
>    widget but not only"
>
> Anyway what would you suggest? You know you can set
> service.http.parameters.require.encrypted=N, is that not enough?

I am not convinced by the explanation or by the non-solution of keeping
an option with confusing security impacts.

IMO for the sake of both simplicity and sanity we should just nuke the
option and accept URI parameters unconditionally.

--
Mathieu Lirzin
GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37

[Samuel-2]

Hi,

On 20/10/2019 12:27, Mathieu Lirzin wrote:

> Hello,
>
> Samuel <[hidden email]> writes:
>
>> Moreover if you don't use a service event in your request map you can
>> access whatever url parameter you want, so I cannot see why service
>> event is so particular in this regards.
>
> Indeed if the issue is about forbidding URI parameters because of
> security reason, this check should be hardcoded in the RequestHandler
> not by individual EventHandler implementations.
>
> Otherwise this is just absurd because every administrator/integrator can
> then implement an ad-hoc Java/Groovy event handler invoking a service
> without being warned about a “known” security issue.
>
> Jacques Le Roux <[hidden email]> writes:
>
>> I agree that it's an overkill. I guess because services give you more
>> power so need to be more secure.
>>
>> The same person that, like you, doubted about the reason of
>> checkSecureParameter() spoke also about the possibility to "inject
>> stuff using parameters"
>>
>> Here is what he said (actually this was reported to me by a 3rd person
>> but I much trust them both)
>>
>>     "The other case I reported has more to do with people being able to
>>     inject stuff using parameters. Because they are not always
>>     escaped. In particular the case for the VIEW_INDEX parameter and
>>     alike  view_size, view_index often in combination with screen
>>     widget but not only"

If I'm correct this is related to XSS attack [1] but this kind of attack
is not limited to url parameters. An attacker can do the same thing with
a POST request (I mean parameter in body instead of url)

>>
>> Anyway what would you suggest? You know you can set
>> service.http.parameters.require.encrypted=N, is that not enough?
>
> I am not convinced by the explanation or by the non-solution of keeping
> an option with confusing security impacts.
>
> IMO for the sake of both simplicity and sanity we should just nuke the
> option and accept URI parameters unconditionally.
>

Agree with Mathieu, an option to desactivate security check with no
clear impact seems to me a bad idea.

So as Mathieu said to make thing simpler I'd like to remove this
function. In my opinion security is mainly about good practices, if we
want to increase security maybe we should provide documentation.

Samuel



[1]: https://en.wikipedia.org/wiki/Cross-site_scripting

[Samuel-2]

Hi all,

my conclusion from previous discussion is that there is no good reason
for checkSecureParameter. So to make ofbiz code simpler I suggest to
remove it.

Here is a Jira issue with patch attached

   https://issues.apache.org/jira/browse/OFBIZ-11260


Samuel

[Jacques Le Roux]

Hi Samuel, Mathieu,

Le 21/10/2019 à 09:43, Samuel a écrit :
>
> If I'm correct this is related to XSS attack [1] but this kind of attack is not limited to url parameters. An attacker can do the same thing with a
> POST request (I mean parameter in body instead of url)

You are right, they just are harder. You need to use CSRF, or an existing stored XSS[1]. We don't handle CSRF yet, with OFBIZ-10427 we are working on
it. Anyway, even if we don't have any known at the moment, we can't never guarantee a stored XSS . So yes it's theoretically possible in 2 more
difficult ways than a simple GET. I guess that's why this was implemented in 1st place. Again, because you can do a lot more with services than with
events:

    "A service with the type Java is much like an event where it is a static method, however with the Services Framework we do not limit to web based
    applications."[2]

>
>>>
>>> Anyway what would you suggest? You know you can set
>>> service.http.parameters.require.encrypted=N, is that not enough?
>>
>> I am not convinced by the explanation or by the non-solution of keeping
>> an option with confusing security impacts.
>>
>> IMO for the sake of both simplicity and sanity we should just nuke the
>> option and accept URI parameters unconditionally.
>>
>
> Agree with Mathieu, an option to desactivate security check with no clear impact seems to me a bad idea.
>
> So as Mathieu said to make thing simpler I'd like to remove this function. In my opinion security is mainly about good practices, if we want to
> increase security maybe we should provide documentation.
>
> Samuel

It was just that attacking with GET is easier than with POST. A lot has already been done with OFBIZ-2330 and related. We did not have much returns
since a while. So I have no problem removing this method... and closing OFBIZ-2330, maybe after "fixing" OFBIZ-9804...

[1] https://security.stackexchange.com/questions/175679/how-to-exploit-xss-in-post-request-when-parameter-is-going-in-body
[2] https://cwiki.apache.org/confluence/display/OFBIZ/Service+Engine+Guide

Jacques

[Samuel-2]

Hi Jacques,

On 27/10/2019 17:42, Jacques Le Roux wrote:

> … So I have no problem removing this method... and
> closing OFBIZ-2330, maybe after "fixing" OFBIZ-9804...

I'm not sure to get your point with OFBIZ-9804, if we simply remove
`checkSecureParameter` we fix this issue, don't we ?

Samuel

[Jacques Le Roux]

Le 30/10/2019 à 15:34, Samuel a écrit :

> Hi Jacques,
>
> On 27/10/2019 17:42, Jacques Le Roux wrote:
>
>> … So I have no problem removing this method... and closing OFBIZ-2330, maybe after "fixing" OFBIZ-9804...
>
> I'm not sure to get your point with OFBIZ-9804, if we simply remove `checkSecureParameter` we fix this issue, don't we ?
>
> Samuel
>

Yes, kinda. I prefer to have all calls to updateContactListPartyNoUserLogin similar. Please wait a bit before I close OFBIZ-9804...

Jacques

[Jacques Le Roux]

Hi Samuel,

You can go ahead. I became entangled with non ending issues while working on this and this change will not change anything about those unrelated issues.

Jacques

Le 30/10/2019 à 17:01, Jacques Le Roux a écrit :

> Le 30/10/2019 à 15:34, Samuel a écrit :
>> Hi Jacques,
>>
>> On 27/10/2019 17:42, Jacques Le Roux wrote:
>>
>>> … So I have no problem removing this method... and closing OFBIZ-2330, maybe after "fixing" OFBIZ-9804...
>>
>> I'm not sure to get your point with OFBIZ-9804, if we simply remove `checkSecureParameter` we fix this issue, don't we ?
>>
>> Samuel
>>
> Yes, kinda. I prefer to have all calls to updateContactListPartyNoUserLogin similar. Please wait a bit before I close OFBIZ-9804...
>
> Jacques
>
>

[James Yong-2]

Hi Jacques, Samuel, all,

I think the security concerns raised are valid.

However we can look into adding an attribute when the url parameter check isn’t required.
For example,
<request-map … >
    
  <security https="true" auth=“true” allow-query-string-for-service-event=“true”/>
    
  …
 
Regards,
James

On 2019/10/31 14:20:11, Jacques Le Roux <[hidden email]> wrote:

> Hi Samuel,
>
> You can go ahead. I became entangled with non ending issues while working on this and this change will not change anything about those unrelated issues.
>
> Jacques
>
> Le 30/10/2019 à 17:01, Jacques Le Roux a écrit :
> > Le 30/10/2019 à 15:34, Samuel a écrit :
> >> Hi Jacques,
> >>
> >> On 27/10/2019 17:42, Jacques Le Roux wrote:
> >>
> >>> … So I have no problem removing this method... and closing OFBIZ-2330, maybe after "fixing" OFBIZ-9804...
> >>
> >> I'm not sure to get your point with OFBIZ-9804, if we simply remove `checkSecureParameter` we fix this issue, don't we ?
> >>
> >> Samuel
> >>
> > Yes, kinda. I prefer to have all calls to updateContactListPartyNoUserLogin similar. Please wait a bit before I close OFBIZ-9804...
> >
> > Jacques
> >
> >
>

[Jacques Le Roux]

Hi James,

We had service.http.parameters.require.encrypted in url.properties. It's was the same but an all or nothing. It's now removed.

I'm not against your late proposition. It now means to revert changes from OFBIZ-11260!

But then it should be reversed. By default allow-query-string-for-service-event would be true and if someone wants to prevent a query string for a
particular event then false can be used.

I'm not sure much people will care of that, not sure what others think...

Jacques

Le 05/11/2019 à 01:28, James Yong a écrit :

> Hi Jacques, Samuel, all,
>
> I think the security concerns raised are valid.
>
> However we can look into adding an attribute when the url parameter check isn’t required.
> For example,
> <request-map … >

>    <security https="true" auth=“true” allow-query-string-for-service-event=“true”/>

>    …
>  
> Regards,
> James
>
> On 2019/10/31 14:20:11, Jacques Le Roux <[hidden email]> wrote:
>> Hi Samuel,
>>
>> You can go ahead. I became entangled with non ending issues while working on this and this change will not change anything about those unrelated issues.
>>
>> Jacques
>>
>> Le 30/10/2019 à 17:01, Jacques Le Roux a écrit :
>>> Le 30/10/2019 à 15:34, Samuel a écrit :
>>>> Hi Jacques,
>>>>
>>>> On 27/10/2019 17:42, Jacques Le Roux wrote:
>>>>
>>>>> … So I have no problem removing this method... and closing OFBIZ-2330, maybe after "fixing" OFBIZ-9804...
>>>> I'm not sure to get your point with OFBIZ-9804, if we simply remove `checkSecureParameter` we fix this issue, don't we ?
>>>>
>>>> Samuel
>>>>
>>> Yes, kinda. I prefer to have all calls to updateContactListPartyNoUserLogin similar. Please wait a bit before I close OFBIZ-9804...
>>>
>>> Jacques
>>>
>>>

[James Yong-2]

Hi Jacques,

Understand the intent of checkSecureParameter function is to avoid sensitive information
in the URL during POST method. A proposal is made to provide an attribute (i.e. allow-query-string-for-service-event) to allow url parameters / query string for certain request. Shouldn't the value for this attribute be false, instead of true, when no value is specified for the attribute?

The property service.http.parameters.require.encrypted is also not required for the proposed change.

As a side note, checkSecureParameter function restricts CSRF attack to a POST method but doesn't prevent CSRF entirely. Maybe can explore the feasibility of supporting CSRF token for OFBiz form in future..

Regards,
James



On 2019/11/05 07:47:19, Jacques Le Roux <[hidden email]> wrote:

> Hi James,
>
> We had service.http.parameters.require.encrypted in url.properties. It's was the same but an all or nothing. It's now removed.
>
> I'm not against your late proposition. It now means to revert changes from OFBIZ-11260!
>
> But then it should be reversed. By default allow-query-string-for-service-event would be true and if someone wants to prevent a query string for a
> particular event then false can be used.
>
> I'm not sure much people will care of that, not sure what others think...
>
> Jacques
>
> Le 05/11/2019 à 01:28, James Yong a écrit :
> > Hi Jacques, Samuel, all,
> >
> > I think the security concerns raised are valid.
> >
> > However we can look into adding an attribute when the url parameter check isn’t required.
> > For example,
> > <request-map … >
>
> >    <security https="true" auth=“true” allow-query-string-for-service-event=“true”/>
>
> >    …
> >  
> > Regards,
> > James
> >
> > On 2019/10/31 14:20:11, Jacques Le Roux <[hidden email]> wrote:
> >> Hi Samuel,
> >>
> >> You can go ahead. I became entangled with non ending issues while working on this and this change will not change anything about those unrelated issues.
> >>
> >> Jacques
> >>
> >> Le 30/10/2019 à 17:01, Jacques Le Roux a écrit :
> >>> Le 30/10/2019 à 15:34, Samuel a écrit :
> >>>> Hi Jacques,
> >>>>
> >>>> On 27/10/2019 17:42, Jacques Le Roux wrote:
> >>>>
> >>>>> … So I have no problem removing this method... and closing OFBIZ-2330, maybe after "fixing" OFBIZ-9804...
> >>>> I'm not sure to get your point with OFBIZ-9804, if we simply remove `checkSecureParameter` we fix this issue, don't we ?
> >>>>
> >>>> Samuel
> >>>>
> >>> Yes, kinda. I prefer to have all calls to updateContactListPartyNoUserLogin similar. Please wait a bit before I close OFBIZ-9804...
> >>>
> >>> Jacques
> >>>
> >>>
>

[Jacques Le Roux]

Hi James,

Inline...

Le 06/11/2019 à 17:53, James Yong a écrit :
> Hi Jacques,
>
> Understand the intent of checkSecureParameter function is to avoid sensitive information
> in the URL during POST method.

Was ;) It does not exist anymore, in trunk at least, after OFBIZ-11260.


> A proposal is made to provide an attribute (i.e. allow-query-string-for-service-event) to allow url parameters / query string for certain request. Shouldn't the value for this attribute be false, instead of true, when no value is specified for the attribute?

Depends, if we want to have the same behaviour than in trunk after OFBIZ-11260 then it should be false by default. Note that we "never" get back from
changes, except in case of regression.


>
> The property service.http.parameters.require.encrypted is also not required for the proposed change.

Yes, re-introducing will need to revert work done with OFBIZ-11260


>
> As a side note, checkSecureParameter function restricts CSRF attack to a POST method but doesn't prevent CSRF entirely. Maybe can explore the feasibility of supporting CSRF token for OFBiz form in future..

Yes, we already do: https://issues.apache.org/jira/browse/OFBIZ-10427

Jacques

>
> Regards,
> James
>
>
>
> On 2019/11/05 07:47:19, Jacques Le Roux <[hidden email]> wrote:
>> Hi James,
>>
>> We had service.http.parameters.require.encrypted in url.properties. It's was the same but an all or nothing. It's now removed.
>>
>> I'm not against your late proposition. It now means to revert changes from OFBIZ-11260!
>>
>> But then it should be reversed. By default allow-query-string-for-service-event would be true and if someone wants to prevent a query string for a
>> particular event then false can be used.
>>
>> I'm not sure much people will care of that, not sure what others think...
>>
>> Jacques
>>
>> Le 05/11/2019 à 01:28, James Yong a écrit :
>>> Hi Jacques, Samuel, all,
>>>
>>> I think the security concerns raised are valid.
>>>
>>> However we can look into adding an attribute when the url parameter check isn’t required.
>>> For example,
>>> <request-map … >
>
>>>     <security https="true" auth=“true” allow-query-string-for-service-event=“true”/>
>
>>>     …
>>>    
>>> Regards,
>>> James
>>>
>>> On 2019/10/31 14:20:11, Jacques Le Roux <[hidden email]> wrote:
>>>> Hi Samuel,
>>>>
>>>> You can go ahead. I became entangled with non ending issues while working on this and this change will not change anything about those unrelated issues.
>>>>
>>>> Jacques
>>>>
>>>> Le 30/10/2019 à 17:01, Jacques Le Roux a écrit :
>>>>> Le 30/10/2019 à 15:34, Samuel a écrit :
>>>>>> Hi Jacques,
>>>>>>
>>>>>> On 27/10/2019 17:42, Jacques Le Roux wrote:
>>>>>>
>>>>>>> … So I have no problem removing this method... and closing OFBIZ-2330, maybe after "fixing" OFBIZ-9804...
>>>>>> I'm not sure to get your point with OFBIZ-9804, if we simply remove `checkSecureParameter` we fix this issue, don't we ?
>>>>>>
>>>>>> Samuel
>>>>>>
>>>>> Yes, kinda. I prefer to have all calls to updateContactListPartyNoUserLogin similar. Please wait a bit before I close OFBIZ-9804...
>>>>>
>>>>> Jacques
>>>>>
>>>>>

[Mathieu Lirzin]

Hello James,

James Yong <[hidden email]> writes:

> Understand the intent of checkSecureParameter function is to avoid sensitive information
> in the URL during POST method. A proposal is made to provide an
> attribute (i.e. allow-query-string-for-service-event) to allow url
> parameters / query string for certain request. Shouldn't the value for
> this attribute be false, instead of true, when no value is specified
> for the attribute?

What would be required before discussing the details of the proposal is
a detailed scenario demonstrating that in the context of OFBiz event
handlers accepting query parameters from a HTTP request is less secure
than accepting only body parameters.

--
Mathieu Lirzin
GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37

[James Yong-2]

Hi Mathieu,

Csrf attack is easier on GET than POST request. While there are plans to implement csrf token within OFBiz (OFBIZ-10427), it is not completed yet. So allowing any GET request to change server data with url parameter values should preferably be done after csrf protection is implemented for GET method.

Regards,
James


On 2019/11/06 19:24:23, Mathieu Lirzin <[hidden email]> wrote:

> Hello James,
>
> James Yong <[hidden email]> writes:
>
> > Understand the intent of checkSecureParameter function is to avoid sensitive information
> > in the URL during POST method. A proposal is made to provide an
> > attribute (i.e. allow-query-string-for-service-event) to allow url
> > parameters / query string for certain request. Shouldn't the value for
> > this attribute be false, instead of true, when no value is specified
> > for the attribute?
>
> What would be required before discussing the details of the proposal is
> a detailed scenario demonstrating that in the context of OFBiz event
> handlers accepting query parameters from a HTTP request is less secure
> than accepting only body parameters.
>
> --
> Mathieu Lirzin
> GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37
>