ofbiz-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jacques Le Roux (Jira)" <j...@apache.org>
Subject [jira] [Comment Edited] (OFBIZ-11306) POC for CSRF Token
Date Sat, 11 Jan 2020 19:44:00 GMT

    [ 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/11/20 7:43 PM:
------------------------------------------------------------------

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!


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 abovet, 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 ssound 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)

Mime
View raw message