Uploaded image for project: 'OFBiz'
  1. OFBiz
  2. OFBIZ-6567

Wrong percent encoding in Webtool/SQL Processor

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: Release Branch 12.04, Release Branch 13.07, Release Branch 14.12, Trunk
    • Fix Version/s: 14.12.01, 12.04.06, 13.07.03, 16.11.01
    • Component/s: framework
    • Labels:
      None

      Description

      This was reported to me by Gareth Carter, the investigation is mine.

      If for instance you use this SQL expression

      select * from Party_Role where role_Type_Id LIKE  '%CA%'
      

      It will be interpreted (and returned to UI) as

      select * from Party_Role where role_Type_Id LIKE  'Ê%'
      

      And no result will be returned when OOTB there is 6 <PartyRole partyId="***" roleTypeId="CARRIER"/> entities

      This is because in UtilHttp.canonicalizeParameterMap() UtilHttp.canonicalizeParameter() is called. And inside the later UtilCodec.canonicalize() is used. So 2 ESAPI codecs are tested HTMLEntityCodec.decode() and PercentCodec.decode(). Only PercentCodec.decode() does a change so it's picked. In this case it should not, because nothing should be decoded. At this point, nothing has been encoded, the String the codec decodes is still "select * from Party_Role where role_Type_Id LIKE '%CA%'"

      I read at https://en.wikipedia.org/wiki/Percent-encoding that though mostly planned for URL encoding percent encoding

      is also used in the preparation of data of the application/x-www-form-urlencoded media type, as is often used in the submission of HTML form data in HTTP requests.

      But in the specific case of a like in an SQL expression coming from the text area of webtools/control/EntitySQLProcessor it should not be used because the % followed by some chars, may be wrongly decoded.

      Because there are no other ways provided by the percent codec to prevent the decoding (it's supposed to have been encoded before), I'm not quite proud of it but I found only this workaround so far

      Index: framework/base/src/org/ofbiz/base/util/UtilCodec.java
      ===================================================================
      --- framework/base/src/org/ofbiz/base/util/UtilCodec.java	(revision 1693397)
      +++ framework/base/src/org/ofbiz/base/util/UtilCodec.java	(working copy)
      @@ -164,16 +164,24 @@
                   while (i.hasNext()) {
                       Codec codec = i.next();
                       String old = working;
      -                working = codec.decode(working);
      -                if (!old.equals(working)) {
      -                    if (codecFound != null && codecFound != codec) {
      -                        mixedCount++;
      +                String upperWorking = working.toUpperCase();
      +                if (codec instanceof PercentCodec
      +                        && upperWorking.contains("WHERE")
      +                        && upperWorking.contains("LIKE")
      +                        && upperWorking.contains("%")) {
      +                    continue;
      +                } else {
      +                    working = codec.decode(working);
      +                    if (!old.equals(working)) {
      +                        if (codecFound != null && codecFound != codec) {
      +                            mixedCount++;
      +                        }
      +                        codecFound = codec;
      +                        if (clean) {
      +                            foundCount++;
      +                        }
      +                        clean = false;
                           }
      -                    codecFound = codec;
      -                    if (clean) {
      -                        foundCount++;
      -                    }
      -                    clean = false;
                       }
                   }
               }
      

      Better ideas?

        Activity

        Hide
        gareth.carter Gareth Carter added a comment - - edited

        I dislike modifying UtilCodec in this way. Its very much a band aid on a problem that shouldn't exist anyway. Percent encoding is used on query parameters and form parameters, the servlet container (tomcat in this instance) has already percent decoded parameters so why attempt to do it again?

        Example:
        1) User types in sql " select * from Party_Role where role_Type_Id LIKE  '%CA%' " in text area and submits.
        2) The browser sends "group=org.ofbiz&sqlCommand=select+*+from+Party_Role+where+role_Type_Id+LIKE++%27%25CA%25%27&rowLimit=200&submitButton=Submit" as form parameters to server
        3) Tomcat decodes each parameter so the sqlCommand parameter from HttpServletRequest reads " select * from Party_Role where role_Type_Id LIKE  '%CA%' "
        4) UtilCodec decodes the correct request parameter and modifies it because it has detected a valid percent encoding (%CA). The output is finally " select * from Party_Role where role_Type_Id LIKE  'Ê%' "
        

        A quick win would be to modify EntitySQLProcessor.groovy and add:

        sqlCommand = context.request.getParameter("sqlCommand");
        context.sqlCommand = sqlCommand;

        I would suggest not to percent decode or "canonicalize" request parameters, I doubt there would be little impact on removing percent decoding from request parameters but unsure about html entity decoding

        Show
        gareth.carter Gareth Carter added a comment - - edited I dislike modifying UtilCodec in this way. Its very much a band aid on a problem that shouldn't exist anyway. Percent encoding is used on query parameters and form parameters, the servlet container (tomcat in this instance) has already percent decoded parameters so why attempt to do it again? Example: 1) User types in sql " select * from Party_Role where role_Type_Id LIKE '%CA%' " in text area and submits. 2) The browser sends "group=org.ofbiz&sqlCommand=select+*+from+Party_Role+where+role_Type_Id+LIKE++%27%25CA%25%27&rowLimit=200&submitButton=Submit" as form parameters to server 3) Tomcat decodes each parameter so the sqlCommand parameter from HttpServletRequest reads " select * from Party_Role where role_Type_Id LIKE '%CA%' " 4) UtilCodec decodes the correct request parameter and modifies it because it has detected a valid percent encoding (%CA). The output is finally " select * from Party_Role where role_Type_Id LIKE 'Ê%' " A quick win would be to modify EntitySQLProcessor.groovy and add: sqlCommand = context.request.getParameter("sqlCommand"); context.sqlCommand = sqlCommand; I would suggest not to percent decode or "canonicalize" request parameters, I doubt there would be little impact on removing percent decoding from request parameters but unsure about html entity decoding
        Hide
        jacques.le.roux Jacques Le Roux added a comment - - edited

        That's an excellent idea Gareth, I see that you thought about it since we spoke . I was also reluctant to change UtilCodec.

        Jacopo reused the ESAPI DefaultEncoder canonicalize() method implementation to decode requests parameters when he did the ESAPI refactoring to introduce ESAPI 2 because of the possible double encoding or alike attacks. But as explained in this article http://security.coverity.com/blog/2013/Nov/to-escape-or-not-to-escape-that-is-the-question.html canonicalizing parameters has possible "drawbacks" and you crossed one similar to the one reported in this article.

        We already knew there are issues with UtilCodec.canonicalize() as reported in OFBIZ-5910, where the same article is referenced. And we also know there is no global solution. So at least this aspect (LIKE in SQL processor) is now handled with your solution and I prefer to keep the canonicalize method with percent and HTML entities decoding for now. As you suggested in OFBIZ-5910 using Commons StringEscapeUtils class should be considered when crossing issues with UtilCodec.canonicalize().

        Show
        jacques.le.roux Jacques Le Roux added a comment - - edited That's an excellent idea Gareth, I see that you thought about it since we spoke . I was also reluctant to change UtilCodec. Jacopo reused the ESAPI DefaultEncoder canonicalize() method implementation to decode requests parameters when he did the ESAPI refactoring to introduce ESAPI 2 because of the possible double encoding or alike attacks. But as explained in this article http://security.coverity.com/blog/2013/Nov/to-escape-or-not-to-escape-that-is-the-question.html canonicalizing parameters has possible "drawbacks" and you crossed one similar to the one reported in this article. We already knew there are issues with UtilCodec.canonicalize() as reported in OFBIZ-5910 , where the same article is referenced. And we also know there is no global solution. So at least this aspect (LIKE in SQL processor) is now handled with your solution and I prefer to keep the canonicalize method with percent and HTML entities decoding for now. As you suggested in OFBIZ-5910 using Commons StringEscapeUtils class should be considered when crossing issues with UtilCodec.canonicalize().
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Fixed in
        trunk r1693579
        R14.12 r1693580

        Before closing, I will check how to fix in previous releases (both 13 and 12 are concerned)

        Show
        jacques.le.roux Jacques Le Roux added a comment - Fixed in trunk r1693579 R14.12 r1693580 Before closing, I will check how to fix in previous releases (both 13 and 12 are concerned)
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Backported in
        R13.07 r1693595
        R12.04 r1693596

        Show
        jacques.le.roux Jacques Le Roux added a comment - Backported in R13.07 r1693595 R12.04 r1693596
        Hide
        gareth.carter Gareth Carter added a comment -

        I was looking for that link. I completely agree with it, Seems like canonicalize would be better at service parameter level or called within an event (or anywhere else where request parameters are manually read). This give developers the choice

        I had a similar issue with a WYSIWYG editor, the uploaded html with encoded ampersands & were decoded to & but then attempting to read that html into DOM gave an error because & is not valid in xml.

        And of course OFBIZ-5910

        Show
        gareth.carter Gareth Carter added a comment - I was looking for that link. I completely agree with it, Seems like canonicalize would be better at service parameter level or called within an event (or anywhere else where request parameters are manually read). This give developers the choice I had a similar issue with a WYSIWYG editor, the uploaded html with encoded ampersands & were decoded to & but then attempting to read that html into DOM gave an error because & is not valid in xml. And of course OFBIZ-5910

          People

          • Assignee:
            jacques.le.roux Jacques Le Roux
            Reporter:
            jacques.le.roux Jacques Le Roux
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development