Opened 10 years ago

Closed 8 years ago

Last modified 7 years ago

#1240 closed defect (fixed)

stop calling getsockname if accept returns a bad address

Reported by: Sebastian Owned by:
Priority: Low Milestone: Tor: 0.2.2.x-final
Component: Core Tor/Tor Version: 0.2.1.22
Severity: Keywords:
Cc: Sebastian, nickm Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by nickm)

r1eo| can anyone explain: 1) why connection_handle_listener_read() goes far

if getsockname() failed and do not return after such. 2) why
getsockname() called at all, what purpose for retrieve local addr and
put it as remote? at least fail of check_sockaddr() can be triggered
remotely in theory.

r1eo| http://archives.seul.org/or/cvs/Apr-2005/msg00100.html it's

introduced getsockname(). but I have the same questions still. And one:
if accept() failed to return correct peer-address, then getpeername()
failing too? http://archives.neohapsis.com/archives/bind/2001/0025.html
http://www.mail-archive.com/freebsd-net@freebsd.org/msg00901.html
funny, but such accepted conn with zero addr is broken anyway. so if tor
filled it with self addr bug him. wonder if it exploitable for spoofing log
at least.

r1eo| someone nmap'ed moria in 2005.
r1eo| just for clear: nothing stops sending created cell and other for today even

if addr faked.

r1eo| http://archives.seul.org/or/cvs/Apr-2005/msg00100.html "This just

happened on moria2"

r1eo| http://paste.debian.net/58458/plain/58458 prevent fooling themself.

socket returned by accept() with zero addr, as result of scan and timing
issues in some kernels. such conn is broken.

[Automatically added by flyspray2trac: Operating System: All]

Child Tickets

Attachments (1)

getsocknameremoval.patch (1.1 KB) - added by Sebastian 10 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 10 years ago by Sebastian

Attached the patch, as the paste will expire soon. Additional comment:

r1eo| getsockname() lead to filling with faked addr, while check_sockaddr()

can be failed remotely in theory with zero source port.

Changed 10 years ago by Sebastian

Attachment: getsocknameremoval.patch added

comment:2 Changed 10 years ago by Sebastian

flyspray2trac: bug closed.
closing by request from rieo...

comment:3 Changed 8 years ago by nickm

Description: modified (diff)

Note for the future: let's not close bugs just because the bug reporter asks for it, without confirming that their initial report is indeed wrong. Apparently this is real, and is #4747

comment:4 Changed 8 years ago by nickm

Resolution: Not a bug
Status: closedreopened

comment:5 Changed 8 years ago by nickm

Milestone: Tor: 0.2.2.x-final
Status: reopenedneeds_review

Please review branch bug1240 in my public repo. (The original patch from r1eo does not seem to have survived the flyspray->trac migration.)

This also fixes #4545 and #4547.

comment:6 in reply to:  5 Changed 8 years ago by nickm

Replying to nickm:

This also fixes #4545 and #4547.

Arg. Make that, "This also fixes #4745 and #4747 ."

comment:7 in reply to:  5 Changed 8 years ago by asn

Replying to nickm:

Please review branch bug1240 in my public repo. (The original patch from r1eo does not seem to have survived the flyspray->trac migration.)

This also fixes #4545 and #4547.

For all I know about networking, the code simplification looks sane. Someone who knows more networks than me should look at it too.

Bikeshedding away, I would replace all the 'nmap' references to 'client sends RST before accept()', since it's not nmap-specific. And maybe replace the 'some versions of Linux' comment, with 'some versions of Unix' or 'some versions of FreeBSD', since r1eo's link points to freebsd-net.

Maybe even cut one 'with', off the 'successfully with with socklen 0' comment.

Also, you forgot:

-  char addrbuf[256];
+  char addrbuf[256];2

in the commit.

comment:8 Changed 8 years ago by nickm

Pushed a fixup commit; have a look?

comment:9 in reply to:  8 Changed 8 years ago by asn

Replying to nickm:

Pushed a fixup commit; have a look?

Looks fine.

comment:10 Changed 8 years ago by arma

Branch looks superficially ok. (Like asn, I'm not in a state currently to make any deep networking syscall evaluations.)

comment:11 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged; thanks for looking at it!

comment:12 Changed 7 years ago by nickm

Component: Tor RelayTor
Note: See TracTickets for help on using tickets.