mirror of
https://github.com/pgadmin-org/pgadmin4.git
synced 2026-11-02 20:30:17 -06:00
fix: tighten DATA_DIR file/dir permissions at creation
Two related hardening changes around the pgAdmin data directory: 1. Atomic 0o600 for pgadmin4.db ---------------------------- pgadmin/__init__.py:run_migration_for_sqlite() and setup.py:setup_db()'s run_migration_for_sqlite() previously chmod'd pgadmin4.db to 0o600 *after* SQLAlchemy/SQLite created it via db_upgrade(). With a typical 0o022 umask, the file existed at 0o644 between SQLite's create and pgAdmin's chmod — a TOCTOU window. Practically narrow because the parent dir is 0o700, but easy to close: wrap the migration call in `os.umask(0o077)` so the file is born 0o600. Both code paths use try/finally so the prior umask is restored even on migration failure (a raise during db_upgrade no longer leaves the rest of the worker running with 0o077 in effect). The post-hoc chmod is kept as belt-and-suspenders for the case where the file already existed at a wider mode from an older install. 2. 0o700 for sensitive DATA_DIR subdirectories -------------------------------------------- setup/data_directory.py previously chmod'd only SESSION_DB_PATH and the parent dir of SQLITE_PATH to 0o700. STORAGE_DIR (user uploads including saved cloud-deployment certs/keys), AZURE_CREDENTIAL_CACHE _DIR (MSAL token cache files — real Azure credentials), KERBEROS_ CCACHE_DIR (Kerberos credential caches), and the directory holding pgadmin4.log (stack traces frequently capture sensitive context) were left at umask-default (typically 0o755 — directory listing readable by any local user). Refactor the create-loop to track which directories were newly created on this invocation, then apply chmod 0o700 uniformly to all sensitive dirs at once. Errors (e.g., chmod on a mounted volume in OpenShift) emit a WARNING but don't abort — same lenient pattern the existing SQLITE_PATH-dir chmod already used. pgadmin4.log itself is still 0o644 (umask-default) because Python's logging.FileHandler doesn't take a mode argument; tracked as a follow-up task to subclass the handler.
This commit is contained in:
1 parent
3b36dd3964
commit
fb9ce563fb
3 files changed
+125
-66
No files matched your search
+56
-39
@@ -403,50 +403,67 @@ def create_app(app_name=None):
|
|||||||
backup_db_file()
|
backup_db_file()
|
||||||
|
|
||||||
def run_migration_for_sqlite():
|
def run_migration_for_sqlite():
|
||||||
# Run migration for the first time i.e. create database
|
# Tighten the process umask while we may be creating the SQLite
|
||||||
# If version not available, user must have aborted. Tables are not
|
# DB file. The post-hoc chmod below sets 0o600, but only AFTER
|
||||||
# created and so its an empty db
|
# SQLAlchemy/SQLite has already created the file with the
|
||||||
if not os.path.exists(SQLITE_PATH) or get_version() == -1:
|
# umask-default mode (0o644 on a typical 0o022 umask). That's a
|
||||||
# If running in cli mode then don't try to upgrade, just raise
|
# TOCTOU window — small in practice because the parent dir is
|
||||||
# the exception
|
# 0o700, but easy to close. With umask 0o077 in effect during
|
||||||
if not cli_mode:
|
# creation, the file is born 0o600.
|
||||||
upgrade_db()
|
_saved_umask = os.umask(0o077)
|
||||||
|
try:
|
||||||
|
# Run migration for the first time i.e. create database
|
||||||
|
# If version not available, user must have aborted. Tables
|
||||||
|
# are not created and so its an empty db
|
||||||
|
if not os.path.exists(SQLITE_PATH) or get_version() == -1:
|
||||||
|
# If running in cli mode then don't try to upgrade, just
|
||||||
|
# raise the exception
|
||||||
|
if not cli_mode:
|
||||||
|
upgrade_db()
|
||||||
|
else:
|
||||||
|
if not os.path.exists(SQLITE_PATH):
|
||||||
|
raise FileNotFoundError(
|
||||||
|
'SQLite database file "' + SQLITE_PATH +
|
||||||
|
'" does not exists.')
|
||||||
|
raise RuntimeError(
|
||||||
|
'The configuration database file is not valid.')
|
||||||
else:
|
else:
|
||||||
if not os.path.exists(SQLITE_PATH):
|
schema_version = get_version()
|
||||||
raise FileNotFoundError(
|
|
||||||
'SQLite database file "' + SQLITE_PATH +
|
|
||||||
'" does not exists.')
|
|
||||||
raise RuntimeError(
|
|
||||||
'The configuration database file is not valid.')
|
|
||||||
else:
|
|
||||||
schema_version = get_version()
|
|
||||||
|
|
||||||
# Run migration if current schema version is greater than the
|
# Run migration if current schema version is greater than
|
||||||
# schema version stored in version table
|
# the schema version stored in version table
|
||||||
if CURRENT_SCHEMA_VERSION > schema_version:
|
if CURRENT_SCHEMA_VERSION > schema_version:
|
||||||
# Take a backup of the old database file.
|
# Take a backup of the old database file.
|
||||||
try:
|
try:
|
||||||
prev_database_file_name = \
|
prev_database_file_name = \
|
||||||
"{0}.prev.bak".format(SQLITE_PATH)
|
"{0}.prev.bak".format(SQLITE_PATH)
|
||||||
shutil.copyfile(SQLITE_PATH, prev_database_file_name)
|
shutil.copyfile(
|
||||||
except Exception as e:
|
SQLITE_PATH, prev_database_file_name)
|
||||||
app.logger.error(e)
|
except Exception as e:
|
||||||
|
app.logger.error(e)
|
||||||
|
|
||||||
upgrade_db()
|
upgrade_db()
|
||||||
else:
|
else:
|
||||||
# check all tables are present in the db.
|
# check all tables are present in the db.
|
||||||
is_db_error, invalid_tb_names = check_db_tables()
|
is_db_error, invalid_tb_names = check_db_tables()
|
||||||
if is_db_error:
|
if is_db_error:
|
||||||
app.logger.error(
|
app.logger.error(
|
||||||
'Table(s) {0} are missing in the'
|
'Table(s) {0} are missing in the'
|
||||||
' database'.format(invalid_tb_names))
|
' database'.format(invalid_tb_names))
|
||||||
backup_db_file()
|
backup_db_file()
|
||||||
|
|
||||||
# Update schema version to the latest
|
# Update schema version to the latest
|
||||||
if CURRENT_SCHEMA_VERSION > schema_version:
|
if CURRENT_SCHEMA_VERSION > schema_version:
|
||||||
set_version(CURRENT_SCHEMA_VERSION)
|
set_version(CURRENT_SCHEMA_VERSION)
|
||||||
db.session.commit()
|
db.session.commit()
|
||||||
|
finally:
|
||||||
|
# Always restore the prior umask — including the exception
|
||||||
|
# paths above, so a migration failure doesn't leave the
|
||||||
|
# whole process running with 0o077.
|
||||||
|
os.umask(_saved_umask)
|
||||||
|
|
||||||
|
# Belt-and-suspenders: covers the case where the file already
|
||||||
|
# existed at a wider mode from an older install.
|
||||||
if os.name != 'nt':
|
if os.name != 'nt':
|
||||||
os.chmod(config.SQLITE_PATH, 0o600)
|
os.chmod(config.SQLITE_PATH, 0o600)
|
||||||
|
|
||||||
|
|||||||
@@ -62,8 +62,10 @@ def create_app_data_directory(config):
|
|||||||
getpass.getuser()))
|
getpass.getuser()))
|
||||||
|
|
||||||
# Create the directory containing the log file (if not present).
|
# Create the directory containing the log file (if not present).
|
||||||
|
log_dir_created = False
|
||||||
try:
|
try:
|
||||||
_create_directory_if_not_exists(os.path.dirname(config.LOG_FILE))
|
log_dir_created = _create_directory_if_not_exists(
|
||||||
|
os.path.dirname(config.LOG_FILE))
|
||||||
except PermissionError as e:
|
except PermissionError as e:
|
||||||
print(FAILED_CREATE_DIR.format(os.path.dirname(config.LOG_FILE), e))
|
print(FAILED_CREATE_DIR.format(os.path.dirname(config.LOG_FILE), e))
|
||||||
print(
|
print(
|
||||||
@@ -78,8 +80,9 @@ def create_app_data_directory(config):
|
|||||||
sys.exit(1)
|
sys.exit(1)
|
||||||
|
|
||||||
# Create the session directory (if not present).
|
# Create the session directory (if not present).
|
||||||
|
session_dir_created = False
|
||||||
try:
|
try:
|
||||||
is_directory_created = \
|
session_dir_created = \
|
||||||
_create_directory_if_not_exists(config.SESSION_DB_PATH)
|
_create_directory_if_not_exists(config.SESSION_DB_PATH)
|
||||||
except PermissionError as e:
|
except PermissionError as e:
|
||||||
print(FAILED_CREATE_DIR.format(config.SESSION_DB_PATH, e))
|
print(FAILED_CREATE_DIR.format(config.SESSION_DB_PATH, e))
|
||||||
@@ -94,12 +97,11 @@ def create_app_data_directory(config):
|
|||||||
config.APP_VERSION))
|
config.APP_VERSION))
|
||||||
sys.exit(1)
|
sys.exit(1)
|
||||||
|
|
||||||
if os.name != 'nt' and is_directory_created:
|
|
||||||
os.chmod(config.SESSION_DB_PATH, 0o700)
|
|
||||||
|
|
||||||
# Create the storage directory (if not present).
|
# Create the storage directory (if not present).
|
||||||
|
storage_dir_created = False
|
||||||
try:
|
try:
|
||||||
_create_directory_if_not_exists(config.STORAGE_DIR)
|
storage_dir_created = _create_directory_if_not_exists(
|
||||||
|
config.STORAGE_DIR)
|
||||||
except PermissionError as e:
|
except PermissionError as e:
|
||||||
print(FAILED_CREATE_DIR.format(config.STORAGE_DIR, e))
|
print(FAILED_CREATE_DIR.format(config.STORAGE_DIR, e))
|
||||||
print(
|
print(
|
||||||
@@ -114,8 +116,10 @@ def create_app_data_directory(config):
|
|||||||
sys.exit(1)
|
sys.exit(1)
|
||||||
|
|
||||||
# Create Azure Credential Cache directory (if not present).
|
# Create Azure Credential Cache directory (if not present).
|
||||||
|
azure_cache_dir_created = False
|
||||||
try:
|
try:
|
||||||
_create_directory_if_not_exists(config.AZURE_CREDENTIAL_CACHE_DIR)
|
azure_cache_dir_created = _create_directory_if_not_exists(
|
||||||
|
config.AZURE_CREDENTIAL_CACHE_DIR)
|
||||||
except PermissionError as e:
|
except PermissionError as e:
|
||||||
print(FAILED_CREATE_DIR.format(config.AZURE_CREDENTIAL_CACHE_DIR, e))
|
print(FAILED_CREATE_DIR.format(config.AZURE_CREDENTIAL_CACHE_DIR, e))
|
||||||
print(
|
print(
|
||||||
@@ -130,9 +134,11 @@ def create_app_data_directory(config):
|
|||||||
sys.exit(1)
|
sys.exit(1)
|
||||||
|
|
||||||
# Create Kerberos Credential Cache directory (if not present).
|
# Create Kerberos Credential Cache directory (if not present).
|
||||||
|
kerberos_cache_dir_created = False
|
||||||
if config.SERVER_MODE and KERBEROS in config.AUTHENTICATION_SOURCES:
|
if config.SERVER_MODE and KERBEROS in config.AUTHENTICATION_SOURCES:
|
||||||
try:
|
try:
|
||||||
_create_directory_if_not_exists(config.KERBEROS_CCACHE_DIR)
|
kerberos_cache_dir_created = _create_directory_if_not_exists(
|
||||||
|
config.KERBEROS_CCACHE_DIR)
|
||||||
except PermissionError as e:
|
except PermissionError as e:
|
||||||
print(FAILED_CREATE_DIR.format(config.KERBEROS_CCACHE_DIR, e))
|
print(FAILED_CREATE_DIR.format(config.KERBEROS_CCACHE_DIR, e))
|
||||||
print(
|
print(
|
||||||
@@ -145,3 +151,30 @@ def create_app_data_directory(config):
|
|||||||
getpass.getuser(),
|
getpass.getuser(),
|
||||||
config.APP_VERSION))
|
config.APP_VERSION))
|
||||||
sys.exit(1)
|
sys.exit(1)
|
||||||
|
|
||||||
|
# Tighten ACLs on directories holding sensitive material to 0o700
|
||||||
|
# (owner-only). These all hold credentials, tokens, log content with
|
||||||
|
# potentially sensitive context, or user-uploaded files. SESSION_DB_PATH
|
||||||
|
# was already 0o700; extend the same policy uniformly. POSIX-only.
|
||||||
|
if os.name != 'nt':
|
||||||
|
sensitive_dirs = []
|
||||||
|
if log_dir_created:
|
||||||
|
sensitive_dirs.append(os.path.dirname(config.LOG_FILE))
|
||||||
|
if session_dir_created:
|
||||||
|
sensitive_dirs.append(config.SESSION_DB_PATH)
|
||||||
|
if storage_dir_created:
|
||||||
|
sensitive_dirs.append(config.STORAGE_DIR)
|
||||||
|
if azure_cache_dir_created:
|
||||||
|
sensitive_dirs.append(config.AZURE_CREDENTIAL_CACHE_DIR)
|
||||||
|
if kerberos_cache_dir_created:
|
||||||
|
sensitive_dirs.append(config.KERBEROS_CCACHE_DIR)
|
||||||
|
for _dir in sensitive_dirs:
|
||||||
|
try:
|
||||||
|
os.chmod(_dir, 0o700)
|
||||||
|
except Exception as e:
|
||||||
|
# On a mounted directory (OpenShift, NFS, etc.) chmod may
|
||||||
|
# fail. Surface a hint without aborting; the app still
|
||||||
|
# functions, just at the existing permissions.
|
||||||
|
print(
|
||||||
|
"WARNING: Failed to set 0o700 ACL on '{}':\n"
|
||||||
|
" {}".format(_dir, e))
|
||||||
+28
-19
@@ -780,28 +780,37 @@ def setup_db(app: Annotated[str, typer.Argument(
|
|||||||
print("======================================\n")
|
print("======================================\n")
|
||||||
|
|
||||||
def run_migration_for_sqlite():
|
def run_migration_for_sqlite():
|
||||||
with app.app_context():
|
# See pgadmin/__init__.py:run_migration_for_sqlite — tighten the
|
||||||
# Run migration for the first time i.e. create database
|
# umask so SQLite creates the DB file at 0o600 directly rather
|
||||||
from config import SQLITE_PATH
|
# than relying on the post-hoc chmod (which has a TOCTOU window
|
||||||
if not os.path.exists(SQLITE_PATH):
|
# between create and chmod).
|
||||||
db_upgrade(app)
|
_saved_umask = os.umask(0o077)
|
||||||
else:
|
try:
|
||||||
version = Version.query.filter_by(name='ConfigDB').first()
|
with app.app_context():
|
||||||
schema_version = version.value
|
# Run migration for the first time i.e. create database
|
||||||
|
from config import SQLITE_PATH
|
||||||
# Run migration if current schema version is greater than the
|
if not os.path.exists(SQLITE_PATH):
|
||||||
# schema version stored in version table
|
|
||||||
if CURRENT_SCHEMA_VERSION >= schema_version:
|
|
||||||
db_upgrade(app)
|
db_upgrade(app)
|
||||||
|
else:
|
||||||
# Update schema version to the latest
|
|
||||||
if CURRENT_SCHEMA_VERSION > schema_version:
|
|
||||||
version = Version.query.filter_by(name='ConfigDB').first()
|
version = Version.query.filter_by(name='ConfigDB').first()
|
||||||
version.value = CURRENT_SCHEMA_VERSION
|
schema_version = version.value
|
||||||
db.session.commit()
|
|
||||||
|
|
||||||
if os.name != 'nt':
|
# Run migration if current schema version is greater
|
||||||
os.chmod(config.SQLITE_PATH, 0o600)
|
# than the schema version stored in version table
|
||||||
|
if CURRENT_SCHEMA_VERSION >= schema_version:
|
||||||
|
db_upgrade(app)
|
||||||
|
|
||||||
|
# Update schema version to the latest
|
||||||
|
if CURRENT_SCHEMA_VERSION > schema_version:
|
||||||
|
version = Version.query.filter_by(
|
||||||
|
name='ConfigDB').first()
|
||||||
|
version.value = CURRENT_SCHEMA_VERSION
|
||||||
|
db.session.commit()
|
||||||
|
finally:
|
||||||
|
os.umask(_saved_umask)
|
||||||
|
|
||||||
|
if os.name != 'nt':
|
||||||
|
os.chmod(config.SQLITE_PATH, 0o600)
|
||||||
|
|
||||||
def run_migration_for_others():
|
def run_migration_for_others():
|
||||||
with app.app_context():
|
with app.app_context():
|
||||||
|
|||||||
Reference in new issue
Block a user