You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ofbiz.apache.org by James Yong <ja...@apache.org> on 2019/11/05 00:28:23 UTC
Re: question about ServiceHandler.checkSecureParameter
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 <ja...@les7arts.com> 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
> >
> >
>
Re: question about ServiceHandler.checkSecureParameter
Posted by Jacques Le Roux <ja...@les7arts.com>.
Hi James, Samuel,
I did not read all your message yet James, but I agree with Samuel's answer when it comes about CSRF.
Jacques
Le 27/11/2019 à 09:28, Samuel Trégouët a écrit :
> Hi James,
>
> I still don't see any reason to keep checkSecureParameter in any form
> because it is related to ServiceEventHandler.
>
> Protection against csrf is a good idea but it has no thing to do with
> Service. It should be done upstream in http request processing so every
> type of event (ServiceEventHandler, JavaEventHandler,…) could benefit
> from this protection.
>
>
> Samuel
>
> Quoting James Yong (2019-11-26 17:26:59)
>> Hi Jacques, all,
>>
>> Haven't look into the POC yet. Please see the following updates:
>>
>> 1. Not a good practice to allow state-changing request via GET method without a token to check for CSRF.
>>
>> 2. Not a good practice to expose CSRF token in url. See [1]. Moreover, token in url may also be exposed via if user posts screenshots.
>>
>> 3. CSRF is not a concern for GET request if we avoid point 1 & 2.
>>
>> 4. In OFBiz, the same request handler generally can process both GET and POST requests. The checkSecureParameter in service events would check for no query string and as a result, state-changing operation is restricted to a POST method when service event was used.
>>
>> 5. Propose to revert the removal of checkSecureParameter, but add new code to allow exceptions to query-string check in service events. Exception can be made by setting a new attribute, allow-query-string-for-event = true | false (default), in security tag within request-map tag. Also extend checkSecureParameter to other event types.
>>
>> 6. Propose to add new attribute, csrfToken = true | false (default), to security tag to support anti-csrf for post request:
>>
>> a) For forms: if true, will generate hidden token field value using CSRF Guard library ( see [3] ) for every rendered form and store this token inside session map variable. Support may be added for token name to be randomized.
>>
>> b) For login form: There is a need to prevent Login CSRF. Token will also be created using pre-session. See [2] for Login CSRF.
>>
>> c) For XMLHTTPRequest: Use default value for same-origin policy. 1 token generated per user session for ajax call. Support may be added for token name to be randomized for each session.
>>
>> 7. The service.http.parameters.require.encrypted property that was removed, seems related to encrypting sensitive parameter values but wasn't implemented. May look into this at a later time.
>>
>> 8. For implementations that aren't using OFBiz screens, a property may be added to disable the check for CSRF token.
>>
>> Reference:
>> [1] https://danielmiessler.com/blog/sensitive-information-sent-in-the-url-over-https.
>> [2] https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html
>> [3] https://www.owasp.org/index.php?title=CSRF_Guard&redirect=no
>>
>> Regards,
>> James
>>
Re: question about ServiceHandler.checkSecureParameter
Posted by Samuel Tr��gou��t <sa...@nereide.fr>.
Hi James,
I still don't see any reason to keep checkSecureParameter in any form
because it is related to ServiceEventHandler.
Protection against csrf is a good idea but it has no thing to do with
Service. It should be done upstream in http request processing so every
type of event (ServiceEventHandler, JavaEventHandler,…) could benefit
from this protection.
Samuel
Quoting James Yong (2019-11-26 17:26:59)
> Hi Jacques, all,
>
> Haven't look into the POC yet. Please see the following updates:
>
> 1. Not a good practice to allow state-changing request via GET method without a token to check for CSRF.
>
> 2. Not a good practice to expose CSRF token in url. See [1]. Moreover, token in url may also be exposed via if user posts screenshots.
>
> 3. CSRF is not a concern for GET request if we avoid point 1 & 2.
>
> 4. In OFBiz, the same request handler generally can process both GET and POST requests. The checkSecureParameter in service events would check for no query string and as a result, state-changing operation is restricted to a POST method when service event was used.
>
> 5. Propose to revert the removal of checkSecureParameter, but add new code to allow exceptions to query-string check in service events. Exception can be made by setting a new attribute, allow-query-string-for-event = true | false (default), in security tag within request-map tag. Also extend checkSecureParameter to other event types.
>
> 6. Propose to add new attribute, csrfToken = true | false (default), to security tag to support anti-csrf for post request:
>
> a) For forms: if true, will generate hidden token field value using CSRF Guard library ( see [3] ) for every rendered form and store this token inside session map variable. Support may be added for token name to be randomized.
>
> b) For login form: There is a need to prevent Login CSRF. Token will also be created using pre-session. See [2] for Login CSRF.
>
> c) For XMLHTTPRequest: Use default value for same-origin policy. 1 token generated per user session for ajax call. Support may be added for token name to be randomized for each session.
>
> 7. The service.http.parameters.require.encrypted property that was removed, seems related to encrypting sensitive parameter values but wasn't implemented. May look into this at a later time.
>
> 8. For implementations that aren't using OFBiz screens, a property may be added to disable the check for CSRF token.
>
> Reference:
> [1] https://danielmiessler.com/blog/sensitive-information-sent-in-the-url-over-https.
> [2] https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html
> [3] https://www.owasp.org/index.php?title=CSRF_Guard&redirect=no
>
> Regards,
> James
>
Re: question about ServiceHandler.checkSecureParameter
Posted by Samuel Tr��gou��t <sa...@nereide.fr>.
yes there is a need for csrf check on get request ;)
I will write details in OFBIZ-11306 [1]
Samuel
[1]: https://issues.apache.org/jira/browse/OFBIZ-11306
Re: question about ServiceHandler.checkSecureParameter
Posted by Jacques Le Roux <ja...@les7arts.com>.
Hi James,
I did not mean that there is a need for CSRF token with GET request. Only that it's easier to implement it to all requests than having to search the
difference.
Jacques
Le 08/12/2019 à 13:07, James Yong a écrit :
> Hi all,
>
> Thanks for the feedback.
>
> Will start with CSRF Token implementation for POST request.
> Please see OFBIZ-11306 which has a patch for POC.
>
> Hi Jacques,
>
> Can explain the need for CSRF token with GET request?
>
> Regards,
> James
>
> On 2019/11/27 09:47:04, Jacques Le Roux <ja...@les7arts.com> wrote:
>> Hi James,
>>
>> Thanks for your detailed analysis and proposition.
>>
>> Le 26/11/2019 à 17:26, James Yong a écrit :
>>> Hi Jacques, all,
>>>
>>> Haven't look into the POC yet. Please see the following updates:
>>>
>>> 1. Not a good practice to allow state-changing request via GET method without a token to check for CSRF.
>>>
>>> 2. Not a good practice to expose CSRF token in url. See [1]. Moreover, token in url may also be exposed via if user posts screenshots.
>>>
>>> 3. CSRF is not a concern for GET request if we avoid point 1 & 2.
>>>
>>> 4. In OFBiz, the same request handler generally can process both GET and POST requests. The checkSecureParameter in service events would check for no query string and as a result, state-changing operation is restricted to a POST method when service event was used.
>>>
>>> 5. Propose to revert the removal of checkSecureParameter, but add new code to allow exceptions to query-string check in service events. Exception can be made by setting a new attribute, allow-query-string-for-event = true | false (default), in security tag within request-map tag. Also extend checkSecureParameter to other event types.
>>>
>>> 6. Propose to add new attribute, csrfToken = true | false (default), to security tag to support anti-csrf for post request:
>>>
>>> a) For forms: if true, will generate hidden token field value using CSRF Guard library ( see [3] ) for every rendered form and store this token inside session map variable. Support may be added for token name to be randomized.
>>>
>>> b) For login form: There is a need to prevent Login CSRF. Token will also be created using pre-session. See [2] for Login CSRF.
>>>
>>> c) For XMLHTTPRequest: Use default value for same-origin policy. 1 token generated per user session for ajax call. Support may be added for token name to be randomized for each session.
>>>
>>> 7. The service.http.parameters.require.encrypted property that was removed, seems related to encrypting sensitive parameter values but wasn't implemented. May look into this at a later time.
>>>
>>> 8. For implementations that aren't using OFBiz screens, a property may be added to disable the check for CSRF token.
>> The security team has already exchanged (since OFBIZ-10427 has been created) about the CSRF situation and we don't want to handle specific cases. We
>> want to keep it as simple as possible.
>>
>> We want to protect OFBiz globally from CSRF using a CSRF token based on the sessionID (maybe enhanced) in ALL requests, including using a X-CSRF-Token
>> for XMLHttpRequests[1]
>>
>> Nevertheless, we will consider your analysis when implementing our defence!
>>
>> Jacques
>>
>>> Reference:
>>> [1] https://danielmiessler.com/blog/sensitive-information-sent-in-the-url-over-https.
>>> [2] https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html
>>> [3] https://www.owasp.org/index.php?title=CSRF_Guard&redirect=no
>>>
>>> Regards,
>>> James
>>>
>>> On 2019/11/10 10:22:05, Jacques Le Roux <ja...@les7arts.com> wrote:
>>>> Hi James,
>>>>
>>>> I see no reasons for you to not open a Jira and provide a patch, other opinions may vary...
>>>>
>>>> Jacques
>>>>
>>>> Le 08/11/2019 à 17:46, James Yong a écrit :
>>>>> Hi Jacques,
>>>>>
>>>>> Inline...
>>>>>
>>>>> On 2019/11/06 18:28:26, Jacques Le Roux <ja...@les7arts.com> wrote:
>>>>>> 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.
>>>>> [James] Thanks for pointing it out. Still need to discuss the checkSecureParameter method below..
>>>>>
>>>>>>> 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.
>>>>> [James] I am ok with either way. The new attribute will be an interim solution if we were to remove the checkSecureParameter function after implementing CSRF protection.
>>>>>
>>>>>>> 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
>>>>>>
>>>>> [James] From the comments inside the removed checkSecureParameter method, original intent of service.http.parameters.require.encrypted is to allow existing code to work after checkSecureParameter was implemented. Don't see a need to get this property back.
>>>>>
>>>>>>> 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
>>>>> [James] Thanks for the info. CSRF protection needs to be implemented for OFBiz form since checkSecureParameter is already removed.
>>>>>
>>>>>> Jacques
>>>>>>
>>>>>>> Regards,
>>>>>>> James
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 2019/11/05 07:47:19, Jacques Le Roux <ja...@les7arts.com> 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 <ja...@les7arts.com> 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
>>>>>>>>>>>
>>>>>>>>>>>
Re: question about ServiceHandler.checkSecureParameter
Posted by James Yong <ja...@apache.org>.
Hi all,
Thanks for the feedback.
Will start with CSRF Token implementation for POST request.
Please see OFBIZ-11306 which has a patch for POC.
Hi Jacques,
Can explain the need for CSRF token with GET request?
Regards,
James
On 2019/11/27 09:47:04, Jacques Le Roux <ja...@les7arts.com> wrote:
> Hi James,
>
> Thanks for your detailed analysis and proposition.
>
> Le 26/11/2019 à 17:26, James Yong a écrit :
> > Hi Jacques, all,
> >
> > Haven't look into the POC yet. Please see the following updates:
> >
> > 1. Not a good practice to allow state-changing request via GET method without a token to check for CSRF.
> >
> > 2. Not a good practice to expose CSRF token in url. See [1]. Moreover, token in url may also be exposed via if user posts screenshots.
> >
> > 3. CSRF is not a concern for GET request if we avoid point 1 & 2.
> >
> > 4. In OFBiz, the same request handler generally can process both GET and POST requests. The checkSecureParameter in service events would check for no query string and as a result, state-changing operation is restricted to a POST method when service event was used.
> >
> > 5. Propose to revert the removal of checkSecureParameter, but add new code to allow exceptions to query-string check in service events. Exception can be made by setting a new attribute, allow-query-string-for-event = true | false (default), in security tag within request-map tag. Also extend checkSecureParameter to other event types.
> >
> > 6. Propose to add new attribute, csrfToken = true | false (default), to security tag to support anti-csrf for post request:
> >
> > a) For forms: if true, will generate hidden token field value using CSRF Guard library ( see [3] ) for every rendered form and store this token inside session map variable. Support may be added for token name to be randomized.
> >
> > b) For login form: There is a need to prevent Login CSRF. Token will also be created using pre-session. See [2] for Login CSRF.
> >
> > c) For XMLHTTPRequest: Use default value for same-origin policy. 1 token generated per user session for ajax call. Support may be added for token name to be randomized for each session.
> >
> > 7. The service.http.parameters.require.encrypted property that was removed, seems related to encrypting sensitive parameter values but wasn't implemented. May look into this at a later time.
> >
> > 8. For implementations that aren't using OFBiz screens, a property may be added to disable the check for CSRF token.
> The security team has already exchanged (since OFBIZ-10427 has been created) about the CSRF situation and we don't want to handle specific cases. We
> want to keep it as simple as possible.
>
> We want to protect OFBiz globally from CSRF using a CSRF token based on the sessionID (maybe enhanced) in ALL requests, including using a X-CSRF-Token
> for XMLHttpRequests[1]
>
> Nevertheless, we will consider your analysis when implementing our defence!
>
> Jacques
>
> >
> > Reference:
> > [1] https://danielmiessler.com/blog/sensitive-information-sent-in-the-url-over-https.
> > [2] https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html
> > [3] https://www.owasp.org/index.php?title=CSRF_Guard&redirect=no
> >
> > Regards,
> > James
> >
> > On 2019/11/10 10:22:05, Jacques Le Roux <ja...@les7arts.com> wrote:
> >> Hi James,
> >>
> >> I see no reasons for you to not open a Jira and provide a patch, other opinions may vary...
> >>
> >> Jacques
> >>
> >> Le 08/11/2019 à 17:46, James Yong a écrit :
> >>> Hi Jacques,
> >>>
> >>> Inline...
> >>>
> >>> On 2019/11/06 18:28:26, Jacques Le Roux <ja...@les7arts.com> wrote:
> >>>> 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.
> >>> [James] Thanks for pointing it out. Still need to discuss the checkSecureParameter method below..
> >>>
> >>>>> 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.
> >>> [James] I am ok with either way. The new attribute will be an interim solution if we were to remove the checkSecureParameter function after implementing CSRF protection.
> >>>
> >>>>> 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
> >>>>
> >>> [James] From the comments inside the removed checkSecureParameter method, original intent of service.http.parameters.require.encrypted is to allow existing code to work after checkSecureParameter was implemented. Don't see a need to get this property back.
> >>>
> >>>>> 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
> >>> [James] Thanks for the info. CSRF protection needs to be implemented for OFBiz form since checkSecureParameter is already removed.
> >>>
> >>>> Jacques
> >>>>
> >>>>> Regards,
> >>>>> James
> >>>>>
> >>>>>
> >>>>>
> >>>>> On 2019/11/05 07:47:19, Jacques Le Roux <ja...@les7arts.com> 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 <ja...@les7arts.com> 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
> >>>>>>>>>
> >>>>>>>>>
>
Re: question about ServiceHandler.checkSecureParameter
Posted by Jacques Le Roux <ja...@les7arts.com>.
Hi James,
Thanks for your detailed analysis and proposition.
Le 26/11/2019 à 17:26, James Yong a écrit :
> Hi Jacques, all,
>
> Haven't look into the POC yet. Please see the following updates:
>
> 1. Not a good practice to allow state-changing request via GET method without a token to check for CSRF.
>
> 2. Not a good practice to expose CSRF token in url. See [1]. Moreover, token in url may also be exposed via if user posts screenshots.
>
> 3. CSRF is not a concern for GET request if we avoid point 1 & 2.
>
> 4. In OFBiz, the same request handler generally can process both GET and POST requests. The checkSecureParameter in service events would check for no query string and as a result, state-changing operation is restricted to a POST method when service event was used.
>
> 5. Propose to revert the removal of checkSecureParameter, but add new code to allow exceptions to query-string check in service events. Exception can be made by setting a new attribute, allow-query-string-for-event = true | false (default), in security tag within request-map tag. Also extend checkSecureParameter to other event types.
>
> 6. Propose to add new attribute, csrfToken = true | false (default), to security tag to support anti-csrf for post request:
>
> a) For forms: if true, will generate hidden token field value using CSRF Guard library ( see [3] ) for every rendered form and store this token inside session map variable. Support may be added for token name to be randomized.
>
> b) For login form: There is a need to prevent Login CSRF. Token will also be created using pre-session. See [2] for Login CSRF.
>
> c) For XMLHTTPRequest: Use default value for same-origin policy. 1 token generated per user session for ajax call. Support may be added for token name to be randomized for each session.
>
> 7. The service.http.parameters.require.encrypted property that was removed, seems related to encrypting sensitive parameter values but wasn't implemented. May look into this at a later time.
>
> 8. For implementations that aren't using OFBiz screens, a property may be added to disable the check for CSRF token.
The security team has already exchanged (since OFBIZ-10427 has been created) about the CSRF situation and we don't want to handle specific cases. We
want to keep it as simple as possible.
We want to protect OFBiz globally from CSRF using a CSRF token based on the sessionID (maybe enhanced) in ALL requests, including using a X-CSRF-Token
for XMLHttpRequests[1]
Nevertheless, we will consider your analysis when implementing our defence!
Jacques
>
> Reference:
> [1] https://danielmiessler.com/blog/sensitive-information-sent-in-the-url-over-https.
> [2] https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html
> [3] https://www.owasp.org/index.php?title=CSRF_Guard&redirect=no
>
> Regards,
> James
>
> On 2019/11/10 10:22:05, Jacques Le Roux <ja...@les7arts.com> wrote:
>> Hi James,
>>
>> I see no reasons for you to not open a Jira and provide a patch, other opinions may vary...
>>
>> Jacques
>>
>> Le 08/11/2019 à 17:46, James Yong a écrit :
>>> Hi Jacques,
>>>
>>> Inline...
>>>
>>> On 2019/11/06 18:28:26, Jacques Le Roux <ja...@les7arts.com> wrote:
>>>> 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.
>>> [James] Thanks for pointing it out. Still need to discuss the checkSecureParameter method below..
>>>
>>>>> 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.
>>> [James] I am ok with either way. The new attribute will be an interim solution if we were to remove the checkSecureParameter function after implementing CSRF protection.
>>>
>>>>> 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
>>>>
>>> [James] From the comments inside the removed checkSecureParameter method, original intent of service.http.parameters.require.encrypted is to allow existing code to work after checkSecureParameter was implemented. Don't see a need to get this property back.
>>>
>>>>> 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
>>> [James] Thanks for the info. CSRF protection needs to be implemented for OFBiz form since checkSecureParameter is already removed.
>>>
>>>> Jacques
>>>>
>>>>> Regards,
>>>>> James
>>>>>
>>>>>
>>>>>
>>>>> On 2019/11/05 07:47:19, Jacques Le Roux <ja...@les7arts.com> 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 <ja...@les7arts.com> 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
>>>>>>>>>
>>>>>>>>>
Re: question about ServiceHandler.checkSecureParameter
Posted by James Yong <ja...@apache.org>.
Hi Jacques, all,
Haven't look into the POC yet. Please see the following updates:
1. Not a good practice to allow state-changing request via GET method without a token to check for CSRF.
2. Not a good practice to expose CSRF token in url. See [1]. Moreover, token in url may also be exposed via if user posts screenshots.
3. CSRF is not a concern for GET request if we avoid point 1 & 2.
4. In OFBiz, the same request handler generally can process both GET and POST requests. The checkSecureParameter in service events would check for no query string and as a result, state-changing operation is restricted to a POST method when service event was used.
5. Propose to revert the removal of checkSecureParameter, but add new code to allow exceptions to query-string check in service events. Exception can be made by setting a new attribute, allow-query-string-for-event = true | false (default), in security tag within request-map tag. Also extend checkSecureParameter to other event types.
6. Propose to add new attribute, csrfToken = true | false (default), to security tag to support anti-csrf for post request:
a) For forms: if true, will generate hidden token field value using CSRF Guard library ( see [3] ) for every rendered form and store this token inside session map variable. Support may be added for token name to be randomized.
b) For login form: There is a need to prevent Login CSRF. Token will also be created using pre-session. See [2] for Login CSRF.
c) For XMLHTTPRequest: Use default value for same-origin policy. 1 token generated per user session for ajax call. Support may be added for token name to be randomized for each session.
7. The service.http.parameters.require.encrypted property that was removed, seems related to encrypting sensitive parameter values but wasn't implemented. May look into this at a later time.
8. For implementations that aren't using OFBiz screens, a property may be added to disable the check for CSRF token.
Reference:
[1] https://danielmiessler.com/blog/sensitive-information-sent-in-the-url-over-https.
[2] https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html
[3] https://www.owasp.org/index.php?title=CSRF_Guard&redirect=no
Regards,
James
On 2019/11/10 10:22:05, Jacques Le Roux <ja...@les7arts.com> wrote:
> Hi James,
>
> I see no reasons for you to not open a Jira and provide a patch, other opinions may vary...
>
> Jacques
>
> Le 08/11/2019 à 17:46, James Yong a écrit :
> > Hi Jacques,
> >
> > Inline...
> >
> > On 2019/11/06 18:28:26, Jacques Le Roux <ja...@les7arts.com> wrote:
> >> 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.
> > [James] Thanks for pointing it out. Still need to discuss the checkSecureParameter method below..
> >
> >>
> >>> 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.
> > [James] I am ok with either way. The new attribute will be an interim solution if we were to remove the checkSecureParameter function after implementing CSRF protection.
> >
> >>
> >>> 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
> >>
> > [James] From the comments inside the removed checkSecureParameter method, original intent of service.http.parameters.require.encrypted is to allow existing code to work after checkSecureParameter was implemented. Don't see a need to get this property back.
> >
> >>> 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
> > [James] Thanks for the info. CSRF protection needs to be implemented for OFBiz form since checkSecureParameter is already removed.
> >
> >> Jacques
> >>
> >>> Regards,
> >>> James
> >>>
> >>>
> >>>
> >>> On 2019/11/05 07:47:19, Jacques Le Roux <ja...@les7arts.com> 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 <ja...@les7arts.com> 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
> >>>>>>>
> >>>>>>>
>
Re: question about ServiceHandler.checkSecureParameter
Posted by Jacques Le Roux <ja...@les7arts.com>.
Hi James,
I see no reasons for you to not open a Jira and provide a patch, other opinions may vary...
Jacques
Le 08/11/2019 à 17:46, James Yong a écrit :
> Hi Jacques,
>
> Inline...
>
> On 2019/11/06 18:28:26, Jacques Le Roux <ja...@les7arts.com> wrote:
>> 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.
> [James] Thanks for pointing it out. Still need to discuss the checkSecureParameter method below..
>
>>
>>> 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.
> [James] I am ok with either way. The new attribute will be an interim solution if we were to remove the checkSecureParameter function after implementing CSRF protection.
>
>>
>>> 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
>>
> [James] From the comments inside the removed checkSecureParameter method, original intent of service.http.parameters.require.encrypted is to allow existing code to work after checkSecureParameter was implemented. Don't see a need to get this property back.
>
>>> 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
> [James] Thanks for the info. CSRF protection needs to be implemented for OFBiz form since checkSecureParameter is already removed.
>
>> Jacques
>>
>>> Regards,
>>> James
>>>
>>>
>>>
>>> On 2019/11/05 07:47:19, Jacques Le Roux <ja...@les7arts.com> 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 <ja...@les7arts.com> 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
>>>>>>>
>>>>>>>
Re: question about ServiceHandler.checkSecureParameter
Posted by James Yong <ja...@apache.org>.
Hi Jacques,
Inline...
On 2019/11/06 18:28:26, Jacques Le Roux <ja...@les7arts.com> wrote:
> 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.
[James] Thanks for pointing it out. Still need to discuss the checkSecureParameter method below..
>
>
> > 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.
[James] I am ok with either way. The new attribute will be an interim solution if we were to remove the checkSecureParameter function after implementing CSRF protection.
>
>
> >
> > 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
>
[James] From the comments inside the removed checkSecureParameter method, original intent of service.http.parameters.require.encrypted is to allow existing code to work after checkSecureParameter was implemented. Don't see a need to get this property back.
>
> >
> > 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
[James] Thanks for the info. CSRF protection needs to be implemented for OFBiz form since checkSecureParameter is already removed.
>
> Jacques
>
> >
> > Regards,
> > James
> >
> >
> >
> > On 2019/11/05 07:47:19, Jacques Le Roux <ja...@les7arts.com> 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 <ja...@les7arts.com> 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
> >>>>>
> >>>>>
>
Re: question about ServiceHandler.checkSecureParameter
Posted by Jacques Le Roux <ja...@les7arts.com>.
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 <ja...@les7arts.com> 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 <ja...@les7arts.com> 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
>>>>>
>>>>>
Re: question about ServiceHandler.checkSecureParameter
Posted by Samuel Tr��gou��t <sa...@nereide.fr>.
Hi James,
actually `checkSecureParameter` is only for service event in a request
map. So it doesn't mean you are updating server data. Moreover you can
also update server data with a java event and in this case
`checkSecureParameter` is not called. So in my opinion this protection
is very limited.
Samuel
Re: question about ServiceHandler.checkSecureParameter
Posted by James Yong <ja...@apache.org>.
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 <ma...@nereide.fr> wrote:
> Hello James,
>
> James Yong <ja...@apache.org> 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
>
Re: question about ServiceHandler.checkSecureParameter
Posted by Mathieu Lirzin <ma...@nereide.fr>.
Hello James,
James Yong <ja...@apache.org> 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
Re: question about ServiceHandler.checkSecureParameter
Posted by James Yong <ja...@apache.org>.
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 <ja...@les7arts.com> 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 <ja...@les7arts.com> 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
> >>>
> >>>
>
Re: question about ServiceHandler.checkSecureParameter
Posted by Jacques Le Roux <ja...@les7arts.com>.
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 <ja...@les7arts.com> 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
>>>
>>>