javaserverfaces
  1. javaserverfaces
  2. JAVASERVERFACES-2436

Security bug with FacesContext in application startup

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.1.7
    • Fix Version/s: 2.1.11, 2.2.0-m05
    • Component/s: None
    • Labels:
      None

      Description

      The FacesContext that is made available during application startup is held in a ThreadLocal. The reference is not properly cleaned up.

      So, if a JSF WAR calls FacesContext.getCurrentInstance() during app startup, another WAR can get access to the leftover context and thus get access to the other WAR's resources.

      Here is how we have reproduced this bug:
      1) Set the app server so that there is only one thread used for application initialization. In JBoss AS7, this can be done by starting the server with -Dorg.jboss.server.bootstrap.maxThreads=1. This is just done to make the bug easier to reproduce.
      2) Call FacesContext.getCurrentInstance() in a ServletContextListener for WAR #1.
      3) Call FacesContext.getCurrentInstance() in a ServletContextListener for WAR #2 and see the context from WAR #1.

      I'm attaching a maven project that demonstrates this issue. I've reproduced it on JBoss AS7 with Mojarra 2.1.7.
      To reproduce with the attached project:
      1) mvn install
      2) Copy target/JSFCtxLeak-1.0-SNAPSHOT.war to target/JSFCtxLeak.war
      3) Open JSFCtxLeak.war and edit web.xml
      4) Change <display-name> from "Hello World Example 1" to "Hello World Example 2".
      5) Save web.xml back into JSFCtxLeak.war
      6) Start JBoss AS7 with "standalone -Dorg.jboss.server.bootstrap.maxThreads=1"
      7) Deploy JSFCtxLeak-1.0-SNAPSHOT.war
      8) Deploy JSFCtxLeak-1.0.war

      You will get this for output in the JBoss console:
      11:10:03,137 INFO [org.jboss.as.server.deployment] (MSC service thread 1-1) JBAS015876: Starting deployment of "JSFCtxLeak-1.0-SNAPSHOT.war"
      11:10:03,192 INFO [org.jboss.as.osgi] (MSC service thread 1-1) JBAS011907: Register module: Module "deployment.JSFCtxLeak-1.0-SNAPSHOT.war:main" from Service Module Load
      er
      11:10:03,206 INFO [stdout] (MSC service thread 1-1) FacesContext == null
      11:10:03,213 INFO [javax.enterprise.resource.webcontainer.jsf.config] (MSC service thread 1-1) Initializing Mojarra 2.1.7-jbossorg-1 (20120227-1401) for context '/JSFCtx
      Leak-1.0-SNAPSHOT'
      11:10:03,251 INFO [org.jboss.web] (MSC service thread 1-1) JBAS018210: Registering web context: /JSFCtxLeak-1.0-SNAPSHOT
      11:10:03,270 INFO [org.jboss.as.server] (management-handler-thread - 1) JBAS018559: Deployed "JSFCtxLeak-1.0-SNAPSHOT.war"
      11:10:12,688 INFO [org.jboss.as.repository] (management-handler-thread - 3) JBAS014900: Content added at location C:\as7trunk\jboss-as\build\target\jboss-as-7.2.0.Alpha1
      -SNAPSHOT\standalone\data\content\61\a7969abab127ca218afce89192c492de6f5d92\content
      11:10:12,700 INFO [org.jboss.as.server.deployment] (MSC service thread 1-1) JBAS015876: Starting deployment of "JSFCtxLeak.war"
      11:10:12,763 INFO [org.jboss.as.osgi] (MSC service thread 1-1) JBAS011907: Register module: Module "deployment.JSFCtxLeak.war:main" from Service Module Loader
      11:10:12,772 INFO [stdout] (MSC service thread 1-1) Context Name: Hello World Example 1
      11:10:12,777 INFO [javax.enterprise.resource.webcontainer.jsf.config] (MSC service thread 1-1) Initializing Mojarra 2.1.7-jbossorg-1 (20120227-1401) for context '/JSFCtx
      Leak'
      11:10:12,814 INFO [org.jboss.web] (MSC service thread 1-1) JBAS018210: Registering web context: /JSFCtxLeak
      11:10:12,834 INFO [org.jboss.as.server] (management-handler-thread - 3) JBAS018559: Deployed "JSFCtxLeak.war"

      Notice that at 11:10:12,772 the JSFCtxLeak.war retrieved the context name for JSFCtxLeak-1.0-SNAPSHOT.war, which is a security violation.

      There are a few issues here:
      1) FacesContext.getCurrentInstance() should always return null if it is called before the PostConstructApplicationEvent. In this case, our ServletContextListener is called before Mojarra's ConfigureListener. It does return null in the first app, but does not return null in the second app.

      2) The JSF spec should explicitly state that FacesContext.getCurrentInstance() should not be called from a ServletContextListener. The reason is that the ordering of ServletContextListeners is undefined by the Servlet and JSP specs. In this case, it is undefined because ConfigureListener is declared in a TLD and there is no ordering guarantee between those declared in a TLD and those declared in web.xml.

      3) Mojarra should clean up the FacesContext in a finally block when all listeners of the PostConstructApplicationEvent have been notified. Apparently, this was not done because the second deployment was able to see the FacesContext from the first deployment.

        Activity

        Hide
        rogerk added a comment -

        Thanks for the info. I think 1) and 2) are spec issues.
        3) is an implementation issue.
        Can you file a separate spec issue for 1) and 2) (you can refer back to this issue as well).

        Show
        rogerk added a comment - Thanks for the info. I think 1) and 2) are spec issues. 3) is an implementation issue. Can you file a separate spec issue for 1) and 2) (you can refer back to this issue as well).
        Hide
        ssilvert added a comment -

        Hi Roger,

        Are we planning to have this fixed for 2.1.10?

        Stan

        Show
        ssilvert added a comment - Hi Roger, Are we planning to have this fixed for 2.1.10? Stan
        Hide
        rogerk added a comment -

        Hi Stan -

        There's a real problem with the proposed fix. We cannot clear the InitFacesContext where you suggest - when all listeners of the PostConstructApplicationEvent have been notified. We intentionally clear it later because other frameworks such as PrettyFaces still need it. One option we were exploring was that Servlet 3.0 would allow JSF to use a ServletContextListener that cleans up the InitFacesContext using a web-fragment.xml file. We could also declare that we want this ServletContextListener to be the last one executed. However, this would not work for JSF because it does not work for shared libraries. JSF libraries would have to be bundled in WEB-INF/lib of the application.

        Show
        rogerk added a comment - Hi Stan - There's a real problem with the proposed fix. We cannot clear the InitFacesContext where you suggest - when all listeners of the PostConstructApplicationEvent have been notified. We intentionally clear it later because other frameworks such as PrettyFaces still need it. One option we were exploring was that Servlet 3.0 would allow JSF to use a ServletContextListener that cleans up the InitFacesContext using a web-fragment.xml file. We could also declare that we want this ServletContextListener to be the last one executed. However, this would not work for JSF because it does not work for shared libraries. JSF libraries would have to be bundled in WEB-INF/lib of the application.
        Hide
        rogerk added a comment -

        Unless someone has a better idea, the best I can do right now is lobby for a JAVAEE fix (servlet container) to accomodate shared libs as well.

        Show
        rogerk added a comment - Unless someone has a better idea, the best I can do right now is lobby for a JAVAEE fix (servlet container) to accomodate shared libs as well.
        Hide
        ssilvert added a comment -

        Why does PrettyFaces still need it? I've never used PrettyFaces so I don't know the details. Why can't they listen for PostConstructApplicationEvent?

        Show
        ssilvert added a comment - Why does PrettyFaces still need it? I've never used PrettyFaces so I don't know the details. Why can't they listen for PostConstructApplicationEvent?
        Hide
        rogerk added a comment -

        I don't know the detail on that. But I don't think they are the only framework.

        Show
        rogerk added a comment - I don't know the detail on that. But I don't think they are the only framework.
        Hide
        ssilvert added a comment -

        I'm guessing that they are simply doing it wrong by getting FacesContext in a ServletContextListener. We are changing Seam for this very reason.

        A compromise would be to have a switch that allows the old behavior while everyone migrates to using PostConstructApplicationEvent instead of ServletContextListener.

        Show
        ssilvert added a comment - I'm guessing that they are simply doing it wrong by getting FacesContext in a ServletContextListener. We are changing Seam for this very reason. A compromise would be to have a switch that allows the old behavior while everyone migrates to using PostConstructApplicationEvent instead of ServletContextListener.
        Hide
        dfj added a comment -

        Please note that CVE ID CVE-2012-2672 has been assigned to this issue:

        http://www.openwall.com/lists/oss-security/2012/06/07/3

        Show
        dfj added a comment - Please note that CVE ID CVE-2012-2672 has been assigned to this issue: http://www.openwall.com/lists/oss-security/2012/06/07/3
        Hide
        rogerk added a comment -

        ok I could not duplicate this issue on GlassFish with Mojarra 2.1.10.
        I would like to try and duplicate this issue on JBoss AS7 with Mojarra 2.1.7 as is reported in this issue.
        How can I do that? I've downloaded jboss-as-web-7.0.0.Final
        What version of JSF is in that AS 7 release?

        Show
        rogerk added a comment - ok I could not duplicate this issue on GlassFish with Mojarra 2.1.10. I would like to try and duplicate this issue on JBoss AS7 with Mojarra 2.1.7 as is reported in this issue. How can I do that? I've downloaded jboss-as-web-7.0.0.Final What version of JSF is in that AS 7 release?
        Hide
        abn added a comment -

        Hi rogerk,

        AS 7.0.0.Final uses jsf-impl-2.0.4-b09
        AS 7.1.1.Final uses jsf-impl-2.1.7

        Show
        abn added a comment - Hi rogerk , AS 7.0.0.Final uses jsf-impl-2.0.4-b09 AS 7.1.1.Final uses jsf-impl-2.1.7
        Hide
        ssilvert added a comment -

        Actually, the Mojarra version in AS 7.1.1.Final is based on a 2.1.8 snapshot.

        Show
        ssilvert added a comment - Actually, the Mojarra version in AS 7.1.1.Final is based on a 2.1.8 snapshot.
        Hide
        rogerk added a comment -

        I'm just curious.. Under what production scenario would a single thread be used for both app deployment and access?

        Show
        rogerk added a comment - I'm just curious.. Under what production scenario would a single thread be used for both app deployment and access?
        Hide
        abn added a comment -

        IIUC any scenario where limited threads are available there is a chance that the same thread could be used for deployment and access.

        Show
        abn added a comment - IIUC any scenario where limited threads are available there is a chance that the same thread could be used for deployment and access.
        Hide
        ssilvert added a comment -

        > I'm just curious.. Under what production scenario would a single thread be used for both app deployment and access?

        In my scenario, the problem is with deployment threads only. As I've see it, the security breach is limited to situations where you have two application deployments that don't trust each other.

        Show
        ssilvert added a comment - > I'm just curious.. Under what production scenario would a single thread be used for both app deployment and access? In my scenario, the problem is with deployment threads only. As I've see it, the security breach is limited to situations where you have two application deployments that don't trust each other.
        Hide
        arjan tijms added a comment -

        As I've see it, the security breach is limited to situations where you have two application deployments that don't trust each other.

        Perhaps a bit off-topic here, but current application servers are not really ideal for co-hosting potentially hostile to each other applications anyway. E.g. many things like JNDI bound resources are often global and accessible to all application deployments.

        Show
        arjan tijms added a comment - As I've see it, the security breach is limited to situations where you have two application deployments that don't trust each other. Perhaps a bit off-topic here, but current application servers are not really ideal for co-hosting potentially hostile to each other applications anyway. E.g. many things like JNDI bound resources are often global and accessible to all application deployments.
        Hide
        dfj added a comment -

        Current application servers are not ideal for multi-tenanted hosting, but it's something they will increasingly need to support as cloud computing proliferates, therefore I think fixing issues like this one is an important pro-active step.

        Show
        dfj added a comment - Current application servers are not ideal for multi-tenanted hosting, but it's something they will increasingly need to support as cloud computing proliferates, therefore I think fixing issues like this one is an important pro-active step.
        Hide
        ssilvert added a comment -

        BTW, this is not just a security issue. If two applications are using ServletContextListener to get a FacesContext, this can result in getting the wrong one. It's not just a security bug, it's a regular bug too.

        Show
        ssilvert added a comment - BTW, this is not just a security issue. If two applications are using ServletContextListener to get a FacesContext, this can result in getting the wrong one. It's not just a security bug, it's a regular bug too.
        Hide
        rogerk added a comment -

        Can someone give me a clue as to how I can get a patched Mojarra into JBoss AS 7.1.1 for testing?

        Show
        rogerk added a comment - Can someone give me a clue as to how I can get a patched Mojarra into JBoss AS 7.1.1 for testing?
        Hide
        abn added a comment -

        The run-time module can be located at:
        $JBOSS_HOME/modules/com/sun/jsf-impl/main/jsf-impl-2.1.7-jbossorg-2.jar

        Show
        abn added a comment - The run-time module can be located at: $JBOSS_HOME/modules/com/sun/jsf-impl/main/jsf-impl-2.1.7-jbossorg-2.jar
        Hide
        rogerk added a comment -

        Update: We have a patch working, but need to get past some Mojarra test failures.

        Show
        rogerk added a comment - Update: We have a patch working, but need to get past some Mojarra test failures.
        Hide
        rogerk added a comment -

        Patched Jars.

        Show
        rogerk added a comment - Patched Jars.
        Hide
        rogerk added a comment -

        Stan - can you please test the jars I've attached to this issue.

        Show
        rogerk added a comment - Stan - can you please test the jars I've attached to this issue.
        Hide
        rogerk added a comment -

        Changes.

        Show
        rogerk added a comment - Changes.
        Hide
        rogerk added a comment -

        Changes.

        Show
        rogerk added a comment - Changes.
        Hide
        rogerk added a comment -

        changes.

        Show
        rogerk added a comment - changes.
        Hide
        Ed Burns added a comment -

        Suggestions:

        • Remove newly added class variables to InitFacesContext. Just use the static methods.

        Otherwise, looks good. Please make sure quicklook runs with this fix in place.

        Show
        Ed Burns added a comment - Suggestions: Remove newly added class variables to InitFacesContext. Just use the static methods. Otherwise, looks good. Please make sure quicklook runs with this fix in place.
        Hide
        rogerk added a comment -

        Changes

        Show
        rogerk added a comment - Changes
        Hide
        rogerk added a comment -

        changes.

        Show
        rogerk added a comment - changes.
        Hide
        rogerk added a comment -

        Changes For trunk (should be similar to branch).

        Show
        rogerk added a comment - Changes For trunk (should be similar to branch).
        Hide
        rogerk added a comment -

        fix version

        Show
        rogerk added a comment - fix version
        Hide
        rogerk added a comment -

        Committed to trunk: Committed revision 10263.
        Committed to MOJARRA_@_1X_ROLLING branch: revision: 10254

        Show
        rogerk added a comment - Committed to trunk: Committed revision 10263. Committed to MOJARRA_@_1X_ROLLING branch: revision: 10254
        Hide
        stuartdouglas added a comment -

        This fix appears to have introduced a class loader leak, as the wrong object is used in the call to remove() on one of the static maps:

        It is a one line fix:

        https://github.com/stuartwdouglas/mojarra/commit/d929222303f46565b60f47ef3a1a4a1ecee2e484#L0R287

        Show
        stuartdouglas added a comment - This fix appears to have introduced a class loader leak, as the wrong object is used in the call to remove() on one of the static maps: It is a one line fix: https://github.com/stuartwdouglas/mojarra/commit/d929222303f46565b60f47ef3a1a4a1ecee2e484#L0R287
        Hide
        rogerk added a comment -

        Thanks for the patch. Can you please test with the jars4jboss.zip file attached to issue:
        http://java.net/jira/browse/JAVASERVERFACES-2477?

        Show
        rogerk added a comment - Thanks for the patch. Can you please test with the jars4jboss.zip file attached to issue: http://java.net/jira/browse/JAVASERVERFACES-2477?

          People

          • Assignee:
            rogerk
            Reporter:
            ssilvert
          • Votes:
            1 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: