Database: Set 0640 permissions on SQLite database file

[aknuds1]

What this PR does / why we need it:
#8283 points out that the we should restrict access to the SQLite database file, so only the owner can use it. This PR ensures the file has permissions 0640 (read/write for owner, read for group) when it gets created. If the file already exists and has broader permissions, a warning will be logged.

@wz2b @austinbutler does this look right to you?

Which issue(s) this PR fixes:
Fixes #8283.

Special notes for your reviewer:

[papagian]

I have tested both with existing and not existing sqlite database file and with mysql database and it works as expected.

[aknuds1]

I have tested both with existing and not existing sqlite database file and with mysql database and it works as expected.

Thanks @papagian - do you have any opinion on whether we should change the permissions of existing SQLite files or not? Maybe just log a warning to the administrator? I'm just worried in case there are valid scenarios where the permissions (or owner?) should be changed by the operator.

[papagian]

Thanks @papagian - do you have any opinion on whether we should change the permissions of existing SQLite files or not? Maybe just log a warning to the administrator? I'm just worried in case there are valid scenarios where the permissions (or owner?) should be changed by the operator.

I didn't think about it. I'm not sure. Maybe the admin could overwrite the permissions by the configuration.

[wz2b]

Thanks @papagian - do you have any opinion on whether we should change the permissions of existing SQLite files or not? Maybe just log a warning to the administrator? I'm just worried in case there are valid scenarios where the permissions (or owner?) should be changed by the operator.

I didn't think about it. I'm not sure. Maybe the admin could overwrite the permissions by the configuration.

I think you should. Not only is it the user table but also the data_source table. In theory plugins will store passwords encrypted in secureJsonData but they don't all. Just an an example, the Zabbix datasource does store in secureJsonData now, but it didn't always, and if you upgrade the datasource it doesn't clear out the plaintext 'password' column that was there previously (even though it doesn't use it any more). What that means is if somebody didn't set up a zabbix service account with the right permissions, you can steal this password from grafana.db and do bad things to zabbix. We don't want somebody to be able to point to some exploit and have the reason be that grafana allowed the password to escape.

[kylebrandt]

Thanks @papagian - do you have any opinion on whether we should change the permissions of existing SQLite files or not? Maybe just log a warning to the administrator? I'm just worried in case there are valid scenarios where the permissions (or owner?) should be changed by the operator.

I didn't think about it. I'm not sure. Maybe the admin could overwrite the permissions by the configuration.

One may want to give a group of users access to the sqlite db. So forcing group permissions to 0 could be problematic.

[wz2b]

One may want to give a group of users access to the sqlite db. So forcing group permissions to 0 could be problematic.

The Ubuntu package (maybe all debian) yields this:

-rw-r--r-- 1 grafana grafana 260056 July 15 13:29 /var/lib/grafana/grafana.db

so it creates a 'grafana' user and a 'grafana' group. So I'd be fine with the permissions being 640 (u+rw,g+r if I did that right). Normally there's nobody in the 'grafana' group other than the 'grafana' user. So, this way, if somebody did have some weird reason to add a second user to the 'grafana' group (like say for backing it up) that would still work.

Regarding changing an existing file's permissions if it exists: I think this is a good idea, but it's not something I would do on the go side. I would have grafana set 0640 if it creates the file; but as for changing a file that already exists, in thinking about this, it's really something I would put into the postinst script. If you want to be nice you'd even make it prompt the user whether or not they wanted you to do it.

[kylebrandt]

I don't know enough about the various files the Grafana process can create, or the packaging ... but my fuzzy old knowledge says that maybe we should be setting the umask for the process so all things created are only accessible by grafana user/group - but I don't know if that might cause problems - and don't think it should be a blocker getting more secure specific DB perms in place.

[marefr]

Think logging a warning if permissions are to "broad" on sqlite file should be enough. Then handle the migration thru documentation because older Grafana instances can apply that change manually without upgrading, which is good from a security perspective.

[aknuds1]

I'll change the permissions to 0640 give the group read access. That sound alright to you @papagian @kylebrandt @marefr?

@papagian @kylebrandt Do you agree with @marefr about just logging a warning if the file exists with broader permissions?

[papagian]

I'll change the permissions to 0640 give the group read access. That sound alright to you @papagian @kylebrandt @marefr?

Being created with the grafana group is not always the case. For instance if you are in development mode then the file is created with the current user/group. However, I think that restricting read access to the group can have implications so I would say yes.

@papagian @kylebrandt Do you agree with @marefr about just logging a warning if the file exists with broader permissions?

yes, I think that is much better.

[aknuds1]

I made a change to only log a warning if the SQLite file permissions are broader than expected (0640).

[aknuds1]

@marefr @kylebrandt @papagian Please have another look.

[kylebrandt]

Two possible code considerations for checking an error and moving this to its own function, but leave those to your judgement.

I agree with the logging choice.

[papagian]

I've tested it again and works as expected in my development environment.