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

Error if rpcpassword contains hash in conf sections #15087

Merged
merged 1 commit into from Jan 9, 2019

Conversation

Projects
None yet
6 participants
@meshcollider
Copy link
Member

commented Jan 3, 2019

Fixes #15075

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 3, 2019

utACK

@promag

This comment has been minimized.

Copy link
Member

commented Jan 3, 2019

Tested ACK 170523f.

I prefer this version though:

diff --git a/src/util/system.cpp b/src/util/system.cpp
index ffe4d21ee..8da476289 100644
--- a/src/util/system.cpp
+++ b/src/util/system.cpp
@@ -863,12 +863,13 @@ static bool GetConfigOptions(std::istream& stream, std::string& error, std::vect
                 error = strprintf("parse error on line %i: %s, options in configuration file must be specified without leading -", linenr, str);
                 return false;
             } else if ((pos = str.find('=')) != std::string::npos) {
-                std::string name = prefix + TrimString(str.substr(0, pos), pattern);
-                std::string value = TrimString(str.substr(pos + 1), pattern);
-                if (used_hash && name.find("rpcpassword") != std::string::npos) {
+                std::string name = TrimString(str.substr(0, pos), pattern);
+                if (used_hash && name == "rpcpassword") {
                     error = strprintf("parse error on line %i, using # in rpcpassword can be ambiguous and should be avoided", linenr);
                     return false;
                 }
+                name = prefix + name;
+                std::string value = TrimString(str.substr(pos + 1), pattern);
                 options.emplace_back(name, value);
                 if ((pos = name.rfind('.')) != std::string::npos) {
                     sections.insert(name.substr(0, pos));

Since Unless regtest.rpcpassword=foo is a valid line?

The above suggestion wouldn't catch lines like regtest.rpcpassword=foo.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jan 3, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #14866 (Improve property evaluation way in bitcoin.conf by AkioNak)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 4, 2019

@promag huh, yes, I hadn't noticed that the prefix was added there, I tend to agree that's better than a fuzzy query like .find(), I didn't consider it important enough to remark as I thought it involved increasing the amount/complexity of string parsing, but that doesn't seem too bad.

Edit after IRC discussion: wrong, this would mean regtest.rpcpassword=foo is no longer detected, better stick to the current code.

@meshcollider meshcollider force-pushed the meshcollider:201901_pass_hash_fix branch to 8cff831 Jan 9, 2019

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

re-utACK 8cff831

@laanwj laanwj merged commit 8cff831 into bitcoin:master Jan 9, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jan 9, 2019

Merge #15087: Error if rpcpassword contains hash in conf sections
8cff831 Error if rpcpassword contains hash in conf sections (MeshCollider)

Pull request description:

  Fixes #15075

Tree-SHA512: 08ba2a2e9a7ea228fc0e0ff9aa76da1fecbe079e3b388304a28b6399e338a4b3a38b03ab03aca880e75f14a8d2ba75ceb31a385d7989cd66db5193a79f32c4e5
@jnewbery

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

tested ACK 8cff831

@promag

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

post merge ACK 8cff831.

@AkioNak

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2019

Sorry for my late commet.
Is "rrrpcpassworddd=foo#bar" an error?
I think that the unknown configuration file options will be ignored.
( as a compromise. - #13799)

@meshcollider meshcollider deleted the meshcollider:201901_pass_hash_fix branch Jan 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.