New issue
Advanced search Search tips

Issue 832398 link

Starred by 2 users
Status: Untriaged
Owner: ----
Cc:
apronin@chromium.org
mnissler@chromium.org
emaxx@chromium.org
pmalani@chromium.org
hashimoto@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

cryptohome: simplify marking vault keysets as LECredential

Project Member Reported by apronin@chromium.org, Apr 13 2018 
Currently, VaultKeyset protobufs contain two potential indications that the keyset is for LECredential:
1) LE_CREDENTIAL in flags;
2) key_data.policy.low_entropy_credential = true

Both are available as plaintext fields. Only the (1) is guaranteed to be set in the keyset file on disk, though (2) is set in most cases (is it possible it is always set?).

When loading the keyset from file, we check (1) and set (2) accordingly. Only (2) is exposed through cryptohome API to callers.

This seems to be over-complicated and error-prone.

Comment 1 by igorcov@chromium.org, Jul 13 

Unfortunately 2 is not always set. Before crypto::DecryptVaultKeyset being called the serialized is set to low entropy credential, but after decryption, the output from vault_keyset->IsLECredential() is still false. This happens at PIN login (in Mount::DecryptVaultKeyset).

That's because serialized inside the vault keyset is set only in Load function. And it's not loaded in that flow. The vault keyset is created and filled with data, while serialized_ is unaffected.

I don't know what would be a good solution to this. We have this data in serialized (the data from disk) when we do the decryption and have it in the vault keyset (in memory) when we need to do encryption.

Comment 2 by emaxx@chromium.org, Jul 13

Cc: hashimoto@chromium.org emaxx@chromium.org
Re comment 1: Just to note, I'm suspecting that my refactoring CL http://crrev.com/c/1098255 (which was originally aimed at bug 832395) could fix this issue, by making Mount::DecryptVaultKeyset() reuse HomeDirs::GetValidKeyset() and transitively VaultKeyset::Load()+VaultKeyset::Decrypt() instead of its own logic of loading keysets.
Admittedly, my CL turned out to be quite non-trivial and got stuck in reviewing, but if it addresses the problem discovered above then maybe we should attempt at landing it?

Comment 3 by apronin@chromium.org, Jul 13 

Re #1: in the current codebase, do we rely somewhere on checking key_data.policy.low_entropy_credential through vault_keyset->IsLECredential() after Mount::DecryptVaultKeyset()? Inside Mount::DecryptVaultKeyset we seem to always check for LE_CREDENTIAL in flags, which is fine. 
Just trying to understand if we need an urgent fix or can wait for the refactoring.

Comment 4 by apronin@chromium.org, Oct 12

Components: OS>Systems>Security

Comment 5 by louiscollard@chromium.org, Dec 10 

Maksim - looks like your change landed in the end, any updates to this bug? thanks!
►

Sign in to add a comment