Add dummy constants for keypair and publickey? #1213

We would like to enable some for of secure erasure downstream. I'm not able to do this but I'd at least like to provide a "non-secure erase" and then people can put compiler fences or whatever around that and vet their own assembler or whatever they think will be sufficient.

However, in rust-secp our types have invariants to maintain -- in particular, secret keys cannot be 0 or out-of-range, public keys must be on the curve, and keypairs must have consistent secret and public keys. Violating these will potentially trigger ARG_CHECKs. So users can't simply memset stuff to 0 and call it a day (or rather, if they do this, they need unsafe code and then need to be very careful not to use the zeroed-out object again).

OTOH, it's hard in rust-secp for us to provide "dummy" values of these types, because they're opaque types and the C library reserves the right to change them out from under us without warning (and already, they're inconsistent between machines of different endianness).

So I'd like upstream libsecp to provide these "dummy" values. Specifically:

• a secp256k1_pubkey corresponding to a secret key whose value is 1 (or 0xDEADBEEF, or whatever)
• a secp256k1_xonly_pubkey with the same story
• a secp256k1_keypair

I don't care whether these have the same secret key, or whether they commit to having any particular secret key, or being consistent across versions, or whatever. They just have to be valid and uncorrelated with any actual secret data.

Opening an issue first rather than PR'ing so we can discuss the merits of this.

cc https://github.com/rust-bitcoin/rust-secp256k1/pull/582

I think only keypair is required? Though others could be useful for something. :man_shrugging:

So in principle, you could just call secp256k1_ec_pubkey_create with a secret key of 1, but that's inconvenient because that incurs a run time overhead and/or non-static initialization? Do I get this right?

@real-or-random Yes, we can't do it statically, and if we do it on every invocation then it'd incur a massive (~10000x) cost to erasure.

Having said this, I talked to @jonasnick offline and he suggested that downstream we just use all-bits-zero as the "dummy" value and then add a flag to our internal types to make sure we never pass that into the C API. This would also benefit the user in that signing functionality wouldn't appear to succeed (since we'd check the flag and return an error) when called with zeroed-out values.

@apoelstra that'd make the API quite inconvenient just to support 1% of users. Perhaps panicking would be OK though.

I also don't like making the size of the type not power of two, I guess it could inhibit some optimizations or even cause a bunch of padding if ffi::KeyPair becomes aligned. (I'm surprised it already isn't, is it even correct?)

I would normally suggest lazy initialization but it's awkward without a no_std blocking facility.

@Kixunil I don't understand why it would affect the API at all.

And KeyPair is already 96 bytes, it's nowhere near a power of two. If we have to add another 8 bytes even to stick a flag in there, it's fine.

Returning Result<Signature, Error> vs Signature.

Well 96 == 32 + 64, which means if the alignment becomes anything between 2 bytes and 32 bytes it'll be a waste. I've already seen optimizations in some code where there was conversion to u32 before computation so just making alignment larger and skipping conversion doesn't seem unreasonable.

Violating these will potentially trigger ARG_CHECKs

Indeed. If panicking is okay, then you can really just set to all zeroes, and rely on the ARG_CHECKs (which will panic IIRC)?

So users can't simply memset stuff to 0 and call it a day (or rather, if they do this, they need unsafe code and then need to be very careful not to use the zeroed-out object again).

Couldn't you provide something like a destructor that takes ownership and memsets to 0? If it takes ownership of the value, then the user can't use the value any more?

@real-or-random because if it takes ownership then it'll copy the memory :P. Any zeroing functionality needs to work on a reference.

Indeed. If panicking is okay, then you can really just set to all zeroes, and rely on the ARG_CHECKs (which will panic IIRC)?

They do panic, but this is technically UB (and even in compiler versions where it wasn't UB, it was defined to abort the process in a way that users could not catch in any way). See https://github.com/rust-bitcoin/rust-secp256k1/issues/354 and linked issues/pulls. So we really don't want to depend on hitting an ARG_CHECK except in cases where our bindings are buggy. @Kixunil ah! I am surprised to see that our signing functions don't return a Result. Agreed, let's not change those APIs. Better to panic (explicitly, not via ARG_CHECK :)).

And I'd be fine with not adding a flag, just memcmp'ing the data against the all-zeros. We can avoid checking the full 96 bytes. The first 32 are enough.

@real-or-random because if it takes ownership then it'll copy the memory :P. Any zeroing functionality needs to work on a reference.

Oh yes, I remember that discussion...

They do panic, but this is technically UB [...]

Oh, and I remember that one even better! :/

@Kixunil ah! I am surprised to see that our signing functions don't return a Result. Agreed, let's not change those APIs. Better to panic (explicitly, not via ARG_CHECK :)).

Yeah, that seems much better.

Should this issue here be closed then?

Yes, I don't think we need anything from upstream here.

I am surprised to see that our signing functions don't return a Result

Why? Signing cannot fail.

just memcmp'ing the data against the all-zeros.

I think we ought to do it in constant time, right?

Anyway, if we're going to zero-out, we need a guarantee that all-zeroed values will never be valid. Can we have that?

Anyway, if we're going to zero-out, we need a guarantee that all-zeroed values will never be valid. Can we have that?

Why do you need this if you're going to check for zeros? Or are you saying you need this to make sure your API is complete, i.e., every value that can be used with our API, can be used with your API too?

Then you'd need it the other way around. If an object is valid (what does this mean)?, then it will never have all zero bytes?

Yeah, if someone passes some otherwise valid key (one that'd parse fine) it'd be terrible to panic just because the same value is used as a sentinel.

The following holds:

If no callback is raised when a secp256k1_pubkey, secp256k1_only_pubkey or secp256k1_keypair object is passed as an argument to an API function, then the first 32 bytes of the object are not all zeros.

Reason:

• The first 32 bytes of secp256k1_pubkey and secp256k1_only_pubkey are the x-coordinate. We call the illegal callback while loading if that's 0.
• The first 32 bytes of secp256k1_keypair are the secret key. We call the illegal callback while loading if that's 0.

The question is more about is this expected to stay such forever?

Yes, I can't imagine that we will change that. We could add a promise to the docs, but that seems a bit arbitrary, to be honest. I'd rather suggest you add a test to secp256k1-sys that checks that if the 32 bytes are all zeros, then a callback is raised. Then you'll notice the (very unlikely) case that we break this guarantee in the future.

The question is more about is this expected to stay such forever?

Given that 0 is not a valid secret key, and there isn't even a well-defined serialization of the corresponding public key, and it's hard to imagine "all bits 0s" ever representing anything else, then yes, I'd say it's "expected".

I'd be happy to add a test to our rust bindings that confirms this, if it makes you more comfortable.

Sounds good then! The test would be nice but I guess we don't need to rush with it.

it's hard to imagine "all bits 0s" ever representing anything else

In MuSig, 33 bytes of zero represent infinity, at least in the case of the aggregate nonce. We needed a serialization for this because the attacker can force it to be infinity, and we don't to abort in this case. We also have an opaque struct for this in the -zkp implementation, but it comes with magic bytes, see https://github.com/BlockstreamResearch/secp256k1-zkp/blob/4f57024d868c2dc59b737e6a02b8990abffd3165/src/modules/musig/session_impl.h#L90. And see also https://github.com/BlockstreamResearch/secp256k1-zkp/pull/218, which proposes something similar. But there's no struct in this case.

edit: It seems reasonable to always add dummy bytes to new public opaque types. We also have it here for the scratch space already. We just can't introduce it retroactively for the existing types because this would be a breaking change. (The size of types is guaranteed.)