Opened 8 years ago

Closed 6 years ago

Last modified 11 months ago

#5127 closed defect (wontfix)

Fix IPv6 support in obfsproxy

Reported by: karsten Owned by: asn
Priority: Medium Milestone:
Component: Archived/Obfsproxy Version:
Severity: Keywords: ipv6
Cc: isis@…, ln5 Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Adding my comment from #5083 here:

Note that strchr() does the wrong thing here, because it finds the first ':' in the string, not the last. That doesn't work for IPv6 addresses. That's a bug in resolve_address_port() in util.c, too!

I think this should be strrchr().

There may be more problems with obfsproxy and IPv6.

Child Tickets

Change History (8)

comment:1 Changed 8 years ago by murble

diff --git a/src/util.c b/src/util.c
index b820981..422280e 100644
--- a/src/util.c
+++ b/src/util.c
@@ -172,7 +172,7 @@ resolve_address_port(const char *address, int nodns, int pas

char *a = xstrdup(address), *cp;
const char *portstr;


  • if ((cp = strchr(a, ':'))) {

+ if ((cp = strrchr(a, ':'))) {

portstr = cp+1;
*cp = '\0';

} else if (default_port) {

is sufficient for basic stuff like:

./obfsproxy --no-safe-logging --log-min-severity=debug obfs2 --dest=::1:1025 server ::1:1024

./obfsproxy --no-safe-logging --log-min-severity=debug obfs2 --dest=::1:1024 client ::1:1234

to work, but IPv6 addresses are not printed correctly:

z.B
2012-02-15 21:40:12 [debug] obfs2: Parsed options nicely!
2012-02-15 21:40:12 [debug] Now listening on [0.0.0.0]:1024 for protocol obfs2.
2012-02-15 21:40:35 [debug] [0.0.0.0]:1024: new connection from [0.0.0.0]:55993 (1 total)
2012-02-15 21:40:35 [debug] [0.0.0.0]:55993: server connection
2012-02-15 21:40:35 [info] [0.0.0.0]:55993 (obfs2): Successful outbound connection to '[0.0.0.0]:1025'.

the problem is in printable_address() due to a typo:

@@ -243,7 +243,7 @@ printable_address(struct sockaddr *addr, socklen_t addrlen)

case AF_INET6: {

char abuf[INET6_ADDRSTRLEN];
struct sockaddr_in6 *sin6 = (struct sockaddr_in6*)addr;

  • if (!inet_ntop(AF_INET, &sin6->sin6_addr, abuf, INET6_ADDRSTRLEN))

+ if (!inet_ntop(AF_INET6, &sin6->sin6_addr, abuf, INET6_ADDRSTRLEN))

break;

obfs_snprintf(apbuf, sizeof apbuf, "[%s]:%d", abuf,

ntohs(sin6->sin6_port));

comment:2 in reply to:  1 Changed 8 years ago by nickm

Status: newneeds_revision

Replying to murble:

[...]

is sufficient for basic stuff like:

./obfsproxy --no-safe-logging --log-min-severity=debug obfs2 --dest=::1:1025 server ::1:1024

"::1:1024" is not a great way to write the IPv6 address ::1 plus the port 1024, since it also looks exactly like the IPv6 address ::1:1024 (that is, 0:0:0:0:0:0:1:1024) .

It would be better to have the format be "[::1]:1024" -- lots of things use that for writing an IPv6 address together with a port.

comment:3 Changed 8 years ago by isis

Status: needs_revisionneeds_review

I implemented a fix for IPv6 address support, see my obfsproxy branch on github. I also wrote a unittest for util.c, although I have run into a new problem that the portstr passed in util.c to evutil_getaddrinfo() allows empty strings ("") and NULL, which should fail the tests.

comment:4 Changed 8 years ago by isis

Cc: isis@… added

I wasn't very clear in my last post. My patch does not introduce any new bugs that were not there before, but through writing the unittest for util.c, I noticed that portstr allows "" and NULL, and it does this regardless of whether the address is v4 or v6. I will eventually get around to fixing that problem too, but if someone beats me to it that's awesome too.

comment:5 Changed 8 years ago by nickm

Status: needs_reviewneeds_revision

Reviewing....

Actual code issues:

  • It looks like the code will accept a bunch of addresses that aren't actually any good, like "[1.2.3.4]:80" and "[::1] random junk :80". For the first one, we probably want to insist that anything between brackets needs to be an IPv6 address. For the second, we probably want to make sure that the : appears immediately after the ], not 'eventually' after.

Minor stuff unrelated to actual code:

  • Next time, use a repository cloned with "git clone" rather than by just re-important all the files into a new repository. Using "git clone" makes your history match the upstream history, so that your branch can be merged with "Git merge" and show up in the history properly. As it stands, we'll have to cherry-pick the commits. Not a big deal since there are just 3, though.
  • What editor are you using? It seems to be giving you weird brace alignments. Like, sometimes your indent is 4 space, and some times it's 1 space.

TODO at merge time, or whenever. No need to do all this yourself; asn or I can do it instead:

  • Replace the printfs in test_util.c with TT_BLATHER so the unit tests aren't loud unless we want them to be.
  • The array of tests in test_util.c should probably be const.
  • Reconsider "fuckeverybody" as a generic nonexistent servicename. (We're not anti-fuck in the Tor source code, but we generally use it for describing stuff like APIs we don't like, the state of cross-platform programming, the complexity of openssl, censorship, censorware, data retention proposals, ugly code, insecure security tools, breaking backward compatibility, and so on.)
  • Should the tests check that the value produced is indeed as expected?
  • Add license boilerplace to new test_util.c.

comment:6 Changed 8 years ago by ln5

Cc: ln5 added
Keywords: ipv6 added

comment:7 Changed 7 years ago by arma

murb reminded us of this ticket today. It seems we're pretty close to having an obfsproxy that can do ipv6?

comment:8 Changed 6 years ago by asn

Resolution: wontfix
Status: needs_revisionclosed

This ticket is about the old C-based obfsproxy. Closing.

Note: See TracTickets for help on using tickets.