Wallet password should be optional

petertodd commented 6 years ago

If the attacker has a copy of your wallet, in most sane threat models they probably also backdoor lnd to capture the password later anyway, rendering it pointless.

I set mine to 12345678, but really, an empty password should be accepted.

Roasbeef commented 6 years ago

If you don't want to use the wallet password, there's the --noencryptwallet setting. It was put in for usage within integration tests (and will likely soon be gated behind a build flag). It just uses a canned password in this setting.

As you note, it depends on your threat model though. If the attacker doesn't have the power to backdoor their running lnd instance, and they obtained a copy of the database from like a compromised Dropbox account or something, then the password is a level of defense.

robtex commented 6 years ago

if you use --noencryptwallet you will not get any cipher seed though?

Roasbeef commented 6 years ago

That's correct.

petertodd commented 6 years ago

@Roasbeef But why would I have the database on a Dropbox account? Lightning doesn't really do backups at this point. Equally, even in a normal wallet, I'd just use GPG for that encryption.

chpio commented 5 years ago

here's a workaround: https://github.com/Stadicus/RaspiBolt/blob/master/raspibolt_6A_auto-unlock.md

yonderblue commented 5 years ago

How to automate the current unlock with the rest api without relying on timing? The unlockwallet endpoint seems to return many things in success cases such as {"error":"context canceled","code":1} a 408 to {"error":"transport is closing","code":14} a 503. I have yet to see a 2xx. But usually any response besides connection refused or a 404 does unlock the wallet. The final resting case for that endpoint after unlocking seems to be 404, but the problem is you can get that before it issues the prompt to unlock too. So putting an infinite loop in that hits the endpoint until 404 isn't reliable. Using expect as in #1611 is probably better but I think all this illustrates that a simple no password option would be best.

yonderblue commented 5 years ago

just in case this helps anyone, this expect script and systemd unit file I believe are at least a reliable way to automate it even without an optional password. For example on a raspberry pi of mine:

/home/pi/lnd/lnd_expect.sh

#!/usr/bin/expect
set lndDist /home/pi/lnd
spawn $lndDist/lnd
set lndID $spawn_id
expect {
    -timeout 300
    "Waiting for wallet encryption password" {}
    default { send_user "default hit 1\n"; exit 1 } 
}
spawn $lndDist/lncli unlock
set cliID $spawn_id
expect {
    -i $cliID
    -timeout 30
    "Input wallet password" { send -i $cliID "password\n" }
    default { send_user "default hit 2\n"; exit 1 }
}
expect {
    -i $lndID
    -timeout -1
    eof send_user "got eof\n"
}

/etc/systemd/system/lnd.service

[Unit]
Description=Lightning Network Daemon
Wants=bitcoin.service
After=bitcoin.service

[Service]
Type=simple
ExecStart=/home/pi/lnd/lnd_expect.sh
Restart=on-failure
RestartSec=30
TimeoutSec=300
User=pi

[Install]
WantedBy=multi-user.target

Kixunil commented 5 years ago

Peter is right, anyone sensible running something like LND on a server would want this setup:

Reasonably isolate LND from the rest of the things
Make it so that LND restarts automatically if it crashes
All backups are encrypted when transferred out
Disk encrypted on system level if desired

Encryption enforced in LND is not only annoyance, but goes directly against "restart if it crashes" requirement. If an attacker can break isolation, he can read memory of LND, or kill LND and spawn a proces in its place pretending to be LND in order to sniff the password. (It can read TLS private key and the key must stay readable.)

Would you be willing to marge a PR providing an option to disable encryption based on these arguments? The unlock scripts complicate the system needlessly and increase attack surface.

Kixunil commented 4 years ago

If anyone is not convinced yet, I may have found the ultimate argument for this: admin macaroon is stored in the same directory. It's astronomically unlikely that the macaroon will be better protected than the wallet file.

Funnily enough, attacking admin macaroon might be even preferable to an attacker as he can trivially use it to send transactions, without any file copying or other complicated setups.

realspencerdupre commented 3 years ago

Came here to voice my agreement. Here's a scenario:

The folks at myNode do a nice job with an automated unlocker script. To make it easy, they automatically generate the password and save it in another folder. What security does this provide if we save our passwords in the clear for our unlocker scripts?

They fail to mention that the password is different from the main myNode login password.

User (me) attempts to transfer the .lnd folder, moving the node to a new machine. Forgets/doesn't know the wallet.db password is elsewhere.

User wipes SSD, because 1TB SSDs aren't cheap and I don't exactly have a bunch of extra ones lying around.

Now have no password, OhNo.gif, must resort to recoverchanbackup and SCB.

This is a foot gun. --noencryptwallet should be a first class flag.

Kixunil commented 3 years ago

@realspencerdupre great point, sometimes too much "security" can cause loss. Forgetting being more probable than theft is considered common knowledge by all Bitcoin security experts I know. Why shouldn't it apply here?

guggero commented 3 years ago

I don't necessarily disagree with the point that the wallet password should be optional. But I do want to clarify a few things:

The --noencryptwallet (or --noseedbackup as it's called now) flag has two effects:
1. It uses a default password for the encryption of the wallet.db and macaroons.db files. All other databases are unencrypted.
2. It initializes a wallet automatically on first startup and the seed for the wallet is never shown to the user and cannot be extracted later. The only thing that's stored in the wallet.db is the HD master root key (xprv). You can use that to manually recover funds but you currently cannot restore your node from it (seed to xprv is irreversible because there's hashing involved). #4717 will change that.
  - If you have the seed, you don't have to care too much about the encrypted content of the wallet.db as that can be recovered by the seed and a chain rescan (some metadata like TX labels and transaction info from Loop/Pool might get lost but no funds). In the situation where a wallet password is lost a node can still be migrated without loss of funds or channels by re-creating the wallet from the seed, stopping the node and replacing the channel.db file from the old node (only if the old node was stopped and didn't run for a single second after the copy of the channel.db file was created!!!).

To make a long story short: Yes, we might want to allow not needing to set a password. But first we need to refactor quite some code to disentangle the two effects of --noseedbackup.

Kixunil commented 3 years ago

@guggero AFAIK everyone in this thread is aware of current working of --noseedbackup - otherwise we would mistakenly use it. :) Indeed, it needs to be refactored to work differently, that's what we (at least I) proposed.

I am very happy that you're willing to accept such change. Maybe I will be able to find some time to help with it but I'm currently very busy.

Kixunil commented 3 years ago

I'd like to move this forward, so here's a proposal to resolve it:

The main goal is to get rid of the unreliability of automatic unlocking via unlocker scripts. The unlocking mechanism must allow consumers to get a seed or set their own It'd also be great to make migration from existing unlocker scripts easy. The mechanism must not introduce unreliability or block consumers from writing their scripts reliably. That means if the consumer wants to make the operation safe even in presence of power failure at any instruction, the API must make this possible.

lnd will accept wallet_password configuration option and will use it to unlock the wallet if present. It may be empty.
If wallet_password is present and the wallet is not initialized, lnd will fail to launch the daemon
A new option--init-wallet is available on lnd binary, that means not lncli. If this option is present lnd will initialize the wallet. --seed-file option is required in that case. The daemon will not be launched after successful initilization. lnd just exits with 0. If initialization fails for any reason, lnd exits with non-zero exit code.
--seed-file option specifies path where the seed is stored. It can be a pipe or a regular file. lnd will only read this file, not modify it in any way.
--init-wallet requires wallet_password to be configured this password will be used for encryption
If --init-wallet is used and the wallet is initialized, then a private key is generated from the seed and compared to the internal private key. If they match, lnd just exits with 0 (making this no-op), if they are different, lnd prints a readable error message and exits with non-zero exit code.
--gen-seed option on lnd will cause it to just output the seed as JSON object and exit with 0. --entropy-source-file may be provided - LND will read exactly 32 bytes of entropy from that file and turn it into seed. Entropy source may be a regular file, pipe or character device.
--gen-seed and --init-wallet can not be specified at the same time

If this is accepted I will codify these requirements as tests written in bash. I will also provide an example of safe, reliable init/unlock script with comments explaining all relevant details.

Rationale

Allowing password to be specified in config file makes migration easier and less error-prone.
Multi-stage initialization with ability to set the seed allows implementing reliable recovery or advanced schemes.
Separating initialization from launching of the daemon makes error reporting easier.
Putting --init-wallet and --gen-seed into lnd binary avoids the requirement to have lncli installed.
Allowing "double initialization" with the same seed enables making the unlock script idempotent. This is critical to ensure reliability and safety.

realspencerdupre commented 3 years ago

This makes sense

lnd will accept wallet_password configuration option and will use it to unlock the wallet if present. It may be empty.

But for the rest, I don't see the need for initializing/generating a wallet automatically. Sounds scary.

Kixunil commented 3 years ago

@realspencerdupre it's required for the whole process to be secure and reliable. Unless you intend all lnd users to be capable of using CLI. This is not some theory, I work on integrating LND with higher-level apps and unlocker script is a pain. There's too many moving parts that can break and lead to terrible UX.

Thus I wrote it based on my experience and what would help me the most. If others have different experience I'll be happy to cooperate and figure out the most optimal approach for everyone.

Edit: forgot to mention my proposal also completely removes security risk when initializing the wallet.

guggero commented 3 years ago

Thank you for your comments and proposal, @Kixunil! There are a lot of very valid points in there. And I agree that we should take this seriously and improve the situation, especially if there is a security issue that can be patched (I wasn't aware of that).

My main concern here is UX and backward compatibility. Because we took too long to even allow reading the unlock password from stdin in lncli, a lot (if not all) end user wallets implemented some kind of unlocker (myself included). While this is bad in itself, we also don't want to make devs even unhappier by suddenly not supporting their unlock scripts anymore.

That's why I'm adding my own counter proposal here. I'm interested in hearing what you think and if you see any potential security flaws with it.

Let's split the issue into the two parts they are:

Initial wallet creation/initialization
- I'd strongly advise against adding yet another wallet related set of flags to lnd itself. The sheer combination of flags that need to be validated against each other would not help the code readability and also not make good UX
- If there was a standalone binary (let's call it lndinit for now) couldn't we solve this in a more user friendly way? That binary would have one task, exactly what you described for the --init-wallet flag.
- lndinit would be included in the default release process and be covered by the same developer signatures as lnd (going to look into that other PR next).
- Once the wallet is created for the first time, you can even delete the binary if you want to.
Wallet unlocking after a restart
- I have to say I like the idea of using a mode where you specify the wallet password somehow (I'd actually prefer environment variables over having my password in the config file which in my eyes defeats the purpose of having a password in the first place) and expect the daemon to error out if the unlocking fails (instead of spinning up the unlocker service).
- To keep everything backward compatible, we could add a flag called --safe-unlock (or whatever) that activates that mode and then tries to read the password from env variable, CLI flag or config file (or a password file)

I'm aware that this proposal does not not satisfy the last two items of your rationale. The question is: how important are these two points to your use case?

Kixunil commented 3 years ago

we also don't want to make devs even unhappier by suddenly not supporting their unlock scripts anymore.

Right, in case it wasn't clear, I didn't want to propose implicitly nor explicitly to remove current init and unlock calls. At least not initially. People should have enough time to migrate. Recommending migration in release notes would be great though.

I'd strongly advise against adding yet another wallet related set of flags to lnd itself

Sure, if we can do it without compromising on other important values, that's great.

If there was a standalone binary (let's call it lndinit for now) couldn't we solve this in a more user friendly way?

Sounds good! I'm assuming this binary operates on the data directly - it doesn't use RPC, am I correct?

prefer environment variables

Interestingly I heard claims that env vars are not safe for storing secrets but I couldn't find an actual exploit yet. I prefer to stay away from them.

Maybe it'd be better to have wallet_password_file instead that reads from a file. The file can be regular or a pipe, allowing various password managers to unlock lnd. I actually used something like this in the past and it worked well.

To keep everything backward compatible, we could add a flag called --safe-unlock

I'm not sure what problem does it solve, can you clarify?

I'm aware that this proposal does not not satisfy the last two items of your rationale. The question is: how important are these two points to your use case?

Do you propose lndinit to fail if the wallet is already initialized (with the same seed and password)? Perhaps use distinct error code so that I know it's safe to ignore?

The last point is very important to me. I went great lengths to ensure reliability of my project. It'd be quite sad to see a hole in my effort caused by upstream software that's supposed to be security-critical.

Not having to install lncli is just perfectionism and it's OK to have dependency on it or some other binary.

I also forgot to mention that I have a plugable system that makes auto-unlocking opt-out. I'm 100% sure that there's at least one instance in the wild that opted out and at least one that didn't, so I'm also trying to avoid breaking any of those. So far nothing proposed here seems to conflict with this but I mention it just in case.

guggero commented 3 years ago

Sounds good! I'm assuming this binary operates on the data directly - it doesn't use RPC, am I correct?

Correct, it would create the wallet DB file directly (nothing else, lnd does the rest automatically, like the macaroon DB and macaroon files themselves) with the seed and password (and entropy if provided). If this is enough to satisfy your last point, then that tool could either exit with a success status if the wallet already exists, or exit with a special error code. That way you could always invoke the command on startup.

Interestingly I heard claims that env vars are not safe for storing secrets

Maybe that's something more common in Docker/Kubernetes environments where you don't want hard coded secrets in files (because that's harder to setup/maintain in a "stateless" environment) or command line flags (because they could be visible even to non-privileged cluster users), so secrets injected in environment variables are quite common. But I envision all three options (flag, file, env) to be supported, just like we did in LiT: https://github.com/lightninglabs/lightning-terminal/blob/master/config.go#L123

I'm not sure what problem does it solve, can you clarify?

Just to turn lnd into the "try to unlock or fail, never spin up unlocker RPC" mode. But maybe that's not needed since the presence of any of the password flags/options would already achieve that.

Cool, I feel like we have a possible way here that could cover all use cases of this thread while remaining fully backward compatible. And to be honest, this would also make our cluster setup way easier. We definitely also started feeling the pain of this.

Kixunil commented 3 years ago

But I envision all three options (flag, file, env) to be supported

I'm not sure about argument, it's quite a big footgun. I contributed to electrs in the past to disable similar option reasoning that failure is better than vulnerability and it did actually save someones ass. Maybe allowing it being empty would be an interesting compromise but the clutter of command line may still be a good reason to prefer other means.

I will leave the decision to you just please document the footgun if you decide to allow argument.

But maybe that's not needed since the presence of any of the password flags/options would already achieve that.

Yeah, I think the presence of password option should do it automatically.

Cool, I feel like we have a possible way here that could cover all use cases of this thread while remaining fully backward compatible.

I agree, I will try to write a test script so the implementors can use it to check the implementation.

guggero commented 3 years ago

@Kixunil: I created a PoC in https://github.com/lightningnetwork/lnd/pull/5150. Everything file based should work, the k8s stuff is mostly for our own cluster setup and is not yet implemented. Please let me know what you think and whether this would work for your use case.

Kixunil commented 3 years ago

Whoa, great speed! I started writing the example script and test script but didn't finish. I guess I will publish the draft and then improve it.

dannydeezy commented 3 years ago

Personally I would love it if there was a single flag added to lnd called --wallet_password or --wallet_password_file, which handles both unlocking and initializing. It could initializes a new wallet (if none exists), or unlock the existing wallet if it does exist. This would allow me to have 1 simple helm chart for my Lnd deployment that works for the first and later deployments, and no manual work required to initialize the wallet.

While I hear this argument @guggero :

I'd strongly advise against adding yet another wallet related set of flags to lnd itself.

Perhaps just this single flag (rather than a set of interdependent flags) might be ok?

I don't like the idea of having different behavior required to init or unlock the wallet, and I think it might be nice to compress these into a single flag. In the past I wrote a script that did this and it worked nicely for my Lnd deployment. Perhaps I'm missing something though :)

guggero commented 3 years ago

That sounds very dangerous. Wouldn't that mean that you never even see your seed? If your wallet.db file is deleted or gets corrupted, you're definitely screwed. Even with only having the wallet.db, there currently isn't a supported or documented way to recover/rescue your node in case of a failure. Or how would you expect the seed to be handled during initialization, @dannydeezy? Also, I really really hope that script isn't used in production!

dannydeezy commented 3 years ago

Good point about the seed, I think that's what I was missing - thanks!

Just also took a look at your PR #5150 and left a question there.

Re: the script, if It remember, I think I manually backed up the mnemonic after writing it to the disk... not great haha

ddiekroeger commented 3 years ago

@Kixunil: I created a PoC in #5150. Everything file based should work, the k8s stuff is mostly for our own cluster setup and > is not yet implemented. Please let me know what you think and whether this would work for your use case.

This is great!

guggero commented 3 years ago

think I manually backed up the mnemonic after writing it to the disk

Please do me a favor and check whether that is the case asap! If you don't have the seed there's still a way to extract the xprv when you have access to your wallet.db file, so that you at least have something!

dannydeezy commented 3 years ago

@guggero the node that used that script was swept and inactivated almost two years ago :)

Roasbeef commented 2 years ago

Thinking of closing this as we have the wallet file option, and also lndinit that will fully automated the creation and storage of the password itself.

petertodd commented 2 years ago

Thinking of closing this as we have the wallet file option, and also lndinit that will fully automated the creation and storage of the password itself.

I do not agree. There are lots of security postures where the password is totally pointless. Personally I set mine to abcdefgh on every lnd wallet I have. I'm far more likely to lose funds from it being inconvenient to auto-restart lnd on reboot - followed by exploitation by a peer - than I am having my backups be compromised.

Kixunil commented 2 years ago

@petertodd there is wallet-pasword-unlock-file now so the problems associated with mandatory password are solved except for minor annoyance.

petertodd commented 2 years ago

...and using that ends up in pretty the same situation as a passwordless wallet file. But more convoluted and with a risk that the password will be lost.

Not to mention, calling this a "password" is misleading anyway. Since the threat model is offline decryption, we actually need something with significantly more entropy than what would normally be considered a password to resist cracking efforts.

On August 20, 2022 10:02:43 AM GMT+03:00, "Martin Habovštiak" @.> wrote: @. there is wallet-pasword-unlock-file now so the problems associated with mandatory password are solved except for minor annoyance.

-- Reply to this email directly or view it on GitHub: https://github.com/lightningnetwork/lnd/issues/899#issuecomment-1221246932 You are receiving this because you were mentioned.

Message ID: @.***>

lightningnetwork / lnd

Wallet password should be optional #899

Rationale