In commit “args: Support -norpccookiefile” (cf3e4edc803bf950f280b24c5283a2838c86faf6)
The changes to httprpc.cpp seem too complicated and I don’t think they are safe because now that an empty strRPCUserColonPass string is allowed by RPCAuthorized, the TimingResistantEqual function can return true when the client provides an empty auth string.
I also think changing strRPCUserColonPass to std::optional makes state more complicated and code more confusing to think about, even as it weakens the “belt and suspenders” check it is trying to preserve by intializing the variable at the top of the InitRPCAuthentication function and leaving it set even when the function fails.
To fix these issue, I would leave strRPCUserColonPass as std::string instead of making it into a std::optional, leave the InitRPCAuthentication function unchanged, and just change the RPCAuthorized function as follows:
0--- a/src/httprpc.cpp
1+++ b/src/httprpc.cpp
2@@ -134,8 +134,6 @@ static bool multiUserAuthorized(std::string strUserPass)
3
4 static bool RPCAuthorized(const std::string& strAuth, std::string& strAuthUsernameOut)
5 {
6- if (strRPCUserColonPass.empty()) // Belt-and-suspenders measure if InitRPCAuthentication was not called
7- return false;
8 if (strAuth.substr(0, 6) != "Basic ")
9 return false;
10 std::string_view strUserPass64 = TrimStringView(std::string_view{strAuth}.substr(6));
11@@ -148,7 +146,8 @@ static bool RPCAuthorized(const std::string& strAuth, std::string& strAuthUserna
12 strAuthUsernameOut = strUserPass.substr(0, strUserPass.find(':'));
13
14 //Check if authorized under single-user field
15- if (TimingResistantEqual(strUserPass, strRPCUserColonPass)) {
16+ // strRPCUserColonPass is empty when -norpccookiefile is specified
17+ if (!strRPCUserColonPass.empty() && TimingResistantEqual(strUserPass, strRPCUserColonPass)) {
18 return true;
19 }
20 return multiUserAuthorized(strUserPass);
I don’t think it is worth complicating the code so much just to support keeping a belt and suspender check that is no longer valid now that strRPCUserColonPass being empty is a valid state.
