Thanks to the NIP-44 security audit, I noticed that the golang secp256k1 package does not make sure if the raw bytes for the keys you provided are indeed on the curve, invalid or weak in any other sense. Makes sense since it's supposed to be a low-level method:
This function primarily exists to provide a mechanism for converting serialized private keys that are already known to be good.
That was interesting to learn since I didn't know about twist attacks before. Also interesting that now I see more people talking about twist attacks suddenly. Or it's just the Baader–Meinhof Phenomenon:
Suppose that you decide to buy a car, and you have set your mind on a specific blue model. In the next few days, you see that blue color wherever you go. It feels like suddenly, everyone is driving a car in that color.
func GenerateConversationKey(sendPrivkey *secp256k1.PrivateKey, recvPubkey *secp256k1.PublicKey) []byte {
// TODO: Make sure keys are not invalid or weak since the secp256k1 package does not.
// See documentation of secp256k1.PrivKeyFromBytes:
// ================================================================================
// | WARNING: This means passing a slice with more than 32 bytes is truncated and |
// | that truncated value is reduced modulo N. Further, 0 is not a valid private |
// | key. It is up to the caller to provide a value in the appropriate range of |
// | [1, N-1]. Failure to do so will either result in an invalid private key or |
// | potentially weak private keys that have bias that could be exploited. |
// ================================================================================
// -- https://pkg.go.dev/github.com/decred/dcrd/dcrec/secp256k1/v4#PrivKeyFromBytes
Let's see how long it takes me to actually fix it 😅
update: Or this is indeed related to the NIP-44 security audit that more people are talking about twist attacks now? The Github link in this nostr note is the same Github link that cure53 used in their audit.
Btw, I think this is a little pedantic:
But, on the other hand, calling it a danger when using secp256k1 for encryption seems a bit wrong when the danger is specifically that you didn't use secp256k1!
Yes, you could say they aren't using the secp256k1 with this reasoning but they might be using implementations of secp256k1 which are vulnerable to twist attacks. So if I use a secpk256k1 library, am I not using secp256k1 just because the library has a vulnerability?
I'm not sure if I'm misunderstanding something, but when you talk about GenerateConversationKey you're talking about generating private keys? But there's no danger from an invalid curve point here, that function just needs to check that a given 32 byte random string is less than the curve order (N or whatever); the comment warning is basically telling you to do that.
Re: "twist attack": It's when "you" (the program) is receiving a public key from an external input (to do an ECDH operation), that you need to check "is this point on the curve" (or, more normally, it's compressed, so the issue doesn't even arise).
Just quickly skimming the audit, they themselves note that this error is basically irrelevant as long as you stick with compression (which everyone using secp256k1 in bitcoin has, for like, 8+ years). Still something an audit should include though, for sure.
I'm not sure if I'm misunderstanding something, but when you talk about GenerateConversationKey you're talking about generating private keys?
No, it's to generate the conversation key (shared secret) as described in NIP-44:
Calculate a conversation key
Execute ECDH (scalar multiplication) of public key B by private key A Output shared_x must be unhashed, 32-byte encoded x coordinate of the shared point
Use HKDF-extract with sha256, IKM=shared_x and salt=utf8_encode('nip44-v2')
HKDF output will be a conversation_key between two users.
It is always the same, when key roles are swapped: conv(a, B) == conv(b, A)
The audit was about NIP-44, not about how secp256k1 is used in bitcoin. This might be where your confusion comes from. nostr keypairs are also using secp256k1.
But there's no danger from an invalid curve point here, that function just needs to check that a given 32 byte random string is less than the curve order (N or whatever); the comment warning is basically telling you to do that.
The function makes sure afaict (I am no expert in ECC) that the used point is on the curve by just using a modulo operation. But this means that the point that was passed might not be on the curve.
The warning is telling me that is that it does not check, it just truncates the value. Checking would mean that it would throw an error imo:
So yes, twist attacks might not apply here since the used point will be on the curve, you are right. But it still might generate invalid or weak private keys:
It is up to the caller to provide a value in the appropriate range of [1, N-1].
Failure to do so will either result in an invalid private key or potentially weak private keys that have bias that could be exploited.
Re: "twist attack": It's when "you" (the program) is receiving a public key from an external input (to do an ECDH operation), that you need to check "is this point on the curve" (or, more normally, it's compressed, so the issue doesn't even arise).
Yes, you are right. And this can happen on nostr. The external input might be the public key of the entity you want to message.
No, it's to generate the conversation key (shared secret)
Ah! I had a feeling I was missing something, indeed :) Should have read the function :)
So the question is whether the operation in the called library is checking the input public key. Where is that function? The decred github link in the comment is (no longer?) valid).
The warning is telling me that is that it does not check, it just truncates the value. Checking would mean that it would throw an error imo:
I think that warning is just talking about how it truncates the input private key argument, i.e. if it's > N but < 2*256. N is a few bits smaller than 2^256. So if you enter a value m s.t. N < m < 2 **256 it will truncate it by doing m mod N, which will be an extremely small number, and therefore a valid, but completely insecure private key. At least that's how I read the comment.
I am talking about this function when I mean GenerateConversationKey:
func GenerateConversationKey(sendPrivkey *secp256k1.PrivateKey, recvPubkey *secp256k1.PublicKey) []byte {
// TODO: Make sure keys are not invalid or weak since the secp256k1 package does not.
// See documentation of secp256k1.PrivKeyFromBytes:
// ================================================================================
// | WARNING: This means passing a slice with more than 32 bytes is truncated and |
// | that truncated value is reduced modulo N. Further, 0 is not a valid private |
// | key. It is up to the caller to provide a value in the appropriate range of |
// | [1, N-1]. Failure to do so will either result in an invalid private key or |
// | potentially weak private keys that have bias that could be exploited. |
// ================================================================================
// -- https://pkg.go.dev/github.com/decred/dcrd/dcrec/secp256k1/v4#PrivKeyFromBytes
shared := secp256k1.GenerateSharedSecret(sendPrivkey, recvPubkey)
return hkdf.Extract(sha256.New, shared, []byte("nip44-v2"))
}
However, since these keys are already of type secp256k1.PrivateKey and secp256k1.PublicKey (to be precise: pointers of these types), these keys could have been generated in a weak way - for example, if the caller used secp256k1.PrivKeyFromBytes which contains the warning that I copied into my GenerateSharedSecret function:
// | WARNING: This means passing a slice with more than 32 bytes is truncated and |
// | that truncated value is reduced modulo N. Further, 0 is not a valid private |
// | key. It is up to the caller to provide a value in the appropriate range of |
// | [1, N-1]. Failure to do so will either result in an invalid private key or |
// | potentially weak private keys that have bias that could be exploited. |
//
This library does not make sure yet that the secp256k1 keys you want to use for the conversation key are valid, protected against twist attacks and not contain any other weaknesses as mentioned in the NIP-44 security audit.
If you really want to use this library before this is fixed, you need to make sure that the keys you use with GenerateConversationKey are not affected yourself.
I guess it's just my lack of knowledge of something about golang? But I can't find github.com/decred/dcrd/dcrec/secp256k1/v4 , nor can I find a v4 (or v4.*) branch?
Sorry, using GenerateSharedSecret was confusing. I didn't mean the function in secpk256k1. I meant the function in my NIP-44 library and that I am currently skipping tests because I assumed that secp256k1 keys are always valid of strong. However, if they are generated usingsecp256k1.PrivKeyFromBytes they might not be valid or contain weaknesses.
See this reply from me, hopefully it unconfuses you :)
No, I don't think there was any confusion in that second point. I read the function in your library, then I went looking for secp256k1.GenerateShared Secret that you use, there.
I don't think there's any issue apart from the one you mention about how the private key is generated, i.e. what they warn about in your original comment. (and this is always a big concern to address, whether doing ECDH or anything else).
And as for pubkeys, which are the only source of a "twist attack" type concern, The secp256k1.PubKey object is almost always going to be generated from ParsePubKey right? The only other way is via a constructor, and there is no concern there anyway.
I noticed that the golang secp256k1 package does not make sure if the raw bytes for the keys you provided are indeed on the curve, invalid or weak in any other sense.
That's the ParsePubKey function. But if I am not using this function but provide my own bytes, then the secp256k1 package does not make sure. At least that's how I understand this warning for the function PrivKeyFromBytes.
WARNING: This means passing a slice with more than 32 bytes is truncated and that truncated value is reduced modulo N. Further, 0 is not a valid private key. It is up to the caller to provide a value in the appropriate range of [1, N-1]. Failure to do so will either result in an invalid private key or potentially weak private keys that have bias that could be exploited.
The privatekey -> pubkey function is “fine”. It isn’t a vector for being subjected to twist attack.
The pubkey you get back from this function will be on the curve as it’s internally munged to be. The risk is that it won’t “match” the private key that you provided.
The attack specifically requires you not to verify a pubkey you’ve been given is on the curve.
am I not using secp256k1 just because the library has a vulnerability?
Technically, yes 😂 in this case the bug is the library is not living up to its name 🤷
So the attack depends on the victim not checking if an externally given public key, is actually on secp256k1.
Uuuh? Is that a theoretical exercise to think about or something with real world occurrences? Is that even possible to create such a keypair that works on Bitcoin? Maybe I'm misunderstanding something here
Nothing in the “core” bitcoin protocol involves a Diffie-Hellman key exchange, so there is no real surface area for such an attack (at least where funds are concerned).
It would be a problem if you had funds derived from a private key that you used in a key-exchange for a messaging protocol where you’re not verifying your peer’s provided pubkey.
If you’re using a seed + HD wallet, make sure your comms keys are on a different, distinct derivation path from your funds keys.
Note that this is going to be wallet implementation details, and probably not easy to verify as a consumer.
I don't think it's only theoretical. It's an implementation error that can leak your private key given the right circumstances (that an adversary could probably engineer). What the NIP-44 security audit has to say about this might help:
NOS-01-001 WP1: Potential twist attacks in secp256k1 implementations (Info)
Cure53 has evaluated NIP44 in terms of its resilience to twist attacks. Although the current implementation was not found to be vulnerable, further steps can be suggested to make sure this state persists.
Twist attacks are an issue with naive implementations of the secp256k1 curve. They envelope problems with certain sanity checks, which can cause leakage of sender’s private key in the NIP44 protocol.
More precisely, this applies to implementations of secp256k1, which accept uncompressed representations of public keys. This means public keys are represented with both x and y coordinates instead of just x and a sign bit. As such, failure to verify whether these public keys are actually on the curve or not should be considered.
Encryption with a key, which is derived from an ECDH-computation on long-term keys of different parties, leaks the private key of the sender. The risk appears when a sender is tricked into encrypting a message with an invalid public key, i.e., one that is not on the real curve, but on a similar curve which admits small subgroups. In this context, the sender can leak their private key. For a more precise description of the twist attack, refer to Lundkvist’s blog post.
It is noteworthy to mention that the related ECIES encryption scheme, due to not using the sender’s private key, does not suffer from this particular issue. Nevertheless, it is still important to avoid such naive implementations, since they may lead to suboptimal security. Similarly, key-exchange variants, where senders provide an ephemeral key pair in addition to their long-term secret key, cannot be securely implemented with uncompressed and unverified public keys.
The current NIP44 implementation includes tests that make sure that deriving keys from invalid public keys fails. Cure53 suggests including more tests specifically designed to catch twist attacks (see NOS-01-003) and also describing valid keys in the specification (see NOS-01-002). It is known that uncompressed and unverified public keys can lead to other security issues.
secp256k1package does not make sure if the raw bytes for the keys you provided are indeed on the curve, invalid or weak in any other sense. Makes sense since it's supposed to be a low-level method:secp256k1with this reasoning but they might be using implementations ofsecp256k1which are vulnerable to twist attacks. So if I use asecpk256k1library, am I not usingsecp256k1just because the library has a vulnerability?GenerateConversationKeyyou're talking about generating private keys? But there's no danger from an invalid curve point here, that function just needs to check that a given 32 byte random string is less than the curve order (N or whatever); the comment warning is basically telling you to do that.GenerateConversationKey:secp256k1keys (see function signature).secp256k1.PrivateKeyandsecp256k1.PublicKey(to be precise: pointers of these types), these keys could have been generated in a weak way - for example, if the caller usedsecp256k1.PrivKeyFromByteswhich contains the warning that I copied into myGenerateSharedSecretfunction:PrivKeyFromBytesmyself in my tests and my library is not checking for weak or invalid private keys:ParsePubKey, here?:GenerateSharedSecretwas confusing. I didn't mean the function insecpk256k1. I meant the function in my NIP-44 library and that I am currently skipping tests because I assumed thatsecp256k1keys are always valid of strong. However, if they are generated usingsecp256k1.PrivKeyFromBytesthey might not be valid or contain weaknesses.secp256k1.GenerateShared Secretthat you use, there.secp256k1.PubKeyobject is almost always going to be generated fromParsePubKeyright? The only other way is via a constructor, and there is no concern there anyway.ParsePubKeyfunction. But if I am not using this function but provide my own bytes, then thesecp256k1package does not make sure. At least that's how I understand this warning for the functionPrivKeyFromBytes.