[TEST] Test "POC for CSRF Token"

[Jacques Le Roux]

Hi James, All,

Done, the CSRF defense is in trunk and I'll backport it ASAP (it has a CVE). But I need to check that's all is OK before.

There are more things to do anyway...

Jacques

Le 04/04/2020 à 17:48, James Yong a écrit :

> Hi Jacques,
>
> Can look at JWT enhancement later.
>
> +1 for commit.
>
> Regards,
> James
>
> On 2020/04/04 13:10:18, Jacques Le Roux <[hidden email]> wrote:
>> Hi James,
>>
>>   1. I like the idea. Maybe we could create the class but let the implementation (with explanations) for those who really need it?
>>   2. I did not mean there was a correlation between csrf-token check and auth check. My main idea is to avoid hardcoded things like
>>
>>           if (requestUri.equals("main")) {             return false;         } else {
>>
>> We can set that in controllers using csrf-token="false", like it is for "logout". It's not difficult, just a global S/R. I thought there were other
>> cases but it seems not. so I now wonder if it's a good idea.
>>
>> I don't think there is a need to systematise a default to csrf-token="false" when auth="false". I just want to work on OFBIZ-4956 and while doing so
>> check that if we change auth="false" to true, as it implies csrf-token="true", there will not be undesired side effects. And in other cases
>> (auth="false" must remain) we need to decide if should set the CSRF token check to false.
>>
>> Anyway it does not hinder our work on CSRF, but I feel I now miss something that I wanted to say then, just an intuitive feeling, certainly nothing
>> serious
>>
>> I think we are ready and I'll soon commit...
>>
>> Jacques
>>
>> Le 29/03/2020 à 03:28, James Yong a écrit :
>>> Hi Jacques,
>>>
>>> For 1, seems like a ICsrfDefenseStrategy class implementation issue. We can use another Jira for the enhancement / discussion when this JIRA (OFBIZ-11306) is completed.
>>>
>>> For 2, csrf-token check is independent of auth check, and the current implementation should work as it is. So reviewing whether auth="false" be "true", should be in another JIRA (i.e. OFBIZ-4956). If there is a need for all auth="false" to default to csrf-token="false", we can implement another ICsrfDefenseStrategy class or modify the existing CsrfDefenseStrategy class.
>>>
>>> Regards,
>>> James
>>>
>>> On 2020/03/27 18:16:58, Jacques Le Roux<[hidden email]>  wrote:
>>>> Hi All,
>>>>
>>>> Before I create a PR as a last opportunity to allow reviews and tests, I'd like to ask 2 last questions:
>>>>
>>>>    1. should we not use a JWT rather than a (pseudo) random value for the CSRF token, this for timeout reason? Don't get me wrong I'm sure that the
>>>>       random values generated by java.security.SecureRandom, as currently used, are safe enough. It's just that I wonder about the timeout. Should we care?
>>>>    2. In relation with OFBIZ-4956, we need to check the remaining 195 cases where auth="false" and decide if we should change to "true", with the CSRF
>>>>       defense then used by default. In other cases (auth="false" must remain) we need to decide if should set the CSRF token check to false.
>>>>
>>>> Apart that myhttps://github.com/JacquesLeRoux/ofbiz-framework/tree/POC-for-CSRF-Token-OFBIZ-11306  branch is ready to create a PR. We can't wait too
>>>> long about those 2 points, even if the 2nd needs a "bit" of work. Anyway, for now I'll wait answers, and hopefully help for OFBIZ-4956.
>>>>
>>>> Thanks
>>>>
>>>> Jacques
>>>>
>>>>
>>>> Le 26/03/2020 à 07:39, James Yong a écrit :
>>>>> +1 with CSRF defense enabled in Demo
>>>>>    
>>>>>> Hi,
>>>>>>
>>>>>> I thought about that a bit more. I suggest to let the stable version (soon, R17) as is, ie with  CSRF defense enabled. This way users, mostly
>>>>>> interested in stable, would  see the real situation.
>>>>>>
>>>>>> And to use the NoCsrfDefenseStrategy in trunk. So developers, often brought to use the trunk for development reasons, would have more latitude; as
>>>>>> they certainly will do locally.
>>>>>>
>>>>>> If nobody disagree we will do so athttps://issues.apache.org/jira/browse/OFBIZ-11472  with Swapnil
>>>>>>
>>>>>> If we do so, the linkhttps://demo-stable.ofbiz.apache.org/ordermgr/control/main?USERNAME=admin&PASSWORD=ofbiz&JavaScriptEnabled=Y  will no longer work.
>>>>>>
>>>>>> https://demo-stable.ofbiz.apache.org/ordermgr  should be used and we need to updatehttps://ofbiz.apache.org/ofbiz-demos.html  for that.
>>>>>>
>>>>>> Jacques
>>>>>>
>>>>>>

[Jacques Le Roux]

Hi James,

The backports in R18 and R17 went well but for RequestHandler.java

We will need to do the merge by hand. I'll begin and let you know

Later...

Jacques

Le 04/04/2020 à 19:19, Jacques Le Roux a écrit :

> Hi James, All,
>
> Done, the CSRF defense is in trunk and I'll backport it ASAP (it has a CVE). But I need to check that's all is OK before.
>
> There are more things to do anyway...
>
> Jacques
>
> Le 04/04/2020 à 17:48, James Yong a écrit :
>> Hi Jacques,
>>
>> Can look at JWT enhancement later.
>>
>> +1 for commit.
>>
>> Regards,
>> James
>>
>> On 2020/04/04 13:10:18, Jacques Le Roux <[hidden email]> wrote:
>>> Hi James,
>>>
>>>   1. I like the idea. Maybe we could create the class but let the implementation (with explanations) for those who really need it?
>>>   2. I did not mean there was a correlation between csrf-token check and auth check. My main idea is to avoid hardcoded things like
>>>
>>>           if (requestUri.equals("main")) {             return false;         } else {
>>>
>>> We can set that in controllers using csrf-token="false", like it is for "logout". It's not difficult, just a global S/R. I thought there were other
>>> cases but it seems not. so I now wonder if it's a good idea.
>>>
>>> I don't think there is a need to systematise a default to csrf-token="false" when auth="false". I just want to work on OFBIZ-4956 and while doing so
>>> check that if we change auth="false" to true, as it implies csrf-token="true", there will not be undesired side effects. And in other cases
>>> (auth="false" must remain) we need to decide if should set the CSRF token check to false.
>>>
>>> Anyway it does not hinder our work on CSRF, but I feel I now miss something that I wanted to say then, just an intuitive feeling, certainly nothing
>>> serious
>>>
>>> I think we are ready and I'll soon commit...
>>>
>>> Jacques
>>>
>>> Le 29/03/2020 à 03:28, James Yong a écrit :
>>>> Hi Jacques,
>>>>
>>>> For 1, seems like a ICsrfDefenseStrategy class implementation issue. We can use another Jira for the enhancement / discussion when this JIRA
>>>> (OFBIZ-11306) is completed.
>>>>
>>>> For 2, csrf-token check is independent of auth check, and the current implementation should work as it is. So reviewing whether auth="false" be
>>>> "true", should be in another JIRA (i.e. OFBIZ-4956). If there is a need for all auth="false" to default to csrf-token="false", we can implement
>>>> another ICsrfDefenseStrategy class or modify the existing CsrfDefenseStrategy class.
>>>>
>>>> Regards,
>>>> James
>>>>
>>>> On 2020/03/27 18:16:58, Jacques Le Roux<[hidden email]>  wrote:
>>>>> Hi All,
>>>>>
>>>>> Before I create a PR as a last opportunity to allow reviews and tests, I'd like to ask 2 last questions:
>>>>>
>>>>>    1. should we not use a JWT rather than a (pseudo) random value for the CSRF token, this for timeout reason? Don't get me wrong I'm sure that the
>>>>>       random values generated by java.security.SecureRandom, as currently used, are safe enough. It's just that I wonder about the timeout.
>>>>> Should we care?
>>>>>    2. In relation with OFBIZ-4956, we need to check the remaining 195 cases where auth="false" and decide if we should change to "true", with
>>>>> the CSRF
>>>>>       defense then used by default. In other cases (auth="false" must remain) we need to decide if should set the CSRF token check to false.
>>>>>
>>>>> Apart that myhttps://github.com/JacquesLeRoux/ofbiz-framework/tree/POC-for-CSRF-Token-OFBIZ-11306 branch is ready to create a PR. We can't wait too
>>>>> long about those 2 points, even if the 2nd needs a "bit" of work. Anyway, for now I'll wait answers, and hopefully help for OFBIZ-4956.
>>>>>
>>>>> Thanks
>>>>>
>>>>> Jacques
>>>>>
>>>>>
>>>>> Le 26/03/2020 à 07:39, James Yong a écrit :
>>>>>> +1 with CSRF defense enabled in Demo
>>>>>>> Hi,
>>>>>>>
>>>>>>> I thought about that a bit more. I suggest to let the stable version (soon, R17) as is, ie with  CSRF defense enabled. This way users, mostly
>>>>>>> interested in stable, would  see the real situation.
>>>>>>>
>>>>>>> And to use the NoCsrfDefenseStrategy in trunk. So developers, often brought to use the trunk for development reasons, would have more
>>>>>>> latitude; as
>>>>>>> they certainly will do locally.
>>>>>>>
>>>>>>> If nobody disagree we will do so athttps://issues.apache.org/jira/browse/OFBIZ-11472 with Swapnil
>>>>>>>
>>>>>>> If we do so, the linkhttps://demo-stable.ofbiz.apache.org/ordermgr/control/main?USERNAME=admin&PASSWORD=ofbiz&JavaScriptEnabled=Y will no
>>>>>>> longer work.
>>>>>>>
>>>>>>> https://demo-stable.ofbiz.apache.org/ordermgr  should be used and we need to updatehttps://ofbiz.apache.org/ofbiz-demos.html  for that.
>>>>>>>
>>>>>>> Jacques
>>>>>>>
>>>>>>>

[Jacques Le Roux]

Hi,

We actually had 2 problems to solve:

 1. 8 tests don't pass on trunk.
 2. Backport, the merge "worked" but we (at least) miss in RequestHandler.java the not backported WIP on REST with notably these missing methods:
      * RequestHandler::resolveURI (OFBIZ-10438)
      * UtilHttp::getRequestMethod "REST: adding segmented URI support", with also OFBIZ-11007 and OFBIZ-11332+OFBIZ-11007

With OFBIZ-11470 "Ensure that the SameSite attribute is set to 'strict' for all cookies" OFBiz is already protected from CSRF OOTB in all supported
branches.

To makes things simple (at least for me) I have decided to not backport the CSRF Token work to current supported releases branches. And, as I have
already expressed, to not use the CSRF Token defense OOTB in trunk. This solves the 2 problems above. This also avoids to backport some related Jiras
like OFBIZ-11317 "Add 'controlPath' attribute to 'ofbizUrl' freemarker macro". It has also the advantage of simplifying committers work and demos.

If an user needs to use 'lax' for the SameSite attribute it would be her/his charge to make the backport. Easiest way is to revert the "REST" commits
from trunk before backporting. Of course if a committer wants to backport the CSRF Token work with "REST" commits in, it's open, another Jira would be
needed.

You may notice that this does not solve the tests issue in case someone wants to use the CSRF Token defense, it just hides it. So I'll create another
Jira for that.

And last but not least I have created: OFBIZ-11585 "Update security.adoc with few words about our CSRF defense strategy"

Jacques

Le 04/04/2020 à 21:02, Jacques Le Roux a écrit :

> Hi James,
>
> The backports in R18 and R17 went well but for RequestHandler.java
>
> We will need to do the merge by hand. I'll begin and let you know
>
> Later...
>
> Jacques
>
> Le 04/04/2020 à 19:19, Jacques Le Roux a écrit :
>> Hi James, All,
>>
>> Done, the CSRF defense is in trunk and I'll backport it ASAP (it has a CVE). But I need to check that's all is OK before.
>>
>> There are more things to do anyway...
>>
>> Jacques
>>
>> Le 04/04/2020 à 17:48, James Yong a écrit :
>>> Hi Jacques,
>>>
>>> Can look at JWT enhancement later.
>>>
>>> +1 for commit.
>>>
>>> Regards,
>>> James
>>>
>>> On 2020/04/04 13:10:18, Jacques Le Roux <[hidden email]> wrote:
>>>> Hi James,
>>>>
>>>>   1. I like the idea. Maybe we could create the class but let the implementation (with explanations) for those who really need it?
>>>>   2. I did not mean there was a correlation between csrf-token check and auth check. My main idea is to avoid hardcoded things like
>>>>
>>>>           if (requestUri.equals("main")) { return false;         } else {
>>>>
>>>> We can set that in controllers using csrf-token="false", like it is for "logout". It's not difficult, just a global S/R. I thought there were other
>>>> cases but it seems not. so I now wonder if it's a good idea.
>>>>
>>>> I don't think there is a need to systematise a default to csrf-token="false" when auth="false". I just want to work on OFBIZ-4956 and while doing so
>>>> check that if we change auth="false" to true, as it implies csrf-token="true", there will not be undesired side effects. And in other cases
>>>> (auth="false" must remain) we need to decide if should set the CSRF token check to false.
>>>>
>>>> Anyway it does not hinder our work on CSRF, but I feel I now miss something that I wanted to say then, just an intuitive feeling, certainly nothing
>>>> serious
>>>>
>>>> I think we are ready and I'll soon commit...
>>>>
>>>> Jacques
>>>>
>>>> Le 29/03/2020 à 03:28, James Yong a écrit :
>>>>> Hi Jacques,
>>>>>
>>>>> For 1, seems like a ICsrfDefenseStrategy class implementation issue. We can use another Jira for the enhancement / discussion when this JIRA
>>>>> (OFBIZ-11306) is completed.
>>>>>
>>>>> For 2, csrf-token check is independent of auth check, and the current implementation should work as it is. So reviewing whether auth="false" be
>>>>> "true", should be in another JIRA (i.e. OFBIZ-4956). If there is a need for all auth="false" to default to csrf-token="false", we can implement
>>>>> another ICsrfDefenseStrategy class or modify the existing CsrfDefenseStrategy class.
>>>>>
>>>>> Regards,
>>>>> James
>>>>>
>>>>> On 2020/03/27 18:16:58, Jacques Le Roux<[hidden email]>  wrote:
>>>>>> Hi All,
>>>>>>
>>>>>> Before I create a PR as a last opportunity to allow reviews and tests, I'd like to ask 2 last questions:
>>>>>>
>>>>>>    1. should we not use a JWT rather than a (pseudo) random value for the CSRF token, this for timeout reason? Don't get me wrong I'm sure that
>>>>>> the
>>>>>>       random values generated by java.security.SecureRandom, as currently used, are safe enough. It's just that I wonder about the timeout.
>>>>>> Should we care?
>>>>>>    2. In relation with OFBIZ-4956, we need to check the remaining 195 cases where auth="false" and decide if we should change to "true", with
>>>>>> the CSRF
>>>>>>       defense then used by default. In other cases (auth="false" must remain) we need to decide if should set the CSRF token check to false.
>>>>>>
>>>>>> Apart that myhttps://github.com/JacquesLeRoux/ofbiz-framework/tree/POC-for-CSRF-Token-OFBIZ-11306 branch is ready to create a PR. We can't wait
>>>>>> too
>>>>>> long about those 2 points, even if the 2nd needs a "bit" of work. Anyway, for now I'll wait answers, and hopefully help for OFBIZ-4956.
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>>> Jacques
>>>>>>
>>>>>>
>>>>>> Le 26/03/2020 à 07:39, James Yong a écrit :
>>>>>>> +1 with CSRF defense enabled in Demo
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> I thought about that a bit more. I suggest to let the stable version (soon, R17) as is, ie with  CSRF defense enabled. This way users, mostly
>>>>>>>> interested in stable, would  see the real situation.
>>>>>>>>
>>>>>>>> And to use the NoCsrfDefenseStrategy in trunk. So developers, often brought to use the trunk for development reasons, would have more
>>>>>>>> latitude; as
>>>>>>>> they certainly will do locally.
>>>>>>>>
>>>>>>>> If nobody disagree we will do so athttps://issues.apache.org/jira/browse/OFBIZ-11472 with Swapnil
>>>>>>>>
>>>>>>>> If we do so, the linkhttps://demo-stable.ofbiz.apache.org/ordermgr/control/main?USERNAME=admin&PASSWORD=ofbiz&JavaScriptEnabled=Y will no
>>>>>>>> longer work.
>>>>>>>>
>>>>>>>> https://demo-stable.ofbiz.apache.org/ordermgr should be used and we need to updatehttps://ofbiz.apache.org/ofbiz-demos.html  for that.
>>>>>>>>
>>>>>>>> Jacques
>>>>>>>>
>>>>>>>>