rpc: Prevent `dumpwallet` from overwriting files by laanwj · Pull Request #9937 · bitcoin/bitcoin

laanwj · 2017-03-07T08:53:00Z

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.

sipa · 2017-03-07T08:54:23Z

concept ACK

jonasschnelli · 2017-03-07T08:55:08Z

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

maflcko · 2017-03-07T09:01:03Z

utACK 1307cd79b859f01a35d68dea9a674e56451f75f5

paveljanik · 2017-03-07T09:10:19Z

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

gmaxwell · 2017-03-07T09:18:03Z

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 · 2017-03-07T09:24:16Z

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 · 2017-03-07T09:31:09Z

Updated the RPC help, too.

laanwj · 2017-03-07T09:39:53Z

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 · 2017-03-07T10:05:18Z

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

laanwj · 2017-03-07T10:14:23Z

Yeah that could work...

laanwj · 2017-03-07T10:14:48Z

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

laanwj · 2017-09-22T11:45:06Z

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.

promag · 2017-09-22T12:48:23Z

src/wallet/rpcdump.cpp

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.

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

Yes, the comment should have paranoia label..

sdaftuar · 2017-09-22T13:15:00Z

Concept ACK

promag · 2017-09-22T14:03:03Z

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.

jnewbery

Concept ACK, but currently fails to build :(

A few comments inline.

jnewbery · 2017-09-22T15:02:52Z

src/wallet/rpcdump.cpp

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)

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.

jnewbery · 2017-09-22T15:02:56Z

src/wallet/rpcdump.cpp

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

Meh, better too long than too short.

jnewbery · 2017-09-22T15:02:59Z

src/wallet/rpcdump.cpp

filename should be filepath?

Yes, that'd make sense.

laanwj · 2017-09-26T14:07:51Z

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.

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 bitcoin#9934. Adds mention to release notes and adds a test.

jnewbery · 2017-09-26T14:31:29Z

Tested ACK 0cd9273

jnewbery · 2017-09-26T14:59:10Z

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

laanwj · 2017-10-04T13:00:41Z

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.

0cd9273 rpc: Prevent `dumpwallet` from overwriting files (Wladimir J. van der Laan) Pull request description: 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. Tree-SHA512: 268c98636d40924d793b55a685a0b419bafd834ad369edaec08227ebe26ed4470ddea73008d1c4beb10ea445db1b0bb8e3546ba8fc2d1a411ebd4a0de8ce9120

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 bitcoin#9934. Adds mention to release notes and adds a test. Github-Pull: bitcoin#9937 Rebased-From: 0cd9273

maflcko · 2017-10-04T13:14:45Z

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

0cd9273 rpc: Prevent `dumpwallet` from overwriting files (Wladimir J. van der Laan) Pull request description: 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 bitcoin#9934. Adds mention to release notes and adds a test. Tree-SHA512: 268c98636d40924d793b55a685a0b419bafd834ad369edaec08227ebe26ed4470ddea73008d1c4beb10ea445db1b0bb8e3546ba8fc2d1a411ebd4a0de8ce9120

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

laanwj mentioned this pull request Mar 7, 2017

dumpwallet walletfile overwrite footgun #9934

Closed

laanwj added the Needs backport label Mar 7, 2017

laanwj added this to the 0.14.1 milestone Mar 7, 2017

laanwj closed this Mar 7, 2017

laanwj removed the Needs backport label Mar 7, 2017

laanwj removed this from the 0.14.1 milestone Mar 7, 2017

laanwj mentioned this pull request Mar 27, 2017

Add 'dumpbootstrap' RPC for creating bootstrap.dat files. #10081

Closed

laanwj mentioned this pull request Sep 22, 2017

Ensure backupwallet fails when attempting to backup to source file #11376

Merged

laanwj reopened this Sep 22, 2017

laanwj force-pushed the 2017_03_walletdump_nooverwrite branch from 47d8fc5 to cc928c5 Compare September 22, 2017 11:51

promag reviewed Sep 22, 2017

View reviewed changes

jnewbery reviewed Sep 22, 2017

View reviewed changes

rpc: Prevent dumpwallet from overwriting files

0cd9273

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 bitcoin#9934. Adds mention to release notes and adds a test.

laanwj force-pushed the 2017_03_walletdump_nooverwrite branch from cc928c5 to 0cd9273 Compare September 26, 2017 14:12

laanwj merged commit 0cd9273 into bitcoin:master Oct 4, 2017

maflcko added this to the 0.15.1 milestone Oct 4, 2017

promag mentioned this pull request Nov 30, 2017

dumpwallet command is not clear and can damage the wallet file #11798

Closed

promag mentioned this pull request Dec 6, 2018

RFC: rpc: Support time specifiers in dumpwallet filename #14887

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

laanwj commented Mar 7, 2017

Uh oh!

sipa commented Mar 7, 2017

Uh oh!

jonasschnelli commented Mar 7, 2017

Uh oh!

maflcko commented Mar 7, 2017

Uh oh!

paveljanik commented Mar 7, 2017

Uh oh!

gmaxwell commented Mar 7, 2017

Uh oh!

laanwj commented Mar 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Mar 7, 2017

Uh oh!

laanwj commented Mar 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paveljanik commented Mar 7, 2017

Uh oh!

laanwj commented Mar 7, 2017

Uh oh!

laanwj commented Mar 7, 2017

Uh oh!

laanwj commented Sep 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdaftuar commented Sep 22, 2017

Uh oh!

promag commented Sep 22, 2017

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj Sep 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Sep 26, 2017

Uh oh!

jnewbery commented Sep 26, 2017

Uh oh!

jnewbery commented Sep 26, 2017

Uh oh!

laanwj commented Oct 4, 2017

Uh oh!

maflcko commented Oct 4, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

laanwj commented Mar 7, 2017 •

edited

Loading

laanwj commented Mar 7, 2017 •

edited

Loading

laanwj commented Sep 22, 2017 •

edited

Loading

laanwj Sep 26, 2017 •

edited

Loading