Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#27325: Rework NETINFO wire format handling to rely on trunnel (2) #445

Merged
merged 9 commits into from Dec 20, 2018

Conversation

Labels
None yet
Projects
None yet
4 participants
@rl1987
Copy link
Contributor

@rl1987 rl1987 commented Oct 26, 2018

https://trac.torproject.org/projects/tor/ticket/27325

@coveralls
Copy link

@coveralls coveralls commented Oct 26, 2018

Pull Request Test Coverage Report for Build 3324

  • 0 of 78 (0.0%) changed or added relevant lines in 2 files are covered.
  • 8306 unchanged lines in 63 files lost coverage.
  • Overall coverage decreased (-1.3%) to 60.779%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/core/or/connection_or.c 0 37 0.0%
src/core/or/channeltls.c 0 41 0.0%
Files with Coverage Reduction New Missed Lines %
src/feature/dirauth/voteflags.c 1 34.6%
src/feature/hs/hs_config.c 2 85.96%
src/feature/dirparse/microdesc_parse.c 2 94.44%
src/feature/rend/rendcache.c 2 93.02%
src/core/or/channeltls.c 2 42.63%
src/lib/evloop/token_bucket.h 2 66.67%
src/lib/fs/files.c 2 67.21%
src/core/or/channel.c 3 83.14%
src/core/mainloop/connection.h 3 50.0%
src/core/or/circuitlist.c 3 57.05%
Totals Coverage Status
Change from base Build 2595: -1.3%
Covered Lines: 43539
Relevant Lines: 71635

💛 - Coveralls

const NETINFO_ADDR_TYPE_IPV4 = 4;
const NETINFO_ADDR_TYPE_IPV6 = 6;

struct netinfo_addr {
Copy link
Contributor

@nmathewson nmathewson Dec 15, 2018

This breaks forward compatibility. It means that Tor will no longer handle any NETINFO cell that ever declares a future address type.

We need to support and ignore unrecognized address types, and let them have whatever type they need to.

Copy link
Contributor Author

@rl1987 rl1987 Dec 16, 2018

Fixed in 3bec371.

decode_address_from_payload(&addr, cp, (int)(end-cp));
if (next == NULL) {

if (tor_addr_from_netinfo_addr(&addr, netinfo_addr) == -1) {
log_fn(LOG_PROTOCOL_WARN, LD_OR,
"Bad address in netinfo cell; closing connection.");
connection_or_close_for_error(chan->conn, 0);
Copy link
Contributor

@nmathewson nmathewson Dec 15, 2018

Hang on, does this mean that we previously rejected unrecognized address types? That's not good...

Copy link
Contributor Author

@rl1987 rl1987 Dec 16, 2018

Fixed in 5b2acbe.

u8 addr_type IN [NETINFO_ADDR_TYPE_IPV4, NETINFO_ADDR_TYPE_IPV6];
u8 len IN [4, 16];
u8 addr_type;
u8 len;
union addr[addr_type] {
NETINFO_ADDR_TYPE_IPV4: u32 ipv4;
NETINFO_ADDR_TYPE_IPV6: u8 ipv6[16];
default: fail;
Copy link
Contributor

@nmathewson nmathewson Dec 17, 2018

This is still going to mean that if addr_type isn't recognized, parsing fails.

Try replacing the union line with union addr[addr_type] with length len {, and the default line with default: ignore -- I think that will do it.

Also, we should have unit tests that make sure that this case actually works.

Copy link
Contributor Author

@rl1987 rl1987 Dec 18, 2018

Fixed netinfo.trunnel in c92c0cb. Added unit test in c659603.

rl1987 added 2 commits Dec 18, 2018
Ignore the address value instead of failing with error condition in case
unrecognized address type is found.
@torproject-pusher torproject-pusher merged commit c659603 into torproject:master Dec 20, 2018
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment