On Fri, 2009-05-15 at 09:23 +0200, Jan Friesse wrote:
> Fabio,
> >> +
> >> 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.
Ok.
>
>
> >
> > 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.
Ok.
>
> >>
> >> 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.
Very simple.. look at the config file and how it reflects into the
objdb.
If you need to debug or dump config info out of the objdb, you will only
see a bunch of uidgid { } with numbers and no idea where they are coming
from.
Adding a few keys to each uidgid in the objdb (doesn't have to be in the
configfile) then you know what uid/gid has been enabled by what and the
correlation between numbers and names at load time (that might not be
the same in a long run).
> >
> > 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.
I have probably missed Steven's email, but I think we really want to
filter what piece of information is coming from where.
Unless you want to get crazy parsing tons of config files from users to
figure out that a totem key was being changed in uidgid.d/cman...
I think it will save us some headaches.. anyway just a suggestion.
Fabio
_______________________________________________
Openais mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/openais
