nextcloud / issuetemplate

:construction: Nextcloud app for easy bug reporting with prefilled issue templates
GNU Affero General Public License v3.0
14 stars 10 forks source link

Privacy issue: Issue template is to "chatty" #27

Open ho1ger opened 7 years ago

ho1ger commented 7 years ago

I like to use issuetemplate because it helps providing important info to the developers. I am totally happy to share what apps I use, what device the instance is running on, etc. But I am not happy to share info that would allow others to identify my server, the url(s) it listens to, etc. In my opinion the following fields should be considered as SENSITIVE VALUE and REMOVED. I even doubt that this info is important for people debugging an issue...

Edit: Probably a good starter issue. I've added some implementation hints here: https://github.com/nextcloud/issuetemplate/issues/27#issuecomment-330947386

enoch85 commented 7 years ago

I'd like to add

Can we get attention on this @juliushaertl ?

Btw, thanks for the last update, it feels like 1.0. ;)

e-alfred commented 7 years ago

mail_from_address should be private too.

Fiech commented 7 years ago

I would also include:

in this list of unnecessarily extensive default information.

enoch85 commented 7 years ago

@juliushaertl Any idea when this could be implemented?

I would do it if I knew how.

juliusknorr commented 7 years ago

@enoch85 Cannot make any estimations. I currently have a lot of other stuff todo.

If you want to dig into, it should be fairly easy. We need a list of the configuration keys that should be filtered and just work though the existing configuration items and remove them when they are in the list: https://github.com/nextcloud/issuetemplate/blob/master/lib/Settings/Admin.php#L248-L263

enoch85 commented 7 years ago

@juliushaertl I feel you, same for me. Can't give much time to Nextcloud I'm afraid. :(

Keep it up!

Fiech commented 7 years ago

I could give it a shot on the weekend and make a pull request. Is this if-true-condition something left in there for testing purposes?

I've also noticed, that there would be the possibility to adjust the $sensitiveValues-Member in the SystemConfig-class, we maybe could adjust. Or you think this would be too intrusive?

juliusknorr commented 7 years ago

@Fiech That would be great. It would indeed make sense to create a PR against the server repo to have the sensitiveValues list updated there.

Fiech commented 7 years ago

Ok, I made two PRs. One against the server repo and one against this one. Everything but the hostname issue is being taken care off by the server one, but for the hostname to disappear, we have to change the ServerSection file.

Actually, the method used (constant PHP_OS) is not the best anyway, because PHP_OS is the OS PHP was built on, not the one it's running on. Also the actual content detail of the constant is not the same for each system.

I changed it to use php_uname() instead. It's a bit ugly, because this function does not accept more than one mode selector at a time...

Also, sorry for the many mentions, but apparently I'm too stupid to write proper english today...

Fiech commented 7 years ago

Ok, so on request by @nickvergessen we redo the filtering for trusted_domains and overwrite.cli.url directly in the app and only filter out the actual domain.

Maybe someone can reopen this issue?

Fiech commented 7 years ago

Ok, so I thought about this a bit... according to @nickvergessen wrongly configured trusted_domains and overwrite.cli.url is one of the more reported issues. So actually completely filtering out these values ist most likely counterproductive in most cases.

We could now come up with a convoluted way to filter out sensitive information like:

in a way that removes the sensitive part allthewhile leaving possibly misconfigured parts as is, for example with a mix of regular expressions and the filter_var function.

The problem is that this would require the configuration already to be at least syntactically corrrect, something we cannot reasonably expect. Also this kind of catch-all solution would be a hell of a hack...

So instead, I would suggest, we offer the possibility to quickly remove all sensitive values with the click of a button (together with a warning that this might impede the helping process) and also notifiy the user to the possible sensitive values being sent to the issue tracker for self-censoring (who knows, maybe they then already notice something being wrong with them).

Thoughts?

enoch85 commented 7 years ago

So instead, I would suggest, we offer the possibility to quickly remove all sensitive values with the click of a button (together with a warning that this might impede the helping process) and also notifiy the user to the possible sensitive values being sent to the issue tracker for self-censoring (who knows, maybe they then already notice something being wrong with them).

I think that sounds like a good idea. :+1:

heinrichmartin commented 6 years ago

Just to make sure that all fields from #7959 are mentioned here, too.

{
    "system": {
        "instanceid": "***FAILED TO REMOVE SENSITIVE VALUE***",
        "trusted_domains": [
            "***FAILED TO REMOVE SENSITIVE VALUE***"
        ],
        "overwrite.cli.url": "***FAILED TO REMOVE SENSITIVE VALUE***",
        "mail_from_address": "***FAILED TO REMOVE SENSITIVE VALUE***",
        "mail_domain": "***FAILED TO REMOVE SENSITIVE VALUE***",
        "mail_smtphost": "***FAILED TO REMOVE SENSITIVE VALUE***",
    }
}
t-markmann commented 6 years ago

we use an S3 Bucket as primary storage. Could this info be removed as well, please?

"objectstore": {
    "class": "OC\\Files\\ObjectStore\\S3",
    "arguments": {
        "bucket": "<PRIVATE>",
        "autocreate": true,
        "key": "<PRIVATE>",
        "secret": "<PRIVATE>",
        "hostname": "<PRIVATE>",
        "port": 443,
        "use_ssl": true,
        "region": "de",
        "use_path_style": true
    }
},

additionally i removed "overwrite.cli.url": "",

and "dbtableprefix": "",

but this might be very special case here

gohrner commented 5 years ago

In the LDAP config info, it includes a key-value-pair:

ldap_agent_password: adkghfadkjhfalsdkghasdfhawofhw=

This probably also should be anonymized - and maybe some additional stuff in the LDAP section like exact user names etc.

TP75 commented 5 years ago

IMHO there is room for improvement in protecting the privacy of a server installation in the template report produced by version 0.5.0 of the tool.

{
    "debug": false,
    "trusted_domains": [
        "### manually REMOVED SENSITIVE VALUE###",
        "### manually REMOVED SENSITIVE VALUE###"
    ],

    "overwrite.cli.url": "### manually REMOVED SENSITIVE VALUE###",

}

see #157

nickvergessen commented 5 years ago

See the comment here, why those values are not "sensitive": https://github.com/nextcloud/server/pull/7004/files#r147828111

TP75 commented 5 years ago

@nickvergessen Thank you. Now I better understand the burden to the forum and the developers involved when basic configuration problems are reported as bugs. Nobody is perfect and this mishap could happen with anybody unfortunately.

However, IMHO this does not prove the a.m. values are not "sensitive" and I continue in calling exposing the server domain and cli.url settings unwise in general. Please recall one of Nextcloud main advertising is:

Protecting your data The self-hosted productivity platform that keeps you in control

Ref the Issue Template app I would prefer a more flexible approach proposedly with an informative text and a checkbox while producing the report by help of the tool.

I am aware I could tamper with the settings of the tool locally and/or may edit any "sensitive" values manually before copying the report as a GitHub issue. Last not least there is an enterprice solution available.

Nevertheless in the light of the Nextcloud principles and of the FOSS project community efforts I would prefer an improvement as proposed above and as mentioned in #157 before.

nickvergessen commented 5 years ago

Please recall one of Nextcloud main advertising is:

Protecting your data The self-hosted productivity platform that keeps you in control

Sorry, I really disagree there. A domain is nothing sensitive. There are public records about those. Just search for ones with a cloud subdomain being registered. You can also look at letsencrypt directories to see which cloud.* domains have a SSL cert.

You should always treat your software/server like it is accessible by anyone, because that is what it is, when it is in the public internet.

Your data is still save and protected. But your login page and domain are no secrets. Any other claim is Security by obscurity

ho1ger commented 5 years ago

I think as long as people feel unwell about certain points of information -- which are not even relevant for debugging -- included in the template, this information should be excluded as it might deter people from submitting bug reports.

nickvergessen commented 5 years ago

Well this issue here is still open. We can add an option to remove the stuff. Just wanted to state why it 1. shouldn't be default 2. does not make anything more secure.

But I also get your points. So fine by me (see the comments in the PR I linked above for a good solution) as long as it's opt in, to allow easy-help if people are not "paranoid".

TP75 commented 5 years ago

I welcome this discussion. However, I disagree in some aspects.

Your data is still save and protected. But your login page and domain are no secrets. Any other claim is Security by obscurity

One should not flip Privacy and Security without applicable difference. Furthermore, the tool is aimed at providing data on presumably "bugged" i.e. possibly "failing" environments. I dont want to reach too far but there is good cause for the principles of Responsible disclosure in failing information technology.

I appreciate the offer of a more open approach and will welcome further efforts. Thank you.

nickvergessen commented 5 years ago

Just so you know, this is a real issue, see: https://github.com/nextcloud/spreed/issues/2312 :P In the meantime I'm experienced enough to know the values which are bugged. But still without the values the user has set, there is no way to help them.

SimJoSt commented 4 years ago

Is there a way for apps to communicate to this one, which app specific values are sensitive?

The Sentry app also has sensitive values that should be redacted:

    "sentry.dsn": "***REMOVED SENSITIVE VALUE***",
    "sentry.public-dsn": "***REMOVED SENSITIVE VALUE***",
    "sentry.csp-report-url": "***REMOVED SENSITIVE VALUE***",
nickvergessen commented 4 years ago

Yes, send a pull request to the server changing this file: https://github.com/nextcloud/server/blob/master/lib/private/SystemConfig.php#L41

jwilleke commented 6 months ago

So how can the "filtered" values be obtained? As I am seeing: Nextcloud trustedProxies has malformed entries

And have no method to view what they are set at in the running instance.

nickvergessen commented 6 months ago

So how can the "filtered" values be obtained?

Either occ config:list system --private or better look into config/config.php directly

jwilleke commented 6 months ago

Thanks for the reply.