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

test: Assert RPC Server binds before creating cookie #26964

Closed
wants to merge 3 commits into from

Conversation

willcl-ark
Copy link
Member

If the RPC server does not bind first there exists a race condition between malware and bitcoind to bind to the port and recieve a cookie request from external application.

This test relies on the order of log messages, which may (I don't know) be slightly brittle. However because both InitHTTPServer() and StartHTTPRPC() are called in single-threaded series from within AppInitServers() it should work well enough.

bitcoin/src/init.cpp

Lines 667 to 672 in 50ac8f5

if (!InitHTTPServer())
return false;
StartRPC();
node.rpc_interruption_point = RpcInterruptionPoint;
if (!StartHTTPRPC(&node))
return false;

This allows specification that log entries should be detected in the
order they are passed in through their argument list.
If we don't bind before creating the authentication cookie a race
condition exists where malware could restart the node, bind to the RPC
port and read the current cookie before bitcoind binds.
@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 25, 2023

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

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

Conflicts

No conflicts as of last run.

@maflcko maflcko changed the title Assert RPC Server binds before creating cookie test: Assert RPC Server binds before creating cookie Jan 25, 2023
@DrahtBot DrahtBot added the Tests label Jan 25, 2023
@DrahtBot DrahtBot mentioned this pull request Jan 26, 2023
@achow101
Copy link
Member

The feature request didn't seem to attract much attention in the past. Also, the issue seems not important enough right now to keep it sitting around idle in the list of open issues.

Closing due to lack of interest. Pull requests with improvements are always welcome.

@achow101 achow101 closed this Apr 25, 2023
@luke-jr
Copy link
Member

luke-jr commented Jun 22, 2023

Is this related to CVE-2018-20587?

@achow101 This appears to be a security fix, not a feature request :|

Edit: This doesn't fix anything, just tests for correct behaviour.

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

Successfully merging this pull request may close these issues.

None yet

4 participants