[jira] [Commented] (OFBIZ-11306) POC for CSRF Token

[Nicolas Malin (Jira)]

    [ https://issues.apache.org/jira/browse/OFBIZ-11306?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17018938#comment-17018938 ]

Jacques Le Roux commented on OFBIZ-11306:
-----------------------------------------

Hi James,

h3.  What I agree about
I agree about the rules. Pagination, finding, listing should also be exempted from CSRF token check, I guess there are other kinds. So there would not be issues for back and forth.
I agree that it's better to put csrf-token information in the controllers (Request Map ). Also as the possibility already exists.

h3. Some questions and "answers"
bq. Explanation for tokens in navigation In OFBiz, there are forms that use the same uri for getting the form and posting the changes.
Have you few examples of that (one would be sufficient)? We need to be sure that we are not missing anything.

bq. While there is CSRF token in the form URL, the token is invalidated during form submission.
Could you please explain where/how is that done? Is that depending on being a POST method as in {{tokenMap.remove(requestUri);}} in CsrfUtil::checkToken?

bq. Explanation for tokens in navigation. This is the reason why token was invalidated each time it was used in a GET request. However, to allow back forward browser actions to work, tokens aren't invalidated for GET request as a compromise between usability and security.
I'd prefer that we change all the "same uri for getting the form and posting the changes.". Somehow what you did for processorder in OFBIZ-11319

bq. Explanation for tokens in form URL (defined in FTL files). In order to manually add a CSRF token field, user need to ensure the uri used is the same in both the form action attribute and the CSRF token field.
Though I'd add preferred rather to add the token in a hidden field. I understand it's an easy way to automatically do it, and seems safe. As with the previous point we need to be sure that all forms use the POST method. Also we need to do it for at least ofbizContentUrl and check no others would miss it.

h3. Suggestions:
* I sugget we make {{return size() > 100;}} in CsrfUtil::getTokenMap a properties to allow users to adjust in function of their needs.
* Some recommend to encrypt IP and "Timeout" in the CSRF token and check. We could do that by using a JWT token rather than a random value. We could then check both IP and "Timeout" to increase safety.

h3. We have few small problems
# with Time limitation due to session timeout in ecommerce (maybe the price to pay), eg add to compare after session ended for an anymous user.
# SetTimeZoneFromBrowser when starting: org.apache.ofbiz.webapp.control.RequestHandlerException: Invalid or missing CSRF token for AJAX call to path '/SetTimeZoneFromBrowser'. Also not only when starting.

h3. We have several issues (at least) in Webtools. All work on trunk demo.
* Entity Reference - Interactive Version (also it's duplicated, we should keep only one entry, the one in Entity Engine Tools section)
* Service Reference (also it's diplicated, we should keep only one entry, the one in Service Engine section)
* Entity Reference - Static Version
* Entity Reference - PDF
* Induce Model XML from Database
* (most?) options in Check/Update Database

I guess the last (and maybe others) are related to nested request. Like with checkLogin that you handled in CsrfUtil::generateTokenForNonAjax, not sure why it does not workfor those. More on that below...

h3. Issues in Entity Data Maintenance related to new REST work at OFBIZ-11007:
Open an entity like webtools/control/ViewGeneric?entityName=Agreement&agreementId=8000
Edit does not work, nor create, nor delete. They work if you revert 6e1c7b5958cc1a80be5746bfd177136a1feabe0d commit (last one for OFBIZ-11007)

It's because we have nested requests with this special syntax:
{code:xml}
    <!-- === Rest Entity Mapping === -->

    <!-- form for creating a record  -->
    <request-map uri="entity/list" method="get"><security https="true" auth="true"/><response name="success" type="view" value="entitymaint" /></request-map>

    <!-- form for creating a record  -->
    <request-map uri="entity/create/{entityName}" method="get"><security https="true" auth="true"/><response name="success" type="view" value="EditGeneric"/></request-map>

    <!-- form for modifying a record  -->
    <request-map uri="entity/edit/{entityName}/{pkValues: .*}" method="get"><security https="true" auth="true"/><response name="success" type="view" value="EditGeneric" /></request-map>

    <!--view relations for a given entity -->
    <request-map uri="entity/relations/{entityName}" method="get"><security https="true" auth="true"/><response name="success" type="view" value="ViewRelations" /></request-map>

    <!-- getting the view of records -->
    <request-map uri="entity/find/{entityName}" method="get"><security https="true" auth="true"/><response name="success" type="view" value="FindGeneric" /></request-map>

    <!-- adding new record (from submitted form) -->
    <request-map uri="entity/change/{entityName}" method="post">
        <security https="true" auth="true"/>
        <event type="java" path="org.apache.ofbiz.webtools.GenericWebEvent" invoke="updateGeneric"/>
        <response name="success" type="view" value="FindGeneric"/>
        <response name="error" type="view" value="ViewGeneric"/>
    </request-map>

    <!-- view entity records -->
    <request-map uri="entity/find/{entityName}/{pkValues: .*}" method="get"><security https="true" auth="true"/><response name="success" type="view" value="ViewGeneric" /></request-map>

    <!-- modifying existing record -->
    <request-map uri="entity/change/{entityName}/{pkValues: .*}" method="put">
        <security https="true" auth="true"/>
        <event type="java" path="org.apache.ofbiz.webtools.GenericWebEvent" invoke="updateGeneric"/>
        <response name="success" type="view" value="ViewGeneric"/>
        <response name="error" type="view" value="ViewGeneric"/>
    </request-map>
    <!-- deleting existing record -->
    <request-map uri="entity/change/{entityName}/{pkValues: .*}" method="delete">
        <security https="true" auth="true"/>
        <event type="java" path="org.apache.ofbiz.webtools.GenericWebEvent" invoke="updateGeneric"/>
        <response name="success" type="view" value="ViewGeneric"/>
        <response name="error" type="view" value="ViewGeneric"/>
    </request-map>
{code}

So we get errors like

2020-01-16 12:41:00,998 |jsse-nio-8443-exec-6 |CsrfUtil                      |E| request map is null for path entity/find
2020-01-16 12:41:49,544 |jsse-nio-8443-exec-6 |CsrfUtil                      |E| request map is null for path entity/edit
2020-01-16 12:27:45,222 |jsse-nio-8443-exec-4 |CsrfUtil                      |E| request map is null for path entity/create
2020-01-16 12:28:48,342 |jsse-nio-8443-exec-4 |CsrfUtil                      |E| request map is null for path entity/relations

I tried to cope with it using

{noformat}
@@ -798,7 +827,11 @@ public class RequestHandler {
         if (pathInfo.get(0).indexOf('?') > -1) {
             return pathInfo.get(0).substring(0, pathInfo.get(0).indexOf('?'));
         } else {
-            return pathInfo.get(0);
+            if (1 < StringUtils.countMatches(path, "/")) {
+                return pathInfo.get(0) + "/" + pathInfo.get(1);
+            } else {
+                return pathInfo.get(0);
+            }
{noformat}

and reusing RequestHandler::getRequestUri in several places notably in CsrfUtil class. It helps but seems not to be enough.

It seems the same(?) problem exist for searchorders when using a serarch parameter (OK w/o, ie empty by default). We get this message:
bq. Invalid or missing CSRF token to path '/orderview'. Click here to continue
Maybe related to tokenMap creation at {{tokenMap = getTokenMap(request, "/"+RequestHandler.getRequestUri(pathOrRequestUri));}}

Also I tried to cope with Rest links containing {noformat}"&#x2f;"{noformat} using:
{noformat}
diff --git framework/webtools/template/entity/ViewGeneric.ftl framework/webtools/template/entity/ViewGeneric.ftl
index 216eb63aaa..60c432d9e4 100644
--- framework/webtools/template/entity/ViewGeneric.ftl
+++ framework/webtools/template/entity/ViewGeneric.ftl
@@ -53,6 +53,7 @@ function ShowTab(lname) {
       <a href='<@ofbizUrl>entity/find/${entityName}</@ofbizUrl>' class="buttontext">${uiLabelMap.WebtoolsBackToFindScreen}</a>
       <#if enableEdit = "false">
         <#if hasCreatePermission>
+        <#assign currentFindString = currentFindString?replace("&#x2f;", "/")!>
           <form action="<@ofbizUrl>entity/edit/${currentFindString}</@ofbizUrl>" method="get">
             <input type="submit" value="${uiLabelMap.CommonEdit}" />
           </form>
{noformat}

But not sure it was a good way, maybe the link issue does not come from that...

h3. Lastly about formatting
Please check https://cwiki.apache.org/confluence/display/OFBIZ/Coding+Conventions. For instance:

{code:java}if (partyTokenMap==null){{code}
should be
{code:java}if (partyTokenMap == null) {{code}

and
{code:java}controlPath + (requestURI.startsWith("/")?requestURI:"/"+requestURI));{code}
should be
{code:java}controlPath + (requestURI.startsWith("/") ? requestUri : "/" + requestUri);{code}
etc.

> 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.patch, OFBIZ-11306_Plugins.patch, OFBIZ-11306_Plugins.patch, OFBIZ-11306_Plugins.patch, OFBIZ-11306_Plugins.patch, OFBIZ-11306_Plugins.patch
>
>
> CRSF tokens are generated using SecureRandom class.
> 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)