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

Add support for new dehydrated device format #199

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

uhoreg
Copy link
Member

@uhoreg uhoreg commented Nov 27, 2024

as documented in the current version of MSC3814

Fixes #196

@uhoreg uhoreg force-pushed the dehydration_format branch from 4fbc6c6 to d867b21 Compare November 30, 2024 01:03
@uhoreg uhoreg force-pushed the dehydration_format branch from 97c798f to fbf8e39 Compare December 4, 2024 02:56
@uhoreg uhoreg marked this pull request as ready for review December 4, 2024 03:00
@uhoreg uhoreg requested review from dkasak and poljar as code owners December 4, 2024 03:00
@poljar
Copy link
Collaborator

poljar commented Dec 5, 2024

Gonna close and reopen this to see if the codecov thing kicks in.

@poljar poljar closed this Dec 5, 2024
@poljar poljar reopened this Dec 5, 2024
@codecov-commenter
Copy link

codecov-commenter commented Dec 5, 2024

Codecov Report

Attention: Patch coverage is 89.65517% with 9 lines in your changes missing coverage. Please review.

Project coverage is 90.24%. Comparing base (5eb25a6) to head (e22dfcb).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/lib.rs 33.33% 6 Missing ⚠️
src/olm/account/mod.rs 97.01% 2 Missing ⚠️
src/types/ed25519.rs 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #199      +/-   ##
==========================================
- Coverage   90.28%   90.24%   -0.05%     
==========================================
  Files          34       34              
  Lines        1905     1989      +84     
==========================================
+ Hits         1720     1795      +75     
- Misses        185      194       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

so that if new keys get generated, the existing keys don't get overwritten
@uhoreg
Copy link
Member Author

uhoreg commented Dec 17, 2024

@poljar can I get a review on this PR? Thanks

Copy link
Collaborator

@poljar poljar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, sorry for the late reply, we miscommunicated on who would do the first review.

A good start but needs a bit more polish.

@@ -22,6 +21,8 @@ use base64::{
engine::{general_purpose, GeneralPurpose},
Engine,
};
#[cfg(not(feature = "libolm-compat"))]
pub(crate) use libolm_compat::{pickle_libolm, unpickle_libolm};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just import this unconditionally and remove the imports from the line bellow.

src/olm/account/mod.rs Outdated Show resolved Hide resolved
src/olm/account/mod.rs Show resolved Hide resolved
@@ -439,6 +439,45 @@ impl Account {

pickle.try_into()
}

/// Create a dehydrated device pickle.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty terse, perhaps we should explain a bit what a dehydrated device is and add an example how this can be used.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also has the IV reuse problem, right? Mentioned here and in the MSC. I wonder if we should fix this properly now and just include a random IV or Nonce in the pickle.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, I had considered fixing the IV problem, but also wasn't sure if I wanted to introduce a new encryption scheme into the code. I'll think about it some more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We now have a dep on Chacha20 in vodozemac due to the QR Login work, and it's likely going to be needed for the PQ support.

So using a new scheme isn't as problematic anymore.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added more to the comment, and switch encryption to using Chacha20poly1305

src/olm/account/mod.rs Outdated Show resolved Hide resolved
src/olm/account/mod.rs Outdated Show resolved Hide resolved
src/olm/account/mod.rs Outdated Show resolved Hide resolved
Comment on lines 801 to 807
pub(super) struct Pickle {
version: u32,
private_curve25519_key: Box<[u8; 32]>,
private_ed25519_key: Box<[u8; 64]>,
one_time_keys: Vec<OneTimeKey>,
opt_fallback_key: OptFallbackKey,
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this format will come from the homeserver, we should add a fuzz target for it and fuzz it for a bit to ensure that we're not introducing a vulnerability. We already have a fuzz target for all the other formats we receive over the network and have to decode.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to respond to this earlier.

I've written the basic code:

use afl::fuzz;
use arbitrary::Arbitrary;
use vodozemac::olm::Account;

#[derive(Arbitrary)]
struct Data {
    ciphertext: String,
    nonce: String,
    key: [u8; 32],
}

fn main() {
    fuzz!(|data: Data| {
        let _ = Account::from_dehydrated_device(&data.ciphertext, &data.nonce, &data.key);
    });
}

It looks like the afl documentation is suggesting to provide some example input? Or can I just let it provide random input?

To use it, do I just run cargo afl fuzz? When I do that, I get a permission error -- which may be because my home directory is mounted noexec, so I need to figure out how to get it to run. But I was wondering if there was something else I need to add to the command line.

src/olm/account/mod.rs Outdated Show resolved Hide resolved
}
}

impl From<chacha20poly1305::aead::Error> for DehydratedDeviceError {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried using Decryption(#[from] chacha20poly1305::aead::Error), but it was complaining about some missing traits. So I had to do this instead. I'm not sure if there's a better way.

pub fn to_dehydrated_device(
&self,
key: &[u8; 32],
) -> Result<(String, String), crate::DehydratedDeviceError> {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I create a new struct instead of returning (String, String)?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that sounds good, and it might implement Serialize

@uhoreg uhoreg requested a review from poljar January 6, 2025 14:05
@uhoreg
Copy link
Member Author

uhoreg commented Jan 9, 2025

The clippy failure seems to be in code that I did not change. I'm guessing that I should create a new PR to fix that, rather than adding the fixes to this PR.

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 this pull request may close these issues.

Dehydrated Device: Serialize and deserialize new format
3 participants