[jira] [Comment Edited] (TRINIDAD-2567) Trinidad secret generation is not thread-safe

["Kyle Stiemann (Jira)" <de...@myfaces.apache.org>]

    [ https://issues.apache.org/jira/browse/TRINIDAD-2567?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17283356#comment-17283356 ] 

Kyle Stiemann edited comment on TRINIDAD-2567 at 2/11/21, 8:40 PM:
-------------------------------------------------------------------

Thanks [~tandraschko], I'm not super worried about this getting fixed as the workaround was trivial for me. I really just wanted to document the issue so that if I ever run into it again (or if someone else does), the problem (and workaround) is googleable.


was (Author: stiemannkj1@gmail.com):
Thanks [~tandraschko], I'm not super worried about this getting fixed as the workaround was trivial for me. I really just wanted to document the issue so that if I ever run into it again (or if someone else does), the problem (and workaround) is in google.

> Trinidad secret generation is not thread-safe
> ---------------------------------------------
>
>                 Key: TRINIDAD-2567
>                 URL: https://issues.apache.org/jira/browse/TRINIDAD-2567
>             Project: MyFaces Trinidad
>          Issue Type: Bug
>          Components: Components, Facelets, Infrastructure, Plugins
>    Affects Versions: 2.2.1-core
>            Reporter: Kyle Stiemann
>            Priority: Minor
>
> 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:
> {code}
> 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;
> }
> {code}
> {{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}}.
> h2. 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*):
> {code:xml}
> <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>
> {code}
> h2. 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).
> h2. Steps to Reproduce:
> # Create 1 WAR with a simple {{ping.xhtml}} endpoint:
> {code:xml}
> <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>
> {code}
> # Create another Trinidad WAR with the following view and bean:
> *{{hello.xhtml}}:*
> {code:xml}
> <?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>
> {code}
> *{{HelloBean.java}}:*
> {code:java}
> @ManagedBean
> @RequestScoped
> public final class HelloBean {
>   private String name;
>   public String getName() {
>     return name;
>   }
>   public void setName(String name) {
>     this.name = name;
>   }
> }
> {code}
> # Start up an app server like Tomcat with both WARs deployed.
> # Using a script:
> ## {{GET}} the {{ping.xhtml}} endpoint to cause the app server to initialize.
> ## {{GET}} the {{hello.xhtml}} endpoint to obtain the view state and session id.
> ## Use the view state and session id to {{POST}} a name to the {{hello.xhtml}} form.
> ## 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: 
> {code:sh}
> #!/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
> {code}
> h3. Result:
> If the bug still exists, 4 of the 5 {{POST}} s will fail with a {{ViewExpiredException}}:
> {code:html}
> <!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>
> {code}
> If the bug is fixed, all 5 requests will succeed and show "Hello Test!" due to a successful {{form}} {{POST}}.
> h2. Related
> If the developer disables secret caching without specifying a key, all {{POST}} requests will fail with similar results.
> {code:xml}
> <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>
> {code}
> 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.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)