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 <jacques.le.r...@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 <jacques.le.r...@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 <jacques.le.r...@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 <jacques.le.r...@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 > >>>>>>> <jacques.le.r...@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 > >>>>>>>>> > >>>>>>>>> >
