Fabio,
Fabio M. Di Nitto wrote:
> Hi Jan,
>
> I have few comments...
>
> On Thu, 2009-05-14 at 11:49 +0200, Jan Friesse wrote:
>
>>
>>
>>
>>
>> differences
>> between files
>> attachment
>> (corosync-support-for-uidgid-try2.patch)
>>
>> diff --git a/trunk/exec/main.c b/trunk/exec/main.c
>> index db22e96..2b41111 100644
>> --- a/trunk/exec/main.c
>> +++ b/trunk/exec/main.c
>> @@ -138,6 +138,18 @@ static void sigusr2_handler (int num)
>> }
>> }
>>
>> +static void corosync_remove_uidgid_list (void) {
>> + struct list_head *iter;
>> +
>> + for (iter = ug_config.uidgid_list.next; iter !=
>> &ug_config.uidgid_list; ) {
>> + struct uidgid_item *ugi = list_entry (iter, struct
>> uidgid_item, list);
>> + iter = iter->next;
>> +
>> + list_del (&ugi->list);
>> + free (ugi);
>> + }
>> +}
>> +
>> /*
>> * TODO this function needs some love
>> */
>> @@ -150,6 +162,10 @@ void corosync_request_shutdown (void)
>> poll_stop (0);
>> totempg_finalize ();
>> coroipcs_ipc_exit ();
>> +
>> + /*Remove uidgid_list*/
>> + corosync_remove_uidgid_list ();
>
> Is there really a need to free this list on exit? we are about to abort
> anyway and the kernel will take care of that.
>
No it's not needed. I can throw it away.
>> +
>> corosync_exit_error (AIS_DONE_EXIT);
>> }
>>
>> @@ -482,12 +498,18 @@ static coroipcs_handler_fn_lvalue
>> corosync_handler_fn_get (unsigned int service,
>>
>> static int corosync_security_valid (int euid, int egid)
>> {
>> + struct list_head *iter;
>> if (euid == 0 || egid == 0) {
>> return (1);
>> }
>> - if (euid == ug_config.uid || egid == ug_config.gid) {
>> - return (1);
>> +
>> + for (iter = ug_config.uidgid_list.next; iter !=
>> &ug_config.uidgid_list; iter = iter->next) {
>> + struct uidgid_item *ugi = list_entry (iter, struct
>> uidgid_item, list);
>> +
>> + if (euid == ugi->uid || egid == ugi->gid)
>> + return (1);
>> }
>> +
>> return (0);
>> }
>
> It's entirely possible that I misunderstood but aren't we looking here
> to allow a certain process name to access IPC via certain uid/gid?
>
> Let say that we drop a file in /etc/corosync/uidgid.d/cman with uid/gid
> foo:foo, it means to me that a process called cman (or identified as
> such) is allowed to access corosync IPC with uid/gid foo:foo.
More or less, yes. If there will be process cman (or every other) with
uid or gid foo, it will be able to use corosync. It's only
ug_config.uid/gid enhancement. See
https://bugzilla.redhat.com/show_bug.cgi?id=484047.
>
> With this check I can see the possibility that a user X can get access
> to IPC because somebody else did drop a file in there... not really sure
> that's what you want.
>
Question similar to this, I asked in some previous mail. Answer was,
only root has access to that directory -> shouldn't be security problem.
>>
>> diff --git a/trunk/exec/mainconfig.c b/trunk/exec/mainconfig.c
>> index 79e01cd..eb0563c 100644
>> --- a/trunk/exec/mainconfig.c
>> +++ b/trunk/exec/mainconfig.c
>> @@ -671,6 +671,54 @@ static void add_logsys_config_notification(
>>
>> }
>>
>> +static int corosync_main_config_read_uidgid (
>> + struct objdb_iface_ver0 *objdb,
>> + const char **error_string,
>> + struct ug_config *ug_config)
>> +{
>> + hdb_handle_t object_find_handle;
>> + hdb_handle_t object_service_handle;
>> + char *value;
>> + int uid, gid;
>> + struct uidgid_item *ugi;
>> +
>> + list_init (&ug_config->uidgid_list);
>> +
>> + objdb->object_find_create (
>> + OBJECT_PARENT_HANDLE,
>> + "uidgid",
>> + strlen ("uidgid"),
>> + &object_find_handle);
>> +
>> + while (objdb->object_find_next (
>> + object_find_handle,
>> + &object_service_handle) == 0) {
>> + uid = -1;
>> + gid = -1;
>> +
>> + if (!objdb_get_string (objdb,object_service_handle,
>> "uid", &value)) {
>> + uid = uid_determine(value);
>> + }
>> +
>> + if (!objdb_get_string (objdb,object_service_handle,
>> "gid", &value)) {
>> + gid = gid_determine(value);
>> + }
>> +
>> + if (uid > -1 || gid > -1) {
>> + ugi = malloc (sizeof (*ugi));
>> + if (ugi == NULL) {
>> + _corosync_out_of_memory_error();
>> + }
>> + ugi->uid = uid;
>> + ugi->gid = gid;
>> + list_add (&ugi->list,
>> &ug_config->uidgid_list);
>> + }
>> + }
>> + objdb->object_find_destroy (object_find_handle);
>> +
>> + return 0;
>> +}
>> +
>
> For the sake of tracking, it would be a good idea to have a record of
> what service/file did open for a certain uid/gid set.
>
> So perhaps add an objdb entry with the name of the file and record the
> values both as strings and numeric (easier to read in debugging).
>
> It adds a bit of complexity to code but imho it's worth the pain.
>
> For example:
>
> /etc/corosync/uidgid.d/foo
>
> contains:
>
> uidgid {
> uid: root
> gid: whatever
> }
>
> it will automatically translate to (into the objdb):
>
> uidgid {
> uid: 0
> gid: $random_number
> service: foo
> file: /etc/corosync/uidgid.d/foo
> uid_requested: root (uid_r or whatever keyname)
> gid_requested: whatever ^^ same as above
> }
>
> a configfile could also contain more than one set:
>
> cat /etc/corosync/uidgid.d/many
>
> uidgid {
> service: foo
> uid: root
> gid: root
> }
> uidgid {
> service: cman
> uid: ais
> gid: ais
> }
>
> that would translate to this in the objdb:
>
> uidgid {
> uid: 0
> gid: 0
> service: foo
> file: /etc/corosync/uidgid.d/many
> uid_requested: root (uid_r or whatever keyname)
> gid_requested: root ^^ same as above
> }
> uidgid {
> uid: $random_number
> gid: $random_number
> service: cman
> file: /etc/corosync/uidgid.d/many
> uid_requested: ais (uid_r or whatever keyname)
> gid_requested: ais ^^ same as above
> }
>
>
Really don't understand the purpose of that.
>> diff --git a/trunk/exec/mainconfig.h b/trunk/exec/mainconfig.h
>> index c9ab7ea..26135ad 100644
>> --- a/trunk/exec/mainconfig.h
>> +++ b/trunk/exec/mainconfig.h
>> @@ -37,6 +37,7 @@
>>
>> #include <corosync/engine/objdb.h>
>> #include <corosync/engine/logsys.h>
>> +#include <corosync/list.h>
>>
>> /*
>> * All service handlers in the AIS
>> @@ -49,14 +50,29 @@ struct dynamic_service {
>> };
>> #define MAX_DYNAMIC_SERVICES 128
>>
>> +/*
>> + * Structure describing cached uidgid item
>> + */
>> +struct uidgid_item {
>> + struct list_head list;
>> + int uid;
>> + int gid;
>> +};
>> +
>> struct ug_config {
>> /*
>> * user/group to run as
>> */
>> int uid;
>> int gid;
>> +
>> + /*
>> + * Allowed users/group to connect. This is of type uidgid
>> item.
>> + */
>> + struct list_head uidgid_list;
>> };
>
> Do we need to have 2 structs? at this point one should be plenty?
>
> struct uidgid {
> struct list_head list;
> int uid;
> int gid;
> }
>
> and init the first one to match corosync default conf.
ug_config uid/gid is used for something different. This is used (or
should/will/was use/d) for corosync. So in previous version, user was
able to run corosync (not process communicating with corosync) as user
ais:ais.
This is only for authentification of process which is trying to
communicate with corosync (using corosync lib).
Please look at https://bugzilla.redhat.com/show_bug.cgi?id=484047.
>
> The loader should make sure to filter the config entries to load only
> uidgid entries (maybe it's already there and I haven't noticed).
Answered in some previous mail by Steve, that we don't care. But yes,
this can be done.
>
> Fabio
>
>
>
Honza
_______________________________________________
Openais mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/openais
