[tor-bugs] #20262 [Core Tor/Tor]: Onion services startup time always gets revealed

[Tor Bug Tracker & Wiki]

#20262: Onion services startup time always gets revealed
-----------------------------------------------+---------------------------
 Reporter:  twim                               |          Owner:  twim
     Type:  defect                             |         Status:
                                               |  needs_review
 Priority:  Medium                             |      Milestone:  Tor:
                                               |  0.3.0.x-final
Component:  Core Tor/Tor                       |        Version:
 Severity:  Major                              |     Resolution:
 Keywords:  tor-hs, research, review-group-11  |  Actual Points:
Parent ID:                                     |         Points:
 Reviewer:                                     |        Sponsor:  SponsorR-
                                               |  can
-----------------------------------------------+---------------------------

Comment (by asn):

 Initial review:

 - I handle dozens of trac tickets every week. The fact that #20262 and
 #20082 change the exact same part of the codebase for different reasons
 and are being developed at the same time, confuses me _a lot_ since I
 can't remember what's the rationale for each change and which ticket I
 should consult.

   I think these two tickets should be merged into a single "Revisit onion
 service startup time logic" ticket and a single branch that rules them all
 should be created.

 - If the only point here is to `reduce linkability between hidden service
 instances on the same tor instance` why do we do it for hosts that only
 have a single onion service?

   Also, why not do it for ephemeral services then as well? Isn't that a
 threat there?

   And talking about threats, if we don't insert a delay, who is the
 attacker here? Who can actually link the multiple HS instances? Their
 descriptors will go to different HSDirs most probably, so it's probably
 not the HSDIr. Who is the attacker? I still have not seen a proper
 security analysis here...

 - I'm still sad that this branch does not simplify that part of the code.
 I was assuming that the result of these two tickets would simplify the
 code there and potentially split it into multiple tidy functions. Instead
 it seems like we add more inline spaggheti code and torrc options. I'm
 inclined to not accept any code here that does not make that function easy
 to follow just by skimming it. Also let's add some tests.

 - The changes file mentions that ''Now there is a random delay up to 5m
 for each on-disk service'', but in the code I see `+#define
 DISKSERVICE_SHUFFLING_PERIOD_TESTING (5*60)`. Isn't that only applied for
 testing network?

 - What's the point of the torrc option? It's not documented, and what does
 it even do?Who will ever toggle that thing on? Please let's reduce the
 torrc option bloat.

   I'm looking at the code here:
 {{{
 if (!service->last_upload_time) {
      /* We do obfuscate onion services startup time. Set random delay */
      /* before the first upload. */
       if (options->ObfuscateOnionStartupTime) {
         service->next_upload_time +=
                            (time_t)
 crypto_rand_int(2*options->RendPostPeriod);
       } else {
         /* Non-ephemeral services are started at the same time that links
 */
         /* them and thus reveals that they are operated by same entity. */
         /* Randomizing initial delay for each of these services. */
         if (!is_ephemeral) {
           service->next_upload_time +=
             (time_t) crypto_rand_int(diskservice_shuffling_period);
         }
       }
     }
 }}}
   and the flow is really confusing to me, and I also don't understand why
 these things happen. Why do we use `diskservice_shuffling_period)` in one
 point and `2*options->RendPostPeriod` on the other point.

 - Nitpicking: What's the point of `+#define DISKSERVICE_SHUFFLING_PERIOD
 (0)`?
   Seems to me it adds more code and complexity without increasing
 readability.

 - Nitpicking: There is no nesting or brackets in the else clause, which
 makes that block of code very hard to read.

 - This branch seems to be editing the changelog of #20082?? Please let's
 have a single branch for this whole project.

 All in all, I feel like the security rationale for this bugfix is
 sprinkled over two different trac tickets and a mailing list thread, and
 this makes it very hard to understand why the changes are as is. Also the
 code patches seem more complex than they need to be and a bit hard to
 follow.

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