Handle m.forwarded_room_key events

[richvdh]

Counterpart to #454: when we get an m.forwarded_room_key, fish out the keys and add it to the OlmDevice for decrypting things.

[NegativeMjark]

Will the getKeysClaimed be correct here?
I suspect it should be claiming the keys of the original sender here?

[richvdh]

well now.

I suspect we actually need both keys. This comes back to a conversation the two of us had about verification:

richvdh:

Alice starts a megolm session, and sends the keys to Bob's device X. Bob hasn't verified Alice's device. Later, Bob logs in on a new device Y, and gets the keys to the megolm session from X
presumably he now needs to verify both X and Alice's device before that session is considered 'verified' at Y
to establish some sort of chain of trust

Mjark:

Yes. Y needs to trust X when X says that Alice's device W is the owner of session S. One way to do that is to verify that X is owned by the same person as Y.
Separately we then need to verify that device W is owned by Alice.

Currently I don't have anywhere to store two sets of keys; I'm accepting that verification is going to be completely broken for now (because the sender_key on the m.room.encrypted message (W) isn't going to match the claimed key on the session (X)).

[NegativeMjark]

Should we make the keysProved and keysClaimed dicts be empty until we have better idea of what the correct behaviour is here?

I'm slightly worried that the constraint on keys proved doesn't hold anymore. https://github.com/matrix-org/matrix-js-sdk/blob/v0.7.10/src/crypto/OlmDevice.js#L780-L782

[richvdh]

ok, for now I'm leaving keysClaimed unset.

keysProved is harder to do much about because it's inferred from the megolm session. In any case I think the constraint holds more-or-less as much as it ever did.

[NegativeMjark]

I'm slightly worried that the constraint on keys proved doesn't hold anymore. https://github.com/matrix-org/matrix-js-sdk/blob/v0.7.10/src/crypto/OlmDevice.js#L780-L782

keysProved is harder to do much about because it's inferred from the megolm session. In any case I think the constraint holds more-or-less as much as it ever did.

Before this PR the keysProved was set by the curve25519 senderKey taken from the olm session.
This was safe because only a valid olm session would have that curve25519 key.

This PR allows the senderKey to be taken from the content.sender_key of a "m.forwarded_room_key" message. My understanding is that the content.sender_key could be set to any value. If that was the case it would weaken the current guarantees on keysProved.

[richvdh]

How would you like me to proceed?

…

On 09/06/17 11:54, Mark Haines wrote: I'm slightly worried that the constraint on keys proved doesn't hold anymore. https://github.com/matrix-org/matrix-js-sdk/blob/v0.7.10/src/crypto/OlmDevice.js#L780-L782 keysProved is harder to do much about because it's inferred from the megolm session. In any case I think the constraint holds more-or-less as much as it ever did. Before this PR the keysProved was set by the curve25519 senderKey taken from the olm session. This was safe because only a valid olm session would have that curve25519 key. This PR allows the senderKey to be taken from the |content.sender_key| of a |"m.forwarded_room_key"| message. My understanding is that the |content.sender_key| could be set to any value. If that was the case it would weaken the current guarantees on keysProved. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#455 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABU1VLsXK_vlIX96vpH1gtl7Pei7RpItks5sCSR_gaJpZM4NxX8h>.

[NegativeMjark]

How would you like me to proceed?

I guess that depends on what you are trying to achieve with this PR?

Obviously I'm not happy for this to be deployed anywhere in it's current state.
But you don't seem to be targeting develop with the PR?
If the intention is just for this PR to be a stepping stone towards getting the key sharing working then as long as you add FIXME/TODO markers explaining the problem with keysProved/keysClaimed then I would be reasonably happy to see it land.

[richvdh]

I am trying to target develop - I just wanted to collect the various bits of the key-sharing together before landing bits of it.

So I'm asking: what is the correct solution to your concerns?

[richvdh]

(I find the keysProved/keysClaimed thing extremely hard to get my head around. We use keysProved to store the curve25519 key of the sender. Agreed that it hasn't been 'Proved' in the same sense as it was before. So is the solution to rename those fields? Or what?)

[NegativeMjark]

I think the keysClaimed/keysProved keys aren't good enough to handle this, we'll probably need to add more information to the event so that we can build a richer verification UI to make this work.

[NegativeMjark]

So without key-sharing we have:

0. The sender device ed25519 key
1. The sender device curve25519 key.
2. The sender megolm session key.

The downloaded device keys (A) prove ownership of an ed25519 key (0) and claim ownership of the curve25519 key (1).
The olm key sharing message (B) claims ownership of an ed25519 key (0) and a megolm session key (2) and proves ownership of a curve25519 key (1).
Each megolm message (C) claims ownership of a curve25519 key (1) and proves ownership of a session key (2).

      0    1    2
    A | -> |    |
    B | <- | -> |
    C |    | <- |

Taken together these prove that (0), (1) and (2) are owned by the someone who has access to all of 3 private keys.

However since the JS SDK has access to both (B) and (C) it can prove that (1) and (2) are owned by someone with access to both private keys. So the JS SDK can say that a megolm message (C) proves that (1) claims (0).

Which it does using keysProved/keysClaimed.

[NegativeMjark]

With key-sharing we have:

0. The sender device ed25519 key
1. The sender device curve25519 key
2. The sender megolm session key
3. The sharing device ed25519 key
4. The sharing device curve25519 key

The downloaded device keys (A) prove that the owner of (0) claims (1), and that owner of (3) claims (4)
The key forwarding message (B) proves that the owner of (4) claims (3).
The megolm message (C) proves that the owner of (2) claims (1).

   0    1    2    3    4
A  | -> |    |    | -> |
B  |    |    |    | <- |
C  |    | <- |    |    |

Additionally (B) tells us that the owner of (4) believes that the owner of (1) claims ownership of (0) and that the owner of (1) claims ownership of (2). We should only use that information if we trust (4) to make that sort of claim.

So I think practically we should:

• Leave "keysProved" in the event for (C) unset.
• Set "keysClaimed" in the event for (C) to (0).
• Add a new property called something like "keysProvedIfYouTrust" that tells the app to consider "keysProved" to be (1) if they trust (4).

I don't think we need to encode that (4) claims ownership of (3). It should be sufficient that (3) claims ownership of (4) for the app to translate it's trust of (3) into a trust of (4).

[richvdh]

I discussed this with Mjark at https://matrix.to/#/!HCXfdvrfksxuYnIFiJ:matrix.org/$1497020413983UJZHQ:sw1v.org.

Points were:

• Apps do not currently use the getKeysProved or getKeysClaimed APIs: I don't find them very intuitive to use. [Actually this turns out to be not quite true: EncryptedEventDialog uses event.getKeysClaimed().ed25519 but I think this argument follows anyway].
• Trying to stuff the new semantics into the keysClaimed/keysProved terminology leads to a keysProvedIfYouTrust field which is even harder to reason about
• It instead makes sense to restructure the key storage with names like originalSenderEd25519 or something.

I'm closing this for now while I sort all this out.