Error if rpcpassword contains hash in conf sections #15087

pull meshcollider wants to merge 1 commits into bitcoin:master from meshcollider:201901_pass_hash_fix changing 2 files +9 −1

meshcollider commented at 11:16 AM on January 3, 2019: contributor

Fixes #15075
meshcollider added the label Utils/log/libs on Jan 3, 2019

in test/functional/feature_config_args.py:41 in 170523fdc5 outdated

  29 | @@ -30,8 +30,8 @@ def test_config_file_parser(self):
  30 |          self.nodes[0].assert_start_raises_init_error(expected_msg='Error reading configuration file: parse error on line 1: nono, if you intended to specify a negated option, use nono=1 instead')
  31 |  
  32 |          with open(inc_conf_file_path, 'w', encoding='utf-8') as conf:
  33 | -            conf.write('server=1\nrpcuser=someuser\nrpcpassword=some#pass')
  34 | -        self.nodes[0].assert_start_raises_init_error(expected_msg='Error reading configuration file: parse error on line 3, using # in rpcpassword can be ambiguous and should be avoided')
  35 | +            conf.write('server=1\nrpcuser=someuser\n[main]\nrpcpassword=some#pass')

laanwj commented at 12:04 PM on January 3, 2019:

maybe keep one outside the section too (to ensure that case is tested)

laanwj commented at 10:19 AM on January 4, 2019:

okay, it's clear we need to test all three, after the recent discussion it is very clear it's easy to miss one:

rpcpassword=   # bare
regtest.rpcpassword=   # explicit section

[regtest]
rpcpassword=   # implicit section

laanwj commented at 12:05 PM on January 3, 2019: member

utACK

promag commented at 2:53 PM on January 3, 2019: member

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 commented at 4:41 PM on January 3, 2019: member


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 commented at 9:59 AM on January 4, 2019: member

@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.
Error if rpcpassword contains hash in conf sections 8cff83124b
laanwj commented at 2:08 PM on January 9, 2019: member

re-utACK 8cff83124bcac936ecc6add6dca72b125a79a08f
laanwj merged this on Jan 9, 2019
laanwj closed this on Jan 9, 2019
laanwj referenced this in commit ff7f7364d6 on Jan 9, 2019
jnewbery commented at 3:02 PM on January 9, 2019: member

tested ACK 8cff83124bcac936ecc6add6dca72b125a79a08f
promag commented at 7:18 PM on January 9, 2019: member

post merge ACK 8cff831.
AkioNak commented at 10:37 AM on January 10, 2019: contributor

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 deleted the branch on Jan 10, 2019
deadalnix referenced this in commit cd52fd65a4 on Apr 17, 2020
ftrader referenced this in commit f0d5817d76 on Aug 17, 2020
PastaPastaPasta referenced this in commit 39db8fe25b on Sep 11, 2021
ogabrielides referenced this in commit ed9f7cb0ee on Sep 13, 2021
MarcoFalke locked this on Dec 16, 2021

Contributors

Labels

Linked (view graph)

#15075 Expected error not printed if rpcpassword contains hash character but is in a [section]

Error if rpcpassword contains hash in conf sections #15087

Conflicts