[tor-bugs] #13338 [Tor]: Rewrite tor-fw-helper in Go (or another memory-safe language)

[Tor Bug Tracker & Wiki]

#13338: Rewrite tor-fw-helper in Go (or another memory-safe language)
-----------------------------+------------------------------
     Reporter:  arma         |      Owner:  yawning
         Type:  enhancement  |     Status:  assigned
     Priority:  minor        |  Milestone:  Tor: unspecified
    Component:  Tor          |    Version:
   Resolution:               |   Keywords:  flashproxy
Actual Points:               |  Parent ID:  #5213
       Points:               |
-----------------------------+------------------------------

Comment (by yawning):

 Replying to [comment:10 yawning]:
 > Turns out miniupnpd crashes when you try to remove mappings via NAT-PMP
 (Removing them via UPnP works fine).  I'm not sure if it's a quirk with
 the version on my test device or a freak accident, but this doesn't look
 good for enhancing our helper to allow for any sort of graceful cleanup,
 at least when NAT-PMP is in the picture (Since I assume crashing NAT-PMP
 stacks running on routers in the wild falls under "Unacceptable
 Behavior").

 Looked into it further, still not sure if crashing is going to be a
 consistent thing, but deleting mappings is broken.  What the RFC says:
 {{{
    A client MAY also send an explicit packet to request deletion of a
    mapping that is no longer needed.  A client requests explicit
    deletion of a mapping by sending a message to the NAT gateway
    requesting the mapping, with the Requested Lifetime in Seconds set to
    zero.  The Suggested External Port MUST be set to zero by the client
    on sending, and MUST be ignored by the gateway on reception.
 }}}

 So a correct "delete this specific mapping yo" request has "Internal Port"
 set, "Suggested External Port" set to 0, and "Requested Port Mapping
 Lifetime in Seconds" set to 0.  Simple enough, the router is supposed to
 have a table of mappings based on the client address/ports.

 What the
 [https://github.com/miniupnp/miniupnp/blob/master/miniupnpd/natpmp.c code]
 does:
 {{{
   if(eport==0)
     eport = iport;
   /* TODO: accept port mapping if iport ok but eport not ok
    * (and set eport correctly) */
   if(lifetime == 0) {
     /* remove the mapping */
     if(iport == 0) {
       /* Yawning: Not taken */
     } else {
       /* To improve the interworking between nat-pmp and
        * UPnP, we should check that we remove only NAT-PMP
        * mappings */
       r = _upnp_delete_redir(eport, proto);
       /*syslog(LOG_DEBUG, "%hu %d r=%d", eport, proto, r);*/
       if(r<0) {
         syslog(LOG_ERR, "Failed to remove NAT-PMP mapping eport %hu,
 protocol %s",
           eport, (proto==IPPROTO_TCP)?"TCP":"UDP");
         resp[3] = 2;    /* Not Authorized/Refused */
       }
     }
     eport = 0; /* to indicate correct removing of port mapping */
 }}}

 So, sending RFC compliant removal requests means that miniupnpd will
 misbehave at best the moment the interal port is not the external port (as
 likely in the use case that caused me to look into this).  I assume what
 is happening is that `_upnp_delete_redir()` with a bogus external port
 (since the daemon is just assuming it's identical to the internal port) is
 blowing up, but even if the stack didn't crash, the mapping won't get
 removed.

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