Opened 2 years ago

Closed 2 years ago

#23304 closed defect (implemented)

prop224: Dump a malformed descriptor in a file and log_warn about it

Reported by: dgoulet Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs, prop224, review-group-23
Cc: Actual Points:
Parent ID: #23300 Points:
Reviewer: nickm Sponsor: SponsorR-can

Description

Once a client fetches a descriptor, it could be unparseable thus malformed. Dump it in a file and warn about it so the user can report it properly to us and we also don't have a 50kb blob of text in the logs.

Worth thinking if we might want that only with SafeLogging 0 since leaving HS descriptors on disk client side might not be ideal?

As for v2 subsystem, the onion address is in the descriptor so I would be very careful to put that on disk.

Child Tickets

Change History (13)

comment:1 in reply to:  description Changed 2 years ago by arma

Replying to dgoulet:

Worth thinking if we might want that only with SafeLogging 0 since leaving HS descriptors on disk client side might not be ideal?

As for v2 subsystem, the onion address is in the descriptor so I would be very careful to put that on disk.

These are two sides of the same argument: a v3 descriptor is still recognizable as corresponding to a given onion address if you know the onion key that generated it. Though heck, an intentionally malformed descriptor is recognizable no matter what our protocol specifications say. ("You tried to fetch the descriptor for this onion service that I hate? Here, let me give you a unique cookie that you'll write to disk.")

comment:2 Changed 2 years ago by dgoulet

Status: newneeds_information

Yeah... I'm uncertain also about this one... And even having the malformed descriptor, chances are that a non malicious tor created it are slim. Not sure I even saw that in v2 in the wild...

comment:3 Changed 2 years ago by arma

I'm ok doing this when SafeLogging is 0 if we think it'll be useful.

That way if some user is experiencing a problem where Tor complains that the hsdesc is malformed, and it's repeatable, we can tell the user to turn off SafeLogging and then tell us what it says.

Maybe it will be simpler in this rare case just to log it into the logs though? (Can Tor's log system handle a 50KB log line? :)

comment:4 Changed 2 years ago by dgoulet

Status: needs_informationneeds_review

Simple fix, I dump the descriptor in the log as a warn if SafeLogging is set to scrub NONE.

Branch: ticket23304_032_01.

comment:5 Changed 2 years ago by arma

Status: needs_reviewneeds_revision
+    if (get_options()->SafeLogging == SAFELOG_SCRUB_NONE) {

you want SafeLogging_ here. Remember,

config.c:  V(SafeLogging,                 STRING,   "1"),

comment:6 Changed 2 years ago by dgoulet

Status: needs_revisionneeds_review

Fixed!... My bad, I orginally used the one with _ and somehow it got removed and failed to compile.

comment:7 Changed 2 years ago by nickm

Keywords: review-group-23 added

Put 0.3.2 needs_review and merge_ready tickets into review-group-23.

comment:8 Changed 2 years ago by dgoulet

Owner: set to dgoulet
Status: needs_reviewaccepted

comment:9 Changed 2 years ago by dgoulet

Status: acceptedneeds_review

comment:10 Changed 2 years ago by nickm

Status: needs_reviewneeds_revision

looks good, but let's use escaped() here, in case there are tricky characters in the descriptor.

comment:11 Changed 2 years ago by nickm

Reviewer: nickm

comment:12 Changed 2 years ago by dgoulet

Status: needs_revisionmerge_ready

Oh agreed! Fixup commit added.

See ticket23304_032_01.

comment:13 Changed 2 years ago by nickm

Resolution: implemented
Status: merge_readyclosed

squashed + merging!

Note: See TracTickets for help on using tickets.