the database is created with insecure permissions by default

RESOLVED FIXED

Status

Cloud Services
Server: Sync
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: lsof, Assigned: rfkelly)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa?])

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:13.0) Gecko/20100101 Firefox/13.0.1
Build ID: 20120616215704

Steps to reproduce:

The sqlite database file is created world readable by default. It should only be readable by owner and perhaps group.

Comment 1

6 years ago
Which database?

Comment 2

6 years ago
I see that all of the sqlite databases in the profile directory appear to be created with permissions 644. Even signons.sqlite.

However, the profile directory itself should be permissions 700. That should prevent access to the individual database files.
Component: Firefox Sync: Backend → General
Product: Mozilla Services → Core
QA Contact: sync-backend → general
(Reporter)

Comment 3

6 years ago
The sqlite database file created by the python sync server has world readable permissions.

Updated

6 years ago
Component: General → Server: Sync
Product: Core → Mozilla Services
QA Contact: general → sync-server
Assignee: nobody → rfkelly
(Assignee)

Comment 4

6 years ago
This is the default behaviour for sqlite.  It can be changed by setting the umask for the server process, e.g.:

    $ umask 007
    $ ./bin/paster serve development.ini
    $ ls -l /tmp/ | grep test.db
    -rw-r----- 1 rfk     rfk     13312 Jul  3 16:01 test.db

We could just document this as a known gotcha, but I think it makes sense for us to set a safe default umask.  I'll have to investigate whether it's better to set it in code in the storage backend itself, or as a default config option in the .ini file.
Whiteboard: [qa?]
(Assignee)

Comment 5

6 years ago
I have added a section to the howto on securing your sync server, including notes on setting the umask and disabling the creation of new user accounts:

  https://github.com/mozilla-services/docs/commit/3b737ed15cfa31e9f07abcf24190999a177b0f66

I'm still undecided about whether syncstorage should set the umask itself.  Our code doesn't take an opinion on any similar security matters (e.g. running as a low-privilege user), and it can easily be done in the webserver or the process-management software.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 6

6 years ago
Created attachment 649952 [details] [diff] [review]
patch providing a create_engine() wrapper that sets umask

What we can do in the code is to set the umask to something secure just before calling create_engine(), then set it back to the previous value when finished.  This will result in the sqlite database file being created with more restrictive permissions.

Attached patch adds a "create_engine" wrapper in server-core, so that this logic can be used across all projects.

Fortunately sqlite seems to do the right thing with its journal files, adjusting their permissions to match those on the database itself.  So this change should be sufficient to make sqlite databases secure by default.

You can, of course, change the permissions on the file after its creation to be whatever you want, and this patch will not affect that.
Attachment #649952 - Flags: review?(telliott)
(Assignee)

Comment 7

6 years ago
Created attachment 649953 [details] [diff] [review]
server-storage patch to use the create_engine wrapper

And this patch makes server-storage use the wrapper version of create_engine().
Attachment #649953 - Flags: review?(telliott)
Attachment #649953 - Flags: review?(telliott) → review+
Comment on attachment 649952 [details] [diff] [review]
patch providing a create_engine() wrapper that sets umask

I'm r+ing because it does what the bug is trying to do. I think there's a deeper philosophical question about the presence of sqlalchemy in a generic util lib. Perhaps we need dblib.py for abstracted db functions?
Attachment #649952 - Flags: review?(telliott) → review+
(Assignee)

Comment 9

6 years ago
(In reply to Toby Elliott [:telliott] from comment #8)
> I think there's a
> deeper philosophical question about the presence of sqlalchemy in a generic
> util lib. Perhaps we need dblib.py for abstracted db functions?

Yep.  I've been poking at extracting the db access code from sync2.0 into a separate lib, per Bug 774976.
(Assignee)

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 11

6 years ago
Also applied identical umask-setting logic in sync2.0 codebase:
https://github.com/mozilla-services/server-syncstorage/commit/606f33b3e07dfd0808edae160873dda59300a76c
You need to log in before you can comment on or make changes to this bug.