Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#18029 closed defect (duplicate)

ADD_ONION doesn't validate its target

Reported by: atagar Owned by:
Priority: Low Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version: Tor: 0.2.7.1-alpha
Severity: Minor Keywords: tor-hs tor-core
Cc: yawning Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The target ADD_ONION accepts is documented as matching HiddenServicePort but evidently doesn't validate that it's given a valid address...

>>> GETINFO version
250-version=0.2.7.6-dev (git-b34c5c6b8ac0d13d)
250 OK

>>> ADD_ONION NEW:BEST Port=4567,not_an_address:4567
250-ServiceID=4e5bpsy6e46onv3k
250-PrivateKey=RSA1024:[crypto blob]
250 OK

Honestly I'm a tad mystified since this is breaking Stem's integ tests, but only started manifesting after I reformatted my system yesterday. I'd expect our Jenkins tests to be failing so this might be something local, but if so I'm stumped.

Child Tickets

Change History (6)

comment:1 Changed 4 years ago by yawning

Cc: yawning added
Keywords: tor-hs tor-core added
Milestone: Tor: 0.2.8.x-final
Status: newneeds_information
Version: Tor: 0.2.7.1-alpha

As far as I know the code does validate (See: rendservice.c:rend_service_parse_port_config()).

I've tried master, 0.2.7.6 (Arch package), and 0.2.7.3-dev (what I happened to have on my craptop), and they all reject the command in the bug report with a 512 Invalid VIRTPORT/TARGET.

>>> GETINFO version
250-version=0.2.8.0-alpha-dev (git-8fc27ac0420c7120)
250 OK

>>> ADD_ONION NEW:BEST Port=4567,not_an_address:4567
512 Invalid VIRTPORT/TARGET

Do you have any local changes that may be causing the validation to break?

comment:2 Changed 4 years ago by atagar

Nope, no local changes. Did an extra thorough clean with 'git reset -df' on commit 8fc27ac and same result. The port part is validating, it's just the address that seems to accept too much (but not everything)...

>>> GETINFO version
250-version=0.2.8.0-alpha-dev (git-8fc27ac0420c7120)
250 OK

>>> ADD_ONION NEW:BEST Port=4567,not_an_address:invalid
512 Invalid VIRTPORT/TARGET

>>> ADD_ONION NEW:BEST Port=4567,-:4567
512 Invalid VIRTPORT/TARGET

>>> ADD_ONION NEW:BEST Port=4567,aaa:4567
512 Invalid VIRTPORT/TARGET

>>> ADD_ONION NEW:BEST Port=4567,aaaaa:4567
250-ServiceID=btx52wpyix3cmsmi
250-PrivateKey=RSA1024:[crypto blob]
250 OK

comment:3 Changed 4 years ago by yawning

I guess if the address portion actually resolves to something using the system resolver (getaddrinfo) it'll be accepted, but the same thing goes for torrc as well (ADD_ONION and torrc based HSes both use common code to handle parsing/validating this stuff).

I still can't reproduce success when it shouldn't.... :/

comment:4 Changed 4 years ago by yawning

I went and filed #18037 so people can argue over if we should be attempting to call the resolver at all when setting up HSes.

comment:5 Changed 4 years ago by atagar

Resolution: duplicate
Status: needs_informationclosed

Thanks Yawning. Resolving in favor of that.

comment:6 Changed 4 years ago by atagar

Revised Stem's tests to account for this. Thanks for digging into this Yawning!

Note: See TracTickets for help on using tickets.