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

Open
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@dbkr
Copy link
Member

dbkr commented Oct 23, 2018

@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Oct 30, 2018

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

@ara4n

This comment has been minimized.

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 added some commits Oct 31, 2018

Add definitions of the various K dirctly in option 3
rather than referring to option 2
Try & clarify mitigation against dict attacks
splits option 2 up into two sub-options
@dbkr

This comment has been minimized.

Copy link
Member

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,

This comment has been minimized.

@ara4n

ara4n Nov 1, 2018

Member

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

This comment has been minimized.

@ara4n

ara4n Nov 1, 2018

Member

s/./. /


## Security considerations

The proposal above is vulnerable to a malicious server admin performing a

This comment has been minimized.

@ara4n

ara4n Nov 1, 2018

Member

surely only option 1 is vuln to dict attack?

This comment has been minimized.

@richvdh

richvdh Jan 2, 2019

Member

and 2b?

This comment has been minimized.

@richvdh

richvdh Jan 2, 2019

Member

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

@ara4n

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

uhoreg commented Nov 2, 2018

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

@turt2live turt2live added the T-Core label Dec 17, 2018

@turt2live

This comment has been minimized.

Copy link
Member

turt2live commented Dec 17, 2018

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

This comment has been minimized.

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

This comment has been minimized.

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 Jan 2, 2019

@richvdh
Copy link
Member

richvdh left a comment

I haz comments...

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

This comment has been minimized.

@richvdh

richvdh Jan 2, 2019

Member

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

```json
{
"private_key": {
salt: "MmMsAlty",

This comment has been minimized.

@richvdh

richvdh Jan 2, 2019

Member
Suggested change Beta
salt: "MmMsAlty",
"salt": "MmMsAlty",
{
"private_key": {
salt: "MmMsAlty",
rounds: 100000

This comment has been minimized.

@richvdh

richvdh Jan 2, 2019

Member
Suggested change Beta
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`)

This comment has been minimized.

@richvdh

richvdh Jan 2, 2019

Member

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

This comment has been minimized.

@richvdh

richvdh Jan 2, 2019

Member
Suggested change Beta
* 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

This comment has been minimized.

@richvdh

richvdh Jan 2, 2019

Member
Suggested change Beta
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

This comment has been minimized.

@richvdh

richvdh Jan 2, 2019

Member

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.

This comment has been minimized.

@richvdh

richvdh Jan 2, 2019

Member

how are all these keys actually used for encryption?


## Security considerations

The proposal above is vulnerable to a malicious server admin performing a

This comment has been minimized.

@richvdh

richvdh Jan 2, 2019

Member

and 2b?


## Security considerations

The proposal above is vulnerable to a malicious server admin performing a

This comment has been minimized.

@richvdh

richvdh Jan 2, 2019

Member

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

@anoadragon453 anoadragon453 removed the T-Core label Jan 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment