Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restrict RPCs that make server-side files #20866

Open
laanwj opened this issue Jan 6, 2021 · 19 comments
Open

Restrict RPCs that make server-side files #20866

laanwj opened this issue Jan 6, 2021 · 19 comments

Comments

@laanwj
Copy link
Member

laanwj commented Jan 6, 2021

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
Copy link
Contributor

practicalswift commented Jan 10, 2021

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
Copy link

ghost commented Jan 11, 2021

Concept ACK

@laanwj
Copy link
Member Author

laanwj commented Jan 20, 2021

@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

@laanwj laanwj added this to the 22.0 milestone Jan 20, 2021
@jonatack
Copy link
Contributor

jonatack commented Jan 20, 2021

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

@theStack
Copy link
Contributor

Concept ACK

@luke-jr
Copy link
Member

luke-jr commented Jan 25, 2021

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
Copy link
Member Author

laanwj commented Jan 25, 2021

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?

@laanwj laanwj removed this from the 22.0 milestone Jun 14, 2021
@luke-jr
Copy link
Member

luke-jr commented Jun 25, 2021

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

@torkelrogstad
Copy link
Contributor

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
Copy link
Member

luke-jr commented Jun 29, 2021

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
Copy link
Contributor

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

@MarcoFalke
Copy link
Member

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

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

@luke-jr
Copy link
Member

luke-jr commented Jun 30, 2021

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

@sipa
Copy link
Member

sipa commented Aug 4, 2021

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

@laanwj
Copy link
Member Author

laanwj commented Aug 5, 2021

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
Copy link
Member

luke-jr commented Sep 12, 2021

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

@RealKeyboardWarrior
Copy link

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
Copy link
Member

luke-jr commented Nov 27, 2021

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

@ghost
Copy link

ghost commented Nov 27, 2021

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 #20741

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

11 participants