February 2021 - tor-commits - lists.torproject.org

[tor/maint-0.4.5] config: Do not compare for duplicate ORPorts with different addresses
by dgoulet＠torproject.org 12 Feb '21

12 Feb '21

commit dfcb050bbf424b6e72acb1bccd2dd99e8c96cb8c Author: David Goulet <dgoulet(a)torproject.org> Date: Thu Feb 11 16:14:56 2021 -0500 config: Do not compare for duplicate ORPorts with different addresses We were just looking at the family which is not correct because it is possible to have two explicit ORPort for the same family but different addresses. One example is: ORPort 127.0.0.1:9001 NoAdvertise ORPort 1.2.3.4:9001 NoListen Thus, this patch now ignores ports that have different addresses iff they are both explicits. That is, if we have this example, also two different addresses: ORPort 9001 ORPort 127.0.0.1:9001 NoAdvertise The first one is implicit and second one is explicit and thus we have to consider them for removal which in this case would remove the "ORPort 9001" in favor of the second port. Fixes #40289 Signe-off-by: David Goulet <dgoulet(a)torproject.org> --- changes/ticket40289 | 6 ++++ src/feature/relay/relay_config.c | 69 ++++++++++++++++++++++++++++++++++++---- 2 files changed, 69 insertions(+), 6 deletions(-) diff --git a/changes/ticket40289 b/changes/ticket40289 new file mode 100644 index 0000000000..cdb36825b0 --- /dev/null +++ b/changes/ticket40289 @@ -0,0 +1,6 @@ + o Minor bugfixes (relay, config): + - Fix a problem in the removal of duplicate ORPort from the internal port + list when loading config file. We were removing wrong ports breaking valid + torrc uses cases for multiple ORPorts of the same address family. Fixes + bug 40289; bugfix on 0.4.5.1-alpha. + diff --git a/src/feature/relay/relay_config.c b/src/feature/relay/relay_config.c index e007a3bd0d..c4a5d7f572 100644 --- a/src/feature/relay/relay_config.c +++ b/src/feature/relay/relay_config.c @@ -188,6 +188,41 @@ describe_relay_port(const port_cfg_t *port) return buf; } +/** Return true iff port p1 is equal to p2. + * + * This does a field by field comparaison. */ +static bool +port_cfg_eq(const port_cfg_t *p1, const port_cfg_t *p2) +{ + bool ret = true; + + tor_assert(p1); + tor_assert(p2); + + /* Address, port and type. */ + ret &= tor_addr_eq(&p1->addr, &p2->addr); + ret &= (p1->port == p2->port); + ret &= (p1->type == p2->type); + + /* Mode. */ + ret &= (p1->is_unix_addr == p2->is_unix_addr); + ret &= (p1->is_group_writable == p2->is_group_writable); + ret &= (p1->is_world_writable == p2->is_world_writable); + ret &= (p1->relax_dirmode_check == p2->relax_dirmode_check); + ret &= (p1->explicit_addr == p2->explicit_addr); + + /* Entry config flags. */ + ret &= tor_memeq(&p1->entry_cfg, &p2->entry_cfg, + sizeof(entry_port_cfg_t)); + /* Server config flags. */ + ret &= tor_memeq(&p1->server_cfg, &p2->server_cfg, + sizeof(server_port_cfg_t)); + /* Unix address path if any. */ + ret &= !strcmp(p1->unix_addr, p2->unix_addr); + + return ret; +} + /** Attempt to find duplicate ORPort that would be superseded by another and * remove them from the given ports list. This is possible if we have for * instance: @@ -241,20 +276,42 @@ remove_duplicate_orports(smartlist_t *ports) if (next->type != CONN_TYPE_OR_LISTENER) { continue; } + /* Remove duplicates. */ + if (port_cfg_eq(current, next)) { + removing[j] = true; + continue; + } /* Don't compare addresses of different family. */ if (tor_addr_family(&current->addr) != tor_addr_family(&next->addr)) { continue; } + /* At this point, we have a port of the same type and same address + * family. Now, we want to avoid comparing addresses that are different + * but are both explicit. As an example, these are not duplicates: + * + * ORPort 127.0.0.:9001 NoAdvertise + * ORPort 1.2.3.4:9001 NoListen + * + * Any implicit address must be considered for removal since an explicit + * one will always supersedes it. */ + if (!tor_addr_eq(&current->addr, &next->addr) && + current->explicit_addr && next->explicit_addr) { + continue; + } - /* Same port, we keep the explicit one. */ + /* Port value is the same so we either have a duplicate or a port that + * supersedes another. */ if (current->port == next->port) { - removing[j] = true; + /* Do not remove the explicit address. As stated before above, we keep + * explicit addresses which supersedes implicit ones. */ if (!current->explicit_addr && next->explicit_addr) { - char *next_str = tor_strdup(describe_relay_port(next)); - log_warn(LD_CONFIG, "Configuration port %s superseded by %s", - describe_relay_port(current), next_str); - tor_free(next_str); + continue; } + removing[j] = true; + char *next_str = tor_strdup(describe_relay_port(next)); + log_warn(LD_CONFIG, "Configuration port %s superseded by %s", + next_str, describe_relay_port(current)); + tor_free(next_str); } } }

1 0

[tor/master] config: Set flag for implicit port address
by dgoulet＠torproject.org 12 Feb '21

12 Feb '21

commit 80b33ae1ca148753558afa7bc8b43ba3df6d7948 Author: David Goulet <dgoulet(a)torproject.org> Date: Thu Feb 11 16:12:59 2021 -0500 config: Set flag for implicit port address Fun bug where we thought we were using the default "false" value when an implicit address was detected but if we had an explicit address before, the flag was set to true and then we would only use that value. And thus, for some configurations, implicit addresses would be flagged as explicit and then configuring ports goes bad. Related to #40289 Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/app/config/config.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/app/config/config.c b/src/app/config/config.c index c7799ec1a2..fa74907b3d 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -6034,6 +6034,7 @@ port_parse_config(smartlist_t *out, port = (int) tor_parse_long(addrport, 10, 0, 65535, &ok, NULL); if (ok) { tor_addr_copy(&addr, &default_addr); + addr_is_explicit = false; } else if (tor_addr_port_lookup(addrport, &addr, &ptmp) == 0) { if (ptmp == 0) { log_warn(LD_CONFIG, "%sPort line has address but no port", portname);

1 0

[tor/maint-0.4.5] config: Set flag for implicit port address
by dgoulet＠torproject.org 12 Feb '21

12 Feb '21

commit 80b33ae1ca148753558afa7bc8b43ba3df6d7948 Author: David Goulet <dgoulet(a)torproject.org> Date: Thu Feb 11 16:12:59 2021 -0500 config: Set flag for implicit port address Fun bug where we thought we were using the default "false" value when an implicit address was detected but if we had an explicit address before, the flag was set to true and then we would only use that value. And thus, for some configurations, implicit addresses would be flagged as explicit and then configuring ports goes bad. Related to #40289 Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/app/config/config.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/app/config/config.c b/src/app/config/config.c index c7799ec1a2..fa74907b3d 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -6034,6 +6034,7 @@ port_parse_config(smartlist_t *out, port = (int) tor_parse_long(addrport, 10, 0, 65535, &ok, NULL); if (ok) { tor_addr_copy(&addr, &default_addr); + addr_is_explicit = false; } else if (tor_addr_port_lookup(addrport, &addr, &ptmp) == 0) { if (ptmp == 0) { log_warn(LD_CONFIG, "%sPort line has address but no port", portname);

1 0

[tor/maint-0.4.5] test: Fix duplicate ORPort test
by dgoulet＠torproject.org 12 Feb '21

12 Feb '21

commit d47e937a5074c8c9799e1d8bbd6645eea74b86ab Author: David Goulet <dgoulet(a)torproject.org> Date: Thu Feb 11 16:32:17 2021 -0500 test: Fix duplicate ORPort test The comment of that specific unit test wanted 4 ORPorts but for some reasons we tested for 3 which before the previous commit related to #40289, test would pass but it was in fact wrong. Now the code is correct and 4 was in fact correct expected number of ports. Related to #40289 Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/test/test_config.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/test/test_config.c b/src/test/test_config.c index 4eb4ac9cf5..cd7a54b97a 100644 --- a/src/test/test_config.c +++ b/src/test/test_config.c @@ -6907,12 +6907,14 @@ test_config_duplicate_orports(void *arg) /* We have four address here, 1 IPv4 on 9050, IPv6 on 9050, IPv6 on 9051 and * a different IPv6 on 9051. */ - tt_int_op(smartlist_len(ports), OP_EQ, 3); + tt_int_op(smartlist_len(ports), OP_EQ, 4); tt_str_op(describe_relay_port(smartlist_get(ports, 0)), OP_EQ, "ORPort 9050"); tt_str_op(describe_relay_port(smartlist_get(ports, 1)), OP_EQ, "ORPort [4242::1]:9051"); tt_str_op(describe_relay_port(smartlist_get(ports, 2)), OP_EQ, + "ORPort [4242::2]:9051"); + tt_str_op(describe_relay_port(smartlist_get(ports, 3)), OP_EQ, "ORPort 9050"); /* Reset. Test different ORPort value. */

1 0

[tor/master] config: Do not compare for duplicate ORPorts with different addresses
by dgoulet＠torproject.org 12 Feb '21

12 Feb '21

commit dfcb050bbf424b6e72acb1bccd2dd99e8c96cb8c Author: David Goulet <dgoulet(a)torproject.org> Date: Thu Feb 11 16:14:56 2021 -0500 config: Do not compare for duplicate ORPorts with different addresses We were just looking at the family which is not correct because it is possible to have two explicit ORPort for the same family but different addresses. One example is: ORPort 127.0.0.1:9001 NoAdvertise ORPort 1.2.3.4:9001 NoListen Thus, this patch now ignores ports that have different addresses iff they are both explicits. That is, if we have this example, also two different addresses: ORPort 9001 ORPort 127.0.0.1:9001 NoAdvertise The first one is implicit and second one is explicit and thus we have to consider them for removal which in this case would remove the "ORPort 9001" in favor of the second port. Fixes #40289 Signe-off-by: David Goulet <dgoulet(a)torproject.org> --- changes/ticket40289 | 6 ++++ src/feature/relay/relay_config.c | 69 ++++++++++++++++++++++++++++++++++++---- 2 files changed, 69 insertions(+), 6 deletions(-) diff --git a/changes/ticket40289 b/changes/ticket40289 new file mode 100644 index 0000000000..cdb36825b0 --- /dev/null +++ b/changes/ticket40289 @@ -0,0 +1,6 @@ + o Minor bugfixes (relay, config): + - Fix a problem in the removal of duplicate ORPort from the internal port + list when loading config file. We were removing wrong ports breaking valid + torrc uses cases for multiple ORPorts of the same address family. Fixes + bug 40289; bugfix on 0.4.5.1-alpha. + diff --git a/src/feature/relay/relay_config.c b/src/feature/relay/relay_config.c index e007a3bd0d..c4a5d7f572 100644 --- a/src/feature/relay/relay_config.c +++ b/src/feature/relay/relay_config.c @@ -188,6 +188,41 @@ describe_relay_port(const port_cfg_t *port) return buf; } +/** Return true iff port p1 is equal to p2. + * + * This does a field by field comparaison. */ +static bool +port_cfg_eq(const port_cfg_t *p1, const port_cfg_t *p2) +{ + bool ret = true; + + tor_assert(p1); + tor_assert(p2); + + /* Address, port and type. */ + ret &= tor_addr_eq(&p1->addr, &p2->addr); + ret &= (p1->port == p2->port); + ret &= (p1->type == p2->type); + + /* Mode. */ + ret &= (p1->is_unix_addr == p2->is_unix_addr); + ret &= (p1->is_group_writable == p2->is_group_writable); + ret &= (p1->is_world_writable == p2->is_world_writable); + ret &= (p1->relax_dirmode_check == p2->relax_dirmode_check); + ret &= (p1->explicit_addr == p2->explicit_addr); + + /* Entry config flags. */ + ret &= tor_memeq(&p1->entry_cfg, &p2->entry_cfg, + sizeof(entry_port_cfg_t)); + /* Server config flags. */ + ret &= tor_memeq(&p1->server_cfg, &p2->server_cfg, + sizeof(server_port_cfg_t)); + /* Unix address path if any. */ + ret &= !strcmp(p1->unix_addr, p2->unix_addr); + + return ret; +} + /** Attempt to find duplicate ORPort that would be superseded by another and * remove them from the given ports list. This is possible if we have for * instance: @@ -241,20 +276,42 @@ remove_duplicate_orports(smartlist_t *ports) if (next->type != CONN_TYPE_OR_LISTENER) { continue; } + /* Remove duplicates. */ + if (port_cfg_eq(current, next)) { + removing[j] = true; + continue; + } /* Don't compare addresses of different family. */ if (tor_addr_family(&current->addr) != tor_addr_family(&next->addr)) { continue; } + /* At this point, we have a port of the same type and same address + * family. Now, we want to avoid comparing addresses that are different + * but are both explicit. As an example, these are not duplicates: + * + * ORPort 127.0.0.:9001 NoAdvertise + * ORPort 1.2.3.4:9001 NoListen + * + * Any implicit address must be considered for removal since an explicit + * one will always supersedes it. */ + if (!tor_addr_eq(&current->addr, &next->addr) && + current->explicit_addr && next->explicit_addr) { + continue; + } - /* Same port, we keep the explicit one. */ + /* Port value is the same so we either have a duplicate or a port that + * supersedes another. */ if (current->port == next->port) { - removing[j] = true; + /* Do not remove the explicit address. As stated before above, we keep + * explicit addresses which supersedes implicit ones. */ if (!current->explicit_addr && next->explicit_addr) { - char *next_str = tor_strdup(describe_relay_port(next)); - log_warn(LD_CONFIG, "Configuration port %s superseded by %s", - describe_relay_port(current), next_str); - tor_free(next_str); + continue; } + removing[j] = true; + char *next_str = tor_strdup(describe_relay_port(next)); + log_warn(LD_CONFIG, "Configuration port %s superseded by %s", + next_str, describe_relay_port(current)); + tor_free(next_str); } } }

1 0

[tor/master] test: Fix duplicate ORPort test
by dgoulet＠torproject.org 12 Feb '21

12 Feb '21

commit d47e937a5074c8c9799e1d8bbd6645eea74b86ab Author: David Goulet <dgoulet(a)torproject.org> Date: Thu Feb 11 16:32:17 2021 -0500 test: Fix duplicate ORPort test The comment of that specific unit test wanted 4 ORPorts but for some reasons we tested for 3 which before the previous commit related to #40289, test would pass but it was in fact wrong. Now the code is correct and 4 was in fact correct expected number of ports. Related to #40289 Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/test/test_config.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/test/test_config.c b/src/test/test_config.c index 4eb4ac9cf5..cd7a54b97a 100644 --- a/src/test/test_config.c +++ b/src/test/test_config.c @@ -6907,12 +6907,14 @@ test_config_duplicate_orports(void *arg) /* We have four address here, 1 IPv4 on 9050, IPv6 on 9050, IPv6 on 9051 and * a different IPv6 on 9051. */ - tt_int_op(smartlist_len(ports), OP_EQ, 3); + tt_int_op(smartlist_len(ports), OP_EQ, 4); tt_str_op(describe_relay_port(smartlist_get(ports, 0)), OP_EQ, "ORPort 9050"); tt_str_op(describe_relay_port(smartlist_get(ports, 1)), OP_EQ, "ORPort [4242::1]:9051"); tt_str_op(describe_relay_port(smartlist_get(ports, 2)), OP_EQ, + "ORPort [4242::2]:9051"); + tt_str_op(describe_relay_port(smartlist_get(ports, 3)), OP_EQ, "ORPort 9050"); /* Reset. Test different ORPort value. */

1 0

[tor/release-0.4.5] test: Fix duplicate ORPort test
by dgoulet＠torproject.org 12 Feb '21

12 Feb '21

commit d47e937a5074c8c9799e1d8bbd6645eea74b86ab Author: David Goulet <dgoulet(a)torproject.org> Date: Thu Feb 11 16:32:17 2021 -0500 test: Fix duplicate ORPort test The comment of that specific unit test wanted 4 ORPorts but for some reasons we tested for 3 which before the previous commit related to #40289, test would pass but it was in fact wrong. Now the code is correct and 4 was in fact correct expected number of ports. Related to #40289 Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/test/test_config.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/test/test_config.c b/src/test/test_config.c index 4eb4ac9cf5..cd7a54b97a 100644 --- a/src/test/test_config.c +++ b/src/test/test_config.c @@ -6907,12 +6907,14 @@ test_config_duplicate_orports(void *arg) /* We have four address here, 1 IPv4 on 9050, IPv6 on 9050, IPv6 on 9051 and * a different IPv6 on 9051. */ - tt_int_op(smartlist_len(ports), OP_EQ, 3); + tt_int_op(smartlist_len(ports), OP_EQ, 4); tt_str_op(describe_relay_port(smartlist_get(ports, 0)), OP_EQ, "ORPort 9050"); tt_str_op(describe_relay_port(smartlist_get(ports, 1)), OP_EQ, "ORPort [4242::1]:9051"); tt_str_op(describe_relay_port(smartlist_get(ports, 2)), OP_EQ, + "ORPort [4242::2]:9051"); + tt_str_op(describe_relay_port(smartlist_get(ports, 3)), OP_EQ, "ORPort 9050"); /* Reset. Test different ORPort value. */

1 0

[tor/master] Merge branch 'maint-0.4.5'
by dgoulet＠torproject.org 12 Feb '21

12 Feb '21

commit 71e9c56578e83e95e4ce622d0733b6f9bba3e11c Merge: c1b5e7fa1b 5887c1f1f3 Author: David Goulet <dgoulet(a)torproject.org> Date: Fri Feb 12 13:13:57 2021 -0500 Merge branch 'maint-0.4.5' changes/ticket40289 | 6 ++++ src/app/config/config.c | 1 + src/feature/relay/relay_config.c | 69 ++++++++++++++++++++++++++++++++++++---- src/test/test_config.c | 4 ++- 4 files changed, 73 insertions(+), 7 deletions(-)

1 0

[tor/master] Merge branch 'tor-gitlab/mr/304' into maint-0.4.5
by dgoulet＠torproject.org 12 Feb '21

12 Feb '21

commit 5887c1f1f3f2c73e0750f0bf98431cba74e300bc Merge: bdca475518 d47e937a50 Author: David Goulet <dgoulet(a)torproject.org> Date: Fri Feb 12 13:13:50 2021 -0500 Merge branch 'tor-gitlab/mr/304' into maint-0.4.5 changes/ticket40289 | 6 ++++ src/app/config/config.c | 1 + src/feature/relay/relay_config.c | 69 ++++++++++++++++++++++++++++++++++++---- src/test/test_config.c | 4 ++- 4 files changed, 73 insertions(+), 7 deletions(-)

1 0

[tor/release-0.4.5] config: Set flag for implicit port address
by dgoulet＠torproject.org 12 Feb '21

12 Feb '21

commit 80b33ae1ca148753558afa7bc8b43ba3df6d7948 Author: David Goulet <dgoulet(a)torproject.org> Date: Thu Feb 11 16:12:59 2021 -0500 config: Set flag for implicit port address Fun bug where we thought we were using the default "false" value when an implicit address was detected but if we had an explicit address before, the flag was set to true and then we would only use that value. And thus, for some configurations, implicit addresses would be flagged as explicit and then configuring ports goes bad. Related to #40289 Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/app/config/config.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/app/config/config.c b/src/app/config/config.c index c7799ec1a2..fa74907b3d 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -6034,6 +6034,7 @@ port_parse_config(smartlist_t *out, port = (int) tor_parse_long(addrport, 10, 0, 65535, &ok, NULL); if (ok) { tor_addr_copy(&addr, &default_addr); + addr_is_explicit = false; } else if (tor_addr_port_lookup(addrport, &addr, &ptmp) == 0) { if (ptmp == 0) { log_warn(LD_CONFIG, "%sPort line has address but no port", portname);

1 0