Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check if schema already exists before create extension [BF-2375] #49

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

0xlianhu
Copy link
Contributor

@0xlianhu 0xlianhu commented Jan 30, 2024

BF-2375

Security bug - privilege escalation

@0xlianhu 0xlianhu requested a review from a team as a code owner January 30, 2024 16:00
sql/aiven_extras.sql Outdated Show resolved Hide resolved
@0xlianhu 0xlianhu force-pushed the 0xlianhu-BF-2375-exist-schema-privilege-escalation branch from d044e6e to f848a9d Compare January 30, 2024 16:44
Copy link
Contributor

@rdunklau rdunklau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are using aiven_extras schema everywhere in the extension code, it makes sense not to check for @extschema@ but for aiven_extras directly.

Things could also be unified the other way, using @extschema@ everywhere.

As for the privileges escalation, is this enough ? Not sure how, but we could end up in a situation where the schema is rightfully owned by the correct user, but some default privileges have been added.

I'm not sure what the implication of this is, but could we instead:

  • remove the schema from the control file, that way it is not created by postgres for us
  • explicitly create the aiven_extras schema in the extension itself. That way it will also be dropped when we drop the extension.

That way we are sure the schema is created afresh, and if it already exists we fail.
This may require a bit more work for the upgrade scripts, which currently are supposed to be idempotent.

@0xlianhu
Copy link
Contributor Author

0xlianhu commented Jan 31, 2024

Since we are using aiven_extras schema everywhere in the extension code, it makes sense not to check for @extschema@ but for aiven_extras directly.

Things could also be unified the other way, using @extschema@ everywhere.

As for the privileges escalation, is this enough ? Not sure how, but we could end up in a situation where the schema is rightfully owned by the correct user, but some default privileges have been added.

I'm not sure what the implication of this is, but could we instead:

  • remove the schema from the control file, that way it is not created by postgres for us
  • explicitly create the aiven_extras schema in the extension itself. That way it will also be dropped when we drop the extension.

That way we are sure the schema is created afresh, and if it already exists we fail. This may require a bit more work for the upgrade scripts, which currently are supposed to be idempotent.

  1. I can use the hard-code aiven_extras schema name instead of @extschema@, less changed lines.
  2. Not sure how, but we could end up in a situation where the schema is rightfully owned by the correct user, but some default privileges have been added. This exploitation is to utilize the vulnerability when the schema is owned by an unprivileged user before creating an extension. If the schema is rightfully owned by the correct user (e.g., postgres) before creating an extension. Then not a problem, because the unprivileged user would not be able to alter schema aiven_extras, which would not be able to exploit the vulnerability.
  3. Will try if can explicitly create the schema in SQL instead of using postgres to create it. But we don't have a specific upgrade script, just re-create everything, what do you mean by a bit more work for the upgrade scripts here?

@0xlianhu 0xlianhu force-pushed the 0xlianhu-BF-2375-exist-schema-privilege-escalation branch from f848a9d to 3e21ac4 Compare January 31, 2024 14:44
@rdunklau
Copy link
Contributor

rdunklau commented Feb 1, 2024

Will try if can explicitly create the schema in SQL instead of using postgres to create it. But we don't have a specific upgrade script, just re-create everything, what do you mean by a bit more work for the upgrade scripts here?

What I mean is that instead of doing a cp of the raw aiven-extras file to generate the upgrade script, we should pipe it through sed to delete the CREATE SCHEMA statement.

@0xlianhu
Copy link
Contributor Author

0xlianhu commented Feb 5, 2024

Will try if can explicitly create the schema in SQL instead of using postgres to create it. But we don't have a specific upgrade script, just re-create everything, what do you mean by a bit more work for the upgrade scripts here?

What I mean is that instead of doing a cp of the raw aiven-extras file to generate the upgrade script, we should pipe it through sed to delete the CREATE SCHEMA statement.

Yes. Maybe this time only fix the bug instead, and keep the existing schema creation as it is?

@0xlianhu
Copy link
Contributor Author

0xlianhu commented Feb 13, 2024

Will try if can explicitly create the schema in SQL instead of using postgres to create it. But we don't have a specific upgrade script, just re-create everything, what do you mean by a bit more work for the upgrade scripts here?

What I mean is that instead of doing a cp of the raw aiven-extras file to generate the upgrade script, we should pipe it through sed to delete the CREATE SCHEMA statement.

This might be more complicated to make the upgrade idempotent.

The creation script file (e.g., aiven_extras--<current_version>.sql) can be omitted if we have an upgrade path to the current version according to https://www.postgresql.org/docs/current/extend-extensions.html#EXTEND-EXTENSIONS-UPDATE-SCRIPTS

This means we may also need the schema to be created in an update script.

@0xlianhu 0xlianhu force-pushed the 0xlianhu-BF-2375-exist-schema-privilege-escalation branch from 3e21ac4 to 005a008 Compare February 14, 2024 09:48
@0xlianhu 0xlianhu requested a review from rdunklau February 14, 2024 12:15
@0xlianhu 0xlianhu force-pushed the 0xlianhu-BF-2375-exist-schema-privilege-escalation branch 2 times, most recently from 7f96c26 to e09a20e Compare February 20, 2024 16:05
@0xlianhu 0xlianhu force-pushed the 0xlianhu-BF-2375-exist-schema-privilege-escalation branch 3 times, most recently from 9042894 to 9a037a4 Compare March 7, 2024 08:33
If the schema aiven_extras already exists and belongs to an
unprivileged user before adding the extension, it’s possible
to abuse it to run some queries in the context of the superuser.

[BF-2375]
@0xlianhu 0xlianhu force-pushed the 0xlianhu-BF-2375-exist-schema-privilege-escalation branch from 9a037a4 to bb93ca9 Compare March 7, 2024 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants