Bug 34324 - Missing: 3×drop_privileges(), stays in prepared=1
Missing: 3×drop_privileges(), stays in prepared=1
Status: CLOSED FIXED
Product: UCS
Classification: Unclassified
Component: Listener (univention-directory-listener)
UCS 3.2
All Linux
: P5 normal (vote)
: UCS 4.1-2-errata
Assigned To: Philipp Hahn
Arvid Requate
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2014-03-12 14:19 CET by Philipp Hahn
Modified: 2016-07-21 15:16 CEST (History)
2 users (show)

See Also:
What kind of report is it?: ---
What type of bug is this?: ---
Who will be affected by this bug?: ---
How will those affected feel about the bug?: ---
User Pain:
Enterprise Customer affected?:
School Customer affected?:
ISV affected?:
Waiting Support:
Ticket number:
Bug group (optional):
Max CVSS v3 score:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Philipp Hahn univentionstaff 2014-03-12 14:19:13 CET
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
Comment 1 Philipp Hahn univentionstaff 2016-06-06 11:51:42 CEST
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
Comment 2 Philipp Hahn univentionstaff 2016-06-07 18:12:55 CEST
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
Comment 3 Arvid Requate univentionstaff 2016-07-13 20:30:48 CEST
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
Comment 4 Philipp Hahn univentionstaff 2016-07-18 17:54:04 CEST
(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
Comment 5 Arvid Requate univentionstaff 2016-07-18 18:20:15 CEST
Code review: Ok
Advisory: Ok (Typo fixed)