On Mon, Oct 14, 2013 at 10:59 AM, Ivan Zhakov <i...@visualsvn.com> wrote:
> On 14 October 2013 15:31, Jeff Trawick <traw...@gmail.com> wrote: > > On Mon, Oct 14, 2013 at 4:34 AM, Ivan Zhakov <i...@visualsvn.com> wrote: > >> > >> On 12 October 2013 18:48, Jeff Trawick <traw...@gmail.com> wrote: > >> > With APR on Windows, creating shared memory or a cross-process mutex > >> > with a > >> > "file" name parameter results in a resource being created with a > >> > "Global" > >> > prefix, which in turn restricts the call to Administrator (or more > >> > accurately, a thread with the specific privilege > >> > to make that call; let's just call that "Administrator" for > simplicity). > >> > (I > >> > think this is just for shared memory???) > >> > > >> > A typical way to encounter this is that some dude in httpd-land > decides > >> > that > >> > mod_lua should start creating APR shared memory at startup and a > >> > filename > >> > should always be specified, and suddenly httpd running as you on > Windows > >> > can > >> > no longer use mod_lua. Any number of other httpd modules try do > >> > something > >> > similar (though I haven't investigated if there's a way to configure > >> > around > >> > it); additionally, APR's testshm won't work as a regular user. > >> > > >> > The attached patch uses "Local" as the prefix if the calling thread > >> > doesn't > >> > have the necessary privilege to create one under the global namespace. > >> > But > >> > this function is also used for attach-type operations, > >> > > >> > Has anyone investigated this before? > >> > > >> Jeff, > >> > >> It looks like very dangerous change. Local and Global namespaces are > >> completely separate, thus priviliged process could not access > >> shm/mutex created by unprivileged process. Objects in local kernel > >> namespace are per session, while objects in global kernel namespace > >> are shared across all sessions, that's why process need additional > >> privilege to create them. > > > > > > First, make sure you're looking at what was committed > > (http://svn.apache.org/viewvc?view=revision&revision=r1531768), which is > > slightly different and may have some different considerations. > > > Yes, I've used final commit for my review. > > > For analysis as a security concern, I think it comes down to this: > > > > * no elevation of privileges > > * no change to ACL/permissions of shared memory segments > > > > The code ends up acting the same as if the apr app had access to the > global > > parameter to res_name_from_filename() and could make the adjustments > itself. > > > > Some of your comments lead me to believe you thought there was a > privilege > > escalation. (The name of the API keyword "PRIVILEGE_SET_ALL_NECESSARY" is > > disturbing, but it means everything-in-privilege-set-is-necessary, not > > set-all-privileges-I-need.) How is the privilege escalation occurring? > > > > Or, do you agree there is no privilege escalation? > Oh, sorry. I missed that you are using PrivilegeCheck(), not > AdjustTokenPrivileges() which is used to enable privileges that > assigned, but not enabled. There is no privilege escalation. It's my > fault. > > >> Services are started in separate "session 0", while interactive > >> processes uses dedicated session for every user/rdp connection. > > > > > > understood > > > >> > >> > >> Your patch may also break atomic "create or open" code: > >> [[ > >> rv == apr_shm_create(); > >> if (rv == ALREADY_EXIST) > >> rv = apr_shm_attach() > >> ]] > > > > > > The patch doesn't break this AFAICT, but it doesn't make this work in all > > circumstances either. > > > > Assume old code (e.g., 1.5.x branch), and a privileged process > successfully > > creates a shm segment with a particular name under Global. If an > > unprivileged client tries the code sequence above, it gets access-denied > > from apr_shm_create(), not already-exist. > > > > (Is that the testcase you're referring to?) > > > Yes, that's test I'm referring. That code should be corrected to > handle access denied error also: > [[[ > rv == apr_shm_create(); > if (rv == ALREADY_EXIST || rv == ACCESS_DENIED) > rv = apr_shm_attach() > ]]] > > For me problem that APR 1.5.x was returning access denied, while new > code will create actually not shared memory object. "not shared" in this case means not shared between processes in different terminal sessions, notably a service and a non-service > This could be > problem. > I think that treating ACCESS_DENIED as already-exists-in-global-namespace is a Windows-specific hack (bad) yet at the same time the app has no control over what is going on (bad). If the check that no longer worked correctly were simply "if (rv == APR_EXIST)" it would be much worse. > > > If the second process to run is also privileged, it gets the > already-exists > > feedback and can then attach. > > > >> > >> Suppose Windows services created shm object in global namespace, > >> interactive process wanted to access it using code above. > >> apr_shm_create() detects that process is not privileged and creates > >> shm object in local namespace, which is wrong IMHO. > > > > > > It doesn't work now either. (See above.) > > > > Is there a way to make it work within the constraints of the current > API? I > > guess apr_shm_create() for unprivileged processes would need to check via > > some other API to see if it already exists, in order to allow that idiom > to > > work for unprivileged processes that need to attach to a shm segment > created > > by privileged processes. > > > I don't have good solution for this problem. May be APR could allow > 'local\' and 'global\' prefixes in SHM object names to give API user > control what kernel namespace will be used? > Or in a more APR-like fashion, add apr_shm_create_ex() and apr_shm_attach_ex() which have a special processing flag (bitmap). For Windows, APR_SHM_NS_LOCAL and APR_SHM_NS_GLOBAL would be respected. That could be used by an app that can't with with APR's imperfect attempts to manage global vs. local. I don't think there is a perfect anyway. As 1.5.x hasn't been released yet, the _ex() functions could be in it. > Btw I noticed that Subversion uses mmap to share data between > processes, instead of apr_shm_*() API. > > >> > >> > >> Change process privileges as side effect of apr_shm_attach() is also > >> nasty behavior imho. > > > > I assert that this is not happening ;) > > > Yes, you're right. I mixed PrivilegeCheck() and > AdjustTokenPrivileges() functions. > > > > Thanks for looking at this in detail/please let me know where we > > agree/disagree at this point. > > > > > > -- > Ivan Zhakov > -- Born in Roswell... married an alien... http://emptyhammock.com/
