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

WidgetWorker.buildHyperlinkUrl generates invalid url when using certain sequences of characters

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: Trunk
    • Fix Version/s: 14.12.01, 16.11.01
    • Component/s: framework
    • Labels:
      None

      Description

      If you define a url with parameters or contains url encoded parameters, the output from WidgetWorker.buildHyperlinkUrl may be invalid. This is because of using StringUtil.defaultWebEncoder.canonicalize(localRequestName).

      eg
      abc=&or1=123 -> abc=?1=123
      abc=&to1=123 -> abc=&to1=123 (this one is fine)
      abc=&and1=123 -> abc=?1=123
      abc=&gtabc=123 -> abc=>abc=123

      The owasp HTMLEntityCodec seems to look for special sequences (or, and, gt, lt etc) and change them. This to me is invalid because url encoding and html encoding are different

      Why are the urls encoding the ampersands anyway? (String localRequestName = UtilHttp.encodeAmpersands(target).

        Issue Links

          Activity

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

          More info:

          This only affects the target method param in buildHyperlinkUrl if target contains url params or javascript code with url params

          eg
          /someapp/control/someurl?abc=123&orderId=987
          javascript:someFunction('abc=123&orderId=987')

          The UtilCodec.canonicalize method must decode &amp to & and then detect &or, &amp, &lt or &gt and convert to values which would incorrect for urls

          My suggesstion would be to ditch UtilCodec.canonicalize and use StringEscapeUtils.unescapeHtml

          Show
          gareth.carter Gareth Carter added a comment - - edited More info: This only affects the target method param in buildHyperlinkUrl if target contains url params or javascript code with url params eg /someapp/control/someurl?abc=123&orderId=987 javascript:someFunction('abc=123&orderId=987') The UtilCodec.canonicalize method must decode &amp to & and then detect &or, &amp, &lt or &gt and convert to values which would incorrect for urls My suggesstion would be to ditch UtilCodec.canonicalize and use StringEscapeUtils.unescapeHtml
          Show
          jacopoc Jacopo Cappellato added a comment - Thank you Gareth Carter ! This analysis is also interesting: http://security.coverity.com/blog/2013/Nov/to-escape-or-not-to-escape-that-is-the-question.html
          Hide
          adrianc@hlmksw.com Adrian Crum added a comment -

          There are problems in the screen widget code where URLs are ALWAYS encoded for HTML - even though a different rendering format may be selected. I added some FIXME comments to point that out. HTML encoding should be done only in HTML renderers.

          Show
          adrianc@hlmksw.com Adrian Crum added a comment - There are problems in the screen widget code where URLs are ALWAYS encoded for HTML - even though a different rendering format may be selected. I added some FIXME comments to point that out. HTML encoding should be done only in HTML renderers.
          Hide
          jacopoc Jacopo Cappellato added a comment -

          Ok, I am giving up for today but I would like to summarize the following:
          1) the changes in this method, that are causing the issue reported here, have been committed from OFBIZ-3382; they were happening (since October 2014) before the recent upgrade/refactoring of ESAPI
          2) the bug reported there is still not completely clear to me and I suspect that the fix was not the right solution to it; I would like to spend more time on that
          3) in the meantime we could apply the fix proposed by Gareth Carter in this ticket.

          Show
          jacopoc Jacopo Cappellato added a comment - Ok, I am giving up for today but I would like to summarize the following: 1) the changes in this method, that are causing the issue reported here, have been committed from OFBIZ-3382 ; they were happening (since October 2014) before the recent upgrade/refactoring of ESAPI 2) the bug reported there is still not completely clear to me and I suspect that the fix was not the right solution to it; I would like to spend more time on that 3) in the meantime we could apply the fix proposed by Gareth Carter in this ticket.
          Hide
          jacopoc Jacopo Cappellato added a comment -

          I agree; the approach to XSS prevention we are implementing is too coarse and we should fine tune it; a good reference is the following:
          https://www.owasp.org/index.php/XSS_(Cross_Site_Scripting)_Prevention_Cheat_Sheet

          Show
          jacopoc Jacopo Cappellato added a comment - I agree; the approach to XSS prevention we are implementing is too coarse and we should fine tune it; a good reference is the following: https://www.owasp.org/index.php/XSS_(Cross_Site_Scripting)_Prevention_Cheat_Sheet
          Hide
          jacopoc Jacopo Cappellato added a comment -

          Thanks Gareth, your patch is in rev. 1660031 with the following commit log:

          Applied patch contributed by Gareth Carter in OFBIZ-5910: it is a fix for a bug introduced by the fix for OFBIZ-3382.
          The issue reported in OFBIZ-3382 and the fix committed for it need to be reviewed and possibly reworked but in the meantime this fix will solve the problem with broken links reported recently.

          Show
          jacopoc Jacopo Cappellato added a comment - Thanks Gareth, your patch is in rev. 1660031 with the following commit log: Applied patch contributed by Gareth Carter in OFBIZ-5910 : it is a fix for a bug introduced by the fix for OFBIZ-3382 . The issue reported in OFBIZ-3382 and the fix committed for it need to be reviewed and possibly reworked but in the meantime this fix will solve the problem with broken links reported recently.

            People

            • Assignee:
              jacopoc Jacopo Cappellato
              Reporter:
              gareth.carter Gareth Carter
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development