The Tor password was sent as a QuotedString with only double quotes escaped; as Tor interprets backslash escapes inside it, any password containing a backslash was mangled (or left the string unterminated).
The Tor Control spec deliberately allows the token to be sent as unquoted hex "to avoid escaping issues", as the SAFECOOKIE response already is. This patch does the same for the password, and also adds a functional test covering backslashes and quotes.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | winterrdog |
| Stale ACK | vasild |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
i think the motivation here is good since hex-encoding removes the need for escaping entirely. concept ACK
also, the TOR control spec explicitly allows this:
The authentication token can be specified as either a quoted ASCII string, or as an unquoted hexadecimal encoding of that same string (to avoid escaping issues).
re: #36637#issue-6008559379
as Tor interprets backslash escapes inside it, any password containing a backslash was mangled (or left the string unterminated).
#36374 is already underway to fix this specific issue
the two PRs solve the same bug in different ways, so whichever approach reviewers prefer may make the other redundant. this one avoids escaping altogether, while #36374 handles it explicitly. i find this PR's approach more desirable
118 | self.restart_node(0, extra_args=[
119 | f"-torcontrol=127.0.0.1:{mock_tor.port}",
120 | "-listenonion=1",
121 | "-debug=tor",
122 | - ])
123 | + ] + (extra_args or []))
nit:
- def restart_with_mock(self, mock_tor, extra_args=None):
+ def restart_with_mock(self, mock_tor, extra_args=[]):
mock_tor.start()
self.restart_node(0, extra_args=[
f"-torcontrol=127.0.0.1:{mock_tor.port}",
"-listenonion=1",
"-debug=tor",
- ] + (extra_args or []))
+ ] + extra_args)
Using extra_args=[] would cause this to fail lint_py_lint in CI, though, yes?
ACK 87ada856584cec489e6589147c5dca065cc9c5d0
Here is the relevant Tor documentation: https://spec.torproject.org/control-spec/commands.html?highlight=hashedpa#authenticate
I find it strange that the auth method is called HASHED PASSWORD but the password is sent in clear text 🤔
<details> <summary>Show Signature</summary>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK 87ada856584cec489e6589147c5dca065cc9c5d0
Here is the relevant Tor documentation:
https://spec.torproject.org/control-spec/commands.html?highlight=hashedpa#authenticate
I find it strange that the auth method is called _HASHED_ PASSWORD but the password is sent in clear text 🤔
-----BEGIN PGP SIGNATURE-----
iQRPBAEBCAA5FiEE5k2NRWFNsHVF2czBVN8G9ktVy78FAmrct7QbFIAAAAAABAAO
bWFudTIsMi41KzEuMTIsMiwzAAoJEFTfBvZLVcu/Dbwf+wT5dRztwyrUVVPaNAXU
bEvgYtJEdvKWZh7eQVlVKc7HAdxgTmDl/QuGWvEPTNOp0tV84pI2TpI5jjAB7Zcc
Jt9I0BpJ4QbyDgU+ik/kG0kJ5vXnrpKjBDyqhxwKFRsv6zJwmpc2p34T7c7tWUjx
MLk1ZWgg9NCsIv0pFqx19RHDZtv6FHwVZXn0MQld//fw23sRFE14/1+G3GhJ/ANg
IYFr0qBC+fuiqvbcVHhrAkUmteav1/3o7JtOJt20VAAB/OE6gFU+ouySQtf1D8Yu
3xUuvo3RjLmoVdjckXT71h20lhJmZXbXT8W1SrvfpjhRtQsz4eTAhwNLYOyuGEPw
X3xT2oHLIqqovVJINOna7n7XuVqTFnpTj2MsZ9rRWwONAoeLye2p9ROpayf9XiCF
Z/ceNmGDx+3xu8BT3R5dqdueEYpwWSeRps+96e51CuT6X3YeOSv0IkstqUg/PAhf
uqh6Rlw1VzL/Srtd0OQOL8VGZIErXllI77GRdGyC+0SNOGm706S6ReLeehHJzkuN
BRYklEVrabzbCw8SPNxrOF3ZMkaY9AyVRS5ZfTYz3qWkwzuEA+ySxMXICalh7WiC
g7nPyIFYNrGHgcSNCZ6nt0RGhEttzSQRqDlx2kPU1BfH3Xj2Ki3J+V55SLOnTQBG
vsixvOuFAIt3oG1d1YaKrhi/r7Mon/0M5d2rx5QrLRJA+je+zKJUpo3A4/0VhzwS
v32e++oyAUlM6P6nJkY02GmBUFUxuV43/ZlCSD9GJ3zhwocqTtTe/iL5tagK+8pm
YUrAxSfu8tkMVG4LyLHm4GFyEIqnzaf4K6C+dXRBEZoNjLXiI+6yMuu1OrnmAJ5k
UajcibE6PNgP5h2RWMyIwX7KnsyqU9YsrTzDQiuL1s1H0a40x4v2usQvPJdYUk7n
MLXw0hfmt6Gqyz+ERpu+UT12MWyPdXyvxLehacqBe9G0hjU+a5iW0KTYcfXFJ51q
E8QJVxXUp2Fwc7ivGxq0xHv252C6IzkajZeUrsdXHzdnQEaxCGDp4HCpYfvl7BfL
bIOaDdiB0GlAmEKlUIuRSJmmEa8EZaWHuwhB2OIm4fTA0Hxu9Fm6DloQCXB9YrKx
If2uNXVaPiwoky+u3CMtBXaMou9sYa5/ycKwDH6DYnNQY/muUK5BFLvdU6Qa45cx
t72IL+wu3zZkQIE95hEc4SnK7L6GOO5ffX0rHuHpc8JKfnJGohYkXFveBcSwuUfb
sxBNAI78pV6xWMplNqwv+o8Rvt77U+YW8mF5sihVUKrJGXzsKtHvXLLCjFMYa+Kq
NEb1rU4uGkQEHYfemz93260rBwiT33Z4kZnKNSeRXkGCWNjqO2CvKER4VIs0YfYX
IyA=
=4C4C
-----END PGP SIGNATURE-----
vasild's public key is on openpgp.org
</details>
<details><summary>ralph llm nit</summary>
Update the password-authentication example (src/torcontrol.cpp:667, head)
The local summary still documents the password as a quoted token, but this command sends its hex encoding. Update the example or remove it so the authentication summary matches the wire format.
</details>
211 | + 212 | + # Authentication succeeded, so the node goes on to create the onion service 213 | + self.wait_until(lambda: any(c.startswith("ADD_ONION ") for c in mock_tor.received_commands), timeout=10) 214 | + 215 | + # Clean up 216 | + mock_tor.stop()
the mock could explicitly check the received hex and reply with a 515 Bad authentication if it does not match, rather than accepting any AUTHENTICATE
<details><summary>diff</summary>
diff --git a/test/functional/feature_torcontrol.py b/test/functional/feature_torcontrol.py
index 064bc87848..568d3ef685 100755
--- a/test/functional/feature_torcontrol.py
+++ b/test/functional/feature_torcontrol.py
@@ -190,6 +190,10 @@ class TorControlTest(BitcoinTestFramework):
self.log.info("Test that -torpassword is sent hex-encoded, so special characters survive")
class HashedPasswordServer(MockTorControlServer):
+ def __init__(self, port, expected_password):
+ self.expected_hex = expected_password.encode().hex()
+ super().__init__(port)
+
def _get_response(self, command):
if command == "PROTOCOLINFO 1":
return (
@@ -199,20 +203,22 @@ class TorControlTest(BitcoinTestFramework):
"250 OK\r\n"
)
elif command.startswith("AUTHENTICATE "):
+ sent_hex = command.removeprefix("AUTHENTICATE ")
+ if sent_hex != self.expected_hex:
+ return "515 Bad authentication\r\n"
return "250 OK\r\n"
return super()._get_response(command)
password = 'pa\\ss"word\\'
- mock_tor = HashedPasswordServer(self.next_port())
+ mock_tor = HashedPasswordServer(self.next_port(), password)
self.restart_with_mock(mock_tor, extra_args=[f"-torpassword={password}"])
self.wait_until(lambda: len(mock_tor.received_commands) >= 2, timeout=10)
- assert_equal(mock_tor.received_commands[1], f"AUTHENTICATE {password.encode().hex()}")
+ assert_equal(mock_tor.received_commands[1], f"AUTHENTICATE {mock_tor.expected_hex}")
# Authentication succeeded, so the node goes on to create the onion service
self.wait_until(lambda: any(c.startswith("ADD_ONION ") for c in mock_tor.received_commands), timeout=10)
- # Clean up
mock_tor.stop()
def test_pow_fallback(self):
</details>
#36637 (review) is worth exploring too :)
tested ACK a1903e3abe515be94e0f345671d9dc67f4c72a6a
successfully built and tested on: Windows 10/MSVC 19.51.36257/x86_64