[
https://issues.apache.org/jira/browse/OFBIZ-11306?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17013563#comment-17013563
]
Jacques Le Roux edited comment on OFBIZ-11306 at 1/12/20 6:04 AM:
------------------------------------------------------------------
Hi James,
First I must apologize. Because I initially suggested that we should not
differentiate GET and POST methods. I undertstood I was wrong after reading
[OWASP Disclosure of Token in
URL|https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#disclosure-of-token-in-url]
and thought about it. As explained in my comment above, it has an impact on
the work already done. Fortunately most is easy to remove. But we need to add
some code.
After reviewing Java code (still a WIP) here are my conclusions:
* OrderServices.xml should not be part of this patch, another Jira should be
created
* ConfigXMLReader: OK with me
* ControlEventListener: OK with me
* WidgetWorker: we don't want tokens in form ULRs, hidden fields only should
be used
* MacroFormRenderer: we don't want tokens in form ULRs, hidden fields only
should be used, as it's done in HtmlFormMacroLibrary.ftl. We don't need tokens
for pagination. It's idempotent, no harm possible.
* CsrfTokenTransform is useless we don't want tokens in form ULRs, as
mentioned for WidgetWorker and MacroFormRenderer
* UrlRegexpTransform, CatalogAltUrlSeoTransform: we don't want tokens in
navigation ULRs. We though need to check that all URLs are idempotent. They
should be since those are only for navigation, and forms are used for changes.
* CsrfTokenAjaxTransform: OK with me
Other misc. stuff:
* We need to adds crsftoken hidden fields in forms in ftl files. There at 706
of them. Only non idempotent forms are concerned. I guess most of them if, not
all. This sounds like a boring task. Maybe we can find a smart way to do that
globally. I did not think about it already.
* I don't think adding {{if (!options.crossDomain)}} in ajaxPrefilter will
work with CORS. I have to check that once we will commit, not a big deal.
* I like the idea of controlPath in ofbizUrl macro. But it's unrelated to
CSRF. I suggest to remove it from this issue and create a specific issue for
that.
After a preliminary preliminary review, I still need to review CsrfUtil and
RequestHandler closer..
Again thanks for the good work!
was (Author: jacques.le.roux):
Hi James,
First I must apologize. Because I initially suggested that we should not
differentiate GET and POST methods. I undertstood I was wrong after reading
[OWASP Disclosure of Token in
URL|https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#disclosure-of-token-in-url]
and thought about it. As explained in my comment above, it has an impact on
the work already done. Fortunately most is easy to remove. But we need to add
some code.
After reviewing Java code (still a WIP) here are my conclusions:
* OrderServices.xml should not be part of this patch, another Jira should be
created
* ConfigXMLReader: OK with me
* ControlEventListener: OK with me
* WidgetWorker: we don't want tokens in form ULRs, hidden fields only should
be used
* MacroFormRenderer: we don't want tokens in form ULRs, hidden fields only
should be used, as it's done in HtmlFormMacroLibrary.ftl. We don't need tokens
for pagination. It's idempotent, no harm possible.
* CsrfTokenTransform is useless we don't want tokens in form ULRs, as
mentioned for WidgetWorker and MacroFormRenderer
* UrlRegexpTransform, CatalogAltUrlSeoTransform: we don't want tokens in
navigation ULRs. We though need to check that all URLs are idempotent. They
should be since those are only for navigation, and forms are used for changes.
* CsrfTokenAjaxTransform: OK with me
Other misc. stuff:
* We need to adds crsftoken hidden fields in forms in ftl files. There at 706
of them. Only non idempotent forms are concerned. I guess most of them if, not
all. This sounds like a boring task. Maybe we can find a smart way to do that
globally. I did not think about it already.
* I don't think adding {{if (!options.crossDomain)}} in ajaxPrefilter will
work with CORS. I have to check that once we will commit, not a big deal.
* I like the idea of controlPath in ofbizUrl macro. But it's unrelated to
CSRF. I suggest to remove it from this issue and create a specific issue for
that.
After a preliminary preliminary review, I still need to review CsrfUtil
closer...
Again thanks for the good work!
> POC for CSRF Token
> ------------------
>
> Key: OFBIZ-11306
> URL: https://issues.apache.org/jira/browse/OFBIZ-11306
> Project: OFBiz
> Issue Type: Improvement
> Components: ALL APPLICATIONS
> Affects Versions: Upcoming Branch
> Reporter: James Yong
> Assignee: Jacques Le Roux
> Priority: Minor
> Labels: CSRF
> Fix For: Upcoming Branch
>
> Attachments: OFBIZ-11306-v2.patch, OFBIZ-11306.patch,
> OFBIZ-11306.patch, OFBIZ-11306.patch, OFBIZ-11306.patch, OFBIZ-11306.patch,
> OFBIZ-11306.patch, OFBIZ-11306.patch, OFBIZ-11306.patch, OFBIZ-11306.patch,
> OFBIZ-11306.patch, OFBIZ-11306.patch, OFBIZ-11306_Plugins.patch,
> OFBIZ-11306_Plugins.patch, OFBIZ-11306_Plugins.patch,
> OFBIZ-11306_Plugins.patch
>
>
> CRSF tokens are generated using CSRF Guard library and used in:
> 1) In widget form where a hidden token field is auto-generated.
> 2) In FTL form where a <@csrfTokenField> macro is used to generate the csrf
> token field.
> 3) In Ajax call where a <@csrfTokenAjax> macro is used to assign csrf token
> to X-CSRF-Token in request header.
> CSRF tokens are stored in the user sessions, and verified during POST request.
> A new attribute i.e. csrf-token is added to the security tag to exempt CSRF
> token check.
> Certain request path, like LookupPartyName, can be exempt from CSRF token
> check during Ajax POST call.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
