Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default settings allow using electrum lnpay $invoice without any confirmation or password #9236

Open
Midar opened this issue Oct 7, 2024 · 6 comments
Labels
CLI/RPC ▶ security 🔐 technical issue that affects security of funds

Comments

@Midar
Copy link

Midar commented Oct 7, 2024

The default settings allow using electrum lnpay $invoice without any confirmation or password. That seems a bit risky, as anyone who can get access to the socket gets to spend all available LN balance.

@SomberNight
Copy link
Member

anyone who can get access to the socket gets to spend all available LN balance

Note: on Linux/macOS, the default is to use unix sockets for the RPC, on Windows we listen on a TCP port on localhost.
In either case, the RPC is protected with a random password, which is stored in the config file. So in practice to be able to run commands such as lnpay, the conditions are that (1) the wallet has to be open, (2) read access for the config file is needed.
(read access to the config file implies access to the unix socket or localhost tcp also)

@SomberNight
Copy link
Member

SomberNight commented Oct 10, 2024

@Midar questioned on IRC how 37d090c / #9238 interacts with hardware wallets.

This is a good question and it is complicated to answer. The short answer is that the RPC does not work for encrypted hw wallets.

If the standard wallet with hw keystore is not encrypted, then CLI/RPC works without auth both before and after the change. (note that there is always rpcpassword-based authentication, see prev comment, I am only considering any auth after that)

If the wallet file is encrypted with a hw device, then offline (meaning --offline, no daemon or GUI running) commands (such as signmessage) work both before and after change. The user is not prompted for the wallet password -- instead there is interaction with the hw device.

If there is a daemon running, note that load_wallet does not support hw-encrypted wallets.

If there is a GUI running, which I guess is the original motivation for this issue, the equivalent of load_wallet is opening a wallet in the GUI, which ofc supports hw-encrypted wallets.

  • Before this change, all commands worked without password-authentication (compare e.g. signmessage (which started a CLI hw handler), and lnpay (which just executed), both of which worked). Quirk: the "requires_password" commands required the user to explicitly pass a dummy password.
  • After the change, the commands that are marked as "requires_password" still require the user to pass a password argument, but a dummy password is no longer accepted. Only the actual xpub-derived password would be accepted, which the user is not supposed to know.

Hence, re encrypted hw wallets, to sum up, some commands worked prior to this change but in a really quirky/accidental fashion only -- and now they do not work.


Another further note is that maybe malware running on the PC could communicate with the connected hardware device and request the encryption password from it. It depends on hw device type and implementation specifics whether there would be any prompt about this (e.g. a trezor one would prompt for PIN, and potentially passphrase).
(for the encryption password we use a serialised pubkey, bip32-derived along a fixed hardened bip32 path)

But note that if an attacker could pull this off, they might as well just copy the wallet file, decrypt it, and do whatever they want there without the RPC.


I am reopening this to signal that it warrants some more thought, but I think we could leave master as-is.
Though we should probably at least add some comment about all this re 37d090c near line 163.

@SomberNight SomberNight reopened this Oct 10, 2024
@SomberNight SomberNight added CLI/RPC ▶ security 🔐 technical issue that affects security of funds labels Oct 10, 2024
@Midar
Copy link
Author

Midar commented Oct 11, 2024

Hm, isn't the wallet always encrypted when using a HW wallet? In any case I'm seeing the lock symbol in Electrum and I can just use lnpay without any confirmation whatsoever.

@SomberNight
Copy link
Member

Hm, isn't the wallet always encrypted when using a HW wallet?

No, you can click the lock and disable file encryption.

I can just use lnpay without any confirmation whatsoever.

On master? Can you give repro steps?

@Midar
Copy link
Author

Midar commented Oct 11, 2024

No, this was on 4.5.5, waiting for the next signed release before I let this touch my wallet ;). What I meant to say is that before this change at least, it already had access, even with a HW wallet.

@SomberNight
Copy link
Member

Before this change, all commands worked without password-authentication (compare e.g. signmessage (which started a CLI hw handler), and lnpay (which just executed), both of which worked). Quirk: the "requires_password" commands required the user to explicitly pass a dummy password.

Yes that's already what I meant there ^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLI/RPC ▶ security 🔐 technical issue that affects security of funds
Projects
None yet
Development

No branches or pull requests

2 participants