GNOME Bugzilla – Bug 743392
Fix check for no credentials passed
Last modified: 2018-05-24 17:23:33 UTC
So there is a race condition in the kernel. So that when we set SO_PASSCRED on an accepted socket it might not take effect before sending/reading. One way to fix it would be to set SO_PASSCRED on the listening socket. Another way is to actually detect this in gunixcredentialmessage.c and fall back to PEERCRED. We want to detect that credentials were not passed on a socket. But we cannot use '(uid_t) -1' as a condition since the kernel value is not the same size as uid_t. We need to explictly check for 65534.
Created attachment 295263 [details] [review] gunixcredentialmessage: Fix check for unset uid
This looks good to me, but i wonder why it broke. Or if it was always broken...
Review of attachment 295263 [details] [review]: I don't like this patch. Why 65534? Did someone actually hardcode that specific value into the kernel? Is that a Linux-only thing? g_credentials_get_unix_user() is documented as returning -1 and setting error in case of unset credentials, but now we seem to be circumventing that by explicitly checking for an explicit value outside of the API. Why don't we move this check inside of that API, or (even better) to the place where we pull the data out of the kernel? At the very least we need a whole lot more documentation here.
In scm_recv() in: http://lxr.free-electrons.com/source/include/net/scm.h It does: if (test_bit(SOCK_PASSCRED, &sock->flags)) { struct user_namespace *current_ns = current_user_ns(); struct ucred ucreds = { .pid = scm->creds.pid, .uid = from_kuid_munged(current_ns, scm->creds.uid), .gid = from_kgid_munged(current_ns, scm->creds.gid), }; This remapping (from_kuid_munged) will convert -1 to overflowuid: static inline uid_t from_kuid_munged(struct user_namespace *to, kuid_t kuid) { uid_t uid = from_kuid(to, kuid); if (uid == (uid_t)-1) uid = overflowuid; return uid; } I guess this changed the ABI of the kernel.
overflowuid is unfortunately not a constant, you can set it via /proc/sys/fs, so the patch above isn't right.
It seems that this is fallout from an unintended change in the kernel -- the overflowuid thing should really only be taking place with namespaces, but this is also hitting the "creds not sent" case, where we used to get -1. The kernel needs to get fixed to behave as it did before.
Introduced here? http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=dbe9a4173ea53b72b2c35d19f676a85b69f1c9fe
Yes, it very much seems that way. So there is two (kernel) issues? 1) A race condition that means that setting the SO_PASSCRED sockopt on the accepted socket does not guarantee that the upcoming send/recv will include credentials. 2) The kernel changed the uid/gid value when there is no credentials from -1 to overflow{g|u}id. Which is user configurable and -2 ('nodbody') by default. So 2) should be fixed in kernel since it is an API break. I have sent a mail to lkml: https://lkml.org/lkml/2015/1/23/334 I guess 1) is trickier? Should glib do something here? The proposed way to deal with this is setting the SO_PASSCRED option on the listening socket. And a mechanism was added to the kernel that makes the accepted socket inherit the flags from listen socket. Should glib set SO_PASSCRED on the listen socket? Where should that be added in that case? And do we still need to check and set if missing on the accepted socket for backwards compatibility?
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/984.