[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 kpdyer):

 Replying to [comment:2 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?

 The name self._max_len is an unfortunate misnomer. It refers to the
 maximum length strings that we precompute for being able to (un)rank
 strings in our language.

 In fact, for efficiency purposes we (un)rank *only* strings that exactly
 self._max_len. That is, for a regular expression R and k = self._max_len,
 all strings we put on the wire are of the format L(R)_k [concat] .*, where
 L(R)_k is the set of all strings in L(R) that are exactly length k. So,
 insufficient, when false, means that we know that our ciphertext is not
 long enough to unrank, so we should bail.

 > - 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.

 The buildTable algorithm is documented in Appendix A of:
 http://eprint.iacr.org/2012/494.pdf.

 Our precomputation table _T for value _T[q][i], where q is a state in our
 DFA and i is a non-negative integer, is the number of unique accepting
 paths in our minimized DFA of length i from state q.

 If it helps, I can put an explicit paragraph that explains this in the
 comments of the header file.

 Also, worth noting that fte/encoder.py:133 we ensure that we never input
 more than self._max_len bytes into rank(). However, it is certainly a good
 idea for us to exercise defense in depth and check at all levels.

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

 Well spotted, I will fix this.

 > - 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.

 I will try replacing all [] operators with .at(), and will report on the
 performance impact.

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

 I agree.

 > 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?

 DFA::rank() in rank_unarnk.cc and DFA_rank() in cDFA.cc are the two to
 look at.

 > 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!

 Thanks to you too!

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