commit b5dceab1751dfa12b27b3042a49d90e0b02c2e0c Author: George Kadianakis desnacked@riseup.net Date: Sat Feb 9 18:46:10 2013 +0000

Fix various issues pointed out by Nick and Andrea.

- Document the key=value format. - Constify equal_sign_pos. - Pass some strings that are about to be logged to escape(). - Update documentation and fix some bugs in tor_escape_str_for_socks_arg(). - Use string_is_key_value() in parse_bridge_line(). - Parenthesize a forgotten #define - Add some more comments. - Add some more unit test cases. --- src/common/util.c | 27 +++++++++++++++------------ src/or/config.c | 11 ++++++----- src/or/connection.c | 7 ++++++- src/test/test_util.c | 14 ++++++++++++-- 4 files changed, 39 insertions(+), 20 deletions(-)

diff --git a/src/common/util.c b/src/common/util.c index b2f12bf..9aba7d6 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -865,30 +865,30 @@ tor_digest_is_zero(const char *digest) return tor_memeq(digest, ZERO_DIGEST, DIGEST_LEN); }

-/** Return true if <b>string</b> is a valid '<key>=<value>' string. +/** Return true if <b>string</b> is a valid '<key>=[<value>]' string. * <value> is optional, to indicate the empty string. */ int string_is_key_value(const char *string) { /* position of equal sign in string */ - char *equal_sign_pos = NULL; + const char *equal_sign_pos = NULL;

tor_assert(string);

- if (strlen(string) < 2) { /* "x=a" is shortest args string */ - log_warn(LD_GENERAL, "'%s' is too short to be a k=v value.", string); + if (strlen(string) < 2) { /* "x=" is shortest args string */ + log_warn(LD_GENERAL, "'%s' is too short to be a k=v value.", escaped(string)); return 0; }

equal_sign_pos = strchr(string, '='); if (!equal_sign_pos) { - log_warn(LD_GENERAL, "'%s' is not a k=v value.", string); + log_warn(LD_GENERAL, "'%s' is not a k=v value.", escaped(string)); return 0; }

/* validate that the '=' is not in the beginning of the string. */ if (equal_sign_pos == string) { - log_warn(LD_GENERAL, "'%s' is not a valid k=v value.", string); + log_warn(LD_GENERAL, "'%s' is not a valid k=v value.", escaped(string)); return 0; }

@@ -1279,9 +1279,10 @@ wrap_string(smartlist_t *out, const char *string, size_t width, } }

-/** Escape every character of <b>string</b> that belongs to the set of - * characters <b>set</b>. Use <b>escape_char</b> as the character to - * use for escaping. */ +/** Escape every ";" or "" character of <b>string</b>. Use + * <b>escape_char</b> as the character to use for escaping. + * The returned string is allocated on the heap and it's the + * responsibility of the caller to free it. */ char * tor_escape_str_for_socks_arg(const char *string) { @@ -1294,8 +1295,8 @@ tor_escape_str_for_socks_arg(const char *string)

length = strlen(string);

- if (!length) - return NULL; + if (!length) /* If we were given the empty string, return the same. */ + return tor_strdup(""); /* (new_length > SIZE_MAX) => ((length * 2) + 1 > SIZE_MAX) => (length*2 > SIZE_MAX - 1) => (length > (SIZE_MAX - 1)/2) */ if (length > (SIZE_MAX - 1)/2) /* check for overflow */ @@ -1304,7 +1305,7 @@ tor_escape_str_for_socks_arg(const char *string) /* this should be enough even if all characters must be escaped */ new_length = (length * 2) + 1;

- new_string = new_cp = tor_malloc_zero(new_length); + new_string = new_cp = tor_malloc(new_length);

while (*string) { if (strchr(chars_to_escape, *string)) @@ -1313,6 +1314,8 @@ tor_escape_str_for_socks_arg(const char *string) *new_cp++ = *string++; }

+ *new_cp = '\0'; /* NUL-terminate the new string */ + return new_string; }

diff --git a/src/or/config.c b/src/or/config.c index d057dd8..a09dda9 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -2875,14 +2875,14 @@ options_validate(or_options_t *old_options, or_options_t *options, size_t len;

len = strlen(options->Socks5ProxyUsername); - if (len < 1 || len > 255) + if (len < 1 || len > MAX_SOCKS5_AUTH_FIELD_SIZE) REJECT("Socks5ProxyUsername must be between 1 and 255 characters.");

if (!options->Socks5ProxyPassword) REJECT("Socks5ProxyPassword must be included with Socks5ProxyUsername.");

len = strlen(options->Socks5ProxyPassword); - if (len < 1 || len > 255) + if (len < 1 || len > MAX_SOCKS5_AUTH_FIELD_SIZE) REJECT("Socks5ProxyPassword must be between 1 and 255 characters."); } else if (options->Socks5ProxyPassword) REJECT("Socks5ProxyPassword must be included with Socks5ProxyUsername."); @@ -4120,11 +4120,12 @@ parse_bridge_line(const char *line, int validate_only) field = smartlist_get(items, 0); smartlist_del_keeporder(items, 0);

- /* If '=', it's a k=v value pair. */ - if (strchr(field, '=')) { + /* If it's a key=value pair, then it's a SOCKS argument for the + transport proxy... */ + if (string_is_key_value(field)) { socks_args = smartlist_new(); smartlist_add(socks_args, field); - } else { /* If no '=', it's the fingerprint. */ + } else { /* ...otherwise, it's the bridge fingerprint. */ fingerprint = field; }

diff --git a/src/or/connection.c b/src/or/connection.c index 6bac59b..b0fbe52 100644 --- a/src/or/connection.c +++ b/src/or/connection.c @@ -1585,7 +1585,7 @@ get_proxy_type(void) /* One byte for the version, one for the command, two for the port, and four for the addr... and, one more for the username NUL: */ -#define SOCKS4_STANDARD_BUFFER_SIZE 1 + 1 + 2 + 4 + 1 +#define SOCKS4_STANDARD_BUFFER_SIZE (1 + 1 + 2 + 4 + 1)

/** Write a proxy request of <b>type</b> (socks4, socks5, https) to conn * for conn->addr:conn->port, authenticating with the auth details given @@ -1688,6 +1688,9 @@ connection_proxy_connect(connection_t *conn, int type) memcpy(buf + 2, &portn, 2); /* port */ memcpy(buf + 4, &ip4addr, 4); /* addr */

+ /* Next packet field is the userid. If we have pluggable + transport SOCKS arguments, we have to embed them + there. Otherwise, we use an empty userid. */ if (socks_args_string) { /* place the SOCKS args string: */ tor_assert(strlen(socks_args_string) > 0); tor_assert(buf_size >= @@ -1951,6 +1954,8 @@ connection_read_proxy_handshake(connection_t *conn) break; }

+ /* Username and password lengths should have been checked + above and during torrc parsing. */ tor_assert(usize <= MAX_SOCKS5_AUTH_FIELD_SIZE && psize <= MAX_SOCKS5_AUTH_FIELD_SIZE); reqsize = 3 + usize + psize; diff --git a/src/test/test_util.c b/src/test/test_util.c index b41f235..a307a79 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -813,9 +813,11 @@ test_util_escape_string_socks(void) test_streq(escaped_string, "First rule: Do not use \;"); tor_free(escaped_string);

- /** Ilegal: Empty string. */ + /** Empty string. */ escaped_string = tor_escape_str_for_socks_arg(""); - test_assert(!escaped_string); + test_assert(escaped_string); + test_streq(escaped_string, ""); + tor_free(escaped_string);

/** Escape all characters. */ escaped_string = tor_escape_str_for_socks_arg(";\;\"); @@ -823,6 +825,11 @@ test_util_escape_string_socks(void) test_streq(escaped_string, "\;\\\;\\"); tor_free(escaped_string);

+ escaped_string = tor_escape_str_for_socks_arg(";"); + test_assert(escaped_string); + test_streq(escaped_string, "\;"); + tor_free(escaped_string); + done: tor_free(escaped_string); } @@ -834,7 +841,10 @@ test_util_string_is_key_value(void *ptr) test_assert(string_is_key_value("key=value")); test_assert(string_is_key_value("k=v")); test_assert(string_is_key_value("key=")); + test_assert(string_is_key_value("x=")); + test_assert(string_is_key_value("xx=")); test_assert(!string_is_key_value("=value")); + test_assert(!string_is_key_value("=x")); test_assert(!string_is_key_value("="));

/* ??? */