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

KeySharing, DevicebasedStrategy | Exclude insecure dehydrated devices when sending messages #4313

Open
uhoreg opened this issue Nov 22, 2024 · 11 comments · May be fixed by #4551
Open

KeySharing, DevicebasedStrategy | Exclude insecure dehydrated devices when sending messages #4313

uhoreg opened this issue Nov 22, 2024 · 11 comments · May be fixed by #4551
Assignees

Comments

@uhoreg
Copy link
Member

uhoreg commented Nov 22, 2024

Don't encrypt to devices marked as dehydrated, if they are not cross-signed by the pinned/verified identity.

Also, drop all incoming to-device messages from devices marked as dehydrated. Factored out to #4466

@andybalaam
Copy link
Member

(Rust) We already have code to avoid encrypting for unsigned devices - we should do this unconditionally for any device with the dehydrated flag set.

@richvdh richvdh self-assigned this Jan 6, 2025
@richvdh
Copy link
Member

richvdh commented Jan 6, 2025

@uhoreg I don't see any mention of this behaviour in MSC3814 -- presumably it is something that should be called out there?

@richvdh
Copy link
Member

richvdh commented Jan 6, 2025

Also: why are dehydrated devices being singled out for this special treatment?

@richvdh richvdh changed the title KeySharing, DevicebasedStrategy | Exclude insecure dehydrated devices KeySharing, DevicebasedStrategy | Exclude insecure dehydrated devices when sending messages Jan 6, 2025
@uhoreg
Copy link
Member Author

uhoreg commented Jan 7, 2025

@uhoreg I don't see any mention of this behaviour in MSC3814 -- presumably it is something that should be called out there?

It's buried in the middle of a paragraph: https://github.com/matrix-org/matrix-spec-proposals/pull/3814/files#diff-1e380a19a30044a5d4a387df67369612e4cdf4dc8b0bcba1853fd221f7dfeeb9R51-R53 I guess it should be called out more.

Also: why are dehydrated devices being singled out for this special treatment?

This was flagged by Denis at matrix-org/matrix-spec-proposals#3814 (comment) . The short version is: dehydrated devices are being singled out because clients may hide dehydrated devices or make them less visible, so it may be less noticeable if a dehydrated device is unsigned, compared with normal devices. Also because dehydrated devices are a new feature and our eventual goal is to drop insecure devices.

@richvdh
Copy link
Member

richvdh commented Jan 7, 2025

ah great, thanks.

@richvdh
Copy link
Member

richvdh commented Jan 15, 2025

(Rust) We already have code to avoid encrypting for unsigned devices - we should do this unconditionally for any device with the dehydrated flag set.

kinda, though that's implemented as a different CollectStrategy, which means we can't directly make use of the same logic. Also: CollectStrategy::IdentityBasedStrategy does not check whether the identity that the device is signed by is the pinned one, so if we want that behaviour, we need to do a bit more work.

richvdh added a commit that referenced this issue Jan 16, 2025
Per #4313, we should not
send outgoing messages to dehydrated devices that are not signed by the current
pinned/verified identity.
richvdh added a commit that referenced this issue Jan 16, 2025
Per #4313, we should not
send outgoing messages to dehydrated devices that are not signed by the current
pinned/verified identity.
@richvdh
Copy link
Member

richvdh commented Jan 17, 2025

Don't encrypt to devices marked as dehydrated, if they are not cross-signed by the pinned/verified identity.

The implication of this is that we should not send encrypted messages to dehydrated devices if the user has a pinning violation. I remain slightly hesitant to do this: it is inconsistent with the way we treat regular devices (we show a warning "user's identity has changed", but continue to encrypt to all their devices. Per element-hq/element-meta#2492 (comment), the idea is that if you care about who you are encrypting to, you need to verify.)

@uhoreg / @dkasak / @mxandreas : would appreciate any further thoughts on this.

richvdh added a commit that referenced this issue Jan 17, 2025
Per #4313, we should not
send outgoing messages to dehydrated devices that are not signed by the current
pinned/verified identity.
richvdh added a commit that referenced this issue Jan 17, 2025
Per #4313, we should not
send outgoing messages to dehydrated devices that are not signed by the current
pinned/verified identity.
@uhoreg
Copy link
Member Author

uhoreg commented Jan 20, 2025

I think that since one of the goals for this in @dkasak's original comment was to use the new invisible crypto behaviour for dehydrated devices, we should be checking if the device is signed by the pinned identity, rather than just the current identity.

@dkasak do you have any further input?

@mxandreas
Copy link

mxandreas commented Jan 20, 2025

we should be checking if the device is signed by the pinned identity, rather than just the current identity

Which is already the case for "normal" devices, right? E.g. if I reset my identity on device A, the device B will receive no messages/keys until I have re/verified B.

@richvdh
Copy link
Member

richvdh commented Jan 20, 2025

we should be checking if the device is signed by the pinned identity, rather than just the current identity

Which is already the case for "normal" devices, right? E.g. if I reset my identity on device A, the device B will receive no messages/keys until I have re/verified B.

Yes, but that's not quite the point.

Here, Alice and Bob share a room. Bob resets his identity and verifies his devices with that new identity. Alice sees that Bob's identity has changed - a "pin violation". We show a banner above the composer warning her of this and allowing her to confirm. The question is what happens if Alice sends a message before clicking "OK" in that identity-change warning banner.

  • Should messages be sent to regular devices signed only by the new identity? Conclusion on this, to date, has been "yes": if Alice needs a higher paranoia level, she should verify Bob.
  • Should messages be sent to dehydrated devices signed only by the new identity? According to this issue, the answer is "no".

There are reasonable arguments for having a different behaviour for dehydrated devices, but I'm just not convinced that we've thought it through properly. For example:

I think that since one of the goals for this in @dkasak's original comment was to use the new invisible crypto behaviour for dehydrated devices,

If that is the goal, then, per the above, we are doing the wrong thing here.

As things stand, I've implemented the behaviour specified in this issue.

@dkasak
Copy link
Member

dkasak commented Jan 20, 2025

Revisiting this, I can't think of a great argument of why we should special-case dehydrated devices to require signatures by the pinned identity. The sender is still in control, and as long as we should them a timely banner/warning, they still have time to react by not sending the message.

Mind you, it still feels somewhat iffy, especially because identity rotation should be relatively rare, so I think we could afford to force the user to be a bit more careful. But I'm unable to pinpoint a particular failure mode specific to dehydrated devices.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants