(2010/02/04 18:32), Toru Maesaka wrote: > Hi! > > Thanks for your proof of concept. > >> From observing your patch, you should be able to develop this as an > engine once we make changes to the modular engine subsystem based on > your feedback. Your feedback on get() being called inside another > function is a good point (thanks!) and I believe some of us have come > to an agreement to refactor that area. As for getting your hands on > the connection socket, Trond is willing to add it to the API.
I have one more point to be issued here. Memcached protects an item from concurrent updates using mutex lock (cache_lock) in store_item. The store_item() acquires a mutex lock during do_store_item(), so all the updates operations shall be serialized. However, it also means we cannot know whether the item to be updated is already on the cache, or not, outside of the critical section. The SET command may work on either creation of a new item or updates of an existing item, depending on existence of the specified item. The REPLACE, APPEND and PREPEND command just returns an error if the specified item does not exist on the cache. Even if the security stuff calls item_get() outside of the critical section to check existence of the specified item, we have no guarantee any other thread does not create/update the target item. It is the reason why I put semcd_update_command() hook inside of the do_store_item(). But, in the engine mechanism, it shall be handled inside of the pluggable module. In fact, the do_store_item() is a part of the default engine routines in the engine subtree. Of course, locking mechanism is one of the significant factor which affects the performance of storage engine. I'm skeptical to enforce a unique locking mechanism for all the engine modules. It is just an idea. If this efforts should be placed on the engine project, it seems to me all the security hooks should be included into server_interface_v1, rather than engine_interface_v1. If I understand correctly, the server_interface_v1 structure is a set of function pointers to be called from engine modules. If we can make clear a set of interfaces that engine module to tell an access control decision of security provider, we can implement this feature much simpler. (Of course, the default security provider should always return 'allowed' to keep backword compatibility.) If necessary, I can introduce it in more details. The idea of such a centralized security provider (independent from implementation details) is called 'reference monitor' in security research, with 30 years history. :-) Thanks, > Toru > > > 2010/2/4 KaiGai Kohei<[email protected]>: >> The attached patch is a proof of the concept. >> >> It enables to apply access controls on every client's request based on >> the access control decision of SELinux. >> In my plan, SELinux specific routines will be pluggable using modular >> architecture, but, right now, this patch implements this feature as >> a static logic to simplify the code. >> >> All the functions are prefixed as "semcd_*" to avoid namespace conflicts. >> It means "secure memcached". >> From viewpoint of the core memcached routines, these functions perform as >> security provider. It anyway returns its access control decision (whether >> it should be allowed, or not) based on its own security policy, and the >> caller has to control the code path according to the decision. >> >> For example, process_get_command() calls semcd_get_command() before returning >> data contents of the specified item, then it returns either true or false. >> If false, the caller (process_get_command) returns error. It does not depend >> on the way to make access control decision. (For example, we can assume >> a security model based on the SASL authenticated user, not only SELinux.) >> >> @@ -2543,6 +2584,11 @@ static inline void process_get_command(conn *c, >> token_t *tokens, size_t ntokens, >> stats_prefix_record_get(key, nkey, NULL != it); >> } >> if (it) { >> + if (!semcd_get_command(c, it)) { >> + out_string(c, "SERVER_ERROR access denied"); >> + item_remove(it); >> + return; >> + } >> >> This patch modified two data structure to contain security attributes. >> The conn::semcd_private and item::semcd_private is used to store >> security_id_t (which is a pointer for a pair of security context in text >> representation and reference counter) object. >> >> And the following hooks allows to make access control decision at the >> strategic points in memcached. >> >> extern bool semcd_get_command(conn *c, item *it); >> extern bool semcd_update_command(conn *c, item *old_it, item *new_it, int >> comm); >> extern bool semcd_delta_command(conn *c, item *it, bool incr); >> extern bool semcd_delete_command(conn *c, item *it); >> extern bool semcd_flush_command(conn *c); >> >> When SELinux support is not enabled, these function calls are replaced by >> (true). It means this code does not affect anything when security feature >> is not enabled. >> >> The "test_policy.te" is a development purpose security policy. >> It can be built and installed as follows: >> >> % make -f /usr/share/selinux/devel/Makefile test_policy.pp >> # su >> # semodule -i test_policy.pp >> # setsebool semcd_enable_auditallow 1 >> >> It is a working example: >> >> [kai...@saba memcached]$ ./memcached -v -s /tmp/memcached.sock >> selinux: semcd_initialize() done >> selinux: new connection from 'unconfined_u:unconfined_r:unconfined_t:s0:c0' >> avc: granted { create } for >> scontext=unconfined_u:unconfined_r:unconfined_t:s0:c0 \ >> tcontext=unconfined_u:object_r:semcd_item_t:s0:c0 tclass=db_blob >> selinux: close connection from >> 'unconfined_u:unconfined_r:unconfined_t:s0:c0' >> --> connection from "...:s0:c0", new item was also labeled as "...:s0:c0". >> >> selinux: new connection from 'unconfined_u:unconfined_r:unconfined_t:s0:c1' >> avc: granted { create } for >> scontext=unconfined_u:unconfined_r:unconfined_t:s0:c1 \ >> tcontext=unconfined_u:object_r:semcd_item_t:s0:c1 tclass=db_blob >> selinux: close connection from >> 'unconfined_u:unconfined_r:unconfined_t:s0:c1' >> --> connection from "...:s0:c1", new item was also labeled as "...:s0:c1" >> >> selinux: new connection from 'unconfined_u:unconfined_r:unconfined_t:s0:c0' >> avc: granted { read } for >> scontext=unconfined_u:unconfined_r:unconfined_t:s0:c0 \ >> tcontext=unconfined_u:object_r:semcd_item_t:s0:c0 tclass=db_blob >> selinux: close connection from >> 'unconfined_u:unconfined_r:unconfined_t:s0:c0' >> --> connection from "...:s0:c0" can read an item labeled as "...:s0:c0" >> >> selinux: new connection from 'unconfined_u:unconfined_r:unconfined_t:s0:c0' >> avc: denied { read } for >> scontext=unconfined_u:unconfined_r:unconfined_t:s0:c0 \ >> tcontext=unconfined_u:object_r:semcd_item_t:s0:c1 tclass=db_blob >> selinux: close connection from >> 'unconfined_u:unconfined_r:unconfined_t:s0:c0' >> --> but connection from "...:s0:c0" cannot read an item labeled as >> "...:s0:c1" >> >> Note that we recommend to use unix domain socket for development purpose, >> because >> it does not need to set up labeled network configuration between server and >> client. >> >> Any comments are welcome. >> >> Thanks, >> >> (2010/01/22 16:10), KaiGai Kohei wrote: >>> (2010/01/22 5:36), Dustin wrote: >>>> >>>> On Jan 21, 1:00 am, KaiGai Kohei<[email protected]> wrote: >>>> >>>>> * Again, the engine framework is not designed to host multiple modules >>>>> in usual way. If a security module occupies the engine framework, it >>>>> also has to load another module that provide physical storage engine, >>>>> by itself. (Or, security module also has to provide i/o feature? it >>>>> is quite nonsense.) >>>> >>>> It's not *entirely* clear what you mean by this. I do use the >>>> engine to load multiple sub-engines that are separate and provide >>>> security isolation. >>> >>> What I wanted to say is, it is not an essential job for the security >>> module to load and manage sub-modules, although it is possible. >>> >>> Basically, I don't think the security module should touch something >>> corresponding to physical i/o operations. As long as user have enough >>> privileges, it should call the secondary engine module without any >>> preventions. >>> >>> If it does not need any additional properties on the session and >>> items to be accessed, the matter is not too hard. >>> The security module just track the entry points of secondary module, >>> and can call it on its invocations. >>> >>> However, the secondary module may want to use conn->engine_storage, >>> although the security module also want to cache the user's privilege. >>> In addition, format of the item object is depending on the secondary >>> module, so the security module cannot modify it in discretionary. >>> >>> If we focus on the security design that enforces simple isolation >>> between (authenticated) users, we don't need store any additional >>> security properties in item objects. But, it is not flexible. >>> >>>>> * The engine framework does not provide a hook just after a connection >>>>> from client is established. It is not a must-requirement. However, >>>>> we want to obtain privilege of the client process, and cache it in >>>>> the userspace. >>>>> If it is not available, we have to invoke a system-call for each >>>>> access control decision, but it will be expensive. >>>> >>>> It certainly does and I use it. You can see it here: >>>> >>>> >>>> http://github.com/northscale/bucket_engine/blob/master/bucket_engine.c#L433 >>> >>> Oh, I could not find it. >>> >>>>> * The security module needs its private storage different from the storage >>>>> engine module. >>>>> Now, the conn structure has "void *engine_storage" to store an engine >>>>> private information, but the right to use is closed for the second >>>>> module. >>>> >>>> The engine should be aware of what you're doing with that data, so >>>> it can supply the API to manipulate it. >>> >>> The matter is both of the security module and engine module want to >>> use this private storage for their own purpose. :( >>> >>>>> In addition, the hash_item structure does not provide any private >>>>> storage to store security context of the item (e.g this context will >>>>> record what process create this object). >>>> >>>> Well, no. Expanding the item structure is *very* costly. That >>>> should should be avoided where possible. >>>> >>>> That said, a given engine can define an item to look like whatever >>>> it wants after the base stuff. You'll notice the base structure >>>> doesn't even have a space for the key or data. >>> >>> Yes, do_item_alloc() allocates sizeof(hash_item) plus various length >>> bytes of memory for each items. It is a well known technique to handle >>> various kind of data objects, not only memcached. >>> >>> How much length do you expect for the security property of an item? >>> I plan to add a "void *" pointer which indicates security_id object >>> managed in libselinux. The security_id structure is a pair of >>> security context (cstring) and reference counter. The number of >>> security context is proportional to the number of users, not # of items, >>> it does not cousume local memory so much. >>> >>>>> * The deployment of some hooks makes hard to decide what permission >>>>> should be applied. For example, process_bin_delete() calls "get" >>>>> method to confirm the object to be removed is existing. >>>>> But the "get" method is only chance for the engine module to know >>>>> a certain item is accessed in reader context. >>>>> Of course, if we consider "delete" is a combination of "read" and >>>>> "delete", it will be resolved. But it will damage the flexibility >>>>> of security design. >>>> >>>> This is a good time to be talking about this, as the engine API >>>> isn't dogma yet. >>> >>> Now I lean toward the security purpose framework which provides >>> meaningful hooks for access controls, and capability to assign >>> a security property on the session and items. >>> >>> Is it too early? >>> >>> Thanks, >> >> >> -- >> OSS Platform Development Division, NEC >> KaiGai Kohei<[email protected]> >> > > > -- OSS Platform Development Division, NEC KaiGai Kohei <[email protected]>
