fix: missing_debug_implementations

[JonathanWoollett-Light]

Sets missing_debug_implementations to warn and adds missing std::fmt::Debug implementations.

When tracing/logging/debugging a program it is common to debug print inputs and outputs of function, to do this generally dependencies need to offer std::fmt::Debug implementations.

I won't repeat what is written under missing_debug_implementations which gives some more explanation.

[newpavlov]

With cryptographic algorithms we should be careful about unintentionally leaking sensitive information. You should not derive Debug for things like CtrNonce*, instead you should write a manual "opaque" implementation like here. Same for your similar PRs in other repositories.

[JonathanWoollett-Light]

With cryptographic algorithms we should be careful about unintentionally leaking sensitive information. You should not derive Debug for things like CtrNonce*, instead you should write a manual "opaque" implementation like here. Same for your similar PRs in other repositories.

I do not think these security concerns are valid.

You cannot prevent a user of the library simply doing println!("{}",transmute::<_,&[u8]>(CtrNonce*)) which accomplishes the same thing.

I do not think there are any reasonable security concerns that should lead to restricting implementation of std::fmt::Debug, by not implementing it, it requires users of the library to manually implement traits and their own workarounds.

[tarcieri]

@JonathanWoollett-Light the important part is to prevent users from accidentally leaking sensitive details like a cipher's internal state.

Debug will often recurse through data structures (especially when derived), and perhaps in an attempt to debug some irrelevant field, users will accidentally log a private key or a cipher's internal state.

I think it's great to have this lint in place and Debug impls for everything, but like @newpavlov said these should be carefully written to ensure they don't accidentally leak secrets.

[JonathanWoollett-Light]

@tarcieri My understanding is that the intended usage of std::fmt::Debug is for giving an extensive overview of the data in the same way you might get from stepping through the program with a debugger, to show the raw data (in the same way a debugger may show all the data even that which is sensitive).

the important part is to prevent users from accidentally leaking sensitive details like a cipher's internal state.

Debug will often recurse through data structures (especially when derived), and perhaps in an attempt to debug some irrelevant field, users will accidentally log a private key or a cipher's internal state.

In the cases where the respective struct member is not public, deriving Debug implementations won't print these values. In other cases I do not think this can be reasonably accomplished, I think this can only be the responsibility of the user.

If to debug an issue a developer wants to view the all the data which may affect this or possibly suspects the issues involves some specific data they would need to write an awkward workaround.

I think it's great to have this lint in place and Debug impls for everything, but like @newpavlov said these should be carefully written to ensure they don't accidentally leak secrets.

I would consider an example like:

#[derive(Debug)]
struct User {
    username: String,
    password: String
}
fn register(user: User) {
    log::trace!("user: {user}");
    // ...
}

The logging here may log the user password to a file (or otherwise store it) which is not secure.

A solution with a custom std::fmt::Debug like suggested may solve this like:

struct User {
    username: String,
    password: String
}
impl std::fmt::Debug for User {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        f.debug_struct("User ")
         .field("username", &self.username)
         .finish()
    }
}
fn register(user: User) {
    log::trace!("user: {user}");
    // ...
}

But this does not seem reasonable to me, the flaw is with log::info!("user: {user}"); itself. Logs are often specifically implemented to help trace and debug the execution, it is possible their may be a bug with the password handling that we obfuscate with custom debug implementations.

If we implement std::fmt::Debug with custom implementations this will be a continuous area of debate and concern, this may cause people problems with debugging, if we implement std::fmt::Debug with the default implementation I would not expect there will be any debate or issues created.

I think here the default implementation is idiomatic and reasonable. I think it is reasonable that if someone prints a data-structure containing sensitive data, the sensitive data is printed. And vice-versa it would not be reasonable to print a data-structure containing sensitive data and expect this to not be printed. I think the default implementation is expected and simple.

[tarcieri]

If we implement std::fmt::Debug with custom implementations...

We already endeavor throughout the project to avoid accidentally logging secrets via custom Debug impls. To the extent there are gaps that's an oversight, if anything.

In my time as an infosec professional for a PCI-DSS Level 1 environment, accidental logging of secrets through debugging was the largest vector of secret exfiltration by far. In fact, trying to armor applications against accidentally logging secrets this way was one of the main things I worked on at my time there.

I think it is reasonable that if someone prints a data-structure containing sensitive data, the sensitive data is printed.

Secrets can get accidentally logged to exception reporting systems this way, even if no explicit attempt was made to log debugging information, simply via things like panic messages. There may be absolutely no intent to print anything, and unless secrets are guarded from Debug impls, they can wind up in 3rd party databases of crash/exception reporting services.

Beyond mere accidental secret logging, an attacker can also potentially get an application to log secrets to a crash reporting service this way.

[JonathanWoollett-Light]

even if no explicit attempt was made to log debugging information, simply via things like panic messages

I think this is an exceptional case, I can only see this occurring when doing an assertion with assert! and I'm not sure in what circumstance you would be asserting on sensitive information.

panic! for instance requires explicitly including all the printed information.

[tarcieri]

Again, a panic message can include an attempt to log a seemingly innocuous value, only for it to contain sensitive cryptographic secrets.

If someone really wants to log a sensitive cryptographic secret, our philosophy is that should come with clear intent, and not be something you can accidentally do via traversing a much larger data structure which is mostly non-sensitive.

[JonathanWoollett-Light]

Updated to use manual Debug implementations for CtrNonce128, CtrNonce64 and CtrNonce32.

[JonathanWoollett-Light]

@newpavlov Could you please re-review.

[newpavlov]

Thank you!