Skip to content
Permalink
Browse files Browse the repository at this point in the history
Merge pull request from GHSA-p867-fxfr-ph2w
* Fix setting permissions for local sqlite database

Thanks to Jan Schejbal for responsible disclosure!

There used to be a brief moment between creation of the sqlite
database and applying chmod, now there is no such delay.

* Rename self.home to self.test_home in tests

* Actually call the test cleanup

* Add a test for sqlite permissions

* Skip the sqlite chmod test on Windows
  • Loading branch information
ppolewicz committed Feb 23, 2022
1 parent 146aaa6 commit 6247663
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 19 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Fixed
* Fix downloading files with unverified checksum
* Fix setting permissions for local sqlite database (thanks to Jan Schejbal for responsible disclosure!)

## [1.14.0] - 2021-12-23

Expand Down
12 changes: 8 additions & 4 deletions b2sdk/account_info/sqlite_account_info.py
Expand Up @@ -165,9 +165,16 @@ def _connect(self):

def _create_database(self, last_upgrade_to_run):
"""
Make sure that the database is created and sets the file permissions.
Make sure that the database is created and has appropriate file permissions.
This should be done before storing any sensitive data in it.
"""
# Prepare a file
fd = os.open(
self.filename,
flags=os.O_RDWR | os.O_CREAT,
mode=stat.S_IRUSR | stat.S_IWUSR,
)
os.close(fd)
# Create the tables in the database
conn = self._connect()
try:
Expand All @@ -176,9 +183,6 @@ def _create_database(self, last_upgrade_to_run):
finally:
conn.close()

# Set the file permissions
os.chmod(self.filename, stat.S_IRUSR | stat.S_IWUSR)

def _create_tables(self, conn, last_upgrade_to_run):
conn.execute(
"""
Expand Down
45 changes: 30 additions & 15 deletions test/unit/account_info/test_account_info.py
Expand Up @@ -14,6 +14,7 @@
import os
import platform
import shutil
import stat
import tempfile

import pytest
Expand Down Expand Up @@ -319,15 +320,29 @@ def setUp(self, request):
os.unlink(self.db_path)
except OSError:
pass
self.home = tempfile.mkdtemp()
self.test_home = tempfile.mkdtemp()

yield
for cleanup_method in [lambda: os.unlink(self.db_path), lambda: shutil.rmtree(self.home)]:
for cleanup_method in [
lambda: os.unlink(self.db_path), lambda: shutil.rmtree(self.test_home)
]:
try:
cleanup_method
cleanup_method()
except OSError:
pass

@pytest.mark.skipif(
platform.system() == 'Windows',
reason='different permission system on Windows'
)
def test_permissions(self):
"""
Test that a new database won't be readable by just any user
"""
s = SqliteAccountInfo(file_name=self.db_path,)
mode = os.stat(self.db_path).st_mode
assert stat.filemode(mode) == '-rw-------'

def test_corrupted(self):
"""
Test that a corrupted file will be replaced with a blank file.
Expand Down Expand Up @@ -371,7 +386,7 @@ def _make_sqlite_account_info(self, env=None, last_upgrade_to_run=None):
:param dict env: Override Environment variables.
"""
# Override HOME to ensure hermetic tests
with mock.patch('os.environ', env or {'HOME': self.home}):
with mock.patch('os.environ', env or {'HOME': self.test_home}):
return SqliteAccountInfo(
file_name=self.db_path if not env else None,
last_upgrade_to_run=last_upgrade_to_run,
Expand All @@ -380,24 +395,24 @@ def _make_sqlite_account_info(self, env=None, last_upgrade_to_run=None):
def test_uses_default(self):
account_info = self._make_sqlite_account_info(
env={
'HOME': self.home,
'USERPROFILE': self.home,
'HOME': self.test_home,
'USERPROFILE': self.test_home,
}
)
actual_path = os.path.abspath(account_info.filename)
assert os.path.join(self.home, '.b2_account_info') == actual_path
assert os.path.join(self.test_home, '.b2_account_info') == actual_path

def test_uses_xdg_config_home(self, apiver):
with WindowsSafeTempDir() as d:
account_info = self._make_sqlite_account_info(
env={
'HOME': self.home,
'USERPROFILE': self.home,
'HOME': self.test_home,
'USERPROFILE': self.test_home,
XDG_CONFIG_HOME_ENV_VAR: d,
}
)
if apiver in ['v0', 'v1']:
expected_path = os.path.abspath(os.path.join(self.home, '.b2_account_info'))
expected_path = os.path.abspath(os.path.join(self.test_home, '.b2_account_info'))
else:
assert os.path.exists(os.path.join(d, 'b2'))
expected_path = os.path.abspath(os.path.join(d, 'b2', 'account_info'))
Expand All @@ -406,12 +421,12 @@ def test_uses_xdg_config_home(self, apiver):

def test_uses_existing_file_and_ignores_xdg(self):
with WindowsSafeTempDir() as d:
default_db_file_location = os.path.join(self.home, '.b2_account_info')
default_db_file_location = os.path.join(self.test_home, '.b2_account_info')
open(default_db_file_location, 'a').close()
account_info = self._make_sqlite_account_info(
env={
'HOME': self.home,
'USERPROFILE': self.home,
'HOME': self.test_home,
'USERPROFILE': self.test_home,
XDG_CONFIG_HOME_ENV_VAR: d,
}
)
Expand All @@ -423,8 +438,8 @@ def test_account_info_env_var_overrides_xdg_config_home(self):
with WindowsSafeTempDir() as d:
account_info = self._make_sqlite_account_info(
env={
'HOME': self.home,
'USERPROFILE': self.home,
'HOME': self.test_home,
'USERPROFILE': self.test_home,
XDG_CONFIG_HOME_ENV_VAR: d,
B2_ACCOUNT_INFO_ENV_VAR: os.path.join(d, 'b2_account_info'),
}
Expand Down

0 comments on commit 6247663

Please sign in to comment.