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