Opened 4 years ago

Closed 4 years ago

#9170 closed defect (fixed)

Don't log IP addresses by default in flashproxy.js

Reported by: dcf Owned by: dcf
Priority: High Milestone:
Component: Archived/Flashproxy Version:
Severity: Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

flashproxy.js's debug option is too sharp for casual use. It shows what's going on but also contains IP addresses. In #9164 was originally posted a log containing lots of IPs.

Add a new query string parameter unsafe_logging to be another obstacle to climb over. Maybe don't document it.

At least these messages need to be scrubbed:

Facilitator: got client:{ host: "X.X.X.X", port: YYYY } relay:{ host: "173.255.221.44", port: 9901 }.
Facilitator: connecting to https://fp-facilitator.org/?r=1&client=X.X.X.X%3AYYYY&client=X.X.X.X%3AYYYY

Child Tickets

Attachments (1)

0001-Don-t-log-IP-addresses-by-default-in-flashproxy.js.patch (5.7 KB) - added by arlolra 4 years ago.

Download all attachments as: .zip

Change History (5)

comment:1 Changed 4 years ago by arlolra

Status: newneeds_review

Attached a patch to scrub those messages.

comment:2 Changed 4 years ago by dcf

Status: needs_reviewneeds_revision

Make the global variable use positive logic: call it SAFE_LOGGING rather than UNSAFE_LOGGING. Make it take the opposite of the query string unsafe_logging boolean. Compare with logic in e.g. flashproxy-client. The SAFE_LOGGING global is because I don't like booleans that have a "not" built into their name. The unsafe_logging parameter is because I want people to have to type the word "unsafe".

Make a safe_repr function instead of calling repr on a safe_obj. Just make safe_repr return "[scrubbed]" for the whole object, don't look inside at the keys.

Don't try to sanitize the URL with a regex. Just print a URL that is the same minus the query string (i.e., "https://fp-facilitator.org/"). If you need another call to build_url that's fine.

I'm now thinking that we should scrub the relay address as well. Someone may be using a private bridge or something.

Did you find any other places where a client address could be logged?

comment:3 in reply to:  2 ; Changed 4 years ago by arlolra

Status: needs_revisionneeds_review

Replying to dcf:

Made the requested revisions and updated the patch.

Did you find any other places where a client address could be logged?

I looked through all the puts(). There were three error conditions worth scrubbing, though that may hinder bug reports.

Not sure if you want to try and mitigate it but if Xhr logging is enabled in the console then a lot of this info is visible. It may be worth changing the facilitator polling method from GET to POST to avoid the query string parameters.

"Firefox can't establish a connection to the server at ws://xxx.xxx.xxx.xxx:xxxx/." can probably be caught and safely discarded.

comment:4 in reply to:  3 Changed 4 years ago by dcf

Resolution: fixed
Status: needs_reviewclosed

Replying to arlolra:

Replying to dcf:

Made the requested revisions and updated the patch.

Did you find any other places where a client address could be logged?

I looked through all the puts(). There were three error conditions worth scrubbing, though that may hinder bug reports.

Not sure if you want to try and mitigate it but if Xhr logging is enabled in the console then a lot of this info is visible. It may be worth changing the facilitator polling method from GET to POST to avoid the query string parameters.

"Firefox can't establish a connection to the server at ws://xxx.xxx.xxx.xxx:xxxx/." can probably be caught and safely discarded.

I think this is adequate. Merged now.

Note: See TracTickets for help on using tickets.