Skip to content

LNURL-pay: Drop metadata description hash validation#234

Open
callebtc wants to merge 3 commits intolnurl:ludsfrom
callebtc:remove_metadata_descrioption_hash
Open

LNURL-pay: Drop metadata description hash validation#234
callebtc wants to merge 3 commits intolnurl:ludsfrom
callebtc:remove_metadata_descrioption_hash

Conversation

@callebtc
Copy link
Copy Markdown
Contributor

@callebtc callebtc commented Aug 25, 2023

This PR removes the description_hashrequirement in LUD06 and LUD18.

Background

LNURL-pay requires the SERVER to provide an invoice with a specific description_hash (metadata in LUD06, and metadata+payerData in LUD18). WALLET is then supposed to verify that the invoice it receives indeed has this description_hash.

Rationale

If I understand correctly, the original intention of this is to make the server commit to the receiver (or receiver+payer) identities. Practically, this doesn't really improve anything: If there was a MITM changing the invoice, they could do so since neither of the data committed to is secret. If the server itself is malicious, they could fake everything as they wish anyway.

Implementing this (possibly redundant) feature is also the biggest challenge when implementing LNURL-pay.

This will also once and for all solve all the issues we have had for years dealing with CLN's requirement to provide the entire description at invoice creation.

I propose to drop this requirement, starting with removing the check WALLET performs on the invoice received in the second LNURL-p response. SERVER's who don't want to upgrade, can remain as is.

Progress

Supported

  • Blixt PR
  • Alby PR
  • Zeus PR
  • Current Comment
  • BlueWallet PR
  • Stacker.News PR
  • lnurl-go PR
  • Phoenix Commit
  • LNbits PR
  • Breez SDK PR
  • Galoy Blink PR
  • SatSale PR
  • sudonym-btc PR
  • LightningTipBot Commit
  • Geyser PR
  • Wallet of Satoshi (tested) ✅
  • Mutiny Comment

Unclear

(please report PRs or test results for more wallets)

  • Green
  • ...

@callebtc callebtc changed the title Remove metadata description hash validation LNURL-pay: Remove metadata description hash validation Aug 25, 2023
@callebtc callebtc changed the title LNURL-pay: Remove metadata description hash validation LNURL-pay: Drop metadata description hash validation Aug 25, 2023
@Kukks
Copy link
Copy Markdown
Collaborator

Kukks commented Aug 26, 2023

ACK from my end. Less hassle dealing with LN implementation support and much easier to build some more complex flows.

@Egge21M
Copy link
Copy Markdown

Egge21M commented Aug 26, 2023

ACK. Current Wallet, in foresight of this PR, never implemented this check.

@kaloudis
Copy link
Copy Markdown

ACK. Will support this change in Zeus.

bumi added a commit to getAlby/lightning-browser-extension that referenced this pull request Aug 27, 2023
@bumi
Copy link
Copy Markdown

bumi commented Aug 27, 2023

ACK
this makes it easier for wallet developers and it makes it also easier for services to offer LNURL/lightning address with different node backends.

Alby will support this in the next release.

@arcbtc
Copy link
Copy Markdown

arcbtc commented Aug 28, 2023

Awesome, description_hash got in the way a bunch

@callebtc
Copy link
Copy Markdown
Contributor Author

Looks like there is broad support for this PR! Anything that should be considered before it can be merged?

@dni
Copy link
Copy Markdown
Collaborator

dni commented Sep 20, 2023

👍

@fiatjaf
Copy link
Copy Markdown
Collaborator

fiatjaf commented Sep 20, 2023

My only worry is that services will drop support for this too soon and start breaking wallets that haven't yet.

@dpad85
Copy link
Copy Markdown
Contributor

dpad85 commented Oct 13, 2023

ACK, committing the lnurl-pay metadata to the invoice does not bring additional security AFAICT. The service is trusted anyway. Next Phoenix release will remove this check.

fiatjaf added a commit to fiatjaf/go-lnurl that referenced this pull request Oct 13, 2023
dpad85 added a commit to ACINQ/phoenix that referenced this pull request Oct 16, 2023
See lnurl/luds#234
This validation brings additional complexity for no real purposes.
@NCrashed
Copy link
Copy Markdown

NCrashed commented Feb 2, 2024

I would like to offer some critical feedback on the proposed change, particularly focusing on a crucial aspect of the lud06: 9. LN WALLET pays the invoice, no additional user confirmation is required at this point. The absence of a description hash eliminates the connection between the invoice and the user's explicit agreement, leaving the user without any evidence of discrepancies or potential misconduct by the server. A practical application of metadata commitment, from my experience, involves embedding exchange order numbers within this field, underscoring its significance. This issue becomes even more critical with payerData in lud18, as this data originates from the client, not the server.

In summary:

  • The principle of commitment is vital for ensuring users explicitly authorize transactions involving their funds.
  • The removal of a cryptographic link between the request and the invoice opens up new vulnerabilities for server attacks, leaving users without verifiable evidence of their payments.
  • Excluding the commitment mechanism compromises other components of the LUD, especially since the metadata format was specifically designed to facilitate hash computation and minimize verification ambiguities.

As an alternative, I suggest making the description hash optional and introducing an additional step for user confirmation when the commitment is absent.

@evd0kim
Copy link
Copy Markdown
Contributor

evd0kim commented Feb 2, 2024

NACK. I do not see a single valid argument here.

Joining to @NCrashed.

I don't care about imaginary services and wallets that would benefit from doing less work. The developer's job is to ensure that the digest corresponds to the received data.

The current protocol ensures that the previous round of communication corresponds to the next one, and the wallet checks for the user in the background that THIS invoice is for THAT menu inputs + information.

The thesis about trusted/non-trusted service is entirely irrelevant here. If meta contained some important info there is no basis for building trust for random user-service as well as no hash to prove misbehaving or indicating for the user that the service showed one info and asked to pay for another.

MITM changing the invoice, they could do so since neither of the data committed to is secret.

The check is not related to invoices but rather to any data put into Metadata.

If the server itself is malicious, they could fake everything as they wish anyway.

Yes. If the service wants to receive payment and doesn't provide any services or goods, the protocol is irrelevant, but this change enables a whole spectrum of situations when the service may attempt to cheat just a little bit. For example, let's consider the original purpose of the LNURL pay protocol. Selling channel liquidity. The service may provide in Metadata specific parameters about channel fees, channel size and active period, and node pubkey. It receives the payment for the service and opens the channel of size not 1.5M but 1.499M, fees not 1000 ppm but 1001 ppm. Now good luck catching such dishonest service in every UI of every wallet you meant while saying "it will make everything much easier". Instead, Metadata could be shown once, and description_hash could be checked in the background.

syusui-s added a commit to syusui-s/rabbit that referenced this pull request Feb 12, 2024
@callebtc
Copy link
Copy Markdown
Contributor Author

The absence of a description hash eliminates the connection between the invoice and the user's explicit agreement

Could you be specific? What does this prevent in your view? If you could make an explicit example of how this improves a payment flow or how it could be abused, it might be easier to understand what you mean.

In my view, if I offer you an invoice, you can choose to pay it or not, that is the only necessary expression of your agreement. The proof of payment is the preimage you'll receive.

What else other than authenticated transport (https) do you need to be sure that this particular invoice is from the server you've actually asked the invoice from? And what misconduct does the payment hash prevent?

@callebtc
Copy link
Copy Markdown
Contributor Author

callebtc commented Apr 18, 2024

The service may provide in Metadata specific parameters about channel fees, channel size and active period, and node pubkey.

This is a good example of a case that almost nobody cares about but it could be absolutely implemented even if the requirement would be dropped. If you run such a service, put the terms of the contract into the description hash and make your user read the contract and explicitly agree to it. That's the only way to make sure that both parties explicitly agree.

I'm guessing that 99.99% of lnurl payments are without any explicit commitments that the user cares about, they just want to pay. I haven't observed a single case where a dispute was resolved by comparing descriptions and hashes. For those cases where it's relevant: just do it.

@evd0kim
Copy link
Copy Markdown
Contributor

evd0kim commented Apr 18, 2024

Could you be specific? What does this prevent in your view? If you could make an explicit example of how this improves a payment flow or how it could be abused, it might be easier to understand what you mean.

I provided an example.

In my view, if I offer you an invoice, you can choose to pay it or not, that is the only necessary expression of your agreement. The proof of payment is the preimage you'll receive.

Checking a hash against a description that is stored or provided off-band may be more relevant for automated payments. It may be important for the client side, too.

What else other than authenticated transport (https) do you need to be sure that this particular invoice is from the server you've actually asked the invoice from? And what misconduct does the payment hash prevent?

The point being made is not about faking the server. The point being made is about the server that provides some imprecise/non-deterministic data.

If you run such a service, put the terms of the contract into the description hash and make your user read the contract and explicitly agree to it. That's the only way to make sure that both parties explicitly agree.

Not relevant at all for machine-machine interfaces.

I haven't observed a single case where a dispute was resolved by comparing descriptions and hashes.

It wouldn't happen with the former rules, even if it were the case. There would not be any dispute at all.

@johnzweng
Copy link
Copy Markdown

johnzweng commented Jan 15, 2025

Hi! 🙂

Is there any chance that this could be merged. Or if this is still a concern:

My only worry is that services will drop support for this too soon and start breaking wallets that haven't yet.

Could we then add at least a deprecation note to the spec? I was not aware of this PR and (as a sending service) implemented this check and was wondering why more and more receivers (LN address owners) do not provide the correct tag h in their returned Bolt11 (effectively blocking the payment and creating support costs for our customer care).

Until someone (@reneaaron , thanks for that 🙏 ) pointed me to this PR here. 🙈

@michaelWuensch
Copy link
Copy Markdown
Contributor

Merging the PR would really make sense. Wallets that implement the spec as is already experience payment failures.
@callebtc I just adopted this change in BitBanana (michaelWuensch/BitBanana@cc59fec)

@dni
Copy link
Copy Markdown
Collaborator

dni commented Mar 14, 2025

Hi! 🙂

Is there any chance that this could be merged. Or if this is still a concern:

My only worry is that services will drop support for this too soon and start breaking wallets that haven't yet.

Could we then add at least a deprecation note to the spec? I was not aware of this PR and (as a sending service) implemented this check and was wondering why more and more receivers (LN address owners) do not provide the correct tag h in their returned Bolt11 (effectively blocking the payment and creating support costs for our customer care).

Until someone (@reneaaron , thanks for that 🙏 ) pointed me to this PR here. 🙈

i agree with adding a deprecating note to the spec, but i wouldn't remove that feature completely, how about making the check optional and the users signal that he want to have it checked, default is no check?

@evd0kim
Copy link
Copy Markdown
Contributor

evd0kim commented Mar 14, 2025

Valet now supports both protocols and it is able to check hash against description.

image

At least recently I've seen few more use cases for this feature, mostly for cross-protocol capabilities, so I keep telling it was a mistake to remove this from the spec.

@evd0kim
Copy link
Copy Markdown
Contributor

evd0kim commented Mar 14, 2025

It is even wrong from the start to me.

If I understand correctly, the original intention of this is to make the server commit to the receiver (or receiver+payer) identities.

Hash to Description is a commitment to the intent/contract/information provided earlier and not to identities. So whole message somehow irrelevant.

@michaelWuensch
Copy link
Copy Markdown
Contributor

michaelWuensch commented Mar 14, 2025

@evd0kim @dni
To be honest, I do not understand what security benefit the descriptionHash verification has. There is no commitment to anything. The service can set the hash correctly in the invoice so that the verification succeeds and still do something completely different. So its no protection against fraud. Or do I miss something?

Now to the suggestion to support both.
Maybe removing the verification was unnecessary. But shifting the burden to the user is an absolute UX nightmare in my opinion. Lightning wallets are already complicated enough, we don't need warnings that make users feel insecure and require a deep dive into some tech specs to understand.

Don't get me wrong, if this is a real security issue and I just don't get it, then I strongly support a warning. But if it is just about signaling an intention and only the intention can be cryptograpically verified, but not the final action, we are better off without a warning.

@dni
Copy link
Copy Markdown
Collaborator

dni commented Mar 14, 2025

#96 so this is one usecase of lud-18

@johnzweng
Copy link
Copy Markdown

Hi! 🙂

I'm on board with @michaelWuensch on this. I don’t see how setting the h tag of the invoice to some hash provides any real security improvement.

Hash to Description is a commitment to the intent/contract/information provided earlier and not to identities.

Yes, it acts as a commitment—but it’s not a binding commitment. Since no secret is involved, any lightning node could recreate the same commitment. If the LN SERVICE (LNURL web server) were compromised and returned a malicious invoice from a different node, it could still include the same h tag without issue. (And this is also true for the lud-18 flow.)

This suggests that preventing such an attack is not the purpose of this feature. So, what is the actual attack scenario which gets prevented by adding this tag h to the invoice?

@michaelWuensch
Copy link
Copy Markdown
Contributor

The only benefit I can see is that in a lud-18 flow you have proof that the service actually received the identity data you sent.
But if the wallet implementation is correct, this is always the case.

@johnzweng
Copy link
Copy Markdown

I think it's the same when looking to @evd0kim's example from above:

For example, let's consider the original purpose of the LNURL pay protocol: selling channel liquidity. The service may provide specific parameters in the metadata, such as channel fees, channel size, active period, and node pubkey. It receives the payment for the service but then opens the channel with a size of 1.499M instead of 1.5M and charges 1001 ppm instead of 1000 ppm.

I’d really love to understand how the h tag in the invoice could effectively prevent this scenario. The liquidity selling node could still return such invoice which commits to the correct specific agreement (channel parameters) but then do something totally different. So what have we gained in this situation by adding the h tag?

It just gives the payer the security that the receiver has actually received the data correctly (without any guarantees what the receiver does with this information or adheres to it). But for "just to be sure that data was transmitted correctly" we have other layers (https, tcp).

Do I miss something? 🙂

@evd0kim
Copy link
Copy Markdown
Contributor

evd0kim commented Mar 14, 2025

@johnzweng

Since no secret is involved

If the host or user puts a secret in description it may contain a secret.

This suggests that preventing such an attack is not the purpose of this feature.

I am not talking about attacks per se. But generally yes, removing description hash check somehow makes protocol less secure if you mean some information provided via description off band.

something totally different.

The point of channel opening example is that there is a payment amount that is fixed in BOLT11 against description with hash. It may prevent something only when wallet does checks. And when it does and something is wrong, the wallet user may prove it otherwise she couldn't.

@dni

Thanks for the link. Looks like LNURL withdraw scheme.

@michaelWuensch

Maybe removing the verification was unnecessary. But shifting the burden to the user is an absolute UX nightmare in my opinion. Lightning wallets are already complicated enough, we don't need warnings that make users feel insecure and require a deep dive into some tech specs to understand.

Well, this is the point of hashes. They should match on the background. I believe LN is capable to facilitate smart contracts logic on top, this PR moves it in entirely opposite direction making LNURL less capable and less "smart".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.