Java secured cookie - security issues?

Question

I tested the play framework the last days and had mixed feelings about it. In the end, I don't use play for my new REST project. I use http://www.sparkjava.com as minimal server setup.

But I really like the server session less idea from play and want them in spark for the option to scale the performance with more servers. For this reason I wrote this little session framework because this framework must be really save. Please let me know if there are any security problems.

import java.security.SecureRandom;
import java.util.HashMap;
import java.util.Map;
import org.jasypt.util.password.BasicPasswordEncryptor;
import org.jasypt.util.text.BasicTextEncryptor;

/**
 *
 * @author Robert Schütte
 */
public class ProtectCookie {

    private static final BasicPasswordEncryptor passwordEncryptor = new BasicPasswordEncryptor();
    private static BasicTextEncryptor textEncryptor;
    private static String privateKey;

    /**
     * This function secures a map of cookies against modification.
     *
     * @param cookies The unsecured cookies
     * @return The secured cookies
     */
    public static Map<String, String> secureCookies(Map<String, String> cookies) {
        //we need a private key
        checkPrivateKey();

        //Return map
        HashMap<String, String> out = new HashMap<String, String>();

        //cookies in the map?
        if (cookies == null || cookies.isEmpty()) {
            return out;
        }

        //loop all cookies
        for (Map.Entry<String, String> entry : cookies.entrySet()) {
            //secure each cookie
            out.put(entry.getKey(), generateProtectedCookie(entry.getKey(), entry.getValue()));
        }

        //give all cookies back
        return out;
    }

    /**
     * This function checks if the cookies are modified or not. When a cookie is
     * modified, his value will be set to null.
     *
     * @param cookies
     * @return
     */
    public static Map<String, String> unsecureCookies(Map<String, String> cookies) {
        //we need a private key
        checkPrivateKey();

        //Return map
        HashMap<String, String> out = new HashMap<String, String>();

        //cookies in the map?
        if (cookies == null || cookies.isEmpty()) {
            return out;
        }

        //loop all cookies
        for (Map.Entry<String, String> entry : cookies.entrySet()) {
            //unsecure each cookie
            out.put(entry.getKey(), generateUnprotectedCookie(entry.getKey(), entry.getValue()));
        }

        //give all cookies back
        return out;
    }

    /**
     * This function set's the privateKey for this application. The privateKey
     * is used to encrypt the cookies and save them against modification.
     *
     * @param privateKey
     */
    public static void setPrivateKey(String privateKey) {
        ProtectCookie.privateKey = privateKey;
        textEncryptor = new BasicTextEncryptor();
        textEncryptor.setPassword(privateKey);
    }

    //Private functions
    /**
     * This functions checks if a privateKey exist. If not the fucntion creates
     * a new random private key.
     */
    private static void checkPrivateKey() {
        //generate a new random key if not exist
        if (privateKey == null) {

            //available key symbols
            String AB = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ";
            int len = 32;

            //SecureRandom protectd against vulnerable to timing attacks
            SecureRandom rnd = new SecureRandom();

            //generate new random key
            StringBuilder sb = new StringBuilder(len);
            for (int i = 0; i < len; i++) {
                sb.append(AB.charAt(rnd.nextInt(AB.length())));
            }

            //set the key and create a textEncryptor
            setPrivateKey(sb.toString());
        }
    }

    /**
     * This function protect a single cookie value.
     *
     * @param name
     * @param value
     * @return
     */
    private static String generateProtectedCookie(String name, String value) {
        //unmask all +
        value = value.replaceAll("\\+", "&#43;");

        //we hash the name and value to check if anyone change something
        String hash = passwordEncryptor.encryptPassword(name + "+" + value);

        //enrypt the hash to protect it against changes
        String crypt = textEncryptor.encrypt(hash);

        //return the name + value + secured hash
        return value + "+" + crypt;
    }

    /**
     * This function unprotect a single cookie value.
     *
     * @param name
     * @param value
     * @return
     */
    private static String generateUnprotectedCookie(String name, String value) {
        //split the value to value and hash
        String split[] = value.split("\\+", 2);

        //we need the value and hash
        if (split.length != 2) {
            return null;
        }

        value = split[0];
        String hash = split[1];

        //get the old hash with decryption
        hash = textEncryptor.decrypt(hash);

        //when the hash's are equals no one has modified them and we can use them
        if (passwordEncryptor.checkPassword(name + "+" + value, hash)) {
            return value;
        }

        return null;
    }
}

Answer 1

First of all, this code is not thread safe. If different threads find the privateKey as null, each of them will generate a different value into it. You should use "static constructor" or have to use Double-checked locking.

From security reason I didn't see any problem, but the code itself is not following object oriented paradigm. You should avoid static fields and methods, except in utility classes (there are many developer, who suggest to avoid "static only classes" totally).

I guess setPrivateKey(String privateKey) should be private.

I didn' find the code where you convert + back.

I'm not sure, so a question: BasicTextEncryptor could decrypt the hash if the privateKey is different than the encrypt one?

Instead of:

HashMap<String, String> out = new HashMap<String, String>();

    //cookies in the map?
    if (cookies == null || cookies.isEmpty()) {
        return out;
    }

you should use:

    if (cookies == null || cookies.isEmpty()) {
        return Collections.emptyMap();
    }
    HashMap<String, String> out = new HashMap<String, String>();