Fix parse_short_policy (bug 7192.)

Our implementation of parse_short_policy was screwed up: it would
ignore the last character of every short policy.  Obviously, that's
broken.

This patch fixes the busted behavior, and adds a bunch of unit tests
to make sure the rest of that function is okay.

Fixes bug 7192; fix on 0.2.3.1-alpha.
This commit is contained in:
Nick Mathewson 2012-10-22 17:34:05 -04:00
parent 4c8b58f900
commit 85659d3964
4 changed files with 85 additions and 9 deletions

10
changes/bug7192 Normal file
View File

@ -0,0 +1,10 @@
o Major bugfixes:
- When parsing exit policy summaries from microdescriptors, we had
previously been ignoring the last character in each one, so that
"accept 80,443,8080" would be treated by clients as indicating a
node that allows access to ports 80, 443, and 808. That would lead
to clients attempting connections that could never work, and
ignoring exit nodes that would support their connections. Now clients
parse these exit policy summaries correctly. Fixes bug 7192;
bugfix on 0.2.3.1-alpha.

View File

@ -1348,8 +1348,10 @@ parse_short_policy(const char *summary)
unsigned low, high;
char dummy;
char ent_buf[32];
size_t len;
next = comma ? comma+1 : strchr(summary, '\0');
len = comma ? (size_t)(comma - summary) : strlen(summary);
if (n_entries == MAX_EXITPOLICY_SUMMARY_LEN) {
log_fn(LOG_PROTOCOL_WARN, LD_DIR, "Impossibly long policy summary %s",
@ -1357,20 +1359,22 @@ parse_short_policy(const char *summary)
return NULL;
}
if (! TOR_ISDIGIT(*summary) || next-summary > (int)(sizeof(ent_buf)-1)) {
if (! TOR_ISDIGIT(*summary) || len > (sizeof(ent_buf)-1)) {
/* unrecognized entry format. skip it. */
continue;
}
if (next-summary < 2) {
if (len < 1) {
/* empty; skip it. */
/* XXX This happens to be unreachable, since if len==0, then *summary is
* ',' or '\0', and the TOR_ISDIGIT test above would have failed. */
continue;
}
memcpy(ent_buf, summary, next-summary-1);
ent_buf[next-summary-1] = '\0';
memcpy(ent_buf, summary, len);
ent_buf[len] = '\0';
if (tor_sscanf(ent_buf, "%u-%u%c", &low, &high, &dummy) == 2) {
if (low<1 || low>65535 || high<1 || high>65535) {
if (low<1 || low>65535 || high<1 || high>65535 || low>high) {
log_fn(LOG_PROTOCOL_WARN, LD_DIR,
"Found bad entry in policy summary %s", escaped(orig_summary));
return NULL;
@ -1415,7 +1419,7 @@ parse_short_policy(const char *summary)
/** Write <b>policy</b> back out into a string. Used only for unit tests
* currently. */
const char *
char *
write_short_policy(const short_policy_t *policy)
{
int i;
@ -1424,7 +1428,7 @@ write_short_policy(const short_policy_t *policy)
smartlist_add_asprintf(sl, "%s", policy->is_accept ? "accept " : "reject ");
for(i=0; i < policy->n_entries; i++) {
for (i=0; i < policy->n_entries; i++) {
const short_policy_entry_t *e = &policy->entries[i];
if (e->min_port == e->max_port) {
smartlist_add_asprintf(sl, "%d", e->min_port);

View File

@ -61,7 +61,7 @@ void policies_free_all(void);
char *policy_summarize(smartlist_t *policy);
short_policy_t *parse_short_policy(const char *summary);
const char *write_short_policy(const short_policy_t *policy);
char *write_short_policy(const short_policy_t *policy);
void short_policy_free(short_policy_t *policy);
int short_policy_is_reject_star(const short_policy_t *policy);
addr_policy_result_t compare_tor_addr_to_short_policy(

View File

@ -1004,6 +1004,28 @@ test_circuit_timeout(void)
return;
}
/* Helper: assert that short_policy parses and writes back out as itself,
or as <b>expected</b> if that's provided. */
static void
test_short_policy_parse(const char *input,
const char *expected)
{
short_policy_t *short_policy = NULL;
char *out = NULL;
if (expected == NULL)
expected = input;
short_policy = parse_short_policy(input);
tt_assert(short_policy);
out = write_short_policy(short_policy);
tt_str_op(out, ==, expected);
done:
tor_free(out);
short_policy_free(short_policy);
}
/** Helper: Parse the exit policy string in <b>policy_str</b>, and make sure
* that policies_summarize() produces the string <b>expected_summary</b> from
* it. */
@ -1014,7 +1036,7 @@ test_policy_summary_helper(const char *policy_str,
config_line_t line;
smartlist_t *policy = smartlist_new();
char *summary = NULL;
const char *summary_after;
char *summary_after = NULL;
int r;
short_policy_t *short_policy = NULL;
@ -1231,6 +1253,46 @@ test_policies(void)
"accept *:*",
"reject 1,3,5,7");
/* Short policies with unrecognized formats should get accepted. */
test_short_policy_parse("accept fred,2,3-5", "accept 2,3-5");
test_short_policy_parse("accept 2,fred,3", "accept 2,3");
test_short_policy_parse("accept 2,fred,3,bob", "accept 2,3");
test_short_policy_parse("accept 2,-3,500-600", "accept 2,500-600");
/* Short policies with nil entries are accepted too. */
test_short_policy_parse("accept 1,,3", "accept 1,3");
test_short_policy_parse("accept 100-200,,", "accept 100-200");
test_short_policy_parse("reject ,1-10,,,,30-40", "reject 1-10,30-40");
/* Try parsing various broken short policies */
tt_ptr_op(NULL, ==, parse_short_policy("accept 200-199"));
tt_ptr_op(NULL, ==, parse_short_policy(""));
tt_ptr_op(NULL, ==, parse_short_policy("rejekt 1,2,3"));
tt_ptr_op(NULL, ==, parse_short_policy("reject "));
tt_ptr_op(NULL, ==, parse_short_policy("reject"));
tt_ptr_op(NULL, ==, parse_short_policy("rej"));
tt_ptr_op(NULL, ==, parse_short_policy("accept 2,3,100000"));
tt_ptr_op(NULL, ==, parse_short_policy("accept 2,3x,4"));
tt_ptr_op(NULL, ==, parse_short_policy("accept 2,3x,4"));
tt_ptr_op(NULL, ==, parse_short_policy("accept 2-"));
tt_ptr_op(NULL, ==, parse_short_policy("accept 2-x"));
tt_ptr_op(NULL, ==, parse_short_policy("accept 1-,3"));
tt_ptr_op(NULL, ==, parse_short_policy("accept 1-,3"));
/* Test a too-long policy. */
{
int i;
char *policy = NULL;
smartlist_t *chunks = smartlist_new();
smartlist_add(chunks, tor_strdup("accept "));
for (i=1; i<10000; ++i)
smartlist_add_asprintf(chunks, "%d,", i);
smartlist_add(chunks, tor_strdup("20000"));
policy = smartlist_join_strings(chunks, "", 0, NULL);
SMARTLIST_FOREACH(chunks, char *, ch, tor_free(ch));
smartlist_free(chunks);
tt_ptr_op(NULL, ==, parse_short_policy(policy));/* shouldn't be accepted */
tor_free(policy); /* could leak. */
}
/* truncation ports */
sm = smartlist_new();
for (i=1; i<2000; i+=2) {