Opened 9 years ago

Closed 8 years ago

Last modified 7 years ago

#2748 closed defect (fixed)

Multiple bogosities in rend_cache_lookup_v2_desc_as_dir

Reported by: rransom Owned by: rransom
Priority: Low Milestone: Tor: 0.2.2.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-hs
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

rend_cache_lookup_v2_desc_as_dir misinterprets the return value of hid_serv_responsible_for_desc_id. As a consequence, when an HSDir is asked for a descriptor it does not consider itself responsible for, it returns error code 404 ('Not found') rather than error code 400 ('Bad request'), as it was apparently intended to.

Child Tickets

Change History (12)

comment:1 Changed 9 years ago by rransom

Status: newneeds_review

See bug2748 ( ssh://mob@repo.or.cz/srv/git/tor/rransom.git bug2748 ) for the fix.

comment:2 Changed 9 years ago by Sebastian

Hrm. The documentation for rend_cache_lookup_v2_desc_as_dir() says that the function returns 0 if we couldn't look up the descriptor but the descriptor request was well-formed, so your patch violates the documentation here. The caller also relies on this behaviour, see comments in directory_handle_command_get(). Also, reading the spec, I don't see why a 400 response would be the right thing to send back if alice doesn't have the descriptor but the request was ok. Please clarify?

comment:3 in reply to:  2 ; Changed 9 years ago by rransom

Summary: v2 HS dirs reply incorrectly to requests for descriptors they are not responsible forMultiple bogosities in rend_cache_lookup_v2_desc_as_dir

Replying to Sebastian:

Hrm. The documentation for rend_cache_lookup_v2_desc_as_dir() says that the function returns 0 if we couldn't look up the descriptor but the descriptor request was well-formed, so your patch violates the documentation here. The caller also relies on this behaviour, see comments in directory_handle_command_get().

When rend_cache_lookup_v2_desc_as_dir was added in commit e136f00c, it was clearly intended to return -1 (indicating failure) if the request was rejected for any reason other than 'descriptor not found' (as was rend_cache_store_v2_dir, introduced in the same commit). The comment on rend_cache_lookup_v2_desc_as_dir was changed to document its return values in the following commit (024798ee); that documentation appears to have been based on the comments in directory_handle_command_get, and only accidentally matched the function's behaviour because it misinterpreted the return value of hid_serv_responsible_for_desc_id.

Then again, that function also logs at level LOG_WARN (not LOG_PROTOCOL_WARN) if it receives a malformed request, so its original behaviour wasn't especially good.

Also, reading the spec, I don't see why a 400 response would be the right thing to send back if alice doesn't have the descriptor but the request was ok. Please clarify?

I still think it would be appropriate to return error code 400 if a directory server refused to try to process a request for any reason, but I'm no longer convinced that refusing to process requests for HS descriptors that a relay is not responsible for is worthwhile.

See bug2748-v2 ( ssh://mob@repo.or.cz/srv/git/tor/rransom.git bug2748-v2 ) for patches to remove this call to hid_serv_responsible_for_desc_id entirely and use the correct log level to report malformed requests.

comment:4 in reply to:  3 Changed 9 years ago by rransom

Replying to rransom:

if the request was rejected for any reason other than 'descriptor not found' (as was rend_cache_store_v2_dir, introduced in the same commit).

Hmm. The part about rend_cache_store_v2_dir is wrong ('descriptor not found' is not relevant to that function). The rest of that comment is correct, though.

comment:5 Changed 9 years ago by nickm

The safe_str call probably wants to be escaped_safe_str if we're reporting about bogus characters.

Is it possible for a hidden service directory to have a stale copy of a descriptor that it no longer considers itself responsible for? If so, removing the hid_serv_responsible_for_desc_id check is not cosmetic; it might make us return something we don't mean to return.

comment:6 in reply to:  5 Changed 9 years ago by rransom

Replying to nickm:

The safe_str call probably wants to be escaped_safe_str if we're reporting about bogus characters.

Yes.

Is it possible for a hidden service directory to have a stale copy of a descriptor that it no longer considers itself responsible for? If so, removing the hid_serv_responsible_for_desc_id check is not cosmetic; it might make us return something we don't mean to return.

rend_cache_clean_v2_descs_as_dir is called once every 30 minutes from run_scheduled_events, and discards all descriptors for which hid_serv_responsible_for_desc_id returns false. If we care about getting rid of HS descriptors quickly, we should also cause rend_cache_clean_v2_descs_as_dir to be called whenever we receive a new consensus.

comment:7 Changed 9 years ago by nickm

If we care about getting rid of HS descriptors quickly,

I think we probably do; we don't want to be serving outdated ones.

comment:8 Changed 8 years ago by nickm

Milestone: Tor: 0.2.1.x-finalTor: 0.2.2.x-final

This should probably target 0.2.2; the bugs fixed in the bug2748-v2 branch look basically harmless. Is there a newer version I should be looking at?

comment:9 in reply to:  7 Changed 8 years ago by rransom

Replying to nickm:

If we care about getting rid of HS descriptors quickly,

I think we probably do; we don't want to be serving outdated ones.

HSDir relays serve outdated descriptors now -- see the following line from 2401#comment:3:

Jan 22 15:18:50.249 [info] rend_cache_store_v2_desc_as_client(): We already have this service descriptor gtvsxxxxxxxxxxxx.

The only way to prevent (non-malicious) HS directories from serving outdated descriptors is to make the set of relays with the HSDir flag stable.

comment:10 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged bug2748-v2 into 0.2.2 and later. Thanks!

comment:11 Changed 7 years ago by nickm

Keywords: tor-hs added

comment:12 Changed 7 years ago by nickm

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