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

        Arran Mccullough created issue -
        Hide
        Arran Mccullough added a comment -

        Case9225Example.war = JSF example

        Show
        Arran Mccullough added a comment - Case9225Example.war = JSF example
        Arran Mccullough made changes -
        Field Original Value New Value
        Attachment Case9225Example2.war [ 12423 ]
        Hide
        Arran Mccullough added a comment -

        Case9225Example.war = ICEfaces example

        Show
        Arran Mccullough added a comment - Case9225Example.war = ICEfaces example
        Arran Mccullough made changes -
        Attachment Case9225Example.war [ 12424 ]
        Hide
        Arran Mccullough added a comment -

        Project source code

        Show
        Arran Mccullough added a comment - Project source code
        Arran Mccullough made changes -
        Attachment Case9225ExampleCode.zip [ 12425 ]
        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>";
        Arran Mccullough made changes -
        Salesforce Case [5007000000C47HV]
        Ken Fyten made changes -
        Fix Version/s 1.8.2-EE-GA_P02 [ 10226 ]
        Fix Version/s 1.8.3 [ 10211 ]
        Ken Fyten made changes -
        Assignee Priority P1
        Assignee Greg Dick [ greg.dick ]
        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.
        Ken Fyten made changes -
        Assignee Greg Dick [ greg.dick ] Ted Goddard [ ted.goddard ]
        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.
        Ken Fyten made changes -
        Fix Version/s 2.0.0 [ 10230 ]
        Fix Version/s 1.8.3 [ 10211 ]
        Fix Version/s 1.8.2-EE-GA_P02 [ 10226 ]
        Assignee Priority P1
        Affects Version/s 2.0-Beta2 [ 10242 ]
        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.
        Ted Goddard made changes -
        Attachment showcase-additions.zip [ 12670 ]
        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.
        Repository Revision Date User Message
        ICEsoft Public SVN Repository #23123 Mon Nov 15 14:11:37 MST 2010 ted.goddard escaping for DOMContext.createTextNode and new DOMContext.createTextNodeUnescaped (ICE-5854)
        Files Changed
        Commit graph MODIFY /icefaces2/trunk/icefaces/compat/core/src/main/java/com/icesoft/faces/renderkit/dom_html_basic/LabelRenderer.java
        Commit graph MODIFY /icefaces2/trunk/icefaces/compat/components/src/main/java/com/icesoft/faces/component/inputrichtext/InputRichTextRenderer.java
        Commit graph MODIFY /icefaces2/trunk/icefaces/compat/components/src/main/java/com/icesoft/faces/component/outputchart/OutputChart.java
        Commit graph MODIFY /icefaces2/trunk/icefaces/compat/components/src/main/java/com/icesoft/faces/component/selectinputdate/SelectInputDateRenderer.java
        Commit graph MODIFY /icefaces2/trunk/icefaces/compat/components/src/main/java/com/icesoft/faces/component/outputconnectionstatus/OutputConnectionStatusRenderer.java
        Commit graph MODIFY /icefaces2/trunk/icefaces/compat/components/src/main/java/com/icesoft/faces/component/selectinputtext/SelectInputTextRenderer.java
        Commit graph MODIFY /icefaces2/trunk/icefaces/compat/core/src/main/java/com/icesoft/faces/renderkit/dom_html_basic/MenuRenderer.java
        Commit graph MODIFY /icefaces2/trunk/icefaces/compat/components/src/main/java/com/icesoft/faces/component/paneltabset/PanelTabSetRenderer.java
        Commit graph MODIFY /icefaces2/trunk/icefaces/compat/core/src/main/java/com/icesoft/faces/renderkit/dom_html_basic/CommandLinkRenderer.java
        Commit graph MODIFY /icefaces2/trunk/icefaces/compat/components/src/main/java/com/icesoft/faces/component/outputprogress/OutputProgressRenderer.java
        Commit graph MODIFY /icefaces2/trunk/icefaces/compat/core/src/main/java/com/icesoft/faces/renderkit/dom_html_basic/SelectManyCheckboxListRenderer.java
        Commit graph MODIFY /icefaces2/trunk/icefaces/compat/components/src/main/java/com/icesoft/faces/component/ext/renderkit/TableRenderer.java
        Commit graph MODIFY /icefaces2/trunk/icefaces/compat/core/src/main/java/com/icesoft/faces/renderkit/dom_html_basic/RadioRenderer.java
        Commit graph MODIFY /icefaces2/trunk/icefaces/compat/core/src/main/java/com/icesoft/faces/renderkit/dom_html_basic/MessageRenderer.java
        Commit graph MODIFY /icefaces2/trunk/icefaces/compat/components/src/main/java/com/icesoft/faces/component/panelcollapsible/PanelCollapsibleRenderer.java
        Commit graph MODIFY /icefaces2/trunk/icefaces/compat/core/src/main/java/com/icesoft/faces/renderkit/dom_html_basic/OutputMessageRenderer.java
        Commit graph MODIFY /icefaces2/trunk/icefaces/compat/core/src/main/java/com/icesoft/faces/renderkit/dom_html_basic/TextRenderer.java
        Commit graph MODIFY /icefaces2/trunk/icefaces/compat/core/src/main/java/com/icesoft/faces/context/DOMContext.java
        Commit graph MODIFY /icefaces2/trunk/icefaces/compat/components/src/main/java/com/icesoft/faces/component/paneldivider/PanelDividerRenderer.java
        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.
        Ted Goddard made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        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.
        Ken Fyten made changes -
        Security Private [ 10001 ]
        Ken Fyten made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Repository Revision Date User Message
        ICEsoft Public SVN Repository #27216 Tue Jan 17 10:32:23 MST 2012 ted.goddard backported output escaping from ICE-5854 (ICE-7658)
        Files Changed
        Commit graph MODIFY /icefaces/trunk/icefaces/core/src/com/icesoft/faces/renderkit/dom_html_basic/SelectManyCheckboxListRenderer.java
        Commit graph MODIFY /icefaces/trunk/icefaces/component/src/com/icesoft/faces/component/outputprogress/OutputProgressRenderer.java
        Commit graph MODIFY /icefaces/trunk/icefaces/component/src/com/icesoft/faces/component/paneltabset/PanelTabSetRenderer.java
        Commit graph MODIFY /icefaces/trunk/icefaces/component/src/com/icesoft/faces/component/panelcollapsible/PanelCollapsibleRenderer.java
        Commit graph MODIFY /icefaces/trunk/icefaces/component/src/com/icesoft/faces/component/paneldivider/PanelDividerRenderer.java
        Commit graph MODIFY /icefaces/trunk/icefaces/core/src/com/icesoft/faces/renderkit/dom_html_basic/LabelRenderer.java
        Commit graph MODIFY /icefaces/trunk/icefaces/core/src/com/icesoft/faces/renderkit/dom_html_basic/CommandLinkRenderer.java
        Commit graph MODIFY /icefaces/trunk/icefaces/core/src/com/icesoft/faces/renderkit/dom_html_basic/MessageRenderer.java
        Commit graph MODIFY /icefaces/trunk/icefaces/component/src/com/icesoft/faces/component/selectinputtext/SelectInputTextRenderer.java
        Commit graph MODIFY /icefaces/trunk/icefaces/core/src/com/icesoft/faces/context/DOMContext.java
        Commit graph MODIFY /icefaces/trunk/icefaces/core/src/com/icesoft/faces/renderkit/dom_html_basic/OutputMessageRenderer.java
        Commit graph MODIFY /icefaces/trunk/icefaces/component/src/com/icesoft/faces/component/outputconnectionstatus/OutputConnectionStatusRenderer.java
        Commit graph MODIFY /icefaces/trunk/icefaces/core/src/com/icesoft/faces/renderkit/dom_html_basic/RadioRenderer.java
        Commit graph MODIFY /icefaces/trunk/icefaces/component/src/com/icesoft/faces/component/outputchart/OutputChart.java
        Commit graph MODIFY /icefaces/trunk/icefaces/component/src/com/icesoft/faces/component/inputrichtext/InputRichTextRenderer.java
        Commit graph MODIFY /icefaces/trunk/icefaces/component/src/com/icesoft/faces/component/ext/renderkit/TableRenderer.java
        Commit graph MODIFY /icefaces/trunk/icefaces/core/src/com/icesoft/faces/renderkit/dom_html_basic/MenuRenderer.java
        Commit graph MODIFY /icefaces/trunk/icefaces/core/src/com/icesoft/faces/renderkit/dom_html_basic/TextRenderer.java
        Commit graph MODIFY /icefaces/trunk/icefaces/component/src/com/icesoft/faces/component/selectinputdate/SelectInputDateRenderer.java

          People

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

            Dates

            • Created:
              Updated:
              Resolved: