Restrict RPCs that make server-side files

[laanwj]

Currently dumpwallet (and other RPCs that create server-side files) can scribble all over the file system, at least as the user running bitcoind permits.

It would be better if these were at the least limited to the data directory, or even a specific directory within the data directory, say, ~/.bitcoin/dumpwallet—to avoid name collisions with wallets, lock files and database files. Overwriting is already prevented.

(Issue originally reported by Florian Mathieu)

[practicalswift]

Concept ACK

These "write all over the file system" RPCs are problematic also when fuzzing :)

FWIW these are the RPC commands I'm currently avoiding when fuzzing (I call them "NSFF": not safe for fuzz):

// RPC commands which are not appropriate for fuzzing: such as RPC commands
// reading or writing to a filename passed as an RPC parameter, RPC commands
// resulting in network activity, etc.
const std::vector<std::string> RPC_COMMANDS_NOT_SAFE_FOR_FUZZING{
    "addnode",              // avoid DNS lookups
    "addpeeraddress",       // avoid DNS lookups
    "analyzepsbt",          // avoid signed integer overflow in CFeeRate::GetFee(unsigned long)
    "decoderawtransaction", // avoid signed integer overflow in ValueFromAmount(long const&)
    "dumptxoutset",         // avoid writing to disk
    "dumpwallet",           // avoid writing to disk
    "generatetoaddress",    // avoid timeout
    "importwallet",         // avoid reading from disk
    "loadwallet",           // avoid reading from disk
    "mockscheduler",        // avoid assertion failure (Assertion `delta_seconds.count() > 0 && delta_seconds < std::chrono::hours{1}' failed.)
    "setban",               // avoid DNS lookups
    "stop",                 // avoid shutdown state
};

[ghost]

Concept ACK

[laanwj]

@practicalswift That's a good point, maybe the same point should apply to RPC calls that read from disk. Reading an arbitrary file can be a potential data leak.

    "importwallet",         // avoid reading from disk
    "loadwallet",           // avoid reading from disk

[jonatack]

Probably somewhat tangential, but same for wallet tool info that should not write to the wallet file, but does.

[theStack]

Concept ACK

[luke-jr]

Not so sure it makes sense to force backups into the origin filesystem like that, especially a hidden directory the user shouldn't need to be aware of.

Typical use would be to another physical storage medium.

[laanwj]

Typical use would be to another physical storage medium.

I don't disagree. But this doesn't warrant having something that can scribble all over the server file system. I think a good compromise would be a -backupdir option, which would default to inside the datadir but can be anywhere (even / if you want the current behavior).

IMO ideally there would be no RPCs that write to server-side storage at all. Any backups could be streamed over HTTP and backed up client side. That said, that would be a much larger API as well as code change.

@practicalswift btw backupwallet is missing from your list?

[luke-jr]

What if we require the wallet name to be in the filename?

[torkelrogstad]

I'm not sure if this is the incorrect place to comment on this, please LMK if I'm in the wrong place.

Isn't the best solution for the file-writing RPCs to return the file in the HTTP response, instead of writing to disk? This has two clear benefits:

1. We avoid writing to disk, which is what this issue is about.
2. It's up to the caller to determine how they want to handle the result. If you're running bitcoind in a containerized environment, it could be rather cumbersome to read a file from the bitcoind container, if the executor of the RPC is in a different container (potentially on a completely different host machine).

It seems like this approach would be an improvement in both security and usability.

[luke-jr]

Isn't the best solution for the file-writing RPCs to return the file in the HTTP response, instead of writing to disk?

Yes, but JSON-RPC is somewhat incompatible with this idea. For dumptxoutset, we could just say "use REST instead", but wallet features really need authentication and a trusted interface.

We could hex-encode the backup, but that's ugly and (in a simple implementation) uses more memory than it really should. A quick search doesn't turn up anything for "attachments" on JSON-RPC responses. :/

[torkelrogstad]

With the exception of dumptxoutset, it seems all the "problematic" RPCs can fit their data into a JSON-RPC response.

I just checked out two different wallets of mine: one with very few addresses and around a 1000 TXs, and another one with very few TXs but a boatload of addresses. They were both below 20MB in size. What's the size of a "really large" wallet?

I agree that neither hex- og base64-encoding is the prettiest solutions, but it doesn't seem too bad. It would also be up to the caller of these RPCs to manage their memory usage, limiting these RPCs (or opting for versions which write to disk) if they are in memory-constrained environments

[maflcko]

What's the size of a "really large" wallet?

mapWallet.size() > 1'000'000 exists in production.

[luke-jr]

I have a mainnet wallet that's 70 MB, and I don't even use it regularly.

[sipa]

How big of a JSON-RPC response can we realistically create?

[laanwj]

I would guess the only limiting factors are available memory and time. Where the latter one is not strict, as RPC timeouts are client-side and can be adjusted. Systems with a huge wallet likely already have lot of memory to support that in the first place.

The only time I really had to look into HTTP streaming (so delivering the data as it was generated without accumulating it into a buffer) was for making it possible to download the UTXO set (#7759). But this turned out to be impossible to do reliably with our execution model and libevent's single-threaded HTTP server.

(Something to keep in mind when using a JSON-RPC though, is that most clients are also going to waste a lot of memory parsing and storing the entire thing instead of downloading it to disk. This is necessary to get the error/status information that wraps it. For larger data it might be better to use HTTP directly so the result can be downloaded as a file.)

[luke-jr]

22541 was merged with no apparent consideration to this issue...?

[RealKeyboardWarrior]

In case someone is not convinced that this should be addressed, I've created a small demo that uses these RPC calls to get remote code execution. The likelihood of pulling off a successful attack using that exact technique is quite limited, but I think there should exist at least one resource detailing the potential risks hence I published it. https://realkeyboardwarrior.github.io/security/2021/10/06/pwning-bitcoind-to-rce.html

[luke-jr]

If you can run bitcoind, you presumably can execute other code too already.

[ghost]

but I think there should exist at least one resource detailing the potential risks hence I published it. https://realkeyboardwarrior.github.io/security/2021/10/06/pwning-bitcoind-to-rce.html

@RealKeyboardWarrior

Thanks for sharing the article link although few things can be mentioned in doc/JSON-RPC-interface.md#security

I had added few things about createwallet in https://github.com/bitcoin/bitcoin/pull/20741