[tor-bugs] #25688 [Obfuscation/Snowflake]: proxy-go is still deadlocking occasionally

[Tor Bug Tracker & Wiki]

#25688: proxy-go is still deadlocking occasionally
--------------------------------------------+------------------------------
 Reporter:  dcf                             |          Owner:  cohosh
     Type:  defect                          |         Status:
                                            |  needs_revision
 Priority:  Low                             |      Milestone:
Component:  Obfuscation/Snowflake           |        Version:
 Severity:  Normal                          |     Resolution:
 Keywords:  network-team-roadmap-2019-Q1Q2  |  Actual Points:
Parent ID:                                  |         Points:  3
 Reviewer:                                  |        Sponsor:
--------------------------------------------+------------------------------

Comment (by cohosh):

 Here's a partial solution for the ICECandidateError problem:
 https://github.com/cohosh/snowflake/compare/deadlock

 Replying to [comment:16 dcf]:

 > This code is weird... At first I saw the `answerChan <- struct{}{}` and
 I thought, this channel is only used to send a single signal; maybe this
 would be better as `close(answerChan)` as in
 [https://blog.golang.org/pipelines#TOC_6. this example]. But then I don't
 understand why it's checking `ok` from the channel read--that seems to
 indicate that it's trying to distinguish a send from a channel close...
 But the code only uses one of those two possibilities.
 >
 The code was a bit unusual. I think CreateAnswer was put into a go routine
 because it's blocking on the ICE negotiation, but we need it to complete
 before calling sendAnswer anyway, so I removed the go routine and it
 appears to be functioning fine. This is not a complete solution (see
 below).

 > Replying to [comment:15 arlolra]:
 > > > This error appears to occur *after* OnIceComplete() is fired,
 meaning that in the proxy-go code:
 > >
 > > Ah, so it is a bug in go-webrtc to assume `iceComplete`, when only
 `iceGathering` has completed,
 > > https://github.com/keroserene/go-
 webrtc/blob/master/peerconnection.go#L518-L522
 >
 > Yes, I think you're right.
 It's also worth pointing out that the proxy's completion of ICE to create
 the SDP answer doesn't necessarily imply that OnDataChannel will be called
 (which is what was causing the problem in the first place). In order for
 OnDataChannel to be called, the client has to receive the answer from the
 broker, call SetRemoteDescripter, and trigger the data channel to open.

 The token is being returned only once OnDataChannel has been called:
 {{{
 func datachannelHandler(conn *webRTCConn, remoteAddr net.Addr) {
         defer conn.Close()
         defer retToken()
 }}}
 and the rest of the code assumes that as long as `pc.CreateAnswer`
 succeeded, it doesn't need to worry about returning the token.

 That means that a client can cause the proxy to lose tokens by sending an
 offer, and then doing nothing with the answer. I think it was happening
 because of an ICE error before, but we need to make sure we fix the
 faulty/malicious client case as well.

 This is tricky to solve because there isn't a PeerConnection callback for
 "SDP answer sent, but data channel was never opened".
 >
 > > > It's also worth pointing out that apparently OnICEComplete is being
 deprecated (​peerconnection.go#L518) we are better off not relying on it
 anyway.
 > >
 > > If that's the case, we should remove it there too.
 >
 > Agreed.
 Just noting that the snowflake client also uses the OnICEComplete callback

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