classification
Title: Add audit events for loading of sqlite3 extensions
Type: security Stage: resolved
Components: Library (Lib) Versions: Python 3.10
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: berker.peksag, christian.heimes, erlendaasland, steve.dower
Priority: normal Keywords: patch

Created on 2021-04-07 10:51 by erlendaasland, last changed 2021-05-01 11:12 by christian.heimes. This issue is now closed.

Files
File name Uploaded Description Edit
patch.diff erlendaasland, 2021-04-24 23:33
Pull Requests
URL Status Linked Edit
PR 25246 merged erlendaasland, 2021-04-07 11:05
PR 25778 open christian.heimes, 2021-05-01 11:12
Messages (9)
msg390414 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-04-07 10:51
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.
- https://discuss.python.org/t/should-we-audit-enabling-loading-of-sqlite3-extensions-shared-libraries/8124
- https://www.sqlite.org/loadext.html
- https://docs.python.org/3/library/sqlite3.html#sqlite3.Connection.enable_load_extension
msg391740 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2021-04-23 21:36
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?
msg391821 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-04-24 23:19
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.
- http://www.sqlite.org/c3ref/load_extension.html
msg391822 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-04-24 23:33
Something like the attached patch, if I understand you correctly?
msg391823 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-04-24 23:37
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;
 }
msg391941 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2021-04-26 15:21
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.
msg391973 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-04-26 19:31
Ah, yes thanks for the heads up! I'll update the PR.
msg391997 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2021-04-26 23:16
New changeset 7244c0060dc3ef909f34b0791c3e7490b0340d5e by Erlend Egeberg Aasland in branch 'master':
bpo-43762: Add audit events for loading of sqlite3 extensions (GH-25246)
https://github.com/python/cpython/commit/7244c0060dc3ef909f34b0791c3e7490b0340d5e
msg391998 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2021-04-26 23:17
Thanks for the PR!
History
Date User Action Args
2021-05-01 11:12:07christian.heimessetpull_requests: + pull_request24468
2021-04-26 23:17:04steve.dowersetstatus: open -> closed
resolution: fixed
messages: + msg391998

stage: patch review -> resolved
2021-04-26 23:16:49steve.dowersetmessages: + msg391997
2021-04-26 19:31:14erlendaaslandsetmessages: + msg391973
2021-04-26 15:21:37steve.dowersetmessages: + msg391941
2021-04-24 23:37:12erlendaaslandsetmessages: + msg391823
2021-04-24 23:33:55erlendaaslandsetfiles: + patch.diff

messages: + msg391822
2021-04-24 23:19:34erlendaaslandsetmessages: + msg391821
2021-04-23 21:36:33steve.dowersetmessages: + msg391740
2021-04-17 00:38:25ned.deilysetnosy: + steve.dower
2021-04-07 11:05:49erlendaaslandsetkeywords: + patch
stage: patch review
pull_requests: + pull_request23985
2021-04-07 10:51:19erlendaaslandcreate