Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added ability to hide sensative logs #825

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JackSeaton
Copy link

I know this isn't ready to be merged, but the main idea is there.

Below is an example of posting some splash code, along with some variables to be executed (I pretty printed the JSON for readability):

[-] Log opened.
[-] Splash version: 3.2
[-] Qt 5.9.1, PyQt 5.9, WebKit 602.1, sip 4.19.3, Twisted 16.1.1, Lua 5.2
[-] Python 3.5.2 (default, Nov 23 2017, 16:37:01) [GCC 5.4.0 20160609]
[-] Open files limit: 1048576
[-] Can't bump open files limit
[-] Xvfb is started: ['Xvfb', ':437071335', '-screen', '0', '1024x768x24', '-nolisten', 'tcp']
QStandardPaths: XDG_RUNTIME_DIR not set, defaulting to '/tmp/runtime-root'
[-] proxy profiles support is enabled, proxy profiles path: /etc/splash/proxy-profiles
[-] verbosity=1
[-] slots=50
[-] argument_cache_max_entries=500
[-] Web UI: enabled, Lua: enabled (sandbox: enabled)
[-] Server listening on 0.0.0.0:8050
[-] Site starting on 8050
[-] Starting factory <twisted.web.server.Site object at 0x7fd12b120a20>
libpng warning: iCCP: known incorrect sRGB profile
libpng warning: iCCP: known incorrect sRGB profile
process 1: D-Bus library appears to be incorrectly set up; failed to read machine uuid: UUID file '/etc/machine-id' should contain a hex string of length 32, not length 0, with no other text
See the manual page for dbus-uuidgen to correct this issue.
In the main
[events] {
    "rendertime": 0.043580055236816406,
    "method": "POST",
    "user-agent": "PostmanRuntime/7.3.0",
    "load": [
        0.12,
        0.05,
        0.01
    ],
    "status_code": 200,
    "fds": 16,
    "timestamp": 1539298202,
    "path": "/execute",
    "active": 0,
    "qsize": 0,
    "args": {
        "posted_json": {
            "password": "Sup3rSaf3",
            "email": "[email protected]"
        },
        "lua_source": "function main(splash)\n\tprint('In the main')\nend",
        "uid": 140536347530744
    },
    "maxrss": 90304,
    "_id": 140536347530744
}
[11/Oct/2018:22:50:01 +0000] "POST /execute HTTP/1.1" 200 - "-" "PostmanRuntime/7.3.0"

As you can clearly see it logs posted_json which contains our sensitive info (email, password, proxy configs, etc.) as well as lua_source which can also contain sensitive information.

As a solution I've added a command line arg that will disable the logging of these features.

Below is an example of the same code being posted to the endpoint with my changes in place

Log opened.
Splash version: 3.2
Qt 5.9.1, PyQt 5.9, WebKit 602.1, sip 4.19.3, Twisted 16.1.1, Lua 5.2
Python 3.5.2 (default, Nov 23 2017, 16:37:01) [GCC 5.4.0 20160609]
Open files limit: 1048576
Can't bump open files limit
Xvfb is started: ['Xvfb', ':1593578320', '-screen', '0', '1024x768x24', '-nolisten', 'tcp']
QStandardPaths: XDG_RUNTIME_DIR not set, defaulting to '/tmp/runtime-root'
proxy profiles support is enabled, proxy profiles path: /etc/splash/proxy-profiles
verbosity=1
slots=50
argument_cache_max_entries=500
Web UI: enabled, Lua: enabled (sandbox: enabled)
Server listening on 0.0.0.0:8050
Site starting on 8050
Starting factory <twisted.web.server.Site object at 0x7fb258b12a20>
libpng warning: iCCP: known incorrect sRGB profile
libpng warning: iCCP: known incorrect sRGB profile
process 1: D-Bus library appears to be incorrectly set up; failed to read machine uuid: UUID file '/etc/machine-id' should contain a hex string of length 32, not length 0, with no other text
See the manual page for dbus-uuidgen to correct this issue.
In the main
[events] {
    "args": {
        "uid": 140403968947704
    },
    "timestamp": 1539298443,
    "client_ip": "172.17.0.1",
    "fds": 16,
    "_id": 140403968947704,
    "status_code": 200,
    "rendertime": 0.03467202186584473,
    "path": "/execute",
    "method": "POST",
    "load": [
        0.08,
        0.07,
        0.01
    ],
    "active": 0,
    "user-agent": "PostmanRuntime/7.3.0",
    "qsize": 0,
    "maxrss": 90444
}
[11/Oct/2018:22:54:02 +0000] "POST /execute HTTP/1.1" 200 - "-" "PostmanRuntime/7.3.0"

As you can see, there's no lua_source or posted_json

Please feel free to comment on this PR and we can work together to get the feature wrapped into splash!

Thanks
Jack

@kmike
Copy link
Member

kmike commented Oct 22, 2018

Hi! I think it makes sense to have a way to prevent args from being put to logs. It is not only about security: if you're doing large crawls, and use lua scripts or cookies heavily, args may take a lot of disk space, and cause quite a lot of IO.


if self.hide_passed_json_and_lua_source:
if 'posted_json' in options:
del options['posted_json']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should be hard-coding specific keys. I can see two ways of handling it:

  1. dont_log argument, which can disable logging of specific fields per-request (posted_json and lua_source in this case);
  2. --log-args 0 startup argument, which disables logging of args completely

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I didn't think this was necessarily the best way to go about it. I was more so trying to get the idea out and we can go from there.

--log-args 0 would be great as long as it only stops the logging of this startup/shutdown args and not stops the logging from within the script (e.x. print())

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking --log-args 0 disables logs only for Splash arguments, nothing else - i.e. "args" value in the log you've pasted

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants