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

Add audit events for loading of sqlite3 extensions #87928

Closed
erlend-aasland opened this issue Apr 7, 2021 · 9 comments
Closed

Add audit events for loading of sqlite3 extensions #87928

erlend-aasland opened this issue Apr 7, 2021 · 9 comments
Labels
3.10 stdlib type-security

Comments

@erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Apr 7, 2021

BPO 43762
Nosy @tiran, @berkerpeksag, @zooba, @erlend-aasland
PRs
  • #25246
  • #25778
  • Files
  • patch.diff
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2021-04-26.23:17:04.808>
    created_at = <Date 2021-04-07.10:51:19.610>
    labels = ['type-security', 'library', '3.10']
    title = 'Add audit events for loading of sqlite3 extensions'
    updated_at = <Date 2021-05-01.11:12:07.693>
    user = 'https://github.com/erlend-aasland'

    bugs.python.org fields:

    activity = <Date 2021-05-01.11:12:07.693>
    actor = 'christian.heimes'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-04-26.23:17:04.808>
    closer = 'steve.dower'
    components = ['Library (Lib)']
    creation = <Date 2021-04-07.10:51:19.610>
    creator = 'erlendaasland'
    dependencies = []
    files = ['49982']
    hgrepos = []
    issue_num = 43762
    keywords = ['patch']
    message_count = 9.0
    messages = ['390414', '391740', '391821', '391822', '391823', '391941', '391973', '391997', '391998']
    nosy_count = 4.0
    nosy_names = ['christian.heimes', 'berker.peksag', 'steve.dower', 'erlendaasland']
    pr_nums = ['25246', '25778']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'security'
    url = 'https://bugs.python.org/issue43762'
    versions = ['Python 3.10']

    @erlend-aasland
    Copy link
    Contributor Author

    @erlend-aasland erlend-aasland commented Apr 7, 2021

    If Python is configured with --enable-loadable-sqlite-extensions, it is possible to load third party SQLite extensions (shared libraries/DLL’s) via the sqlite3 extension module. When enabled, the sqlite3.Connection.enable_load_extension() class method will enable the loading of third party extensions via SQL queries, using the SQL function load_extension(). It also enables loading extension via C, using the sqlite3.Connection.load_extension() class method.

    Suggesting to add the following audit event names to respectively the sqlite3.Connection.enable_load_extension() and sqlite3.Connection.load_extension() methods:

    • sqlite3.enable_load_extension
    • sqlite3.load_extension

    Ref.

    @erlend-aasland erlend-aasland added 3.10 stdlib type-security labels Apr 7, 2021
    @zooba
    Copy link
    Member

    @zooba zooba commented Apr 23, 2021

    Left some minor suggestions on the PR, but wanted to copy this comment here as well:

    I wonder if it's worth returning the connection object when it's created (through a new event in module.c) and then reference it in these events? That can then correlate these (and other) events with the file - we do this already for sockets.

    After some thought, I think it's probably not worth it for these ones. The important information is in the extension being loaded, and it doesn't really relate to the connection at all. However, if we wanted to add it later, we couldn't. So might be worth doing now?

    @erlend-aasland
    Copy link
    Contributor Author

    @erlend-aasland erlend-aasland commented Apr 24, 2021

    Good question. sqlite3_load_extension() loads an extension into a database connection, so it would make sense to also pass the connection object. I'd say we do it; it's a small change, and as you say: if we wanted to add it later, we couldn't.

    Ref.

    @erlend-aasland
    Copy link
    Contributor Author

    @erlend-aasland erlend-aasland commented Apr 24, 2021

    Something like the attached patch, if I understand you correctly?

    @erlend-aasland
    Copy link
    Contributor Author

    @erlend-aasland erlend-aasland commented Apr 24, 2021

    Maybe it's better to send the event only if the connection succeeded:

    diff --git a/Modules/_sqlite/module.c b/Modules/_sqlite/module.c
    index 8dbfa7b38a..0220978cf2 100644
    --- a/Modules/_sqlite/module.c
    +++ b/Modules/_sqlite/module.c
    @@ -97,6 +97,12 @@ static PyObject* module_connect(PyObject* self, PyObject* args, PyObject*
     
         result = PyObject_Call(factory, args, kwargs);
     
    +    if (result) {
    +        if (PySys_Audit("sqlite3.connected", "O", self) < 0) {
    +            return -1;
    +        }
    +    }
    +
         return result;
     }

    @zooba
    Copy link
    Member

    @zooba zooba commented Apr 26, 2021

    Yeah, along those lines. I believe the event is ".../result" in other places, just to be clear that it's not a function with that name.

    Also, don't forget to clean up when returning early from the connected event. We don't want to leak the connection object if the hook raises an error.

    @erlend-aasland
    Copy link
    Contributor Author

    @erlend-aasland erlend-aasland commented Apr 26, 2021

    Ah, yes thanks for the heads up! I'll update the PR.

    @zooba
    Copy link
    Member

    @zooba zooba commented Apr 26, 2021

    New changeset 7244c00 by Erlend Egeberg Aasland in branch 'master':
    bpo-43762: Add audit events for loading of sqlite3 extensions (GH-25246)
    7244c00

    @zooba
    Copy link
    Member

    @zooba zooba commented Apr 26, 2021

    Thanks for the PR!

    @zooba zooba closed this as completed Apr 26, 2021
    @zooba zooba closed this as completed Apr 26, 2021
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.10 stdlib type-security
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants