[tor-bugs] #2748 [Tor Hidden Services]: Multiple bogosities in rend_cache_lookup_v2_desc_as_dir (was: v2 HS dirs reply incorrectly to requests for descriptors they are not responsible for)

[Tor Bug Tracker & Wiki]

#2748: Multiple bogosities in rend_cache_lookup_v2_desc_as_dir
---------------------------------+------------------------------------------
 Reporter:  rransom              |          Owner:  rransom           
     Type:  defect               |         Status:  needs_review      
 Priority:  minor                |      Milestone:  Tor: 0.2.1.x-final
Component:  Tor Hidden Services  |        Version:                    
 Keywords:                       |         Parent:                    
   Points:                       |   Actualpoints:                    
---------------------------------+------------------------------------------

Comment(by rransom):

 Replying to [comment:2 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
 [https://gitweb.torproject.org/tor.git/commitdiff/e136f00ca8a955afc7507ba5e5e91374a7b59401
 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
 [https://gitweb.torproject.org/tor.git/commitdiff/024798ee4c6d504f7aec3ddcb3dfc07d96e475f1
 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 [http://repo.or.cz/w/tor/rransom.git/shortlog/refs/heads/bug2748-v2
 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.

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/2748#comment:3>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online