Opened 10 months ago

Closed 9 months ago

#20559 closed defect (fixed)

rend_config_services ignores failures in rend_add_service

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version: Tor:
Severity: Normal Keywords: tor-hs, review-group-12
Cc: Actual Points: 0.3
Parent ID: Points: 0.1
Reviewer: Sponsor:


Tor used to warn but continue on some hidden service misconfigurations.
Instead, the fix I am adding #20484 will consider these misconfigurations a config error, and terminate tor.

I think it's better to stop with an error, than potentially reveal sensitive information due to a misconfiguration (or, far more likely, just ignore hidden services the user has misconfigured).

Bugfix on commit 915c743 in #6411 in tor-

Child Tickets

Change History (11)

comment:1 Changed 10 months ago by teor

and fixes on 03d6a3171 in, 60a0ae198 in

comment:2 Changed 10 months ago by teor

Actual Points: 0.1
Keywords: tor-hs added
Parent ID: #20484

comment:3 Changed 10 months ago by teor

This is fixed in #20484 in my bug20484_029:
428ee66 Stop ignoring misconfigured hidden services

comment:4 Changed 9 months ago by nickm

Milestone: Tor: 0.2.9.x-finalTor: 0.3.0.x-final
Status: newneeds_review

428ee66 is now part of bug20484_030_v2

comment:5 Changed 9 months ago by nickm

Keywords: review-group-12 added

comment:6 Changed 9 months ago by teor

Actual Points: 0.10.3
Parent ID: #20484

My branch bug20599_030_v2 on github fixes this issue.

It passes all the standard checks, plus those in:

It fails as expected on the following config, which would previously have been ignored:

src/or/tor HiddenServiceDir /tmp/tor.$$.anon HiddenServicePort 80 DataDirectory /tmp/tor.$$ SocksPort 0 PidFile /tmp/ ControlPort 12345 HiddenServiceDir /tmp/tor.$$.anon HiddenServicePort 80

comment:7 Changed 9 months ago by asn

Status: needs_reviewmerge_ready

I like this branch! It basically addresses almost all of my comments in #20638!

The patch looks good to me.

comment:8 Changed 9 months ago by teor

(The fixup 5f63f2d in bug20638_029_v2 in #20638 has already been applied to this branch. That might cause a minor merge issue once both branches have been merged. We want to end up with the code containing no duplicate comment, and a call to rend_service_is_ephemeral().)

I changed a commit message in #20638, the corresponding change is in bug20599_030_v3.

comment:9 Changed 9 months ago by nickm

Having merged the branch from #20638, I've cherry-picked the remaining commits from this branch onto a new branch, "bug20599_030_v4". I've read them and they seem fine fine to me.

Teor, can you confirm whether this branch is still okay to you (and I didn't miss anything)? If so, I'm happy to merge it into master.

comment:10 Changed 9 months ago by teor

Yes, that looks like exactly what I wanted - and I am glad we left it for 0.3.0.
Let's get it merged.

comment:11 Changed 9 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

Merged it is!

Note: See TracTickets for help on using tickets.