signrpc: Add no_hashing flag to signMessage

[hsjoberg]

Closes #4474.

To be able to support lnurl-auth, a message ("k1") needs to be signed by lnd without it being hashed.
This PR addresses this by adding a new flag no_hashing to the signrpc.signMessage method. It defaults to false to keep current behavior. If it is set to true, no hashing of the message will be made before signing.

I have not updated verifyMessage with the flag but I could do that as well if you want me to.

Cheers

[yyforyongyu]

Thanks for the PR! I took a look at the k1 value mentioned in the issue, as defined here,

k1 (hex encoded 32 bytes of challenge) which is going to be signed by user's linkingPrivKey.

And it seems like it's already 32 bytes, so the function SignDigest could be stay put? When signing a message, it's always required the message to be hashed before signing. I think what we are dealing here is to decide whether we want to perform a SHA256 on the message or not. If not, we should only allow a 32 byte message to be directly passed to SignDigest.

[hsjoberg]

Thank you for your feedback @yyforyongyu!
I didn't know that cryptographic signing requires a fixed amount of bytes, but yes that absolutely makes sense.

I made the code prior to this knowledge, which is why I named the flag to the rather cryptic raw_msg.
Perhaps naming it no_hashing or something similar and mandating the input to be 32 bytes makes more sense.
This is also why removed the 32 bytes declaration to various functions (go failed to compile because msg is of type []byte).

[hsjoberg]

@yyforyongyu Please take another look, I think I addressed all your feedback.

[hsjoberg]

Rebased and uppercase'd "MUST" in error message (msg MUST be 32 bytes).

[guggero]

Thank you for the PR.
Unfortunately I have serious security concerns with this seemingly simple change. I see why this is wanted, but let me first ask some questions (also directed at @michaelWuensch and @btcontract):

1. Why would the lnurl-auth scheme not require k1 to be hashed before signing it? This is even a value that is user provided. I think this would make it quite trivial for an attacker to extract private key material with just a few calls with specially constructed k1 values.
2. You cannot sign with a key under m/138'/0 with this RPC anyway. How would you get around that obstacle? Or would there be a special "lnd lnurl auth scheme"?

While I do think the intentions (and the need for) lnurl-auth is good and justified, I worry that it hasn't been looked at properly from a cryptography standpoint. "Rolling your own crypto protocol" just never is a good idea.

@cfromknecht what's your opinion here?

[hsjoberg]

Hi @guggero, I appreciate your reply.

1. Why would the lnurl-auth scheme not require k1 to be hashed before signing it? This is even a value that is user provided. I think this would make it quite trivial for an attacker to extract private key material with just a few calls with specially constructed k1 values.

I think this question is best suited for @btcontract.
It looks like this could be changed so that the wallet will always hash the message before signing it.

Unfortunately the current schema is widely deployed by different wallets and services. But if this is a security concern, I think it definitely makes sense to address this.

2. You cannot sign with a key under m/138'/0 with this RPC anyway. How would you get around that obstacle? Or would there be a special "lnd lnurl auth scheme"?

Yes, it's not possible to get the hashingKey according to the specs, as we cannot do hmacSHA256 on the private key derived from m/138'/0 using the RPC.
It's also not possible to use this hashingKey to be fully complaint on how to get the linkingKey, because lnd doesn't support full BIP44 derivation, only two levels deep (BIP43).

So there will have to be special cases for the linkingKey and hashingKey with lnd.

[cfromknecht]

@guggero your spidey senses are not unfounded.

I think this would make it quite trivial for an attacker to extract private key material with just a few calls with specially constructed k1 values.

No, this does not leak your private key.

That said, opening this PR did lead to the identification of weaknesses in the construction. The relevant details were disclosed privately to @btcontract 2-3 weeks ago and we remain in contact. Even though there are inherent weaknesses in the primitives, there doesn't appear to be any immediate security implications for LNURL as specified. However, we both agree that these weaknesses are worth fixing and are actively investigating a v2 scheme for lnurl-auth.

@hsjoberg As it relates to this PR, unfortunately I am not comfortable exposing insecure signing primitives generically via lnd's rpc interface. Therefore we probably won't see lnurl-auth support in lnd until a replacement is standardized, however we are working to make that happen sooner than later.

NOTE: I consulted @btcontract before replying to ensure we were both in agreement that this preliminary disclosure is reasonable given the security risk is minimal. More details will be available soon, as of now there is no immediate action that needs to be taken by clients or servers.

[hsjoberg]

@hsjoberg As it relates to this PR, unfortunately I am not comfortable exposing insecure signing primitives generically via lnd's rpc interface.

@cfromknecht Totally understandable. As long as we come up with another solution that works for everyone, I'm happy.

Therefore we probably won't see lnurl-auth support in lnd until a replacement is standardized, however we are working to make that happen sooner than later.

Sounds good, keep me in the loop if there's anything I can do.

Cheers