Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
rcubetrac opened this issue Feb 16, 2015 · 10 comments

Comments

Projects
None yet
2 participants
@rcubetrac
Copy link

commented Feb 16, 2015

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

This comment has been minimized.

Copy link
Author

commented Feb 16, 2015

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

This comment has been minimized.

Copy link
Author

commented Feb 16, 2015

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

later => 1.1.1

@rcubetrac

This comment has been minimized.

Copy link
Author

commented Mar 2, 2015

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

This comment has been minimized.

Copy link
Author

commented Mar 2, 2015

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

This comment has been minimized.

Copy link
Author

commented Mar 10, 2015

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

new => assigned

@rcubetrac

This comment has been minimized.

Copy link
Author

commented Mar 10, 2015

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

=> thomasb

@rcubetrac

This comment has been minimized.

Copy link
Author

commented Mar 12, 2015

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

This comment has been minimized.

Copy link
Author

commented Mar 12, 2015

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

This comment has been minimized.

Copy link
Author

commented Mar 12, 2015

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

assigned => closed

@rcubetrac rcubetrac closed this Mar 12, 2015

@rcubetrac

This comment has been minimized.

Copy link
Author

commented Mar 12, 2015

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

Backported to release-1.1 branch in commit 3e09bcd

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.