Panic when decrypting if use in different crates

[xuxiaocheng0201]

I wrapped muddy as a workspace crate to add some pretreatments like trim, but paniced when decrypting at runtime. File tree:

.
├── Cargo.toml
├── obscure
│   ├── Cargo.toml
│   └── src
│       └── lib.rs
├── obscure_macro
│   ├── Cargo.toml
│   └── src
│       └── lib.rs
└── src
    └── main.rs

Cargo.toml:

[package]
name = "tester"
version = "0.1.0"
edition = "2021"

[dependencies]
obscure = { path = "obscure" }

[workspace]
members = [
    "obscure",
    "obscure_macro",
]

obscure/Cargo.toml:

[package]
name = "obscure"
version = "0.1.0"
edition = "2021"

[dependencies]
muddy = "~0.2"
obscure_macro = { path = "../obscure_macro" }

obscure/src/lib.rs:

pub extern crate muddy;

muddy::muddy_init!();

pub use obscure_macro::e;

obscure_macro/Cargo.toml:

[package]
name = "obscure_macro"
version = "0.1.0"
edition = "2021"

[dependencies]
syn = "^2"
quote = "^1"

[lib]
proc-macro = true

obscure_macro/src/lib.rs:

use proc_macro::TokenStream;
use quote::quote;
use syn::{parse_macro_input, LitStr};

#[proc_macro]
pub fn e(input: TokenStream) -> TokenStream {
    let input = parse_macro_input!(input as LitStr);
    let secret = input.value();
    let secret = trim(secret);
    quote!({
        use ::obscure::muddy_internal;
        let secret = ::obscure::muddy::m!(#secret);
        debug_assert_eq!(&secret, #secret);
        secret
    }).into()
}

fn trim(secret: String) -> String {
    let mut trim = String::new();
    let mut whitespace = false;
    for ch in secret.trim().chars() {
        if ch.is_ascii_whitespace() {
            whitespace = true;
            continue;
        } else {
            if whitespace {
                trim.push(' ');
                whitespace = false;
            }
            trim.push(ch);
        }
    }
    trim
}

src/main.rs

use obscure::e;

fn main() {
    let str = e!("Hello world");
    println!("{str}");
}

Output:

thread 'main' panicked at obscure/src/lib.rs:3:1:
called `Result::unwrap()` on an `Err` value: Error
stack backtrace:
   0: rust_begin_unwind
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/std/src/panicking.rs:665:5
   1: core::panicking::panic_fmt
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/panicking.rs:74:14
   2: core::result::unwrap_failed
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/result.rs:1679:5
   3: core::result::Result<T,E>::unwrap
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/result.rs:1102:23
   4: obscure::muddy_internal::decrypt
             at ./obscure/src/lib.rs:3:1
   5: tester::main
             at ./src/main.rs:4:15
   6: core::ops::function::FnOnce::call_once
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

The error is when verifying mac: code. It seems different keys and nonces will be generated when used in different crates?

[orph3usLyre]

Interesting. Thanks for opening an issue! I'll try and take a look at this over the weekend and see if I can figure this out.

[orph3usLyre]

I've poked at this over the weekend and can't say that I've made too much progress. I've confirmed that the data for both the nonce and the encrypted data is the same at build time/run time. The panic occurs from the call to cipher.decrypt(&nonce, ciphertext.as_ref()) (see https://docs.rs/chacha20poly1305/latest/chacha20poly1305/type.ChaCha20Poly1305.html), and both the embedded and the env-based obfuscation fail to decrypt. There is no associated error data since errors provided by chacha20poly1305 are purposefully opaque. My first suspicion was that it was related to the associated data of the cipher (see Payload) but I don't use this feature in the crate.

I'll have to keep digging. Thanks again for bringing this up.

[xuxiaocheng0201]

I run the code in debug mode, and found the error is returned when verifying mac (see this code) (I have told at the end of this issue).

I don't think the Payload is the key, because its implementation is very simple.

[xuxiaocheng0201]

I found the reason: The key generate twice, one is proc_macro time, and one is build time.

To reproduce, you can change this line to this:

pub(crate) static KEY: Lazy<Key> = Lazy::new(|| {
    eprintln!("Run once");
    ChaCha20Poly1305::generate_key(&mut OsRng)
});

and run cargo build. You will see Run once twice.

A solution is to split the KEY to another crate, like muddy_key. And muddy_macro crate depends the new crate to read the key.

BTW, a crate like chacha20poly1305 is written by rust-lang owners group, and it has been running stably for two years with no update. It shouldn't have such obvious errors.

[orph3usLyre]

Good catch! I'll test this out throughout the week. Thanks for the contributions :tada:

[orph3usLyre]

After some quick testing (granted, not completely in-depth), I don't believe adding a new crate solves this issue - I've tested briefly and run into the same problem. If you happen to have a POC I'd be happy to look at it.

On another note, one of my current goals for the crate is to get rid of the muddy_init!() crate root initializer since it's not very idiomatic and feels unnatural. To accomplish this I need a more intelligent implementation of both the in-place m!() macro as well as the attribute macros. Not sure that this will solve your problem but it might lead to a more ergonomic interface, which could in turn be easier to wrap by external crates.

[xuxiaocheng0201]

Oh yes, simple add a new crate is useless.

Another idea: we can use build.rs to ensure only one key can be generated. muddy_macros/build.rs:

use std::fs::File;
use std::io::Write;
use std::path::Path;

use chacha20poly1305::aead::OsRng;
use chacha20poly1305::{ChaCha20Poly1305, KeyInit};

fn main() -> std::io::Result<()> {
    let key = ChaCha20Poly1305::generate_key(&mut OsRng);
    let out_dir = std::env::var("OUT_DIR").unwrap();
    let out_dir = Path::new(&out_dir);

    let flag_path = out_dir.join("ok");
    if flag_path.is_file() {
        return Ok(()); // Key already written
    }

    let file = out_dir.join("key.rs");
    let mut file = File::options().write(true).create(true).open(&file)?;
    file.write_all(generate_rs(&key).as_bytes())?; // Write key to file

    File::create_new(flag_path)?; // Set key written flag
    Ok(())
}

fn generate_rs(key: &[u8]) -> String {
    let mut array = String::new();
    for b in key {
        array.push_str(&format!("0x{:02x}, ", b));
    }
    format!("[{array}]")
}

And replace KEY with this line:

pub(crate) static KEY: Lazy<Key> = Lazy::new(|| {
    *Key::from_slice(&include!(concat!(env!("OUT_DIR"), "/key.rs")))
});

I tested, and it solves this issus.

May be this also is a way to get rid of muddy_init!? We can generate muddy_internal module in build.rs and include them in the crate.

[orph3usLyre]

Interesting, this can definitely be one way to solve this. I wonder if we can avoid the tmp file by using the Lazy pattern to define the key and cipher (the way that we currently handle multiple static strings, for example)