6
\$\begingroup\$

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;
    }
}
\$\endgroup\$
  • \$\begingroup\$ Hello there and welcome to Code Review! I took the liberty to edit your question a little, the reasons are outlined in the edit-summary. If you disagree with my edit, feel free to roll back in revision history. Feel free to participate on Code Review Meta and in Code Review Chat (when you got 20 rep). I hope you become a regular contributor to our site ;) \$\endgroup\$ – Vogel612 Jul 24 '14 at 8:02
  • \$\begingroup\$ Thank you for the welcome! I agree with your changes ;) - Now I hope to get a feedback to the code :) \$\endgroup\$ – Roba Jul 24 '14 at 8:36
  • \$\begingroup\$ You might find the following links useful: #4 here, codereview.stackexchange.com/a/41876/7076, #9 here, #1 here \$\endgroup\$ – palacsint Jul 24 '14 at 20:34
3
\$\begingroup\$

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>();
\$\endgroup\$
  • \$\begingroup\$ Thank you for the answer. I made all the changes you said. The function setPrivateKey(String privateKey) must be public, to set a Key from the outside, to use the same key on multiple server. \$\endgroup\$ – Roba Oct 20 '14 at 14:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.