search

status-im / doubleratchet

The Double Ratchet Algorithm implementation in Go
MIT License
112 stars 14 forks source link

Change handling of skipped/deleted keys #6

Closed cammellos closed 5 years ago

cammellos commented 5 years ago

The purpose of limiting the number of skipped keys generated is to avoid a dos attack whereby an attacker would send a large N, forcing the device to compute all the keys between currentN..N .

Previously the logic for handling skipped keys was:

This is problematic as in long-lived session dropped/unreceived messages starts piling up, eventually reaching the threshold (1000 dropped/unreceived messages).

This logic has been changed to be more inline with signals spec https://signal.org/docs/specifications/doubleratchet/, and now it is:

The purpose of limiting the number of skipped keys stored is to avoid a dos attack whereby an attacker would force us to store a large number of keys, filling up our storage.

Previously the logic for handling old keys was:

This, in combination with the maxSkip implementation, capped the number of stored keys to maxSkip * maxKeep.

The logic has been changed to:

and additionally we delete any key that has a sequence number < currentSeqNum - maxKeep , as suggested by the signal specs, in order to delete old keys which have not been used.

I have only updated the non-encrypted header session as that's the one we are using, I will update the encrypted header version in a second time.

cammellos commented 5 years ago

@adambabik thanks for the review, incorporated the changes, updated!

cammellos commented 5 years ago

thanks to both for the conversation, i have learned something new!

On Wed, Oct 31, 2018, 3:19 PM Adam Babik notifications@github.com wrote:

@adambabik commented on this pull request.

In keys_storage.go https://github.com/status-im/doubleratchet/pull/6#discussion_r229712873:

// KeysStorage is an interface of an abstract in-memory or persistent keys storage. type KeysStorage interface { // Get returns a message key by the given key and message number. Get(k Key, msgNum uint) (mk Key, ok bool, err error)

// Put saves the given mk under the specified key and msgNum.

  • Put(k Key, msgNum uint, mk Key) error
  • Put(sessionID []byte, k Key, msgNum uint, mk Key, keySeqNum uint) error

@cammellos https://github.com/cammellos to sum up, I believe using []byte like in the original code is the best way. Sorry for the confusion.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/status-im/doubleratchet/pull/6#discussion_r229712873, or mute the thread https://github.com/notifications/unsubscribe-auth/AA-EsIjLyCWxC0HyPgtMjt4iqwx7DMqFks5uqbGCgaJpZM4X-gL4 .

cammellos commented 5 years ago

@adambabik @divan I think I have addressed all the feedback, could you review again please? Thanks!