[IMP] gmail: encrypt the database with the sub of the Gmail token

[std-odoo]

Purpose

Encrypt the database with the sub of the Gmail token, to reduce the damage in case of a database leak.

Technical

We don't even need to store the email anymore (the hash of the key is all we need).

What's not encrypted is

• user_id, for the foreign key between the log and the users
• create_date, so the CRON can clean the old log

Nor of those information are really important, and we won't be able to link them to a specific Odoo / Gmail user without the encryption key.

The tricky point is the Odoo authentication process, the user is redirected to Odoo to accept the plugin, and then is redirected to the plugin with the Odoo token in the URL. We need to save the Odoo token in database, and so we need the encryption key. So we derive an application key from the application secret (for convenience, so there's no restriction on application secret size), and we encrypt with the application key, the user's key, user's email, login token, etc. That way those informations do not appear in the URL (and his browser history).

Task-6366629

[odony]

Thanks for the work!

Here are some very quick, very raw comments, to be discussed together, as I barely understand the high level concepts.

[odony]

☝ as discussed above, we should include the app secret in this kdf, to protect our encrypted data better from a database leak, considering that the sub/email are not real secrets. If the attacker knows the victim, they can probably trick them to obtain this pair.

We can't do anything if the attacker is able to steal the APP_SECRET too, but if they only get the database contents, this will make it much more robust, so it's worth it.

Also, hashing the app key with the current payload gives a more robust 256b seed for the kdf.

e.g.:

const userSub = `${payload.sub}-${payload.email.toLowerCase()}`;
const userKeySeed = hmacSha256(await getApplicationKey(), userSub).toString("hex");
const encryptionKey = await deriveKeyScryptCached(userKeySeed, "Odoo-Gmail-Addin-Salt");

this is worth explicit comments on the rationale, including the conscious decision of including the email (raises the bar for guessing / brute-force when not knowing the target victim, and we can consider that users's email don't change - or it's fine if they have to reconnect)

[std-odoo]

ok to add the application key

about doing a first hmac, I'm not 100% sure its' really useful compare to just scrypt(app_secret + sub + email), but it doesn't hurt, so I did it

I added a comment about the choices

[odony]

☝ not too fond of passing the encryptionKey here, as it would be permanent in logs/browser history.

How about keeping an in-memory map {loginToken → (sub, email)}? This way the state is useless after expiration of the loginToken?
We delete the entry after use, and we benefit from the cache for re-derivation of the encryptionKey.

As an added safety we could include a cleanup in the cron job for never-used entries, but they are not much of a threat if the db token is not yet exchanged?
(IIRC there's a cron job of some sort)

[std-odoo]

☝ not too fond of passing the encryptionKey here, as it would be permanent in logs/browser history.

It's encrypted with the application key, so we will need both user history / app key to decrypt it (and then a db leak to read the user Odoo token)

How about keeping an in-memory map {loginToken → (sub, email)}? This way the state is useless after expiration of the loginToken?

I'm afraid about multi node process (that won't share the memory).

I also though about not encrypting the login token, having a second column "temporary state encryption key", we get the sql row thanks to the login token, then the temporary encryption key, and we decrypt the sub-email. But I think it's over engineering and make it too complex, let me know what you think.

As an added safety we could include a cleanup in the cron job for never-used entries

Ok (we will just need to not encrypt enc_login_token_expire_at, but not a big deal)

but they are not much of a threat if the db token is not yet exchanged?

Indeed, one would need the user sub, email, app secret (to produce a valid ciphertext encState) in addition to the login token to be able to log the user in a different database (and then trick him to log an email to get it)

Login token is useful to temporary replace the logic of "we check that the user is logged in gmail", see getUserFromGoogleToken (but encrypting the login token in db does not cost that much since everything is encrypted, it can also be hashed instead if we want)

[std-odoo]

@odony Hi 🙂

Thanks for the review 🙂 I did most change except "storing in memory login_token:(sub, email)" because I'm afraid of multi node process in the production server. I think the current approach is good enough, but have a proposition if we want to be protected against "app secret leak + user browser history leak" (see bellow)

To be discussed tomorrow 🙂

[std-odoo]

ok to add the application key

about doing a first hmac, I'm not 100% sure its' really useful compare to just scrypt(app_secret + sub + email), but it doesn't hurt, so I did it

I added a comment about the choices

[std-odoo]

☝ not too fond of passing the encryptionKey here, as it would be permanent in logs/browser history.

It's encrypted with the application key, so we will need both user history / app key to decrypt it (and then a db leak to read the user Odoo token)

How about keeping an in-memory map {loginToken → (sub, email)}? This way the state is useless after expiration of the loginToken?

I'm afraid about multi node process (that won't share the memory).

I also though about not encrypting the login token, having a second column "temporary state encryption key", we get the sql row thanks to the login token, then the temporary encryption key, and we decrypt the sub-email. But I think it's over engineering and make it too complex, let me know what you think.

As an added safety we could include a cleanup in the cron job for never-used entries

Ok (we will just need to not encrypt enc_login_token_expire_at, but not a big deal)

but they are not much of a threat if the db token is not yet exchanged?

Indeed, one would need the user sub, email, app secret (to produce a valid ciphertext encState) in addition to the login token to be able to log the user in a different database (and then trick him to log an email to get it)

Login token is useful to temporary replace the logic of "we check that the user is logged in gmail", see getUserFromGoogleToken (but encrypting the login token in db does not cost that much since everything is encrypted, it can also be hashed instead if we want)

[std-odoo]

@tde-banana-odoo Hi 🙂

When you have some time can you have a look and merge it? 🙂

It's about encrypting the database of the gmail plugin to reduce the damage in case of a database leak (sql injection, backup that leak, etc), with the email, the sub from the token we get from gmail (unique and constant per user), and the application secret stored in the env

(so an attacker will need to get the application secret, the sub for each individual users, instead of getting all Odoo tokens at once with a db leak)

I discussed the implementation with @odony , and we re-checked with him this afternoon in live 🙂

Thanks in advance