Univention Bugzilla – Bug 34324
Missing: 3×drop_privileges(), stays in prepared=1
Last modified: 2016-07-21 15:16:02 CEST
In 3 error cased the privileges are not dropped. If postrun() is not defined, the handler will stay in prepared=1, which leads to prerun() to never be called again. diff --git a/branches/ucs-3.2/ucs-3.2-1/management/univention-directory-listener/src/handlers.c b/branches/ucs-3.2/ucs-3.2-1/management/univention-directory-listener/src/handlers.c index 9425383..91c3379 100644 --- a/branches/ucs-3.2/ucs-3.2-1/management/univention-directory-listener/src/handlers.c +++ b/branches/ucs-3.2/ucs-3.2-1/management/univention-directory-listener/src/handlers.c @@ -318,20 +318,18 @@ static int handler_prerun(Handler *handler) { PyObject *result; - if (handler->prerun == NULL || handler->prepared) { - handler->prepared = 1; - return 0; - } + if (handler->prerun == NULL || handler->prepared) + goto out; if ((result = PyObject_CallObject(handler->prerun, NULL)) == NULL) { PyErr_Print(); drop_privileges(); return 1; } + Py_XDECREF(result); +out: drop_privileges(); handler->prepared=1; - - Py_XDECREF(result); return 0; } @@ -344,17 +342,17 @@ static int handler_postrun(Handler *handler) univention_debug(UV_DEBUG_LISTENER, UV_DEBUG_INFO, "postrun handler: %s (prepared=%d)", handler->name, handler->prepared); if (handler->postrun == NULL || !handler->prepared) - return 0; + goto out; if ((result = PyObject_CallObject(handler->postrun, NULL)) == NULL) { PyErr_Print(); drop_privileges(); return 1; } + Py_XDECREF(result); +out: drop_privileges(); handler->prepared=0; - - Py_XDECREF(result); return 0; } @@ -401,6 +399,7 @@ static int handler_exec(Handler *handler, char *dn, CacheEntry *new, CacheEntry if (result == NULL) { PyErr_Print(); + drop_privileges(); return -1; } /* make sure that privileges are properly dropped in case the handler @@ -915,6 +914,7 @@ int handler_set_data(Handler *handler, PyObject *argtuple) if ((result = PyObject_CallObject(handler->setdata, argtuple)) == NULL) { PyErr_Print(); + drop_privileges(); return -1; } /* make sure that privileges are properly dropped in case the handler
r69832 | Bug #34324 repl: Stop using setuid() Package: univention-directory-replication Version: 9.0.1-3.104.201606061133 Branch: ucs_4.1-0 Scope: errata4.1-2 Package: ucs-test Version: 6.0.33-68.1484.201606061134 Branch: ucs_4.1-0 Scope: errata4.1-2
r69894 | Bug #34324 UDL: Drop privileges after calling handler functions Package: univention-directory-listener Version: 10.0.0-11.315.201606071717 Branch: ucs_4.1-0 Scope: errata4.1-2 r69910 | Bug #22383,Bug #30227,Bug #30263,Bug #34324,Bug #34507,Bug #34738,Bug #3490,Bug #38696,Bug #39509,Bug #40600,Bug #41261: UDL YAML univention-directory-listener.yaml
The adjustments you made to UDL look good. Suggestions for improvement: drop_privileges is not called in handler_set_data in case PyObject_CallObject returned NULL, i.e. hunk 4 from the patch above is not applied. Coding style: Maybe adjust handler_clean and handler_initialize like you did for handler_postrun? Personally I don't like that univention-directory-replication has been modified too. Also, the changes are good, but they should be at least mentioned and not be made silently: * ownership change for the STATE_DIR /var/lib/univention-directory-replication before: root.root, now: listener.root * The failed.ldif and the modrdn cache files are now owned by "listener" instead of "root" * LDAP connection to the local LDAP server is now made as user "listener" instead of "root", which should be ok, both read the same ldap.conf Advisory entry: Ok for UDL, missing for UDR
(In reply to Arvid Requate from comment #3) > The adjustments you made to UDL look good. Suggestions for improvement: > > drop_privileges is not called in handler_set_data in case > PyObject_CallObject returned NULL, i.e. hunk 4 from the patch above is not > applied. > > Coding style: Maybe adjust handler_clean and handler_initialize like you did > for handler_postrun? Thanks: r71069 | Bug #34324 UDL: Drop privileges after calling handler functions Package: univention-directory-listener Version: 10.0.0-14.325.201607181745 Branch: ucs_4.1-0 Scope: errata4.1-2 r71075 | Bug #40600,Bug #41261,Bug #34324,Bug #3490 UDL: YAML univention-directory-listener.yaml > Personally I don't like that univention-directory-replication has been > modified too. Also, the changes are good, but they should be at least > mentioned and not be made silently: > > * ownership change for the STATE_DIR > /var/lib/univention-directory-replication > before: root.root, now: listener.root > > * The failed.ldif and the modrdn cache files are now owned by "listener" > instead of "root" > > * LDAP connection to the local LDAP server is now made as user "listener" > instead of "root", which should be ok, both read the same ldap.conf Documented. r71076 | Bug #31757,Bug #33594 repl: YAML univention-directory-replication.yaml
Code review: Ok Advisory: Ok (Typo fixed)
<http://errata.software-univention.de/ucs/4.1/215.html> <http://errata.software-univention.de/ucs/4.1/216.html>