[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [tor-bugs] #18320 [Core Tor/Tor]: Clear old entries from the key-pinning journal file



#18320: Clear old entries from the key-pinning journal file
-------------------------------------------------+-------------------------
 Reporter:  teor                                 |          Owner:  andrea
     Type:  defect                               |         Status:
 Priority:  Medium                               |  needs_information
Component:  Core Tor/Tor                         |      Milestone:  Tor:
 Severity:  Normal                               |  0.2.9.x-final
 Keywords:  tor-dos, TorCoreTeam201607, review-  |        Version:
  group-3                                        |     Resolution:
Parent ID:  #17293                               |  Actual Points:
 Reviewer:  dgoulet                              |         Points:  3
                                                 |        Sponsor:
                                                 |  SponsorU-can
-------------------------------------------------+-------------------------
Changes (by dgoulet):

 * keywords:  tor-dos, TorCoreTeam201606, review-group-3 => tor-dos,
     TorCoreTeam201607, review-group-3
 * status:  needs_review => needs_information


Comment:

 Only one thing. The rest lgtm;

 '''DG1''': In `keypin_load_journal_impl()`, the last "add line to pruner"
 call below assumes no duplicate.

 {{{
 if (pruner) keypin_add_line_to_pruner(pruner, ent, cp, len);
 }}}

 Now because there is an entry `ent`, we get to
 `keypin_add_or_replace_entry_in_map()` which if duplicate, returns `0`
 then setting the `ent` pointer to NULL. Then back in
 `keypin_load_journal_impl()`, if `also_add_to_main_map` is set, then we'll
 try to add a NULL entry which I doubt it will go well :).

 Here is the good news though, I don't see any call to
 `keypin_load_journal_impl()` with both a pruner and `also_add_to_main_map`
 set to `1` (even in the test). So in the end, why is
 `also_add_to_main_map` useful at all? If it is useful for let's say future
 use maybe?, we should handle the duplicate issue above then. If the
 assumption is that there shouldn't be duplicates in the pruner when
 loading it, we should make sure of it because this data comes from disk
 thus "untrusted" source.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/18320#comment:28>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs