Validate user input and keep path in a separate argument for importwallet, createwallet and dumpwallet

[ghost]

Is your feature request related to a problem? Please describe.

Wallets with weird names possible which can be exploited in vulnerable web applications that use Bitcoin Core and allow the users to create and import/export wallet

../testwallet for Linux
..\testwallet for Windows

Tried on Bitcoin Core v 0.20.0

#20080 (comment)

Describe the solution you'd like

• Keep two arguments:
  wallet_name and wallet_path for createwallet
filename and filepath for dumpwallet importwallet

• Validate user input. Name should not contain special characters. Path should be optional and if not specified use a default value.

Describe alternatives you've considered
Web developers should create secure web apps that use Bitcoin Core.

Additional context

I understand its not a vulnerability in Bitcoin Core and only affects vulnerable web apps that use Bitcoin Core. However we can at least consider it a bug that may affect something in future or other projects that use Bitcoin Core. Basic checks for user input can improve the security. In 2017 I had a website in which someone had reported a vulnerability that could be used to change price of flight tickets and book with almost zero bitcoin. It was an issue with the website and we had to fix it although nobody could exploit it because the third party APIs that we were using to book the fight tickets were validating all the things. So a ticket couldn't be processed after tampering and changing the price by attacker. Similarly, if any web developer makes a mistake and using Bitcoin Core for the web app would still be unaffected if Bitcoin Core itself doesn't allow such things for wallet names.

There have been lot of directory traversal related vulnerabilities, recently one was reported in Facebook android app:

https://portswigger.net/daily-swig/vulnerability-in-facebook-android-app-nets-10k-bug-bounty

[maflcko]

I think it has previously been suggested to only allow absolute paths

[ghost]

    std::__cxx11::regex invalid_chars("(/|\\.)(.*)");

    if (regex_match(request.params[0].get_str(), invalid_chars)) {
        throw JSONRPCError(RPC_WALLET_ERROR, "Invalid characters in wallet name");
    }

How about this? And can someone help me understand what all files need to be changed for this if statement to work?

rpcwallet.cpp and wallet.cpp ?

[promag]

Not sure what needs to be validated, if it's a valid file path then it should be fine.

[ghost]

Not sure what needs to be validated, if it's a valid file path then it should be fine.

I think we can just validate if the wallet name string has any characters in the beginning that can be used to do more than just creating a wallet in an expected directory. I am thinking to include few more characters in it like % mentioned here: https://owasp.org/www-community/attacks/Path_Traversal

Trying to stop the user to do anything in the blue area on left when he uses the web application.

I have never tried to fix things in past, only break lol so not really sure if this thing may affect web apps using bitcoin core but its always good to restrict the user by validating inputs. Other thing which I understood after reading everything is that users should not have privileges to access other directories. This is something we can't control so just basic checks in place. If validating the wallet name string for 3-4 special chars in the beginning breaks anything we can ignore this issue and PR. I will do more research this week and add everything in the PR with tests.

[promag]

when he uses the web application

Clearly you could validate user input on the web app. You could even assign a "safe" wallet name for the desired wallet name.

[ghost]

Sure. That's why I mentioned 4 things in the issue description earlier:

1. Describe alternatives you've considered
   Web developers should create secure web apps that use Bitcoin Core.

2. can be exploited in vulnerable web applications that use Bitcoin Core

3. It was an issue with the website and we had to fix it although nobody could exploit it because the third party APIs that we were using to book the fight tickets were validating all the things. So a ticket couldn't be processed after tampering and changing the price by attacker. Similarly, if any web developer makes a mistake and using Bitcoin Core for the web app would still be unaffected if Bitcoin Core itself doesn't allow such things for wallet names.

4. not a vulnerability in Bitcoin Core and only affects vulnerable web apps that use Bitcoin Core

[ryanofsky]

I can't figure out what the issue is here. The problem is with a web app allows users to specify bitcoin wallet names? Like the web app has some code like:

if (wallet_name == "big wallet") {
   throw error("Can't spend from big wallet!")
}

but the user can get around this by spending from "./big wallet" instead of "big wallet"?

[ghost]

I can't figure out what the issue is here.

@ryanofsky

Alice is trying to test few websites and apps that use bitcoin core. Red boxes on right represent vulnerable apps and Blue boxes are secure. Grey box is bitcoin core. We are trying to prevent Alice to exploit any of those small boxes (bitcoin projects using bitcoin core) on right by making bitcoin core more secure and not depend on devs involved in different bitcoin projects.

[sipa]

But what is the vulnerability you're trying to protect against?

[ghost]

Path/Directory Traversal with the help of createwallet's wallet_name argument

Example: https://medium.com/tenable-techblog/android-mx-player-path-traversal-to-code-execution-9134b623eb34