[tor-bugs] #11243 [Tor]: Don't fetch any descriptor which we already fetched and found to be ill-formed

[Tor Bug Tracker & Wiki]

#11243: Don't fetch any descriptor which we already fetched and found to be ill-
formed
------------------------+-------------------------------------------------
     Reporter:  nickm   |      Owner:
         Type:  defect  |     Status:  needs_review
     Priority:  major   |  Milestone:  Tor: 0.2.6.x-final
    Component:  Tor     |    Version:
   Resolution:          |   Keywords:  tor-relay 026-triaged-1 nickm-patch
Actual Points:          |  Parent ID:
       Points:          |
------------------------+-------------------------------------------------

Comment (by andrea):

 Begin code review for nickm's bug11243 branch:

 c062758486dd75b3fb3030324cd76a2a206676bd:

  - /* XXXX Change this to digest256_len */ ?

  - Relevant changes are microdesc.c, routerlist.c, routerparse.{c,h}, the
    rest just carries along and doesn't use the extra parameter.

  - Theory here is router_parse_entry_from_string() and
    extrainfo_parse_entry_from_string() use the can_dl_again_out pointer
    to pass out an indication of whether the descriptor should be ignored
    because immutably flawed - i.e., something covered by the hash is
 invalid,
    or it's possible for a future download to yield something different and
    succeed.

  - Then microdescs_parse_from_string() and router_parse_list_from_string()
    use that flag to assemble lists of known-immutably-invalid descriptor
    hashes to be blacklisted from future download attempts.

  - In router_parse_list_from_string(), it'd be possible for a suitably
 formed
    string to result in the same hash being added to invalid_digests_out
    multiple times.  Are the callers of this code aware of that?

  - The same comments as for router_parse_list_from_string() apply to
    microdescs_parse_from_string().

  - In router_parse_entry_from_string(), we parse things that would make
 the
    descriptor immutably invalid first, having can_dl_again set to zero,
 and
    then set can_dl_again to one before the one check that would possibly
    reject the descriptor but accept one with the same hash: the signature
    verification.  This seems to be correct.

  - The changes to extrainfo_parse_entry_from_string() parallel those to
    router_parse_entry_from_string() and appear to be correct.
  - In both router_parse_entry_from_string() and
    extrainfo_parse_entry_from_string(), correctness depends on all of the
    reasons for rejecting an input before setting can_dl_again = 1 be
 functions
    only of the input itself as covered by the hash, and with no
 dependencies
    on any other state of the router.  Seems unlikely we'd want to ever
 change
    the parsing to be any other way, but perhaps a comment warning future
    editors of those functions would be advisable.

  - Use of digestmap_t in microdescs_add_to_cache() looks like it will
 handle
    repeated hashes, so that worry is assuaged.  These magic 1/2/3
 constants
    seem a bit ugly though.  Can we get some #defines or something?

 a99a7fdfdf5e02ef1460f5a49960e77f2ecc6c8b:

  - This looks okay

 d64041a1ba4ff765a3fe65db8819f20dce3cf8e0:

  - Good catch

 69d7a56972e2982c7c5817ab2139f3e83cba27d5:

  - This is just a whitespace fixup

 24b24e4a97117c295b310ccf0f8da327e9d7e8a3:

  - Yay, we get new unit tests even for code we already had!

 cbde09755392a705b56b4c04290bb2885cef55a7:

  - This looks fine.

 cf4cdd034174600ddedb5145d5eaa7e7601082e9:

  - Looks good.

 dc145f2ec77ed812d24d7c4e4f639e6200533647:

  - Looks good.

 87b32019e93f12dc37f8352750928e5f183ddfd2:

  - Looks good.

 185868241b7130b2f1b24cf8ed2e50c998d1ca4c:

  - Change description sounds fine, but isn't it redundant with the one
    before?

 End code review.

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