fix(dht)!: add message padding for message decryption, to reduce message length leaks (fixes #4140) #4362
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stringhandler
previously approved these changes
Aug 2, 2022
…tari into ja-message-length-leaked
Co-authored-by: Stan Bondi <sdbondi@users.noreply.github.com>
Co-authored-by: Stan Bondi <sdbondi@users.noreply.github.com>
…tari into ja-message-length-leaked
Co-authored-by: Stan Bondi <sdbondi@users.noreply.github.com>
…tari into ja-message-length-leaked
sdbondi
approved these changes
Aug 3, 2022
sdbondi
changed the title
sdbondi
changed the title
Happy to hear that ! :) |
sdbondi
added a commit
to sdbondi/tari
that referenced
this pull request
Aug 4, 2022
* development: fix: wallet database encryption does not bind to field keys tari-project#4137 (tari-project#4340) fix: use SafePassword struct instead of String for passwords (tari-project#4320) feat(dan): template macro handles component state (tari-project#4380) fix(dht)!: add message padding for message decryption, to reduce message length leaks (fixes tari-project#4140) (tari-project#4362) fix(wallet): update seed words for output manager tests (tari-project#4379)
stringhandler
added a commit
that referenced
this pull request
Sep 2, 2022
Description --- [PR 4362](#4362) mitigates a metadata leak whereby encrypted messages are the same length as plaintext messages due to the use of a stream cipher. This work adds more complete length checks, such that padding can fail. It also more efficiently handles the edge case where no padding is needed. Motivation and Context --- To avoid directly leaking the length of plaintext messages after stream cipher encryption, [PR 4362](#4362) pads such messages to a multiple of a fixed base length after first prepending the original message length using a fixed encoding. However, the following cases do not appear to be handled by the padding and unpadding code: - The plaintext message length exceeds the fixed encoding length - The ciphertext message is not long enough for extraction of the fixed encoding length - The ciphertext message is not a multiple of the base length Further, in the case where the message length (after length prepending) is exactly a multiple of the base length, an entire base length of padding is unnecessarily applied. This work addresses these issues. The padding process now checks that the plaintext message does not exceed the limit enforced by the length encoding; as a result, it can now return an error that propagates to the encryption function caller. The padding algorithm has been simplified and now handles the multiple-of-the-base-length edge case by correctly applying no padding. The unpadding process now checks that it can safely extract the message length, and checks that the ciphertext message is a multiple of the base length. How Has This Been Tested? --- No test has been added for the case where the message length exceeds the limit allowed by the encoding, as this would imply very high memory usage (or swapping) exceeding 4 GB. Existing tests pass. A new test exercises the other failure modes. * Updates to message padding Adds better length checks. Simplifies the padding algorithm and handles an edge case hitting a base length multiple. * Add test * Propagate padding errors * Rename parameter for clarity * Better overflow and error handling * Formatting Co-authored-by: stringhandler <mikethetike@tari.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
--- Message length is leaked while performing ChaCha20 encryption for message outbound/inbound. In order to mitigate this vulnerability, we use padding to every message before encryption. In this way, message length is always a multiple of a base length value.
Motivation and Context
--- Tackle issue #4140, see here.
How Has This Been Tested?
--- Add unit tests