From b93cedafe455b95a3c4e0387bce4cf294c615068 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Tue, 11 Apr 2017 18:30:42 +1200 Subject: [PATCH 1/6] torcontrol: Handle escapes in Tor QuotedStrings https://trac.torproject.org/projects/tor/ticket/14999 is tracking an encoding bug with the Tor control protocol, where many of the QuotedString instances that Tor outputs are in fact CStrings, but it is not documented which ones are which. https://spec.torproject.org/control-spec section 2.1.1 provides a future-proofed rule for handing QuotedStrings, which this commit implements. --- src/test/torcontrol_tests.cpp | 30 +++++++++++++++++----- src/torcontrol.cpp | 48 ++++++++++++++++++++++++++++++++--- 2 files changed, 68 insertions(+), 10 deletions(-) diff --git a/src/test/torcontrol_tests.cpp b/src/test/torcontrol_tests.cpp index 68516599d..8d3bb8f77 100644 --- a/src/test/torcontrol_tests.cpp +++ b/src/test/torcontrol_tests.cpp @@ -119,27 +119,43 @@ BOOST_AUTO_TEST_CASE(util_ParseTorReplyMapping) {"Foo", "Bar Baz"}, }); - // Escapes (which are left escaped by the parser) + // Escapes CheckParseTorReplyMapping( "Foo=\"Bar\\ Baz\"", { - {"Foo", "Bar\\ Baz"}, + {"Foo", "Bar Baz"}, }); CheckParseTorReplyMapping( "Foo=\"Bar\\Baz\"", { - {"Foo", "Bar\\Baz"}, + {"Foo", "BarBaz"}, }); CheckParseTorReplyMapping( "Foo=\"Bar\\@Baz\"", { - {"Foo", "Bar\\@Baz"}, + {"Foo", "Bar@Baz"}, }); CheckParseTorReplyMapping( "Foo=\"Bar\\\"Baz\" Spam=\"\\\"Eggs\\\"\"", { - {"Foo", "Bar\\\"Baz"}, - {"Spam", "\\\"Eggs\\\""}, + {"Foo", "Bar\"Baz"}, + {"Spam", "\"Eggs\""}, }); CheckParseTorReplyMapping( "Foo=\"Bar\\\\Baz\"", { - {"Foo", "Bar\\\\Baz"}, + {"Foo", "Bar\\Baz"}, + }); + + // C escapes + CheckParseTorReplyMapping( + "Foo=\"Bar\\nBaz\\t\" Spam=\"\\rEggs\" Octals=\"\\1a\\11\\17\\18\\81\\377\\378\\400\" Final=Check", { + {"Foo", "Bar\nBaz\t"}, + {"Spam", "\rEggs"}, + {"Octals", "\1a\11\17\1" "881\377\37" "8400"}, + {"Final", "Check"}, + }); + CheckParseTorReplyMapping( + "Valid=Mapping Bare=\"Escape\\\"", {}); + CheckParseTorReplyMapping( + "OneOctal=\"OneEnd\\1\" TwoOctal=\"TwoEnd\\11\"", { + {"OneOctal", "OneEnd\1"}, + {"TwoOctal", "TwoEnd\11"}, }); // A more complex valid grammar. PROTOCOLINFO accepts a VersionLine that diff --git a/src/torcontrol.cpp b/src/torcontrol.cpp index 60181b440..b2f229cba 100644 --- a/src/torcontrol.cpp +++ b/src/torcontrol.cpp @@ -292,10 +292,52 @@ static std::map ParseTorReplyMapping(const std::string if (ptr == s.size()) // unexpected end of line return std::map(); ++ptr; // skip closing '"' - /* TODO: unescape value - according to the spec this depends on the - * context, some strings use C-LogPrintf style escape codes, some - * don't. So may be better handled at the call site. + /** + * Escape value. Per https://spec.torproject.org/control-spec section 2.1.1: + * + * For future-proofing, controller implementors MAY use the following + * rules to be compatible with buggy Tor implementations and with + * future ones that implement the spec as intended: + * + * Read \n \t \r and \0 ... \377 as C escapes. + * Treat a backslash followed by any other character as that character. */ + std::string escaped_value; + for (size_t i = 0; i < value.size(); ++i) { + if (value[i] == '\\') { + // This will always be valid, because if the final character + // in the QuotedString was a \ then the parser would already + // have returned above, due to a missing terminating + // double-quote. + ++i; + if (value[i] == 'n') { + escaped_value.push_back('\n'); + } else if (value[i] == 't') { + escaped_value.push_back('\t'); + } else if (value[i] == 'r') { + escaped_value.push_back('\r'); + } else if ('0' <= value[i] && value[i] <= '7') { + size_t j; + // Octal escape sequences have a limit of three octal digits, + // but terminate at the first character that is not a valid + // octal digit if encountered sooner. + for (j = 1; '0' <= value[i+j] && value[i+j] <= '7' && j < 3; ++j) {} + // Tor restricts first digit to 0-3 for three-digit octals. + if (j < 3 || ('0' <= value[i] && value[i] <= '3')) { + escaped_value.push_back(strtol(value.substr(i, j).c_str(), NULL, 8)); + // Account for automatic incrementing at loop end + i += j - 1; + } else { + escaped_value.push_back(value[i]); + } + } else { + escaped_value.push_back(value[i]); + } + } else { + escaped_value.push_back(value[i]); + } + } + value = escaped_value; } else { // Unquoted value. Note that values can contain '=' at will, just no spaces while (ptr < s.size() && s[ptr] != ' ') { value.push_back(s[ptr]); From 519713d32e3b16f996a43bf9f7401d41a404323b Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Tue, 11 Apr 2017 18:31:02 +1200 Subject: [PATCH 2/6] torcontrol: Add missing copyright header --- src/torcontrol.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/torcontrol.cpp b/src/torcontrol.cpp index b2f229cba..d43a8823e 100644 --- a/src/torcontrol.cpp +++ b/src/torcontrol.cpp @@ -1,3 +1,8 @@ +// Copyright (c) 2015-2017 The Bitcoin Core developers +// Copyright (c) 2017 The Zcash developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + #include "torcontrol.h" #include "utilstrencodings.h" #include "net.h" From 0b431fbdb507f5bfb34bba8014963c23856f2b71 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Wed, 19 Apr 2017 15:23:28 +1200 Subject: [PATCH 3/6] Address Daira's comments --- src/test/torcontrol_tests.cpp | 5 +++++ src/torcontrol.cpp | 13 +++++++------ 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/test/torcontrol_tests.cpp b/src/test/torcontrol_tests.cpp index 8d3bb8f77..608991250 100644 --- a/src/test/torcontrol_tests.cpp +++ b/src/test/torcontrol_tests.cpp @@ -150,6 +150,11 @@ BOOST_AUTO_TEST_CASE(util_ParseTorReplyMapping) {"Octals", "\1a\11\17\1" "881\377\37" "8400"}, {"Final", "Check"}, }); + CheckParseTorReplyMapping( + "Valid=Mapping Escaped=\"Escape\\\\\"", { + {"Valid", "Mapping"}, + {"Escaped", "Escape\\"}, + }); CheckParseTorReplyMapping( "Valid=Mapping Bare=\"Escape\\\"", {}); CheckParseTorReplyMapping( diff --git a/src/torcontrol.cpp b/src/torcontrol.cpp index d43a8823e..4ca6f8571 100644 --- a/src/torcontrol.cpp +++ b/src/torcontrol.cpp @@ -290,7 +290,8 @@ static std::map ParseTorReplyMapping(const std::string ++ptr; // skip opening '"' bool escape_next = false; while (ptr < s.size() && (escape_next || s[ptr] != '"')) { - escape_next = (s[ptr] == '\\'); + // Repeated backslashes must be interpreted as pairs + escape_next = (s[ptr] == '\\' && !escape_next); value.push_back(s[ptr]); ++ptr; } @@ -298,7 +299,7 @@ static std::map ParseTorReplyMapping(const std::string return std::map(); ++ptr; // skip closing '"' /** - * Escape value. Per https://spec.torproject.org/control-spec section 2.1.1: + * Unescape value. Per https://spec.torproject.org/control-spec section 2.1.1: * * For future-proofing, controller implementors MAY use the following * rules to be compatible with buggy Tor implementations and with @@ -310,10 +311,10 @@ static std::map ParseTorReplyMapping(const std::string std::string escaped_value; for (size_t i = 0; i < value.size(); ++i) { if (value[i] == '\\') { - // This will always be valid, because if the final character - // in the QuotedString was a \ then the parser would already - // have returned above, due to a missing terminating - // double-quote. + // This will always be valid, because if the QuotedString + // ended in an odd number of backslashes, then the parser + // would already have returned above, due to a missing + // terminating double-quote. ++i; if (value[i] == 'n') { escaped_value.push_back('\n'); From d15cab21bc7218f3812bdbf5714be77205b4bd2a Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Fri, 21 Apr 2017 13:15:39 +1200 Subject: [PATCH 4/6] Address Daira's further comments --- src/test/torcontrol_tests.cpp | 10 ++++++++++ src/torcontrol.cpp | 2 +- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/test/torcontrol_tests.cpp b/src/test/torcontrol_tests.cpp index 608991250..2a4457721 100644 --- a/src/test/torcontrol_tests.cpp +++ b/src/test/torcontrol_tests.cpp @@ -163,6 +163,16 @@ BOOST_AUTO_TEST_CASE(util_ParseTorReplyMapping) {"TwoOctal", "TwoEnd\11"}, }); + // Special handling for null case + // (needed because string comparison reads the null as end-of-string) + BOOST_TEST_MESSAGE(std::string("CheckParseTorReplyMapping(Null=\"\\0\")")); + auto ret = ParseTorReplyMapping("Null=\"\\0\""); + BOOST_CHECK_EQUAL(ret.size(), 1); + auto r_it = ret.begin(); + BOOST_CHECK_EQUAL(r_it->first, "Null"); + BOOST_CHECK_EQUAL(r_it->second.size(), 1); + BOOST_CHECK_EQUAL(r_it->second[0], '\0'); + // A more complex valid grammar. PROTOCOLINFO accepts a VersionLine that // takes a key=value pair followed by an OptArguments, making this valid. // Because an OptArguments contains no semantic data, there is no point in diff --git a/src/torcontrol.cpp b/src/torcontrol.cpp index 4ca6f8571..446d4402a 100644 --- a/src/torcontrol.cpp +++ b/src/torcontrol.cpp @@ -327,7 +327,7 @@ static std::map ParseTorReplyMapping(const std::string // Octal escape sequences have a limit of three octal digits, // but terminate at the first character that is not a valid // octal digit if encountered sooner. - for (j = 1; '0' <= value[i+j] && value[i+j] <= '7' && j < 3; ++j) {} + for (j = 1; j < 3 && (i+j) < value.size() && '0' <= value[i+j] && value[i+j] <= '7'; ++j) {} // Tor restricts first digit to 0-3 for three-digit octals. if (j < 3 || ('0' <= value[i] && value[i] <= '3')) { escaped_value.push_back(strtol(value.substr(i, j).c_str(), NULL, 8)); From 8df5fd1116b93c08614c05790d5c3d905ed64b82 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Fri, 21 Apr 2017 13:22:51 +1200 Subject: [PATCH 5/6] Correctly handle three-digit octals with leading digit 4-7 --- src/test/torcontrol_tests.cpp | 2 +- src/torcontrol.cpp | 13 +++++++------ 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/test/torcontrol_tests.cpp b/src/test/torcontrol_tests.cpp index 2a4457721..5624c818c 100644 --- a/src/test/torcontrol_tests.cpp +++ b/src/test/torcontrol_tests.cpp @@ -147,7 +147,7 @@ BOOST_AUTO_TEST_CASE(util_ParseTorReplyMapping) "Foo=\"Bar\\nBaz\\t\" Spam=\"\\rEggs\" Octals=\"\\1a\\11\\17\\18\\81\\377\\378\\400\" Final=Check", { {"Foo", "Bar\nBaz\t"}, {"Spam", "\rEggs"}, - {"Octals", "\1a\11\17\1" "881\377\37" "8400"}, + {"Octals", "\1a\11\17\1" "881\377\37" "8\40" "0"}, {"Final", "Check"}, }); CheckParseTorReplyMapping( diff --git a/src/torcontrol.cpp b/src/torcontrol.cpp index 446d4402a..a5cd1b7c6 100644 --- a/src/torcontrol.cpp +++ b/src/torcontrol.cpp @@ -329,13 +329,14 @@ static std::map ParseTorReplyMapping(const std::string // octal digit if encountered sooner. for (j = 1; j < 3 && (i+j) < value.size() && '0' <= value[i+j] && value[i+j] <= '7'; ++j) {} // Tor restricts first digit to 0-3 for three-digit octals. - if (j < 3 || ('0' <= value[i] && value[i] <= '3')) { - escaped_value.push_back(strtol(value.substr(i, j).c_str(), NULL, 8)); - // Account for automatic incrementing at loop end - i += j - 1; - } else { - escaped_value.push_back(value[i]); + // A leading digit of 4-7 would therefore be interpreted as + // a two-digit octal. + if (j == 3 && value[i] > '3') { + j--; } + escaped_value.push_back(strtol(value.substr(i, j).c_str(), NULL, 8)); + // Account for automatic incrementing at loop end + i += j - 1; } else { escaped_value.push_back(value[i]); } From 409606118b547c729691b9caa504287766096fc2 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Fri, 21 Apr 2017 13:23:29 +1200 Subject: [PATCH 6/6] Check that >3-digit octals are truncated. --- src/test/torcontrol_tests.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/torcontrol_tests.cpp b/src/test/torcontrol_tests.cpp index 5624c818c..b7affaacd 100644 --- a/src/test/torcontrol_tests.cpp +++ b/src/test/torcontrol_tests.cpp @@ -144,10 +144,10 @@ BOOST_AUTO_TEST_CASE(util_ParseTorReplyMapping) // C escapes CheckParseTorReplyMapping( - "Foo=\"Bar\\nBaz\\t\" Spam=\"\\rEggs\" Octals=\"\\1a\\11\\17\\18\\81\\377\\378\\400\" Final=Check", { + "Foo=\"Bar\\nBaz\\t\" Spam=\"\\rEggs\" Octals=\"\\1a\\11\\17\\18\\81\\377\\378\\400\\2222\" Final=Check", { {"Foo", "Bar\nBaz\t"}, {"Spam", "\rEggs"}, - {"Octals", "\1a\11\17\1" "881\377\37" "8\40" "0"}, + {"Octals", "\1a\11\17\1" "881\377\37" "8\40" "0\222" "2"}, {"Final", "Check"}, }); CheckParseTorReplyMapping(