Make comparison functions stable

[tcharding]

Supersedes #515

The primary aim of this PR is to fix the fact that we currently implement various comparison traits (Ord, PartialEq) by comparing the inner byte arrays. These bytes are meant to be opaque and are not guaranteed across versions of libsecp256k1.

This PR is a bit involved because I had to detangle all the various types (across both secp256k1 and secp256k1-sys) that use the impl_array_newtype macro.

• Patch 1: is trivial cleanup
• Patch 2: Step one of the PR is duplicating the macro into both crates so we can tease them apart.
• Patch 3-5: Are cleanup of the now duplicated impl_array_newtype macros
• Patch 6: Is the meat and potatoes
• Patch 7,8: Further minor clean ups to the macro

I had a lot of fun with this PR, I hope you enjoy it too.

Fix: #463

[tcharding]

Will fix Rust 1.41.1 problems tomorrow.

[apoelstra]

Needs rebase.

[tcharding]

Rebased and fixed Rust 1.41.1 stuff which meant removing derives and implementing them, when fuzzing, in impl_array_newtype. Also, please note that I had to patch recovery.rs even after you asked me not to touch that module because of the changes to the macro.

[apoelstra]

As an semi-related thing, all this

let mut Thing(ref thing) = x`

crap is because I didn't know about the .0 notation when I wrote these macros, in 2014. We could probably simplify a lot of them.

[apoelstra]

I don't think we should implement Copy on secret key types (SecretKey and KeyPair). Partly to encourage users to pass by reference, minimizing the number of copies of these objects that rustc makes in memory, and partly because eventually we probably want to implement Drop in a way that zeroizes them. If we impl Drop we'll have to get rid of Copy and then that'll be a breaking change.

The reason we haven't done any of this yet is that it's impossible to get right; see this blog post by benma where he eventually starts boxing secret data, which we can't do without requiring alloc :/. But maybe we should just bite the bullet and do something even if it's an incomplete solution.

[apoelstra]

Done reviewing 9850550. This looks great! The only thing blocking an ACK is the secret key Copy thing.

[tcharding]

Thanks for review man, will respin without Copy.

[tcharding]

Quick Rust question please, while implementing the suggestion to remove Copy the compiler pushed me to add a bunch of calls to clone, this made be realise that I do not know how many copies of a secret will exist in the following code during the call to baz? If the clone inside the call to bar creates a second copy on the stack (which is my guess as to what will happen) then this is no better than implementing Copy and a second secret being on the stack as part of the function call to bar - its kind of worse because the stack frame of bar would be overwritten sooner than the stack frame of the calling function (ignoring the fact that that is main here).

#[derive(Clone, Debug)]
struct Secret([u8; 32]);

impl Secret {
    pub fn foo(self) -> u8 {
        0                       // Some meaningful use of secret.
    }

    pub fn baz(&self) {
        // Some function that borrows self.
    }
}

pub fn bar(x: u8) {
    println!("got {}", x);
}

fn main() {
    let s = Secret([0_u8; 32]);
    bar(s.clone().foo());

    s.baz()
}

I guess the main takeaway here is that if I implement changes designed around secrets then its going to be guess work at best without me putting in a decent amount of time and thought.

[apoelstra]

@tcharding are we taking SecretKey by value in a lot of places? Even prior to this PR? How did we get by without Copy?

[apoelstra]

Oh, I see, we already have Copy on the secret types through impl_array_newtype. Jeez, that macro was hiding some pretty dangerous stuff.

In that case I don't think the Copy thing has anything to do with this PR. We can revisit it in a later iteration.

[apoelstra]

ACK 9850550

[tcharding]

Jeez, that macro was hiding some pretty dangerous stuff.

For real. I'm keen to work on the secret stuff at some stage but I get the feeling that it will require a bunch of time spent reading assembler to make sure we know exactly what is going on where.

[Kixunil]

will require a bunch of time spent reading assembler

Don't get your hopes high. Neither Rust nor C compiler makes any guarantees about the resulting instructions that would help hide the data. You may see good assembly code on your machine but on a different one the optimizer arranges the code differently and you're screwed.

However, the only way to exfiltrate secrets (apart from logic bugs zeroing won't protect you from) is to exploit UB. Rust prevents almost all UB and I think we would better spend our time at minimizing UB in unsafe code instead.

boxing secret data, which we can't do without requiring alloc

The closest you could get would be to Pin the key but I bet lot of people would become annoyed by such API and probably fork the crate. And I guess I would be the first one to do it. :)

[apoelstra]

Don't worry, I've tried pinning and I couldn't even deal with the ergonomic hit within this crate ;)