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

MSC1703: encrypting recovery keys for online megolm backups #1703

Closed
wants to merge 10 commits into from

Conversation

dbkr
Copy link
Member

@dbkr dbkr commented Oct 23, 2018

@ara4n ara4n added proposal-in-review proposal A matrix spec change proposal labels Oct 29, 2018
@ara4n
Copy link
Member

ara4n commented Oct 30, 2018

hm, this would be much easier to review if it were wordwrapped

@ara4n
Copy link
Member

ara4n commented Oct 30, 2018

So, it feels there are three actual options here:

1a. Generate the recovery key from the passphrase (vuln to dictionary attacks on the server)
1b. Generate the recovery key randomly, and then encrypt it with a key derived from the passphrase (the original plan)
3. Generate a backup encryption key from random; a passphrase-derived key (which is then XORed with the backup encryption key); and a recovery key from random (which is also then XORed with the backup encryption key). I'm not clear on how these interweave together, or the benefits they bring.

What are we actually proposing in practice? I'm a bit confused as to what option 3 buys us over option 1b?

@dbkr
Copy link
Member Author

dbkr commented Nov 1, 2018

My suggestion here is to go with 1a (now just 1 in my renumbering). The summary is that I don't think there's any realistic way of mitigating the dictionary attack: option 2b. runs through a proposed mitigation and why it doesn't really work.

The kicker is that the user will get a new recovery key whenever they change their passphrase. Options 3 adds the ability to retain the same recovery key when you change your passphrase, so if we want that feature, option 1 won't suffice. It sounds like we don't though.

### Option 2b

A variant on option 2a is to regenerate K<sup>-1</sup> when the passphrase is
changed, meaning the recovery does change when the passphrase is changed,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/recovery/recovery key/

### Option 3

The backup encryption private key, K<sup>-1</sup>, and a private,
passphrase-derived key, K<sup>-1</sup><sub>p</sub> are generated as above.The
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/./. /


## Security considerations

The proposal above is vulnerable to a malicious server admin performing a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

surely only option 1 is vuln to dict attack?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and 2b?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(and possibly 3; I haven't grokked how it works yet)

@ara4n
Copy link
Member

ara4n commented Nov 1, 2018

this is much clearer now; thanks. let's just say "for now let's just generate the key from the passphrase and go with option 1" as a conclusion on the MSC and give it a go.

@uhoreg
Copy link
Member

uhoreg commented Nov 2, 2018

Yup, option 1 seems to be sufficient, and is simpler.

@turt2live
Copy link
Member

Option 1 does indeed look easiest, even with the potential risk documented later in the proposal. It's also the option that was proofed in riot-web, correct?

@ara4n
Copy link
Member

ara4n commented Dec 17, 2018

yup

@richvdh richvdh changed the title MSC1703: More detailed MSC for encrypting recovery keys for online megolm backups MSC1703: encrypting recovery keys for online megolm backups Dec 27, 2018
@richvdh
Copy link
Member

richvdh commented Dec 27, 2018

[It's now the only MSC for encrypting recovery keys for online megolm backups]

@richvdh richvdh self-requested a review January 2, 2019 07:46
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haz comments...

@@ -0,0 +1,154 @@
# Proposal for storing an encrypted recovery key on the server to aid recovery of megolm key backups
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could the file be renamed so that it matches the MSC number?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please?

```json
{
"private_key": {
salt: "MmMsAlty",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
salt: "MmMsAlty",
"salt": "MmMsAlty",

{
"private_key": {
salt: "MmMsAlty",
rounds: 100000
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
rounds: 100000
"rounds": 100000

string, b is as follows:
* Prepend the two bytes 0x8B, 0x01 to the byte string b
* Compute a parity bit by XORing all bytes of the resulting string (ie. prefix
+ `byte string`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this explodes into a bullet list in the rendered version.

also why is byte string in backticks? perhaps you just mean b, but then you probably want to use backticks each time b is used.

In all options below, the process for generating a recovery key from a byte
string, b is as follows:
* Prepend the two bytes 0x8B, 0x01 to the byte string b
* Compute a parity bit by XORing all bytes of the resulting string (ie. prefix
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Compute a parity bit by XORing all bytes of the resulting string (ie. prefix
* Compute a parity byte by XORing all bytes of the resulting string (ie. prefix

### Option 3

The backup encryption private key, K<sup>-1</sup>, and a private,
passphrase-derived key, K<sup>-1</sup><sub>p</sub> are generated as above.The
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
passphrase-derived key, K<sup>-1</sup><sub>p</sub> are generated as above.The
passphrase-derived key, K<sup>-1</sup><sub>p</sub> are generated as above. The


The backup encryption private key, K<sup>-1</sup>, and a private,
passphrase-derived key, K<sup>-1</sup><sub>p</sub> are generated as above.The
passphrase key counterpart, K<sup>-1</sup><sub>p</sub>', is also generated as
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are a bunch of formulae here which are imprecisely and inconsistently described. How about giving each calculation its own line?

the passphrase key counterpart, K-1p', is calculated as:

K-1p' = K-1 XOR K-1p

K<sup>-1</sup><sub>r</sub>' is generated by XORing K<sup>-1</sup><sub>r</sub>
with K<sup>-1</sup>. Both K<sup>-1</sup><sub>p</sub>' and
K<sup>-1</sup><sub>r</sub>' are stored in the `private_key` in the backup under
keys `passphrase_counterpart` and `recovery_key_counterpart` respectively.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how are all these keys actually used for encryption?


## Security considerations

The proposal above is vulnerable to a malicious server admin performing a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and 2b?


## Security considerations

The proposal above is vulnerable to a malicious server admin performing a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(and possibly 3; I haven't grokked how it works yet)

@cedricvanrompay
Copy link

Is there a reason why PBKDF2 is used instead of the state of the art Argon2 ?

Reference: https://password-hashing.net/

@uhoreg
Copy link
Member

uhoreg commented Apr 29, 2019

Mainly because there are more implementations of it. In particular, it's in WebCrypto.

@cedricvanrompay
Copy link

Mainly because there are more implementations of it. In particular, it's in WebCrypto.

OK. I think it would be nice to have an attribute specifying the key derivation algorithm though, to make it easier to switch to a different one.

I cannot find quantitative results on state-of-the-art PBKDF2 cracking but I keep reading that PBKDF2 is very weak against ASICs.

@turt2live turt2live added the kind:core MSC which is critical to the protocol's success label Apr 20, 2020
@uhoreg
Copy link
Member

uhoreg commented Jun 15, 2020

This proposal is obsoleted by #1946

@uhoreg uhoreg closed this Jun 15, 2020
@turt2live turt2live added obsolete A proposal which has been overtaken by other proposals and removed proposal-in-review labels Jun 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:core MSC which is critical to the protocol's success obsolete A proposal which has been overtaken by other proposals proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants