-
-
Notifications
You must be signed in to change notification settings - Fork 582
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
Key verification #818
Key verification #818
Conversation
- add the start message contents to the hash commitment to prevent bid-down attacks - add the user/device IDs to the info parameter when generating the SAS and MAC
- calculate hex properly - rename SASReceive to SASRespond - use the right method name in the start message - add some done checks - don't automatically finish after checking keys in base class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Phew, this is complicated! Looking plausible though.
} | ||
const now = Date.now(); | ||
if (now < content.timestamp - (5 * 60 * 1000) | ||
|| now > content.timestamp + (10 * 60 * 1000)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use the age
on the event rather than relying on client clocks being accurate to within 5 mins?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly. This would be a spec question. I'll leave it as-is for now, and we can discuss it in the spec.
|
||
Crypto.prototype._onKeyVerificationMessage = function(event) { | ||
const sender = event.getSender(); | ||
const transactionId = event.getContent().transaction_id; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should probably null-check here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function only gets called if event.getContent().transaction_id is truthy, so I think it should be OK.
key_agreement_protocols: ["curve25519"], | ||
hashes: ["sha256"], | ||
message_authentication_codes: ["hmac-sha256"], | ||
short_authentication_string: ["hex"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment belong on the spec, but these feel a bit redundant and implied by method == m.sas.v1
.
Otherwise there was no way to tell when the other side cancelled before you accepted since you didn't yet have the promise.
@dbkr it looks like some key backup changes got mixed in when I pulled in your branch. Are they safe to merge in? If so, go ahead and hit the merge button. |
Oh I see, yes, it looks like this has picked up changes from experimental. I think the fix here is to merge experimental into this branch so github fixes its diff (did you merge in the branch which was at a later point than where you forked off? Probably could have avoided it in this case by cherry picking just |
merging the latest experimental in seems to have fixed it, so I'm going to merge this in now... |
Reference implementations: * https://gitlab.matrix.org/matrix-org/olm/commit/94f664e7256215f33639dbbad6aaf87ada082a9f * matrix-org/matrix-react-sdk#2461 * matrix-org/matrix-js-sdk#818 * matrix-org/matrix-react-sdk#2596 * matrix-org/matrix-js-sdk#837 Proposals: * [MSC1717](#1717) * [MSC1267](#1267) No alterations to either proposal have been made intentionally here.
Implements short-authentication-string verification and some QR-code verification, though bi-directional QR-code verification isn't complete. SAS verification requires https://github.com/matrix-org/olm-backup/pull/80