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.
reply
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 Outputshared_xmust be unhashed, 32-byte encoded x coordinate of the shared point Use HKDF-extract with sha256,IKM=shared_xandsalt=utf8_encode('nip44-v2')HKDF output will be aconversation_keybetween 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.
reply
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.
reply
Oh, I think I found another source of confusion.
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")) }
This is part of my NIP-44 golang library. (NIP-44 was recently merged.) So this code is meant to be called with
secp256k1 keys (see function signature).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 https://pkg.go.dev/github.com/decred/dcrd/dcrec/secp256k1/v4#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. | //
So currently, there is a disclaimer in the README of my NIP-44 implementation since not all test vectors are passing. I am using
PrivKeyFromBytes myself in my tests and my library is not checking for weak or invalid private keys:DISCLAIMER - READ BEFORE USINGThis 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.
See this line in my tests:
func assertConversationKeyFail(t *testing.T, sk1 string, pub2 string, msg string) { // TODO: Update GenerateConversationKey since secp256k1 does accept invalid or weak keys t.Skip("secp256k1 keys are not validated yet during conversation key generation.")
reply
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?
reply
So I guess it must be this one?
What's input is a PubKey object, which I guess will be got by calling
ParsePubKey, here?:and it is checking that it's a valid point (actually, it's also checking in the case when the point is uncompressed, as you'd hope/expect. (line 141)
reply
So I guess it must be this one?
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 :)
deleted by author
reply
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.
Also, bitcoin does not use the https://pkg.go.dev/github.com/decred/dcrd/dcrec/secp256k1/v4 package. It uses libsecp256k1 since it's written in C. LND however is written in golang and uses the aforementioned package.
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?