ICEfaces
  1. ICEfaces
  2. ICE-9032

Remove Class.forName from setEventPhase for improved security

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.2
    • Fix Version/s: 3.3
    • Component/s: Framework, ICE-Components
    • Labels:
      None
    • Environment:
      icecore / compat
    • Assignee Priority:
      P1
    • Salesforce Case Reference:

      Description

      When using client-side state saving, component property values are populated from data received from the client. Thus the possibility exists for a maliciously constructed request to give properties hostile values. It's unrecommended for ICEfaces applications to use client-side state, for this security reason, as well as performance reasons, given the network load of transmitting data during frequent AJAX requests.

      As well, applications could conceivably have bugs where bean values used for some component properties may accidentally be tied into an input component's value, so that maliciously constructed requests could affect the application in inadvertent ways.

      Due to the above possibilities, static analysis scanning software, such as Veracode, show that setEventPhase's use of a component property, to specify the parameters to Class.forName, is potentially unsafe, as that could lead to a malicious class being class loaded, which could cause its static block code to execute. This would probably require having the malicious code already on your classpath, so again is not an expected scenario. Best to remove all possibility though.

        Activity

        Mark Collette created issue -
        Hide
        Mark Collette added a comment -

        In the code in setEvantPhase that takes the events property and uses it as a space delimited list of class names, to use as a mask for the current event, rather than using Class.forName on them, and isInstance on those classes, instead walk up the current event's super classes, doing a String comparison on the class names with the strings in the events property. We will lose the type checking on the events property that ensured there was no typo in a specified class name, but will close the Class.forName security hole.

        Show
        Mark Collette added a comment - In the code in setEvantPhase that takes the events property and uses it as a space delimited list of class names, to use as a mask for the current event, rather than using Class.forName on them, and isInstance on those classes, instead walk up the current event's super classes, doing a String comparison on the class names with the strings in the events property. We will lose the type checking on the events property that ensured there was no typo in a specified class name, but will close the Class.forName security hole.
        Evgheni Sadovoi made changes -
        Field Original Value New Value
        Salesforce Case Reference 5007000000RVFs8AAH
        Ken Fyten made changes -
        Assignee Mark Collette [ mark.collette ]
        Fix Version/s 3.3 [ 10370 ]
        Assignee Priority P3 [ 10012 ]
        Ken Fyten made changes -
        Assignee Priority P3 [ 10012 ] P1 [ 10010 ]
        Repository Revision Date User Message
        ICEsoft Public SVN Repository #34180 Mon Apr 01 13:45:27 MDT 2013 mark.collette ICE-9032 : Remove Class.forName from setEventPhase for improved security
        Files Changed
        Commit graph MODIFY /icefaces3/trunk/icefaces/compat/components/src/main/java/com/icesoft/faces/component/seteventphase/SetEventPhase.java
        Commit graph MODIFY /icefaces3/trunk/icefaces/core/src/main/java/org/icefaces/impl/component/SetEventPhase.java
        Hide
        Mark Collette added a comment -

        Updated icecore:setEventPhase and ice:setEventPhase to use a new technique of walking up the event interfaces and superclasses to do a String comparison of the class / interface names against the specified events. Now, now Class.forName call is used. Note that there is a variant of Class.forName that won't initialise the classes, but it still will class (try to) load them, so possibly this is faster.

        icefaces3 trunk
        Subversion 34180

        Show
        Mark Collette added a comment - Updated icecore:setEventPhase and ice:setEventPhase to use a new technique of walking up the event interfaces and superclasses to do a String comparison of the class / interface names against the specified events. Now, now Class.forName call is used. Note that there is a variant of Class.forName that won't initialise the classes, but it still will class (try to) load them, so possibly this is faster. icefaces3 trunk Subversion 34180
        Mark Collette made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Ken Fyten made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            Mark Collette
            Reporter:
            Mark Collette
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: