#36200 rpc: invalid credentials can occupy every HTTP worker
https://github.com/bitcoin/bitcoin/pull/36200 · · +36/-6 in 2 files, 1 commits · labels: RPC/REST/ZMQ · draft
Goal
- Prevent invalid RPC requests from occupying every worker thread and blocking legitimate users
Removes the 250 ms UninterruptibleSleep executed upon HTTP RPC authentication failure and adds a functional test ensuring valid RPCs are not delayed by failed authentications. The sleep was originally added to mitigate timing attacks and slow brute-forcing, but timing resistance is now handled by TimingResistantEqual.
Problem: Any client with network access to the node's RPC port can easily tie up all HTTP worker threads by submitting requests with invalid Basic auth credentials, causing denial of service for legitimate authenticated callers.
Category: RPC / REST / ZMQ (#29 of 52)
P3 · DoS protection
- P3 because it stops clients with RPC network access from easily exhausting HTTP worker threads
- Severity is moderate since attackers must already have permission to reach the RPC port
Addresses resource exhaustion in the HTTP server interface by removing a thread sleep that lets an admitted client tie up all HTTP workers. The severity is low because an attacker must already be permitted network access to the RPC listener, making this a worthwhile but deferrable DoS fix.
Membership: Modifies HTTP RPC request authentication handling in src/httprpc.cpp
Factors: security/stability 2, bug 0, performance 1, user value 0, leverage 0
Reviewability: Ready
- Ready for review
- Open design trade-offs around password guessing invite review input
The code is small, passes CI, and is ready for evaluation, with design trade-offs on password guessing inviting discussion.
Author status: active; evaluating design trade-offs and alternative approaches
Open concerns:
- Removing the sleep increases the rate at which an unauthorized client with network access can guess RPC passwords
- The author is considering alternatives such as rate-limiting, a dedicated slow-reply thread, or documenting external fail2ban configurations
Resolved concerns:
- PR description updated to acknowledge that removing the sleep increases the guess rate
Agreement: Strong
- Strong support because closing the easy DoS vector outweighs slower brute-forcing (willcl-ark)
- Author is exploring alternatives to mitigate increased password guess rates (hodlinator)
Strong support from willcl-ark to fix the DoS vector, with ongoing discussion on guess-rate trade-offs
Reviewer willcl-ark noted that the sleep helped deter brute-forcing but explicitly supported removing it because the DoS vector is cheaper and easier to exploit.
- willcl-ark noted the sleep also slows password guessing, but concluded: 'I think the DoS vector identified here is cheaper/easier to hit (and more annoying), so I'd be in favour of patching that up as you propose.'
- hodlinator moved to draft to explore trade-offs between DoS prevention and guess-rate limiting
Review verdicts (DrahtBot): 0
Files
36 lines under test/bench/ci.
- test/functional/interface_http.py +36/-0
- src/httprpc.cpp +0/-6
Card
This PR removes a 250 ms worker-thread sleep triggered on RPC authentication failure in src/httprpc.cpp to prevent clients with RPC network access from exhausting HTTP workers and causing a denial of service. While removing the sleep increases the potential password guessing rate, timing attacks are already addressed by TimingResistantEqual. willcl-ark favored patching the DoS vector over keeping the sleep, while discussion remains open on alternatives like external fail2ban documentation. The patch is small and ready for review on the security trade-off.