[tor-bugs] #10362 [Pluggable transport]: Deploy FTE as a pluggable transport in PTTBBs

[Tor Bug Tracker & Wiki]

#10362: Deploy FTE as a pluggable transport in PTTBBs
-------------------------------------+-----------------
     Reporter:  asn                  |      Owner:
         Type:  task                 |     Status:  new
     Priority:  normal               |  Milestone:
    Component:  Pluggable transport  |    Version:
   Resolution:                       |   Keywords:
Actual Points:                       |  Parent ID:
       Points:                       |
-------------------------------------+-----------------

Comment (by asn):

 We did a preliminary code review with Nick and Yawning. Here are some
 comments:

 - The max_len check in encoder.py:decode() is a bit weird. IIUC it's the
   only check that controls the size of the input string, and the
   max_len check does:
  {{{
         insufficient = (len(covertext) < self._max_len)
         if insufficient:
             raise DecodeFailureError(
                 "Covertext is shorter than self._max_len, can't decode.")
  }}}

   This confuses me, because I'm used to checks that abort if
   the input string is '''larger''' than the maximum length, whereas your
   check aborts if the input string is smaller than the maximum
   length. Why is that?

 - rank_unrank.cc:DFA::rank() is a bit sketchy. It's not entirely
   obvious to me why the _T array has enough space to accomodate any
   value of 'n' (which is the size of the input string). I don't really
   understand _buildTable() which builds the _T array, and it's also
   undocumented (what is _T even?).

   This, combined with the previous comment might get ugly. But I might be
 misreading the code.

 - Yawning notes that 'std::string attFstFromRegex' is not exception safe.

 - Nick notes that it might be better if we blindly replaced all operator[]
 uses
   with .at()s, which do bound checking. If at() is too slow for us,
   parts of the code that are on the critical path should get analyzed
   and optimized accordingly.

   Parts of the code that can't use at() (if any) should use asserts to
 make
   sure that no out-of-bound read/write ever happens.

 - It seems to me that some of the TODOs left on the rank_unrank.cc
   file might be security related.

 BTW, I mainly looked at the rank() function since IIUC it's the one that
 receives network data. What other functions should we pay attention
 to?

 Also note that our review was mainly looking for security issues. We
 didn't look for correctness.

 We will come back with more comments!

 Thanks!

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