rpc: Prevent `dumpwallet` from overwriting files

laanwj commented at 8:53 am on March 7, 2017: member

Prevent arbitrary files from being overwritten by dumpwallet. There have been reports that users have overwritten wallet files this way. It may also avoid other security issues.

Fixes #9934. Adds mention to release notes and adds a test.

laanwj added the label RPC/REST/ZMQ on Mar 7, 2017

sipa commented at 8:54 am on March 7, 2017: member

concept ACK

jonasschnelli commented at 8:55 am on March 7, 2017: contributor

Important fix. utACK 1307cd79b859f01a35d68dea9a674e56451f75f5. IMO this is a backport candidate.

laanwj added the label Needs backport on Mar 7, 2017

laanwj added this to the milestone 0.14.1 on Mar 7, 2017

MarcoFalke commented at 9:01 am on March 7, 2017: member

utACK 1307cd79b859f01a35d68dea9a674e56451f75f5

paveljanik commented at 9:10 am on March 7, 2017: contributor

The change as written now allows to test the existence of any file.

gmaxwell commented at 9:18 am on March 7, 2017: contributor

only downside I see to this is an in automated backup that leaves files behind, your backup will stop and you might not notice. (I think that is a preferable failure mode to what we have now)

laanwj commented at 9:24 am on March 7, 2017: member

The change as written now allows to test the existence of any file.

That seems preferable to overwriting any file, right? I tend to agree that being able to write arbitrary non-existing files is still a security risk, but it is tons better than what we have now.

Do you have a suggestion that would avoid this usage but still offers the same functionality?

only downside I see to this is an in automated backup that leaves files behind, your backup will stop and you might not notice. (I think that is a preferable failure mode to what we have now)

Suggestion: simply append a counter, or a date, or uniq-id. This is safer, too.

Note that it throws an exception when the attempt fails, so if you don’t notice you’re not handling errors at all, which is dangerous for a ton of other reasons too.

laanwj commented at 9:31 am on March 7, 2017: member

Updated the RPC help, too.

laanwj commented at 9:39 am on March 7, 2017: member

Do you have a suggestion that would avoid this usage but still offers the same functionality?

We could make the check even stricter, only allowing writing of files within the data directory, or even better a dumpwallet directory within the data directory. It could even come up with its own unique filename and return that (e.g. #9740). But that has a much larger impact to the API. Could do that for 0.15 but certainly not for backporting.

paveljanik commented at 10:05 am on March 7, 2017: contributor

What about this: if there is a file already present, add a timestamp to the file name and save.

laanwj commented at 10:14 am on March 7, 2017: member

Yeah that could work…

laanwj commented at 10:14 am on March 7, 2017: member

Closing this, feel free to implement a different solution, have no time for this at the moment.

laanwj closed this on Mar 7, 2017

laanwj removed the label Needs backport on Mar 7, 2017

laanwj removed this from the milestone 0.14.1 on Mar 7, 2017

MarcoFalke referenced this in commit 10e020a7f9 on Mar 27, 2017

laanwj commented at 11:45 am on September 22, 2017: member

Reopening this and rebasing, I think this is basically a good fix, and was closed for unimportant concerns (compared to the problem it solves), and no one picked it up subsequently.

laanwj reopened this on Sep 22, 2017

laanwj force-pushed on Sep 22, 2017

in src/wallet/rpcdump.cpp:627 in cc928c5837 outdated

623+    /* Prevent arbitrary files from being overwritten. There have been reports
624+     * that users have overwritten wallet files this way:
625+     * https://github.com/bitcoin/bitcoin/issues/9934
626+     * It may also avoid other security issues.
627+     */
628+    if (boost::filesystem::exists(filepath)) {

promag commented at 12:48 pm on September 22, 2017:

What if a dumpwallet is executed after this check with the same filename, or the filename is created by other means? IMO we should use fopen with wx mode where x stands for:

File access mode flag “x” can optionally be appended to “w” or “w+” specifiers. This flag forces the function to fail if the file exists, instead of overwriting it. (C11)

and drop exists() test.

laanwj commented at 2:09 pm on September 26, 2017:

I’m not terribly worried about race conditions here, just about overwriting existing files such as the wallet.

promag commented at 2:26 pm on September 26, 2017:

Yes, the comment should have paranoia label..

sdaftuar commented at 1:15 pm on September 22, 2017: member

Concept ACK

promag commented at 2:03 pm on September 22, 2017: member

And there is also the option to make the filename optional, where the default value is /some/base/path/ + wallet-name + some-timestamp + .extension and return the filename.

in src/wallet/rpcdump.cpp:605 in cc928c5837 outdated

599@@ -600,7 +600,7 @@ UniValue dumpwallet(const JSONRPCRequest& request)
600     if (request.fHelp || request.params.size() != 1)
601         throw std::runtime_error(
602             "dumpwallet \"filename\"\n"
603-            "\nDumps all wallet keys in a human-readable format.\n"
604+            "\nDumps all wallet keys in a human-readable format to a server-side file. This does not allow overwriting existing files.\n"
605             "\nArguments:\n"
606             "1. \"filename\"    (string, required) The filename with path (either absolute or relative to bitcoind)\n"

jnewbery commented at 3:02 pm on September 22, 2017:

This help text is misleading. The relative path is relative to the directory from where bitcoind was started.

(really I think we should disallow relative paths entirely unless they’re relative to some fixed point like the datadir)

laanwj commented at 2:07 pm on September 26, 2017:

I agree relative paths should ideally be relative to the datadir (or refused altogether - at the least we should be consistent about them). However I am not changing that functionality here, and am also not touching this comment here.

in src/wallet/rpcdump.cpp:622 in cc928c5837 outdated

615@@ -616,9 +616,19 @@ UniValue dumpwallet(const JSONRPCRequest& request)
616 
617     EnsureWalletIsUnlocked(pwallet);
618 
619-    std::ofstream file;
620     boost::filesystem::path filepath = request.params[0].get_str();
621     filepath = boost::filesystem::absolute(filepath);
622+
623+    /* Prevent arbitrary files from being overwritten. There have been reports

jnewbery commented at 3:02 pm on September 22, 2017:

nit: I don’t think this needs such a long comment. Just say we don’t allow files to be overwritten for safety.

laanwj commented at 2:08 pm on September 26, 2017:

Meh, better too long than too short.

in src/wallet/rpcdump.cpp:628 in cc928c5837 outdated

624+     * that users have overwritten wallet files this way:
625+     * https://github.com/bitcoin/bitcoin/issues/9934
626+     * It may also avoid other security issues.
627+     */
628+    if (boost::filesystem::exists(filepath)) {
629+        throw JSONRPCError(RPC_INVALID_PARAMETER, filename.string() + " already exists. If you are sure this is what you want, move it out of the way first");

jnewbery commented at 3:02 pm on September 22, 2017:

filename should be filepath?

laanwj commented at 2:09 pm on September 26, 2017:

Yes, that’d make sense.

jnewbery commented at 3:04 pm on September 22, 2017: member

Concept ACK, but currently fails to build :(

A few comments inline.

laanwj commented at 2:07 pm on September 26, 2017: member

And there is also the option to make the filename optional, where the default value is /some/base/path/ + wallet-name + some-timestamp + .extension and return the filename.

Yes, many other things are possible. I’m not going to do that here. That’s the reason I closed this before. However I only want to solve the overwrite problem here, which is not solved by making up filenames, only by refusing to overwrite files.

rpc: Prevent `dumpwallet` from overwriting files

Prevent arbitrary files from being overwritten. There have been reports
that users have overwritten wallet files this way. It may also avoid
other security issues.

Fixes #9934. Adds mention to release notes and adds a test.

0cd9273fd9

laanwj force-pushed on Sep 26, 2017

jnewbery commented at 2:31 pm on September 26, 2017: member

Tested ACK 0cd9273fd959c6742574259d026039f7da0309a2

jnewbery commented at 2:59 pm on September 26, 2017: member

Requires release notes (although this is fixing up undefined behaviour, there may be users who currently rely on that behaviour)

laanwj commented at 1:00 pm on October 4, 2017: member

Requires release notes (although this is fixing up undefined behaviour, there may be users who currently rely on that behaviour)

A release notes change is already part of the PR.

laanwj merged this on Oct 4, 2017

laanwj closed this on Oct 4, 2017

laanwj referenced this in commit 7f11ef2608 on Oct 4, 2017

MarcoFalke referenced this in commit a43be5bcdb on Oct 4, 2017

MarcoFalke commented at 1:14 pm on October 4, 2017: member

@laanwj I also pushed this to the backports pull for 0.15.1

MarcoFalke added this to the milestone 0.15.1 on Oct 4, 2017

codablock referenced this in commit 9e3cb7599e on Sep 25, 2019

barrystyle referenced this in commit 788faa875a on Jan 22, 2020

DrahtBot locked this on Sep 8, 2021

rpc: Prevent dumpwallet from overwriting files #9937

rpc: Prevent `dumpwallet` from overwriting files #9937