Re: question about ServiceHandler.checkSecureParameter
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
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 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=no Regards, James On 2019/11/10 10:22:05, Jacques Le Roux 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 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
Re: question about ServiceHandler.checkSecureParameter
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 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=no > > > > Regards, > > James > > > > On 2019/11/10 10:22:05, Jacques Le Roux > > 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 > >>> 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
Re: question about ServiceHandler.checkSecureParameter
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=no Regards, James On 2019/11/10 10:22:05, Jacques Le Roux 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 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
Re: question about ServiceHandler.checkSecureParameter
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=no Regards, James
Re: question about ServiceHandler.checkSecureParameter
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=no > > Regards, > James >
Re: question about ServiceHandler.checkSecureParameter
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=no Regards, James On 2019/11/10 10:22:05, Jacques Le Roux 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 > > 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 > >>> 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
Re: question about ServiceHandler.checkSecureParameter
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 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 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, > > … Regards, James On 2019/10/31 14:20:11, Jacques Le Roux 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
Hi Jacques, Inline... On 2019/11/06 18:28:26, Jacques Le Roux 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 > > 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, > >>> > > >>> >>> allow-query-string-for-service-event=“true”/> > > >>> … > >>> > >>> Regards, > >>> James > >>> > >>> On 2019/10/31 14:20:11, Jacques Le Roux > >>> 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
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
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 wrote: > Hello James, > > James Yong 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
Hello James, James Yong 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
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 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, > > … Regards, James On 2019/10/31 14:20:11, Jacques Le Roux 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
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 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, > > > > > > allow-query-string-for-service-event=“true”/> > > >… > > > > Regards, > > James > > > > On 2019/10/31 14:20:11, Jacques Le Roux > > 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
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, … Regards, James On 2019/10/31 14:20:11, Jacques Le Roux 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
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, … Regards, James On 2019/10/31 14:20:11, Jacques Le Roux 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
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
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
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
Re: question about ServiceHandler.checkSecureParameter
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
Re: question about ServiceHandler.checkSecureParameter
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
Re: question about ServiceHandler.checkSecureParameter
Hi, On 20/10/2019 12:27, Mathieu Lirzin wrote: Hello, Samuel 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 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
Re: question about ServiceHandler.checkSecureParameter
Hello, Samuel 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 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
Re: question about ServiceHandler.checkSecureParameter
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=ExistingEcommerceUser=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=764286
Then I did http://svn.apache.org/viewvc?view=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
Re: question about ServiceHandler.checkSecureParameter
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=ExistingEcommerceUser=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=764286
Then I did http://svn.apache.org/viewvc?view=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
Re: question about ServiceHandler.checkSecureParameter
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=ExistingEcommerceUser=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=764286
Then I did http://svn.apache.org/viewvc?view=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
Re: question about ServiceHandler.checkSecureParameter
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=764286 Then I did http://svn.apache.org/viewvc?view=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
Re: question about ServiceHandler.checkSecureParameter
Hi Samuel, It started with http://svn.apache.org/viewvc?view=revision=764286 Then I did http://svn.apache.org/viewvc?view=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
Re: question about ServiceHandler.checkSecureParameter
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
