Reject invalid wallets

promag commented at 0:06 am on July 20, 2017: member

This PR prevents loading the same wallet more than once in a multi wallet scenario. It also prevents loading with invalid files: non regular files or symlinks.

in src/test/util_tests.cpp:107 in 4b58cae787 outdated

103@@ -104,7 +104,7 @@ class TestArgsManager : public ArgsManager
104     {
105         return mapArgs;
106     };
107-    const std::map<std::string, std::vector<std::string> >& GetMapMultiArgs()
108+    std::map<std::string, std::vector<std::string> >& GetMapMultiArgs()

promag commented at 0:06 am on July 20, 2017:

This is a helper, like the above. For the test below I had to drop const,

in src/test/util_tests.cpp:169 in 4b58cae787 outdated

164+{
165+    TestArgsManager testArgs;
166+
167+    BOOST_CHECK_EQUAL(testArgs.GetUniqueArgs("a").size(), 0);
168+
169+    testArgs.GetMapArgs()["a"] = "biz";

promag commented at 0:07 am on July 20, 2017:

This is set because IsArgSet uses mapArgs.

promag commented at 0:08 am on July 20, 2017: member

Some notes for reviewers.

laanwj added the label Wallet on Jul 20, 2017

laanwj added the label Bug on Jul 20, 2017

laanwj added this to the milestone 0.15.0 on Jul 20, 2017

in src/util.h:220 in 099e5efba0 outdated

201@@ -202,6 +202,7 @@ class ArgsManager
202     void ParseParameters(int argc, const char*const argv[]);
203     void ReadConfigFile(const std::string& confPath);
204     std::vector<std::string> GetArgs(const std::string& strArg);
205+    std::vector<std::string> GetUniqueArgs(const std::string& strArg);

instagibbs commented at 3:09 pm on July 20, 2017:

Can you briefly describe what it does in a comment above this line?

laanwj commented at 3:28 pm on July 20, 2017:

Is this use-case common enough to add a new argument getter to the API for? If not, I’d prefer chaining more general functions, for example defining a function std::vector<string> UniqueStrings(const std::vector<string>&) then using it as

0UniqueStrings(GetArgs("-wallet"))

promag commented at 3:43 pm on July 20, 2017:

There are other multiargs that may use this getter, but I’m checking it. There is also the option of GetArgs(const std::string& strArg, fUnique = false).

promag commented at 3:45 pm on July 20, 2017:

But I also like that option @laanwj.

jnewbery commented at 9:35 pm on July 20, 2017:

+1 to comment.

I can imagine this having wider use, so I don’t think adding a ArgsManager member function is a problem.

in src/util.cpp:433 in 4b58cae787 outdated

426@@ -427,6 +427,19 @@ std::vector<std::string> ArgsManager::GetArgs(const std::string& strArg)
427     return {};
428 }
429 
430+std::vector<std::string> ArgsManager::GetUniqueArgs(const std::string& strArg)
431+{
432+    LOCK(cs_args);
433+    std::vector<std::string> args;

jnewbery commented at 9:28 pm on July 20, 2017:

Why not let std::set do the heavy lifting for you:

0    std::vector<std::string> args = GetArgs(strArg);
1    std::set<std::string> uniques(args.begin(), args.end());
2    args.assign(uniques.begin(), uniques.end());
3    return args;

promag commented at 9:51 pm on July 20, 2017:

We want to keep order.

jnewbery commented at 9:53 pm on July 20, 2017:

ah. Makes sense. Thanks.

promag commented at 9:58 pm on July 20, 2017:

I was tempted to name it GetStableUniqueArgs but maybe it’s too much for most of you.

jnewbery commented at 11:05 am on July 21, 2017:

I think just make sure there’s a comment at the declaration saying that ordering is stable.

jnewbery commented at 9:36 pm on July 20, 2017: member

Concept ACK.

An easy way to add a functional test for this would be to rebase it on top of #10604 , add a duplicate wallet argument and then add a test that listwallets returns the correct number of wallets. Check with @laanwj because we don’t want to add that PR dependency if #10604 doesn’t get into v0.15

jnewbery commented at 2:07 pm on July 21, 2017: member

#10604 has been merged. Suggest ~~you rebase on master and~~ add something like the following to multiwallet.py as a functional test:

 0@@ -15,7 +15,8 @@ class MultiWalletTest(BitcoinTestFramework):
 1         super().__init__()
 2         self.setup_clean_chain = True
 3         self.num_nodes = 1
 4-        self.extra_args = [['-wallet=w1', '-wallet=w2', '-wallet=w3']]
 5+        # bitcoind should ignore duplicate wallets in config
 6+        self.extra_args = [['-wallet=w1', '-wallet=w2', '-wallet=w3', '-wallet=w2']]
 7 
 8     def run_test(self):
 9         w1 = self.nodes[0] / "wallet/w1"
10@@ -41,7 +42,7 @@ class MultiWalletTest(BitcoinTestFramework):
11         w3_name = w3.getwalletinfo()['walletname']
12         assert_equal(w3_name, "w3")
13 
14-        assert_equal({"w1", "w2", "w3"}, {w1_name, w2_name, w3_name})
15+        assert_equal(self.nodes[0].listwallets(), ["w1", "w2", "w3"])
16 
17         w1.generate(101)
18         assert_equal(w1.getbalance(), 100)

promag force-pushed on Jul 21, 2017

promag commented at 2:48 pm on July 21, 2017: member

@jnewbery thanks for the tip, it’s in 7605a8d. @instagibbs added the missing comment in 8cf06d7.

in src/util.h:206 in 7605a8d4c1 outdated

202@@ -203,6 +203,10 @@ class ArgsManager
203     void ReadConfigFile(const std::string& confPath);
204     std::vector<std::string> GetArgs(const std::string& strArg);
205 
206+    // Returns unique values of the given argument, preserving the order

jnewbery commented at 3:14 pm on July 21, 2017:

ultranit: match the doxygen style comments below.

jnewbery commented at 3:14 pm on July 21, 2017: member

Looks good @promag . I’ve added one nitty comment. utACK either way.

promag force-pushed on Jul 21, 2017

promag commented at 9:57 pm on July 21, 2017: member

@jnewbery added missing comment to GetArgs too.

arvidn commented at 1:05 am on July 22, 2017: none

please correct me if I’m wrong. As far as I can tell, since this only prevents specifying the same filename multiple times, it still doesn’t prevent you from loading the same file twice. You just have to use different names/paths to refer to it:

--wallet=w1 --wallet=../wallets/w1 (assuming we’re in a directory called wallets)

One could also use hard- or soft links to give a single wallet multiple names.

promag commented at 7:30 am on July 22, 2017: member

@arvidn I’ll test. Thanks!

in src/util.cpp:434 in 15489827ef outdated

426@@ -427,6 +427,19 @@ std::vector<std::string> ArgsManager::GetArgs(const std::string& strArg)
427     return {};
428 }
429 
430+std::vector<std::string> ArgsManager::GetUniqueArgs(const std::string& strArg)
431+{
432+    LOCK(cs_args);
433+    std::vector<std::string> args;
434+    std::set<std::string> uniques;

benma commented at 7:16 am on July 24, 2017:

you should #include <set>

promag commented at 7:36 am on July 24, 2017: member

I’m updating to take into account @arvidn review.

promag force-pushed on Jul 24, 2017

promag commented at 10:46 pm on July 24, 2017: member

After @arvidn observation, I took a different approach. In order to correctly detect duplicate wallets I’m using boost:filesystem::equivalent in bool CWallet::Verify(). This means that if a wallet is duplicate then an initialization error is raised. @jnewbery removed the functional test because the app now quits with an error, AFAIK we don’t test these?

promag commented at 10:49 pm on July 24, 2017: member

@arvidn there is already a -wallet validation to prevent relative paths. However the initial approach failed with symlinks. Thanks for the feedback.

in src/wallet/wallet.cpp:483 in b1b4e8ac16 outdated

480+
481+        if (SanitizeString(walletFile, SAFE_CHARS_FILENAME) != walletFile) {
482             return InitError(_("Invalid characters in -wallet filename"));
483         }
484 
485+        fs::path wallet_path = fs::absolute(walletFile, GetDataDir());

sipa commented at 2:38 am on July 25, 2017:

Based on the documentation, I’m not sure that boost::filesystem::absolute resolves symlinks; only that it resolves relative path elements (lack of root, and . and .. path elements). boost::filesystem::canonical resolves symlinks too.

EDIT: nevermind, it seems that fs::equivalent further takes care of this.

promag commented at 10:10 pm on July 25, 2017:

@sipa fs::equivalent is not used in the latest version.

sipa commented at 2:40 am on July 25, 2017: member

utACK b1b4e8ac1687fac1dd275b7bab2c45eb879be30d

in src/wallet/wallet.cpp:486 in b1b4e8ac16 outdated

482             return InitError(_("Invalid characters in -wallet filename"));
483         }
484 
485+        fs::path wallet_path = fs::absolute(walletFile, GetDataDir());
486+
487+        if (fs::exists(wallet_path)) {

TheBlueMatt commented at 7:59 pm on July 25, 2017:

Doesn’t this imply you can specify duplicate wallets if the wallet doesn’t (yet) exist? It seems strange mostly because it would result in you being able to restart your node and then suddently it will fail but succeed the first time.

promag commented at 10:03 pm on July 25, 2017:

7c4058d disallows having wallets as symlinks whereas addffb2 resolves symlink to the concrete absolute path.

ryanofsky commented at 9:51 pm on July 25, 2017: member

This approach seems pretty fragile, but maybe it’s the simplest safeguard we can implement right now. I’d think ideally bitcoin would try to acquire an exclusive write lock on a wallet database file while opening it, and just not open files that can’t be locked.

in src/wallet/wallet.cpp:490 in addffb2ba3 outdated

486+        if (fs::exists(wallet_path)) {
487+            if (!fs::is_regular_file(wallet_path)) {
488+                return InitError(_("Invalid -wallet file"));
489+            }
490+
491+            if (fs::is_symlink(wallet_path)) {

sipa commented at 0:40 am on July 26, 2017:

It seems this could be done using fs::canonical ?

laanwj commented at 6:37 am on July 26, 2017: member

This approach seems pretty fragile, but maybe it’s the simplest safeguard we can implement right now. I’d think ideally bitcoin would try to acquire an exclusive write lock on a wallet database file while opening it, and just not open files that can’t be locked.

Right, ideally it’s something that would be handled at the database layer, in the same way it’s avoided that multiple applications open the wallet at once. It’s unfortunate that BerkeleyDB itself doesn’t provide protection against opening a database multiple times.

Anyhow it’s better to have some check in place, so concept ACK.

promag commented at 7:00 am on July 26, 2017: member

At this point there’s no file opening, it’s a configuration error. Like you said it’s a runtime error. Both make sense for me.

jnewbery commented at 8:10 am on July 26, 2017: member

the app now quits with an error, AFAIK we don’t test these?

We have a way of testing this: if you’re expecting bitcoind to fail on startup, take a look at assert_start_raises_init_error(). If you’re expecting bitcoind to startup successfully and then exit, look at the new test in #10882 : https://github.com/bitcoin/bitcoin/pull/10882/commits/ec59b2eaaccd4d42d98345e9719265f102494e46#diff-8d6207d4df9ed2a74441a199714a612aR92

promag force-pushed on Jul 26, 2017

promag commented at 12:27 pm on July 26, 2017: member

@jnewbery thanks, will add the missing tests.

The current approach is to raise an error when a wallet absolute path:

exists and it’s not a regular file or a symlink, or (@sipa FYI)
does not exists and it’s a duplicate (@TheBlueMatt FYI).

in src/wallet/wallet.cpp:485 in c84a5a9e56 outdated

481             return InitError(_("Invalid characters in -wallet filename"));
482         }
483 
484+        fs::path wallet_path = fs::absolute(walletFile, GetDataDir());
485+
486+        if (fs::exists(wallet_path) && (!fs::is_regular_file(wallet_path) || fs::is_symlink(wallet_path))) {

TheBlueMatt commented at 6:48 pm on July 26, 2017:

Boost documentation seems to indicate that is_regular_file implies !is_symlink.

promag commented at 0:23 am on July 27, 2017:

No, one does not implies the other. A symlink to a file is a regular file. A symlink to a directory is not a regular file.

promag commented at 0:30 am on July 27, 2017:

BTW, if I remove the !is_symlink condition then multiwallet.py test fails.

promag force-pushed on Jul 26, 2017

promag force-pushed on Jul 27, 2017

promag renamed this:
~~Prevent duplicate wallets~~
Reject invalid wallets
on Jul 27, 2017

promag commented at 0:27 am on July 27, 2017: member

@jnewbery tests added, thank you.

Splitted in two commits: the first tests for duplicate arguments while the second tests for invalid files (must be regular file and not a symlink).

in src/wallet/wallet.cpp:486 in fa11691d59 outdated

482         }
483 
484+        fs::path wallet_path = fs::absolute(walletFile, GetDataDir());
485+
486+        if (fs::exists(wallet_path) && (!fs::is_regular_file(wallet_path) || fs::is_symlink(wallet_path))) {
487+            return InitError(_("Invalid -wallet file"));

jnewbery commented at 6:50 am on July 27, 2017:

Suggestion: can you include the invalid wallet name in this error message? If the user has specified multiple wallets, then this error won’t indicate which one of them is invalid.

Same goes for the other error conditions.

promag commented at 7:04 am on July 27, 2017:

Other errors doesn’t specify the invalid argument.

jnewbery commented at 7:28 am on July 27, 2017:

right, because until now there’s only been a single wallet, so it’s clear what the problem is. With multiwallet, it’s helpful to know which wallet is the problem.

laanwj commented at 5:59 am on July 28, 2017:

Yes, please add the name here. @jnewbery has a good point that it matters when loading multiple wallets.

in test/functional/multiwallet.py:23 in fa11691d59 outdated

19@@ -18,6 +20,21 @@ def __init__(self):
20         self.extra_args = [['-wallet=w1', '-wallet=w2', '-wallet=w3']]
21 
22     def run_test(self):
23+        self.stop_node(0)

jnewbery commented at 6:59 am on July 27, 2017:

Note for other reviewers: It seemed slightly weird to start-stop the node at the beginning of this test. The reason we need to do that is that the w1 file doesn’t exist until the node has been started, and we can’t do the symlink test below without an existing wallet file.

micronit: I think it feels more natural to do these invalid tests at the end of this test script, and removes the confusing start-stop at the beginning of the test. That’s just personal taste though.

promag commented at 7:05 am on July 27, 2017:

I’m used to see the success tests at bottom. Can change if it’s better as you suggest.

jnewbery commented at 7:29 am on July 27, 2017:

sure, like I said just personal taste to remove the weird start-stop behaviour at the top. Doesn’t make any difference.

jnewbery commented at 6:59 am on July 27, 2017: member

Looking good. Just a couple of nits.

TheBlueMatt commented at 8:35 pm on July 27, 2017: member

utACK. Would be nice to print the wallet which generated the error as @jnewbery suggested, but ready for merge either way.

jnewbery commented at 0:19 am on July 28, 2017: member

Yep - as Matt says, ready for merge with or without the error message improvement. Can always be added later.

jnewbery commented at 10:05 am on July 28, 2017: member

Suggested change to error messages here: https://github.com/jnewbery/bitcoin/tree/pr10885 . Feel free to cherry-pick or squash in. Or else I can open a PR after this is merged.

promag commented at 10:18 am on July 28, 2017: member

Thanks @jnewbery, if you don’t mind I’ll squash.

EDIT: Actually since you fixed the other error messages I’ll leave it as is.

Reject duplicate wallet filenames 3ef77a0c12

Reject invalid wallet files a6da027d83

[wallet] Specify wallet name in wallet loading errors d84e78ec39

promag force-pushed on Jul 28, 2017

laanwj merged this on Jul 28, 2017

laanwj closed this on Jul 28, 2017

laanwj referenced this in commit 70888a39c4 on Jul 28, 2017

bittylicious commented at 8:21 am on September 15, 2017: none

I know it’s a bit late for this, but on upgrading to v0.15, this is a bit of a showstopper for me.

I have the wallet.dat file as a symlink to another partition. The reasons for this are not entirely crazy:

The other partition is block encrypted and I have encryption at rest.
The other partition is on a SSD which has great I/O, but is limited in space.

Checking for the wallet being a symlink stops me from being able to do this, and I can’t specify the full path on the -wallet command line argument as that only supports files in the datadir, not full paths.

I don’t think there’s a neat solution to my problem, but I’ve discussed a few on #bitcoin-dev anyway.

“You could put everything on the SSD” - it uses up too much space.
“You could use wallet encryption” - yes, but it can’t then be on the SSD. Being on the SSD is great as there’s quite a bit of I/O on the wallet and this speeds things up considerably.
“You could move the datadir to the SSD and symlink everything else out of there (especially blocks, chainstate and debug.log)” - yes, but yuck. That just feels a bit wrong, since I want to just move the one file.

I will live with option 3 if I need to, but was wondering if the symlink check is a bit of an overkill safety check here. Is it really likely the user will specify a -wallet argument for the real wallet, and another for a symlink to that real wallet?

sipa commented at 8:31 am on September 15, 2017: member

@bittylicious I understand this sounds like an unnecessary measure, but perhaps it is a bit more complicated than you realize. Effectively, the wallet is not just a single file, but a whole directory. Bitcoin Core will clean up all other files before shutdown, but in case of a crash, other files in the directory are needed for recovery. Symlinks may break these assumptions - making it look like a wallet file could be shared across datadirs, for example.

I think the correct approach for the issue you have is an ability to specify a wallet datadir, separate from the node’s datadir.

bittylicious commented at 8:42 am on September 15, 2017: none

@sipa Thanks for the response. I appreciate what you’re saying and there’s a reliance between the wallet and other files in the datadir.

I think what I’m saying is that if you’re messing around with symlinks, you’re probably technical enough to know what you’re doing, so having artificial restrictions like the symlink check probably won’t result in much real safety. Having to find a workaround in this case is probably more risky.

Re: wallet datadir, I’m not sure how that could work given dependencies between that and the regular datadir, or do you mean that it would essentially work as a flag to say “I really do know what I’m doing”?

sipa commented at 8:46 am on September 15, 2017: member

There are no dependencies between the wallet and the rest of the datadir, in either direction.

bittylicious commented at 8:55 am on September 15, 2017: none

Sorry, I meant wallet.dat dependencies (those that make up the full wallet during runtime).

bittylicious commented at 9:07 am on September 15, 2017: none

Having discussed this a bit more on #bitcoin-dev I see what you’re saying - the wallet should really be in its own directory as a BDB database expects this, hence the suggestion for a -walletdir argument.

PastaPastaPasta referenced this in commit 24e13ff803 on Aug 6, 2019

PastaPastaPasta referenced this in commit 9d810f53a7 on Aug 6, 2019

PastaPastaPasta referenced this in commit c1af924ed8 on Aug 6, 2019

PastaPastaPasta referenced this in commit f958aaa788 on Aug 7, 2019

PastaPastaPasta referenced this in commit 2477e21fe2 on Aug 8, 2019

PastaPastaPasta referenced this in commit ae488ba3da on Aug 12, 2019

barrystyle referenced this in commit 1a63cbd29b on Jan 22, 2020

random-zebra referenced this in commit 2d50b6e3b9 on Jun 9, 2021

DrahtBot locked this on Sep 8, 2021

Reject invalid wallets #10885