move decrypt into the try catch so we handle if bytes are not valid

[codylerum]

This place the ByteArrayGuardAESCTR.decrypt inside the existing try catch so that general errors such as the follow are properly caught and the cookie is reported as bad. This still allows the InvalidKeyException to propagate up to it's specific catch handling.

See: #4547 and https://github.com/javaserverfaces/mojarra/issues/4386

java.lang.ArrayIndexOutOfBoundsException
	at java.lang.System.arraycopy(Native Method)
	at com.sun.faces.util.ByteArrayGuardAESCTR.decrypt(ByteArrayGuardAESCTR.java:133)
	at com.sun.faces.context.flash.ELFlash$PreviousNextFlashInfoManager.decode(ELFlash.java:1391)
	at com.sun.faces.context.flash.ELFlash.getCurrentFlashManager(ELFlash.java:1214)
	at com.sun.faces.context.flash.ELFlash.doPrePhaseActions(ELFlash.java:618)
	at com.sun.faces.lifecycle.Phase.handleBeforePhase(Phase.java:190)
	at com.sun.faces.lifecycle.Phase.doPhase(Phase.java:74)
	at com.sun.faces.lifecycle.RestoreViewPhase.doPhase(RestoreViewPhase.java:109)
	at com.sun.faces.lifecycle.LifecycleImpl.execute(LifecycleImpl.java:177)
	at javax.faces.webapp.FacesServlet.executeLifecyle(FacesServlet.java:707)
	at javax.faces.webapp.FacesServlet.service(FacesServlet.java:451)

[juneau001]

Reviewed code and ran standard test suite. However, please provide tests for this repair, as it is a standard practice to provide a test and I want to ensure that it doesn't break anything. Thanks for the contribution, it is appreciated!

[vonnai]

Hi, when are you going to merge this fix into master? It's fixed a half of year and nothing in production. It's real problem with not valid cookie value, application using mojarra is unusable. Thanks so much.

[codylerum]

@juneau001 do you have a similar test you could point me at to replicate? I'm not familiar with the whole suite and project layout to really know where to look.

[vonnai]

Guys, is it really safe, that function decrypt of class ByteArrayGuardAESCTR doesn't check the input? There is not any null check, length check of parameter value, then there is a copy of array which requests the 16 bytes. and thats the problem. You should catch NullPointerException and ArrayIndexOutOfBoundExcetion too. The invalid csfcfc cookie can be e.g:

• AAAABBBBCCCCDDDDEEEEFFB, which is not decryptable, array copy doens't fail, InvalidKeyException is thrown, Application works
• hjkfsd%kd, array copy failed, ArrayIndexOutOfBoundException is thrown, Application doesn't work, user have to delete cookie.

Here is my fix code:

/**
 * !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
 * FIX, HACK what you want but we must solve invalid csfcfc cookie value which is stored in browser where run our web app in previous version.
 * https://github.com/eclipse-ee4j/mojarra/pull/4548
 * CAUSE is upgrade mojarra from 2.2 to 2.3
 * DISADVANTAGE of this fix is missing login message after redirect from login page.
 * @param req 
 */
 private void fixFlashCookieUgly(HttpServletRequest req) {
      if (req.getCookies() != null) {
          for (int i = 0; i < req.getCookies().length; i++) {
              Cookie c = req.getCookies()[i];
              if ("csfcfc".equals(c.getName())) {
                  if (c.getValue() == null || c.getValue().length() < 21) {
                      //set dummy value which will cause InvalidKeyException in com.sun.faces.context.flash.ByteArrayGuardAESCTR.java(152)
                      c.setValue("AAAABBBBCCCCDDDDEEEEFFB");
                  }
              }
          }
      }
  }

[juneau001]

@codylerum I will look into making a quick test for this one. I appreciate the contribution. @vonnai Thanks for the follow up, I appreciate the information.

[jasonex7]

I've just open PR #4668, which, in my opinion, is a better a approach to resolve this issue because it deals with class ByteArrayGuardAESCTR which is propagating an unnecessary unchecked exception when cookie value doesn't has IV (Initialization Vector) bytes, instead it should propagate a checked exception indicating the value is not valid.

[juneau001]

I've just open PR #4668, which, in my opinion, is a better a approach to resolve this issue because it deals with class ByteArrayGuardAESCTR which is propagating an unnecessary unchecked exception when cookie value doesn't has IV (Initialization Vector) bytes, instead it should propagate a checked exception indicating the value is not valid.

I will take a look a the PR, thanks.

[codylerum]

Handled via #4668