[tor-bugs] #21304 [Obfuscation/Snowflake]: Sanitize snowflake.log

[Tor Bug Tracker & Wiki]

#21304: Sanitize snowflake.log
-----------------------------------+-----------------------------
 Reporter:  arlolra                |          Owner:  cohosh
     Type:  defect                 |         Status:  merge_ready
 Priority:  Medium                 |      Milestone:
Component:  Obfuscation/Snowflake  |        Version:
 Severity:  Normal                 |     Resolution:
 Keywords:  starter                |  Actual Points:
Parent ID:                         |         Points:  1
 Reviewer:                         |        Sponsor:
-----------------------------------+-----------------------------
Changes (by dcf):

 * status:  needs_review => merge_ready


Old description:

> For starters, the timestamps are in the local timezone.  We can make that
> UTC
>
> See also #19026
>
> Known problems:
>  * When the websocket server panics (as in #29125), it writes the client
> IP address to the log:
>    {{{
> 2019/01/18 18:56:29 http2: panic serving X.X.X.X:YYYY: interface
> conversion: *http2.responseWriter is not http.Hijacker: missing method
> Hijack
>    }}}
>  * The broker logs the raw bytes of ICE answers:
>    {{{
> 2019/03/22 06:28:32 Received answer:  [XXX XXX XXX XXX ... XXX]
>    }}}

New description:

 For starters, the timestamps are in the local timezone.  We can make that
 UTC

 See also #19026

 Known problems:
  * The client logs full SDP stanzas, which include local IP addresses.
  * When the websocket server panics (as in #29125), it writes the client
 IP address to the log:
    {{{
 2019/01/18 18:56:29 http2: panic serving X.X.X.X:YYYY: interface
 conversion: *http2.responseWriter is not http.Hijacker: missing method
 Hijack
    }}}
  * The websocket server logs an IP address on certain connection errors.
 (These are probably automated scanners, but still...)
    {{{
 2019/04/01 07:02:50 http: TLS handshake error from X.X.X.X:YYYY:
 acme/autocert: missing server name
 2019/04/01 07:53:03 http: TLS handshake error from X.X.X.X:YYYY: tls:
 oversized record received with length 21536
    }}}
  * The broker logs the raw bytes of ICE answers:
    {{{
 2019/03/22 06:28:32 Received answer:  [XXX XXX XXX XXX ... XXX]
    }}}

--

Comment:

 This looks good to me.

 I think this is ready to merge. It looks like we are not keeping permanent
 logs for snowflake-server; they are getting rotated in /var/log/tor. So we
 don't have to do anything special to purge the old logs.

 Apart from the specific client and broker changes you made
 [https://github.com/cohosh/snowflake/commit/3eb9064438ca6242f935173aed88ec29a0c16c7d
 here], do the client and broker benefit from using the same log scrubber?
 I.e., is it worth it to factor out the log scrubber into its own package
 and share it, after merging it to server?


 Replying to [comment:15 cohosh]:
 > I think for now it's okay to just have the log scrubber. Before I
 started it, I went through and looked at all of the returned errors and I
 don't recall finding any net.OpErrors anyway.

 Agreed, I think the string-based scrubbing is the way to go.

 > {{{
 > const ipv4Address = `\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}`
 > const ipv6Address = `(([0-9a-fA-F]{0,4}:){2,7}([0-9a-fA-F]{0,4})?(` +
 ipv4Address + `))` +
 >         `|(([0-9a-fA-F]{0,4}:){2,7}([0-9a-fA-F]{0,4})?)`
 > const optionalPort = `(:\d{1,5})?`
 > const addressPattern = `((` + ipv4Address + `)|(\[(` + ipv6Address +
 `)\])|(` + ipv6Address + `))` + optionalPort
 > }}}
 >
 > My reason for not going with the full one was that I found it
 unnecessary after expanding my tests with the ones you used. I also found
 that this massive regexp was still scrubbing the fingerprint, meaning
 additional changes still had to be made.

 This looks good to me.

 > Go does not yet support look-ahead or look-behind, which is what we'd
 want to pass this test case. Instead I implemented a work-around that will
 find regular expressions that check for characters surrounding the
 address, but only replaces the address portion of it:

 This is a good way to do it. I'm also fine if fingerprints get scrubbed, I
 don't think that necessarily has to be a test case. On the comment "go
 does ''not yet'' support look ahead or look behind," I think as a
 philosophical matter they plan never ever to implement those.

 > > That's what the "SEND" button at
 https://snowflake.torproject.org/snowflake.html, and the
 {{{NewCopyPasteDialer}}} in client, are for. I'm okay with that mode of
 operation going away.
 > I removed this from the client logs, but if there's now unused
 functionality in the client or proxy code perhaps we should remove that
 too?

 Yes; personally I think we can remove all the fifo copy-paste code. Let's
 do that in another ticket though.

 > Also please let me know if I should be doing something different for
 attributing the code you wrote for this.

 Nothing needed in this regard.

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