Update security restrictions to allow non-superuser extension installation#572
Update security restrictions to allow non-superuser extension installation#572
Conversation
Bodobolero
left a comment
Bodobolero
left a comment
There was a problem hiding this comment.
LGTM but added some suggestions and questions
src/pgduckdb_options.cpp
Outdated
There was a problem hiding this comment.
I think I read some code comment or documentation that this GUC (disabled_filesystems) can only be disabled once but not re-enabled once it has been disabled.
Quote: Security-related configuration settings generally lock themselves for safety reasons.
I didn't test if it works or not.
Did you test that you could enable LocalFileSystem temporarily (for the session) as planned AFTEr it had been previously disabled globally (as a pg_duckdb GUC)?
Your non_superuser.out below shows that you did, which is great. Just wanted to verify that you really tested with "disabled_filesystems = LocalFileSystem" globally disabled as a GUC?
There was a problem hiding this comment.
what happens if a user uses another session at the same time while extension install is running (and GUC is set to disabled_filesystem = ""). Could the other session access LocalFilesystem concurrently - thus having an attack vector?
Note: depending on where the extension is downloaded from and how long it takes the time window could be long enough for the attack.
If the temporary GUC change is limited to single PostgreSQL backend process we should be fine. If it applies to all back-ends it would be a problem.
There was a problem hiding this comment.
Updated the code comment to address these questions.
…ation This updates a bunch of our security related code. For previous releases we needed to be very careful with allowing arbitrary SQL code to be executed in DuckDB because DuckDB queries could read all Postgres tables. This is not the case anymore since #466 was merged, because now any access to Postgres tables goes through the Postgres planner and executor instead of custom code. Lots of code wasn't updated with that new behaviour in mind though. 1. Allow running `duckdb.raw_query` and `duckdb.cache` as any user (with the duckdb role). 2. Allow running `duckdb.install_extension` as regular users, if required permissions are explicitly granted. This is not allowed by default for non-superusers because it's still considered a very high privilege. 3. Disallow running queries on tables with RLS enabled in a different place, so that it is checked for every Postgres table that DuckDB opens (also when using `duckdb.query`/`duckdb.raw_query`). 4. Add `duckdb.allow_community_extensions` setting.
JelteF
force-pushed
the
security-changes
branch
from
640bc8b to
fe034d8
Compare
3 participants
This updates a bunch of our security related code. For previous releases we needed to be very careful with allowing arbitrary SQL code to be executed in DuckDB because DuckDB queries could read all Postgres tables. This is not the case anymore since #466 was merged, because now any access to Postgres tables goes through the Postgres planner and executor instead of custom code. Lots of code wasn't updated with that new behaviour in mind though.
duckdb.raw_query,duckdb.cache,duckdb.cache_info,duckdb.cache_deleteandduckdb.recycle_dbas any user (with the duckdb role).duckdb.install_extensionas regular users, if required permissions are explicitly granted. This is not allowed by default for non-superusers because it's still considered a very high privilege.duckdb.query/duckdb.raw_query).duckdb.allow_community_extensionssetting.