This could be considered a two phase patch. The first issue was that
with the way ByteArrayGuard handled failed decoding in relation to the
ResponseStateManager. The second issue was password storage/retrieval
where the password could change on the user depending on if she/he has a
session allocated. This could cause problems decoding view state while
in transition. The short solution is to have an application-wide
password that is used for both stateful and stateless requests-- see
summary.
Summary
=========================================
ByteArrayGuard is dependent on Session state for encoding/decoding
VIEW_STATE. If password retrieval fails or decoding
fails, it returns null. A few things needed to be fixed:
1. ByteArrayGuard.decrypt(...) should never return null, it will now
throw an IOException if decoding fails.
(This allows ResponseStateManager to correctly return null to the
StateManager.restoreState)
2. Checking Session existence should be done with
ExternalContext.getSession(false), not getSessionMap().
3. ByteArrayGuard should not use a hard coded literal that would be
known from src
4. Logic was modified and comments were added to password retrieval.
Use Cases - FIXME
=========================================
Stateless to Stateful - both will use the same 'application' password,
so no problems.
Stateful to Stateless - invalidating the session will cause the
application password to be used (same one). Unless the server
restarted, decrypting will still work.
Stateless Restart - FIXME, application loses its password, and a new one
will be generated for all succeeding requests. I
suggest adding an init-param to web.xml to define the application
password, if null, generate one. This is rare because
most apps have some session state.
Stateful Restart - you will continue to use the key stored in your session
Clustered - Each machine would have it's own password, but sticky
session cookies in load balancers and session password
storage by this implementation should run correctly.
Modified Files
=========================================
jsf-ri/src/com/sun/faces/renderkit/ByteArrayGuard.java
Diff
=========================================
RCS file:
/cvs/javaserverfaces-sources/jsf-ri/src/com/sun/faces/renderkit/ByteArrayGuard.java,v
retrieving revision 1.2
diff -r1.2 ByteArrayGuard.java
11a12
> import java.io.IOException;
72a74,76
>
> // generate random password in memory
> this.password = getRandomString(DEFAULT_PASSWORD_LENGTH);
84c88
< public byte[] encrypt(FacesContext context, byte[] plaindata) {
---
> public byte[] encrypt(FacesContext context, byte[] plaindata)
throws IOException {
128c132
< public byte[] decrypt(FacesContext context, byte[] securedata) {
---
> public byte[] decrypt(FacesContext context, byte[] securedata)
throws IOException {
153,157c157
< if (logger.isLoggable(Level.WARNING)) {
< // PENDING (visvan) localize
< logger.warning("ERROR: MAC did not verify!");
< }
< return null;
---
> throw new IOException("Could not Decrypt Secure View
State, passwords did not match.");
167c167,168
< /** This method provides a password to be used for
encryption/decryption of
---
> /**
> * This method provides a password to be used for
encryption/decryption of
168a170,179
> * <p>
> * We have two password options. The first is the 'application'
scoped password
> * that is used by all requests. In the case of an application
restart, where
> * session state is persisted, we MUST use the legacy password
from the session
> * instead of our application password.
> * </p>
> * <p>
> * Theoretically after a restart, you could have multiple
passwords being used
> * at once: the application's and legacy sessions'.
> * </p>
171,190c182,199
< Map sessionMap = context.getExternalContext().getSessionMap();
< if ( sessionMap == null ) {
< // Setting it to an arbitrary value. As long as the same
value is used
< // by both serializer and deserializer, the encryption
will work.
< // However, the encryption will be useless since this
arbitrary
< // value can be guessed by an attacker.
< // PENDING (visvan) localize
< if (logger.isLoggable(Level.WARNING)) {
< logger.warning("Key to retrieve password from session
could not "+
< "be found. Using default value. This will
enable " +
< "the encryption and decryption to work, but
the " +
< "client-side state saving method is NO longer
secure.");
< }
< password = "easy-to-guess-password";
< } else {
< password = (String) sessionMap.get(SESSION_KEY_FOR_PASSWORD);
< if (password == null) {
< password = (String)
< ByteArrayGuard.getRandomString(DEFAULT_PASSWORD_LENGTH);
< sessionMap.put(SESSION_KEY_FOR_PASSWORD, password);
---
> // default is to use application scoped password
> String statePwd = this.password;
>
> // check if there is a session available
> Object sessionObj =
context.getExternalContext().getSession(false);
>
> // if there is a session....
> if ( sessionObj != null ) {
> Map sessionMap =
context.getExternalContext().getSessionMap();
>
> // try to get a legacy password from the session to use
> statePwd = (String) sessionMap.get(SESSION_KEY_FOR_PASSWORD);
>
> // if no legacy password, use our application password
> // and store it for session persistence
> if (statePwd == null) {
> statePwd = this.password;
> sessionMap.put(SESSION_KEY_FOR_PASSWORD, statePwd);
192,193c201,202
< }
< return password;
---
> }
> return statePwd;
336,339c345,348
< private int keyLength;
< private int macLength;
< private int ivLength;
< private String password = null;
---
> private final int keyLength;
> private final int macLength;
> private final int ivLength;
> private final String password;
ChangeBundle for Issue #137
Summary
=========================================
ByteArrayGuard is dependent on Session state for encoding/decoding VIEW_STATE. If password retrieval fails or decoding fails, it returns a null. A few things needed to be fixed:
1. ByteArrayGuard.decrypt(...) should never return null, it will now throw an IOException if decoding fails.
2. Checking Session existence should be done with ExternalContext.getSession(false), not getSessionMap().
3. ByteArrayGuard should not use a hard coded literal that would be known from src
4. Logic was modified and comments were added to password retrieval.
Use Cases - FIXME
=========================================
Stateless to Stateful - both will use the same 'application' password, so no problems.
Stateful to Stateless - invalidating the session will cause the application password to be used. Unless the server restarted, decrypting will still work.
Stateless Restart - FIXME, application loses its password, and a new one will be generated for all succeeding requests. I suggest adding an init-param to web.xml to define the application password, if null, generate one. This is rare because most apps have some session state.
Stateful Restart - you will continue to use the key stored in your session
Modified Files
=========================================
jsf-ri/src/com/sun/faces/renderkit/ByteArrayGuard.java
Diff
=========================================
RCS file: /cvs/javaserverfaces-sources/jsf-ri/src/com/sun/faces/renderkit/ByteArrayGuard.java,v
retrieving revision 1.2
diff -r1.2 ByteArrayGuard.java
11a12
> import java.io.IOException;
72a74,76
>
> // generate random password in memory
> this.password = getRandomString(DEFAULT_PASSWORD_LENGTH);
84c88
< public byte[] encrypt(FacesContext context, byte[] plaindata) {
---
> public byte[] encrypt(FacesContext context, byte[] plaindata) throws IOException {
128c132
< public byte[] decrypt(FacesContext context, byte[] securedata) {
---
> public byte[] decrypt(FacesContext context, byte[] securedata) throws IOException {
153,157c157
< if (logger.isLoggable(Level.WARNING)) {
< // PENDING (visvan) localize
< logger.warning("ERROR: MAC did not verify!");
< }
< return null;
---
> throw new IOException("Could not Decrypt Secure View State, passwords did not match.");
167c167,168
< /** This method provides a password to be used for encryption/decryption of
---
> /**
> * This method provides a password to be used for encryption/decryption of
168a170,179
> * <p>
> * We have two password options. The first is the 'application' scoped password
> * that is used by all requests. In the case of an application restart, where
> * session state is persisted, we MUST use the legacy password from the session
> * instead of our application password.
> * </p>
> * <p>
> * Theoretically after a restart, you could have multiple passwords being used
> * at once: the application's and legacy sessions'.
> * </p>
171,190c182,199
< Map sessionMap = context.getExternalContext().getSessionMap();
< if ( sessionMap == null ) {
< // Setting it to an arbitrary value. As long as the same value is used
< // by both serializer and deserializer, the encryption will work.
< // However, the encryption will be useless since this arbitrary
< // value can be guessed by an attacker.
< // PENDING (visvan) localize
< if (logger.isLoggable(Level.WARNING)) {
< logger.warning("Key to retrieve password from session could not "+
< "be found. Using default value. This will enable " +
< "the encryption and decryption to work, but the " +
< "client-side state saving method is NO longer secure.");
< }
< password = "easy-to-guess-password";
< } else {
< password = (String) sessionMap.get(SESSION_KEY_FOR_PASSWORD);
< if (password == null) {
< password = (String)
< ByteArrayGuard.getRandomString(DEFAULT_PASSWORD_LENGTH);
< sessionMap.put(SESSION_KEY_FOR_PASSWORD, password);
---
> // default is to use application scoped password
> String statePwd = this.password;
>
> // check if there is a session available
> Object sessionObj = context.getExternalContext().getSession(false);
>
> // if there is a session....
> if ( sessionObj != null ) {
> Map sessionMap = context.getExternalContext().getSessionMap();
>
> // try to get a legacy password from the session to use
> statePwd = (String) sessionMap.get(SESSION_KEY_FOR_PASSWORD);
>
> // if no legacy password, use our application password
> // and store it for session persistence
> if (statePwd == null) {
> statePwd = this.password;
> sessionMap.put(SESSION_KEY_FOR_PASSWORD, statePwd);
192,193c201,202
< }
< return password;
---
> }
> return statePwd;
336,339c345,348
< private int keyLength;
< private int macLength;
< private int ivLength;
< private String password = null;
---
> private final int keyLength;
> private final int macLength;
> private final int ivLength;
> private final String password;