768856 - the database is created with insecure permissions by default

Status: RESOLVED FIXED

(Cloud Services Graveyard :: Server: Sync, defect)

Description

[lsof]

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

[Gregory Szorc [:gps]]

Which database?

Comment 2

[Gregory Szorc [:gps]]

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.

Comment 3

[lsof]

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

Comment 4

[Ryan Kelly [:rfkelly]]

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.

Comment 5

[Ryan Kelly [:rfkelly]]

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.

Comment 6

[Ryan Kelly [:rfkelly]]

Attachment: 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.

Comment 7

[Ryan Kelly [:rfkelly]]

Attachment: server-storage patch to use the create_engine wrapper

And this patch makes server-storage use the wrapper version of create_engine().

Comment 8

[Toby Elliott [:telliott]]

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?

Comment 9

[Ryan Kelly [:rfkelly]]

(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.

Comment 10

[Ryan Kelly [:rfkelly]]

http://hg.mozilla.org/services/server-core/rev/c509dc2b827b

http://hg.mozilla.org/services/server-storage/rev/057dc637d87f

Comment 11

[Ryan Kelly [:rfkelly]]

Also applied identical umask-setting logic in sync2.0 codebase:
https://github.com/mozilla-services/server-syncstorage/commit/606f33b3e07dfd0808edae160873dda59300a76c