HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Java secured cookie - security issues?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
issuescookiejavasecuritysecured

Problem

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 secureCookies(Map cookies) {
//we need a private key
checkPrivateKey();

//Return map
HashMap out = new HashMap();

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

//loop all cookies
for (Map.Entry 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 unsecureCookies(Map cookies) {
//we need a private key
checkPrivateKey();

//Return map
HashM

Solution

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 out = new HashMap();

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


you should use:

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

Code Snippets

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

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

Context

StackExchange Code Review Q#57897, answer score: 3

Revisions (0)

No revisions yet.