[TRINIDAD-2567] Trinidad secret generation is not thread-safe - ASF JIRA

XML

Word

Printable

JSON

Details

Type: Bug
Status: Open
Priority: Minor
Resolution: Unresolved
Affects Version/s: 2.2.1-core
Fix Version/s: None
Component/s: Components, Facelets, Infrastructure, Plugins
Labels:
None

Description

Sending multiple requests in rapid succession to a Trinidad application that has just started will cause multiple different secret keys to be generated. If multiple POST s are sent, all but 1 will fail with a ViewExpiredException. Trinidad generates the secret keys in StateUtils somewhat like this:

private static SecretKey getSecret(ExternalContext ctx) {
  SecretKey secretKey = (SecretKey) ctx.getApplicationMap().get(INIT_SECRET_KEY_CACHE);
  if (secretKey == null) {
    secretKey = createSecretKey(KeyGenerator.getInstance(getAlgorithm(ctx)).generateKey().getEncoded());
    ctx.getApplicationMap().put(INIT_SECRET_KEY_CACHE, secretKey);
  }
  return secretKey;
}

FormRenderer calls ViewHandler.writeState() which calls the StateUtils.getSecret() method on each request. If more than 1 request calls getSecret() before the secret key is set in INIT_SECRET_KEY_CACHE, each call to getSecret() has the chance to see a null value for INIT_SECRET_KEY_CACHE, generate a new secret key, and replace any existing secret in INIT_SECRET_KEY_CACHE. Any view state that was generated using the discarded secrets will be unusable and cause a ViewExpiredException.

Workarounds

The simplest workaround is to set values for the secret keys as init-param s: https://cwiki.apache.org/confluence/display/MYFACES2/Secure+Your+Application. For example, in the web.xml (note that the provided values are examples and should not be used in a production application):

<context-param>
    <param-name>org.apache.myfaces.SECRET</param-name>
    <!-- Decodes to TEST_KEY -->
    <param-value>VEVTVF9LRVk=</param-value>
</context-param>
<context-param>
    <param-name>org.apache.myfaces.MAC_SECRET</param-name>
    <!-- Decodes to TRINIDAD_TEST_MAC_SECRET -->
    <param-value>VFJJTklEQURfVEVTVF9NQUNfU0VDUkVU</param-value>
</context-param>

Potential Fixes

Save 1 generated key in the application scope using either Map.putIfAbsent() or some other kind of synchronization. Use only the key from the application scope to generate the SecretKey object. Even if secret object caching is disabled, only 1 key would be used to generate the secret object, so the application would still function.
Use Map.putIfAbsent() to ensure only 1 secret is ever cached in the application. If secret caching is disabled, the application would still not function (which is the same as the existing behavior).

Steps to Reproduce:

Create 1 WAR with a simple ping.xhtml endpoint:

<html
  xmlns:h="http://java.sun.com/jsf/html"
  xmlns="http://www.w3.org/1999/xhtml">
    <h:head/>
    <h:body>
        <code>pong</code>
    </h:body>
</html>

Create another Trinidad WAR with the following view and bean:
hello.xhtml:

<?xml version="1.0" encoding="UTF-8"?>
<tr:document
  xmlns:tr="http://myfaces.apache.org/trinidad"
  title="hello">
    <tr:form id="form">
        <tr:inputText id="name" label="Please enter your name:" required="true" value="#{helloBean.name}" />
        <tr:commandButton id="submitName" text="Submit" /><br />
        <tr:outputText value="Hello #{helloBean.name}!" />
    </tr:form>
</tr:document>

HelloBean.java:

@ManagedBean
@RequestScoped
public final class HelloBean {
  private String name;

  public String getName() {
    return name;
  }

  public void setName(String name) {
    this.name = name;
  }
}

Start up an app server like Tomcat with both WARs deployed.
Using a script:
1. GET the ping.xhtml endpoint to cause the app server to initialize.
2. GET the hello.xhtml endpoint to obtain the view state and session id.
3. Use the view state and session id to POST a name to the hello.xhtml form.
4. Repeat the GET and POST 5 times in rapid succession with different sessions.

Here's an example bash script which uses curl to execute the above steps:

#!/bin/bash

sendPost() {
  ENCODED_VIEW_STATE="$(curl -s --cookie-jar /tmp/cookie-jar-$1 --cookie /tmp/cookie-jar-$1 \
    'http://localhost:8080/trinidad-2.2/faces/hello.xhtml' | \
    tr -d '\n' | sed 's/.*name="javax.faces.ViewState".*value="\([^"][^"]*\)".*/\1/' | \
    sed -e 's|/|\%2F|g' -e 's/+/%2B/g' -e 's/=/%3D/g')"
  curl --cookie-jar /tmp/cookie-jar-$1 --cookie /tmp/cookie-jar-$1 \
    -d "javax.faces.ViewState=$ENCODED_VIEW_STATE&org.apache.myfaces.trinidad.faces.FORM=form&source=submitName&name=Test" \
    -X POST 'http://localhost:8080/trinidad-2.2/faces/hello.xhtml'
}

rm /tmp/cookie-jar*; until curl -s 'http://localhost:8080/other-app/ping.xhtml'; do :; done &&
for i in {1..5}; do sendPost $i & done

Result:

If the bug still exists, 4 of the 5 POST s will fail with a ViewExpiredException:

<!doctype html>
<html lang="en">
<head><title>HTTP Status 500 – Internal Server Error</title>
    <style type="text/css">body {font-family: Tahoma, Arial, sans-serif;} h1, h2, h3, b {color: white; background-color: #525D76; } h1 { font-size: 22px; } h2 { font-size: 16px; } h3 { font-size: 14px; } p { font-size: 12px; } a { color: black; } .line { height: 1px; background-color: #525D76; border: none; }</style>
</head>
<body><h1>HTTP Status 500 – Internal Server Error</h1>
<hr class="line"/>
<p><b>Type</b> Exception Report</p>
<p><b>Message</b> viewId:&#47;hello.xhtml - View &#47;hello.xhtml could not be restored.</p>
<p><b>Description</b> The server encountered an unexpected condition that prevented it from fulfilling the request.</p>
<p><b>Exception</b></p>
<pre>javax.servlet.ServletException: viewId:&#47;hello.xhtml - View &#47;hello.xhtml could not be restored.
        javax.faces.webapp.FacesServlet.service(FacesServlet.java:671)
        org.apache.myfaces.trinidadinternal.webapp.TrinidadFilterImpl._doFilterImpl(TrinidadFilterImpl.java:350)
        org.apache.myfaces.trinidadinternal.webapp.TrinidadFilterImpl.doFilter(TrinidadFilterImpl.java:226)
        org.apache.myfaces.trinidad.webapp.TrinidadFilter.doFilter(TrinidadFilter.java:92)
        org.apache.tomcat.websocket.server.WsFilter.doFilter(WsFilter.java:53)
</pre>
<p><b>Root Cause</b></p>
<pre>javax.faces.application.ViewExpiredException: viewId:&#47;hello.xhtml - View &#47;hello.xhtml could not be restored.
        com.sun.faces.lifecycle.RestoreViewPhase.execute(RestoreViewPhase.java:212)
        com.sun.faces.lifecycle.Phase.doPhase(Phase.java:101)
        com.sun.faces.lifecycle.RestoreViewPhase.doPhase(RestoreViewPhase.java:123)
        com.sun.faces.lifecycle.LifecycleImpl.execute(LifecycleImpl.java:198)
        javax.faces.webapp.FacesServlet.service(FacesServlet.java:658)
        org.apache.myfaces.trinidadinternal.webapp.TrinidadFilterImpl._doFilterImpl(TrinidadFilterImpl.java:350)
        org.apache.myfaces.trinidadinternal.webapp.TrinidadFilterImpl.doFilter(TrinidadFilterImpl.java:226)
        org.apache.myfaces.trinidad.webapp.TrinidadFilter.doFilter(TrinidadFilter.java:92)
        org.apache.tomcat.websocket.server.WsFilter.doFilter(WsFilter.java:53)
</pre>
<p><b>Note</b> The full stack trace of the root cause is available in the server logs.</p>
<hr class="line"/>
<h3>Apache Tomcat/9.0.39</h3></body>
</html>

If the bug is fixed, all 5 requests will succeed and show "Hello Test!" due to a successful form POST.

If the developer disables secret caching without specifying a key, all POST requests will fail with similar results.

<context-param>
    <param-name>org.apache.myfaces.SECRET.CACHE</param-name>
    <param-value>false</param-value>
</context-param>
<context-param>
    <param-name>org.apache.myfaces.MAC_SECRET.CACHE</param-name>
    <param-value>false</param-value>
</context-param>

I'm unsure whether the secret cache was ever intended to be disabled when there is no key set, but it may be worth fixing that issue alongside this one.

Attachments

Activity

People

Assignee:: Unassigned

Reporter:: Kyle Stiemann

Votes:: 0 Vote for this issue

Watchers:: 2 Start watching this issue

Dates

Created:: 11/Feb/21 03:27

Updated:: 11/Feb/21 20:40