fix: add support for CORS headers and pre-flight request

lionello commented at 2:51 pm on December 28, 2017: none

This PR implements basic Cross-Origin Resource Sharing (CORS) support to the RPC server, as per the spec at https://www.w3.org/TR/cors/#resource-requests . The spec has been quoted verbatim in the source code for easier validation and maintenance of the code.

added support for OPTIONS HTTP method
interpret CORS request headers for pre-flight requests
set CORS response headers
two test cases: standard CORS request, and pre-flight request

In practice this PR allows the REST interface to be used directly from a browser.

All the existing restrictions to the REST interface still apply: IP subnet, port, username, password. For this reason this PR doesn’t explicitly check the request’s Origin with a whitelist.

fanquake added the label RPC/REST/ZMQ on Dec 28, 2017

lionello force-pushed on Dec 29, 2017

TheBlueMatt commented at 4:41 pm on January 3, 2018: member

Hmm, I mean in general its probably a terrible idea to be calling bitcoind’s RPC from a browser directly…I would probably be happy to see this if we force the setting of an -alloworigin=$ORIGIN option (which implies IP whitelist as well) to force users to use it at least somewhat securely.

lionello commented at 1:19 am on January 5, 2018: none

There’s plenty of good reason to enable RPC from browser, but I agree we have to make it opt-in. Will add that to the patch.

gmaxwell commented at 2:01 am on January 8, 2018: contributor

Things like this are not sufficient to make access from browsers safe, because we must also be safe even if the user has old browsers with CORS bugs on their system. (e.g. IIRC old IE was trivially bypassable).

As a result there would have to be a lot of developer resources available to implement, review, and characterize the full set of protections needed to prevent that from being a gaping vulnerability– and a commensurate really good application for it.

lionello force-pushed on Feb 6, 2018

lionello commented at 4:27 am on February 6, 2018: none

@gmaxwell Done: I’ve added the flag -rpccorsdomain to match the existing options (and similar option in Ethereum’s Geth.)

I’ve also added tests for that flag.

As for your question: we have an internal website that we use to sign transactions with hardware wallets. We use RPC to send out the signed transactions. We’d prefer to do this without a service (which needs audits, security updates, etc..)

Sjors commented at 4:06 pm on February 20, 2018: member

Needs rebase. I’m generally a fan of correct CORS / CSP headers, will review later.

Sjors commented at 8:32 pm on February 20, 2018: member

POST works (assuming bitcoin:bitcoin credentials, running on port 8085):

0
1curl -X POST \
2  http://localhost:8085/ \
3  -H 'Authorization: Basic Yml0Y29pbjpiaXRjb2lu=' \
4  -H 'Cache-Control: no-cache' \
5  -H 'Content-Type: application/json' \
6  -d '{
7	"method": "getblockchaininfo"
8}'

But OPTIONS doesn’t:

0curl -X OPTIONS \
1  http://localhost:8085/ \
2  -H 'Authorization: Basic Yml0Y29pbjpiaXRjb2lu=' \
3  -H 'Cache-Control: no-cache' \
4  -H 'Content-Type: application/json'
5JSONRPC server handles only POST requests

That was using Postman which doesn’t set the origin header, but I get the same error from a browser for the OPTIONS request.

lionello commented at 0:22 am on February 21, 2018: none

@Sjors Just using OPTIONS doesn’t make it a valid CORS pre-flight request. Try this:

0curl -X OPTIONS \
1    http://localhost:8085/ \
2    -H 'Authorization: Basic Yml0Y29pbjpiaXRjb2lu=' \
3    -H 'Cache-Control: no-cache' \
4    -H 'Content-Type: application/json' \
5    -H 'Origin: null' \
6    -H 'access-control-request-method: POST' \
7    -d '{
8	"method": "getblockchaininfo"
9  }'

lionello force-pushed on Feb 21, 2018

Sjors commented at 9:35 am on February 22, 2018: member

Same error unfortunately.

Incorrect OPTIONS request should probably throw a unique error regardless.

I tried your full example, but since the JSON payload itself shouldn’t be needed in an OPTIONS request, I think it can be shortened a bit:

0curl -X OPTIONS \
1    http://localhost:8085/ \
2    -H 'Authorization: Basic Yml0Y29pbjpiaXRjb2lu=' \
3    -H 'Cache-Control: no-cache' \
4    -H 'Content-Type: application/json' \
5    -H 'Origin: null' \
6    -H 'access-control-request-method: POST'

lionello commented at 10:30 am on February 22, 2018: none

@Sjors Did you provide the -rpccorsdomain command line arg?

Sjors commented at 5:50 pm on February 22, 2018: member

Yes, I used -rpccorsdomain=http://localhost:8080. However I used the wrong port in my Origin header.

Better error messages would be very useful here, either through the browser or rpc log entries.

This worked:

0curl -v -X OPTIONS \
1    http://localhost:8085/ \
2    -H 'Origin: http://localhost:8080' \
3    -H 'Access-Control-Request-Method: POST' \
4    -H 'Access-Control-Request-Headers: authorization,content-type'

I added -v so you can see the returned headers. I also removed the Authorization header from the OPTIONS request in this example, because browsers won’t send credentials during such a request.

If anyone else wants to test a POST request:

0curl 'http://localhost:8085/' \
1   -H 'Authorization: Basic Yml0Y29pbjpiaXRjb2lu=' \
2   -H 'Origin: http://localhost:8080' \
3   -H 'Content-Type: application/json' \
4   -H 'Accept: application/json' \
5   --data-binary '{"method":"getblockchaininfo"}'

Nice to have: support for multiple -rpccorsdomain arguments.

Sjors commented at 6:12 pm on February 22, 2018: member

Any malicious website would need to trick the user into handing out RPC authentication details, as well as setting server=1 in bitcoin.conf.

For HTTP Basic Auth they need to trick the user into configuring rpcuser= and rpcpassword= or run the credentials script. Otherwise they need to make the user find and upload the cookie.

I think it would be particularly useful for data visualization. E.g. a mempool chart is much easier to build using client-side javascript that just fetches the data through ajax.

Although potentially very useful for hardware wallets, facilitating wallet access from a browser potentially opens a can of worms. For a non-wallet node I don’t see much downside.

Perhaps a compromise (for now) could be to only allow this flag when built with --disable-wallet or --seatbelts=0. A less strict rule could work too, but is likely more difficult to implement and maintain. @gmaxwell regarding old browsers, I think we should work under the assumption that CORS is broken and any request can be made. Afaik enabling CORS is just a way to allow modern browsers to use the RPC (they require the OPTIONS request to return the right data), not to prevent older browsers from doing that.

lionello commented at 8:50 am on April 18, 2018: none

@Sjors The default is still CORS disabled. Would you still want it to only be available with --disable-wallet or --seatbelts=0?

MarcoFalke commented at 6:40 pm on May 9, 2018: member

Needs rebase if still relevant

lionello force-pushed on May 10, 2018

lionello commented at 0:26 am on May 10, 2018: none

@MarcoFalke @Sjors Rebased.

MarcoFalke added the label Needs rebase on Jun 6, 2018

takahser commented at 5:12 pm on November 6, 2018: none

any news on that one? is it going to be merged anytime soon?

lionello force-pushed on Nov 7, 2018

fix: add support for CORS headers and pre-flight request eefdf1b685

lionello force-pushed on Nov 7, 2018

lionello commented at 1:41 am on November 7, 2018: none

Rebased, if anybody still cared.

This patch got merged in Bitcoin ABC which was what I cared about ;)

DrahtBot removed the label Needs rebase on Nov 7, 2018

takahser commented at 8:43 am on November 7, 2018: none

@lionello that was quick, thank you. I hope this will get merged soon, since I’d like to spin up some CORS-enabled bitcoind nodes.

ryanofsky commented at 4:11 pm on November 7, 2018: member

This seems like a nice change, and I’d really curious to know what some use-cases are. (Lacking web background, or enough of an imagination myself…)

Maybe something about this could also be added to the documentation https://github.com/bitcoin/bitcoin/blob/master/doc/REST-interface.md

takahser commented at 4:45 pm on November 7, 2018: none

@ryanofsky in my case, I’m considering to use in disablewallet mode as a bitcoin node, that is used by a web wallet, for fetching and sending transactions (transactions are signed on the client, so no private key’s are sent around the network). Ofc we can’t say for sure, who (which IP) will use the web wallet, but they have one thing in common: the URL of the webwallet. We can leverage this piece of information if the btc node is CORS-enabled.

in test/functional/interface_http.py:18 in 53e054aa1d outdated

14@@ -15,6 +15,7 @@ def set_test_params(self):
15         self.num_nodes = 3
16 
17     def setup_network(self):
18+        self.extra_args =[["-rpccorsdomain=null"], [], []]

practicalswift commented at 6:03 pm on November 7, 2018:

Nit: Missing space after = :-)

lionello commented at 0:58 am on November 8, 2018:

Weird, it was fixed in 20d3c18964dff877c2fbf7189819412289eaaab2 but still shows without the space.

lionello commented at 1:00 am on November 8, 2018:

My bad. Fixed in 210b8ee1fbf79cff98bf76d53b4f35bc9cdd02d2

lionello commented at 0:53 am on November 8, 2018: none

@ryanofsky The scenarios are having a website interact with the blockchain node directly, without the need for a backend: browser –rpc–> bitcoind. You can do transaction signing in the browser (JS libs, or HW device) and send transactions directly.

lionello force-pushed on Nov 8, 2018

fix: add -rpccorsdomain=value command line option 210b8ee1fb

lionello force-pushed on Nov 8, 2018

in src/httprpc.cpp:70 in 210b8ee1fb

65@@ -66,6 +66,8 @@ class HTTPRPCTimerInterface : public RPCTimerInterface
66 static std::string strRPCUserColonPass;
67 /* Stored RPC timer interface (for unregistration) */
68 static std::unique_ptr<HTTPRPCTimerInterface> httpRPCTimerInterface;
69+/* RPC CORS Domain, allowed Origin */
70+static std::string strRPCCORSDomain;

Sjors commented at 4:34 pm on November 10, 2018:

Let’s call this str_rest_cors_domain (new naming convention) and the parameter -restcorsdomain (since it’s only used with -rest=1).

lionello commented at 5:53 am on November 12, 2018:

The other options still use the old -rpc... naming though, so I’m not convinced this helps anyone. Even the variables declared right before strRPCCORSDomain use the old naming.

Sjors commented at 3:05 pm on November 20, 2018:

I find the use of -rpc confusing when a setting is about the REST interface. -rpcauth, -rpcuser and -rpcpassword is shared between the regular RPC and REST so there it makes sense (although I’m not a fan of that sharing). The rest is just internal variables.

sipa commented at 4:18 pm on November 20, 2018:

REST doesn’t have authentication, I thought?

Sjors commented at 4:57 pm on November 20, 2018:

@sipa it does, see my command-line example above. It has full wallet access too.

sipa commented at 5:11 pm on November 20, 2018:

@Sjors I’m very confused. The REST interface only exposes public information (blocks, transactions with txindex, headers, chaininfo, utxoset, mempool), and should never require authentication. Does this PR change that? Your example seems to just use the RPC interface.

Sjors approved

Sjors commented at 4:54 pm on November 10, 2018: member

tACK 210b8ee (with a preference for renaming the variables)

Recipe to test:

launch REST server at port 8080:

0src/bitcoind -server -rest=1 -rpcport=8080 -rpcuser=bitcoin -rpcpassword=bitcoin -rpccorsdomain=http://localhost:8080

pretend to be a browser visiting a website on port 8085 which calls the REST server:

0curl 'http://localhost:8080/'    -H 'Authorization: Basic Yml0Y29pbjpiaXRjb2lu='    -H 'Origin: http://localhost:8080'    -H 'Content-Type: application/json'    -H 'Accept: application/json'    --data-binary '{"method":"getblockchaininfo"}'

do some wallet operation on wallet “A”:

0curl 'http://localhost:8080/'    -H 'Authorization: Basic Yml0Y29pbjpiaXRjb2lu='    -H 'Origin: http://localhost:8080/wallet/A'    -H 'Content-Type: application/json'    -H 'Accept: application/json'    --data-binary '{"method":"getwalletinfo"}'

You can also dynamically load any wallet on the nodes filesystem, so indeed disablewallet=1 is probably a good idea.

There’s no way to configure this by accident and for good measure most users are behind a NAT. I think a warning in the release notes should suffice, i.e. to use disablewallet=1, to never expose a machine with a wallet to the internet, and not to share RPC credentials in general.

Longer term I think we should add some sort of user-permission system, which is also quite useful in a Lightning node context (e.g. a connected Lightning node should be able to get information, request an address to refund on, etc, but not steal coins off the node).

Sjors commented at 5:04 pm on November 10, 2018: member

Actually NAT doesn’t help, because an attacker could tell a user to run bitcoind -rpccorsdomain=http://evil.com/ and have them visit http://evil.com/ which would then empty the users wallet (https://evil.com might not work because some browsers refuse to XHR to http from an https site, but I’d have to test that).

Sjors commented at 6:54 pm on November 20, 2018: member

I missed how much RPC and REST are intertwined; it’s one and the same server, not a separate one as I assumed. So in the examples above I’m just making normal (authenticated) JSON-RPC requests. Let’s try that again:

0curl 'http://localhost:8080/rest/chaininfo.json' -H 'Origin: http://localhost:8080'    -H 'Content-Type: application/json'  -H 'Accept: application/json'

The REST interace doesn’t expose anything private.

So all you need to change is the only add CORS headers to /rest/*. In that case -restcorsdomain would still be a better param name I think.

Sjors commented at 7:33 pm on November 20, 2018: member

After brief IRC discussion today I’m switching to light Concept NACK.

The REST API wasn’t intended for this use case. It’s meant as a more performant alternative to the JSON-RPC (using bin and hex formats). It’s not battle hardened so should only be called by semi trusted software locally. It’s not safe against DoS.

I think it’s better to recommend users who really want this to run nginx and have that add the headers, as well as shield URLs that are not part of /rest/, only whitelist URLs their application really needs, etc. That seems both safer and to prevent scope creep.

DrahtBot commented at 4:47 pm on May 13, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

fanquake commented at 9:28 am on June 17, 2019: member

Going to close this. A couple of (implied?) NACKs, and a reversal from a utACK to NACK, no further discussion for 8 months.

fanquake closed this on Jun 17, 2019

DrahtBot locked this on Dec 16, 2021

fix: add support for CORS headers and pre-flight request #12040

Conflicts