Opened 2 months ago

Last modified 3 weeks ago

#31408 merge_ready defect

torrc : ClientOnionAuthDir after include directives breaks client to v2 services

Reported by: xaho Owned by: asn
Priority: Very High Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version: Tor: 0.3.1.1-alpha
Severity: Normal Keywords: BugSmashFund tor-hs regression 042-must consider-backport-after-0423, 035-backport, 040-backport, 041-backport, 042-backport
Cc: danielpinto52@… Actual Points: 0.4
Parent ID: Points: 0.3
Reviewer: Sponsor:

Description

If I append these two statements to torrc, in this order :

ClientOnionAuthDir /etc/tor/auth/
%include /etc/tor/torrc.d/

and restart tor.service, I can connect to 1 × v3 and 4 × v2 services, within seconds.

But if I reverse their order ( %include first ), I can only connect to the v3 service -- all other connections will eventually time out.

In man, I missed to find any recommandation about ordering these statements.

( this is on Debian Stretch with torproject's stretch repo )

Thanks!

Child Tickets

Change History (20)

comment:1 Changed 2 months ago by xaho

n.b. if it matters, all of these 5 services are set with an auth/stealth key

comment:2 Changed 2 months ago by dgoulet

Keywords: tor-hs regression added; torrc include ClientOnionAuthDir order removed
Milestone: Tor: 0.4.2.x-final

comment:3 Changed 5 weeks ago by nickm

Keywords: 042-must added

comment:4 Changed 4 weeks ago by nickm

Priority: MediumVery High

Make all 042-must objects "Very High" priority.

comment:5 Changed 4 weeks ago by teor

What's in the rest of your torrc file?
How many files are in torrc.d, and what's in them?

comment:6 Changed 4 weeks ago by teor

(We have a new option testing framework in #31637, we could try using it to replicate this issue. And to ensure it doesn't happen again.)

comment:7 Changed 4 weeks ago by xaho

In torrc, the above two lines are the only non-commented lines, appended to the maintainer's version.

In torrc.d, there are twelve files containing one or more commented or empty lines, and only one non-commented HidServAuth statement each, all in the format :

HidServAuth name.onion stealthkey # client : hostname

Yet another file is named 'wip' and contains only commented lines. :)

comment:8 Changed 4 weeks ago by asn

Owner: set to asn
Status: newassigned

comment:9 Changed 4 weeks ago by Jigsaw52

I think it might be useful to have both torrc and the actual files within /etc/tor/torrc.d/ with any sensitive information replaced by 'x'. I am thinking this could be a parsing bug and things like whitespace and new line differences and empty files vs files with only comments/whitespace could matter here.

comment:10 Changed 4 weeks ago by xaho

I chased the culprit and it is... wip ! :) As I wrote, it has only comments and empty lines.

Now to break the config/option parser, I just need any file in torrc.d, with a single # char in it *and* still, order ClientOnionAuthDir after %include in torrc, as reported.

An empty file or line, seems not enough to break the option parser : it has to contain at least one, or more comment(s), but only comments and ( optional ) empty lines, so I suspect, the file as a whole end up « nullified ». Inserting any valid option in that file, does fix the issue.

Last edited 4 weeks ago by xaho (previous) (diff)

comment:11 Changed 3 weeks ago by Jigsaw52

Status: assignedneeds_review

The cause of this problem is a bug in the processing of included folders containing files with only comments or whitespace. I was able to reproduce this problem with the following configuration:

torrc:

%include /etc/tor/torrc.d/
Log notice stderr # can be any valid option

/etc/tor/torrc.d/01_invalid:

InvalidOption

/etc/tor/torrc.d/02_comment:

# comment

Running tor --verify-config -f torrc will incorrectly identify this as a valid configuration despite the invalid option on /etc/tor/torrc.d/01_invalid.

The bug is that any files on a %included folder where the last file contains only comments or whitespace are being ignored. Also the bug is only triggered if there is a valid option after the %include.

The cause of this bug is that, when creating the list of configurations options from the included files, a pointer to the last element of the list is incorrectly set to NULL when processing a file with only comments or whitespace. This can cause the options list to be built incorrectly, causing this behaviour.

I've made a pull request with a fix for this problem and a test case: https://github.com/torproject/tor/pull/1347

comment:12 Changed 3 weeks ago by Jigsaw52

Cc: danielpinto52@… added

comment:13 Changed 3 weeks ago by xaho

Thanks, and sorry for the confusion ! This statement of mine was bogus:

any file in torrc.d, with a single # char

Indeed the filename also must be last, I just verified it.

comment:14 Changed 3 weeks ago by teor

Keywords: 035-backport 040-backport 041-backport 042-backport added

This looks like a fix we should backport to 0.3.5 and later.

I'll make branches for 0.3.5 and later, using git cherry-pick, and our git-merge-forward.sh test branch mode.

comment:15 Changed 3 weeks ago by teor

Actual Points: 0.3
Keywords: BugSmashFund consider-backport-after-0423 added
Points: 0.3
Status: needs_reviewmerge_ready
Version: Tor: 0.4.0.5Tor: 0.3.1.1-alpha

Here is a backported pull request, with some changes file fixes:

I added a conf regression test on master using the test framework from #31637:

The merge forward is clean, the test branches are here:

comment:16 Changed 3 weeks ago by Jigsaw52

Status: merge_readyneeds_revision

I've commented on your pull request: your test uses an empty (0 byte) file but the issue is only reproduced with a non-empty file containing no options.

comment:17 Changed 3 weeks ago by teor

Status: needs_revisionneeds_review

Thanks, I originally had a non-empty file, but I must have made a mistake while minimising the test case.

I fixed the changes file, test, and test commit message, and force-pushed.

comment:18 Changed 3 weeks ago by teor

Actual Points: 0.30.4

comment:19 Changed 3 weeks ago by teor

Status: needs_reviewmerge_ready

comment:20 Changed 3 weeks ago by nickm

Milestone: Tor: 0.4.2.x-finalTor: 0.4.1.x-final

Merged to master, and marked for backport. Good bug-fixing, and nice to see so many tests!

Note: See TracTickets for help on using tickets.