Local storage prefix should not be calculated from substring of DES key

[rcubetrac]

Reported by mgrum on 16 Feb 2015 10:47 UTC as Trac ticket #1490279

The prefix string for local storage is generated from a variable called rcmail.env.user_id, which is a hash that uniquely identifies a user. This hash is calculated on the server side using the following function:

function get_hash()
{
    $key = substr($this->rc->config->get('des_key'), 1, 4);
    return md5($this->data[. $key . $this->data['username']('user_id']) . '@' . $this->data['mail_host']);
}

The problem is that this hash is visible to users (you can find it by using the javascript console in your browser or something like that).

This means that an attacker who knows the mail host only has to iterate over all possible user ids and all possible four character ASCII strings (until the MD5 hash matches the one in rcmail.env.user_id) in order to find out the internal user_id in the database and four characters of the DES key. Four characters is not much, but still, it should never be possible for an attacker to find out any part of a secret key at all.

This attack needs about 128^4 * number_of_users hashes, so depending on how many users there are, it might take a few hours or a few days if you use a modern GPU that can do something in the range of 10^8 MD5 hashes per second.

I am not even sure why it is necessary to use a secret salt for this hash in the first place. I think it would be sufficient to use only a hash of the username or something like that. Of course, that would be reproducible, so users could calculate these hashes on their own, but I don't see why this would be a problem, since it is only used as a local storage prefix anyway, which is nothing secret (and also, using a deterministic hash like this would allow server administrators to change the DES key without breaking the local storage). Or am I missing something here?

Migrated-From: http://trac.roundcube.net/ticket/1490279

[rcubetrac]

Comment by @alecpl on 16 Feb 2015 11:49 UTC

Maybe it would be safer to just encrypt the string like this:

function get_hash()
{
    return md5($this->rc->encrypt($this->data[. $this->data['username']('user_id']) . '@' . $this->data['mail_host']));
}

[rcubetrac]

Milestone changed by @alecpl on 16 Feb 2015 11:49 UTC

later => 1.1.1

[rcubetrac]

Comment by @alecpl on 2 Mar 2015 12:04 UTC

Stupid me. We can't use encryption as it generates different results on every execution.

[rcubetrac]

Comment by @alecpl on 2 Mar 2015 12:26 UTC

So, I see two options here:

1. Just don't use any part of des_key in the hash, it should be enough.
2. Generate a unique hash once, not based on des_key nor user data, and store it in the database (user preferences).

We should also consider using sha256, but since this hash really do not secure the local storage, it does not matter, I suppose.

[rcubetrac]

Status changed by @thomascube on 10 Mar 2015 08:02 UTC

new => assigned

[rcubetrac]

Owner changed by @thomascube on 10 Mar 2015 08:02 UTC

=> thomasb

[rcubetrac]

Comment by @thomascube on 12 Mar 2015 08:06 UTC

1. is likely the way to go. However, changing the user_id will "reset" all user prefs (e.g. splitter positions, etc.) stored in local storage because the old value cannot be accessed anymore.

[rcubetrac]

Comment by @thomascube on 12 Mar 2015 08:53 UTC

Fixed by generating a random hash and storing that in user prefs. See commit a74d023

[rcubetrac]

Status changed by @thomascube on 12 Mar 2015 08:53 UTC

assigned => closed

[rcubetrac]

Comment by @thomascube on 12 Mar 2015 08:55 UTC

Backported to release-1.1 branch in commit 3e09bcd