| Message ID | 0a6888edc9d7899fe3b4af249c4f25088e196422.1369085762.git.ydroneaud@opteya.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
On Mon, May 20, 2013 at 11:43:05PM +0200, Yann Droneaud wrote: > libibverbs must refuse to load arbitrary shared objects. > > This patch check the configuration directory and files for > - being owned by root; > - not being writable by others. I really don't like this. Is there some exploit against /etc/ now that requires this sort of checking? Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 20, 2013 at 2:43 PM, Yann Droneaud <ydroneaud@opteya.com> wrote: > libibverbs must refuse to load arbitrary shared objects. > > This patch check the configuration directory and files for > - being owned by root; > - not being writable by others. uverbs is an unprivileged interface. Right now I can develop and test libibverbs and driver code as an unprivileged user. If I'm understanding correctly, this patch would break that -- I'd have to install to a root-owned directory to test. What's the exploit this protects against? -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Le mercredi 22 mai 2013 à 14:32 -0700, Roland Dreier a écrit : > On Mon, May 20, 2013 at 2:43 PM, Yann Droneaud <ydroneaud@opteya.com> wrote: > > libibverbs must refuse to load arbitrary shared objects. > > > > This patch check the configuration directory and files for > > - being owned by root; > > - not being writable by others. > > uverbs is an unprivileged interface. Right now I can develop and test > libibverbs and driver code as an unprivileged user. If I'm > understanding correctly, this patch would break that -- I'd have to > install to a root-owned directory to test. > I've missed this use case. Indeed user should be able to use his own version of libibverbs and configuration files. > What's the exploit this protects against? The configuration mechanism allow loading arbitrary shared object: this should be done with care when running setuid binaries / running program as root. Adding some basic sanity check is welcome to protect from someone tampering the configuration files. I'm going to post an updated patchset which will secure (in-depth) access to configuration files while allowing user to use their own files. Regards.
Hi, Le mardi 21 mai 2013 à 14:57 -0600, Jason Gunthorpe a écrit : > On Mon, May 20, 2013 at 11:43:05PM +0200, Yann Droneaud wrote: > > libibverbs must refuse to load arbitrary shared objects. > > > > This patch check the configuration directory and files for > > - being owned by root; > > - not being writable by others. > > I really don't like this. Is there some exploit against /etc/ now that > requires this sort of checking? > Loading shared object as part of a setuid binary should be handled with extra care. Adding checks to the configuration loader is required so that only trusted shared object get loaded. Regards.
On Thu, Aug 08, 2013 at 09:24:16PM +0200, Yann Droneaud wrote: > Hi, > > Le mardi 21 mai 2013 ?? 14:57 -0600, Jason Gunthorpe a ??crit : > > On Mon, May 20, 2013 at 11:43:05PM +0200, Yann Droneaud wrote: > > > libibverbs must refuse to load arbitrary shared objects. > > > > > > This patch check the configuration directory and files for > > > - being owned by root; > > > - not being writable by others. > > > > I really don't like this. Is there some exploit against /etc/ now that > > requires this sort of checking? > > > > Loading shared object as part of a setuid binary should be handled > with extra care. Adding checks to the configuration loader is > required so that only trusted shared object get loaded. Well, still, I'm not sure this is required. IBV_CONFIG_DIR is hardwired and not overriable (via environment, etc), so it is a simple installation error to have the wrong permissions for your environment on these files. But lots of files need to have the correct permissions for setuid to be secure (the binary, the library itself, the libraries it dlopens, the directories that contain all of these things, etc) - not sure it makes any sense at all to single out the config files for special checking. In any event, if these checks really are necessary they should be only done if running in a setuid context, and they almost certainly need to extend to the dlopen paths as well.. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Le 12.08.2013 21:05, Jason Gunthorpe a écrit : > On Thu, Aug 08, 2013 at 09:24:16PM +0200, Yann Droneaud wrote: >> >> Loading shared object as part of a setuid binary should be handled >> with extra care. Adding checks to the configuration loader is >> required so that only trusted shared object get loaded. > > Well, still, I'm not sure this is required. IBV_CONFIG_DIR is > hardwired and not overriable (via environment, etc), so it is a simple > installation error to have the wrong permissions for your environment > on these files. > It's an installation error that can allow an attackant to tamper the configuration files. Once the configuration files are modified to load a payload, the attackant can either trick root to execute a verbs/RDMA program or use a verbs/RDMA setuid program to gain root access. libibverbs should protect its users from loading arbitrary shared object/library. I fixed the code regarding Roland remarks on user's own libibverbs, so I think there's no more use case were my proposed check would harm. But I cannot imagine all of them alone, so I need your help to find some valid use case broken by my proposed checks. > But lots of files need to have the correct permissions for setuid to > be secure (the binary, the library itself, the libraries it dlopens, > the directories that contain all of these things, etc) - not sure it > makes any sense at all to single out the config files for special > checking. > It took a lot of time to fixes those setuid programs and the libraries used by them. And it still an on-going work. We don't have to wait for exploit to secure the loading mechanism of libibverbs. > In any event, if these checks really are necessary they should be only > done if running in a setuid context, and they almost certainly need to > extend to the dlopen paths as well.. > They need to be done when the configuration files are owned by a different user that the one using the library (eg. running a program tied to the library). I put the emphasis on setuid use case, but a program run as root using configuration file owned by another user is more likely to happen (for devel, test purpose). Regards.
On Mon, Aug 12, 2013 at 10:24:55PM +0200, Yann Droneaud wrote: > It's an installation error that can allow an attackant to tamper > the configuration files. Once the configuration files are modified > to load a payload, the attackant can either trick root to execute > a verbs/RDMA program or use a verbs/RDMA setuid program to gain root > access. libibverbs does not have to defend itself against installation errors. If you install stuff wrong, with the wrong permissions, you will have an insecure system. You have added checks for a single set of permissions, but there are many other permissions that must also be correct to be secure for setuid. Fundamentally it is not the Unix Way to enforce security policy in code. > We don't have to wait for exploit to secure the loading mechanism of > libibverbs. What possible exploit? If installed properly the code is already correct and not exploitable. You are not fixing *ANYTHING* exploitable here. You are mis-applying the CWE's you quoted. They fundamentally do not apply when all the path components have secure permissions. > >In any event, if these checks really are necessary they should be only > >done if running in a setuid context, and they almost certainly need to > >extend to the dlopen paths as well.. > They need to be done when the configuration files are owned by a > different user that the one using the library (eg. running a program > tied to the library). No, that is not the Unix Way. Only the setuid case ever needs special handling, and that handling should be designed to prevent the unprivileged user from co-opting the program while it is running as root. All other case, including running verbs non-setuid as root, should respect the normal permission systems. > I put the emphasis on setuid use case, but a program run as root > using configuration file owned by another user is more likely to > happen (for devel, test purpose). This should work fine as well. It is the sysadmin's choice to install the config files under alternate permissions. Funamentally a library should not enforce security policy if it not necessary to do so. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
This is slightly off topic, but what is the underlying reason for having the libibverbs config directory? Is it simply to support the library name being different than the device name, or is there something more? - Sean -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Aug 12, 2013 at 08:59:10PM +0000, Hefty, Sean wrote: > This is slightly off topic, but what is the underlying reason for > having the libibverbs config directory? Is it simply to support the > library name being different than the device name, or is there > something more? I use it quite frequently to setup ibverbs to use non-system paths to find the libaries. It seems to me the whole idea of this system is that a vendor could ship a binary interface blob in /opt/foo/lib/ and just drop one file into libibverbs.d to configure that blob. Frankly, I'm not really sure there is much value in this whole loadable mechanism. I'm unaware of people actually using the dynamic load in a meaningful way and it seems many of the drivers would be better maintained if they were part of verbs directly... The realities of the kernel GPL seem to preclude having a binary user-space, and drivers are not really being shipped separately from verbs.. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/src/init.c b/src/init.c index 1981da7..a37b57d 100644 --- a/src/init.c +++ b/src/init.c @@ -294,10 +294,24 @@ static void read_config_file(const char *path) static void read_config(void) { + struct stat buf; DIR *conf_dir; struct dirent *dent; char *path; + if (stat(IBV_CONFIG_DIR, &buf) || !S_ISDIR(buf.st_mode)) { + fprintf(stderr, PFX "Warning: couldn't stat config directory '%s'.\n", + IBV_CONFIG_DIR); + return; + } + + if (buf.st_uid != 0 || buf.st_gid != 0 || + (buf.st_mode & S_IWOTH) != 0) { + fprintf(stderr, PFX "Warning: unsecure config directory '%s'.\n", + IBV_CONFIG_DIR); + return; + } + conf_dir = opendir(IBV_CONFIG_DIR); if (!conf_dir) { fprintf(stderr, PFX "Warning: couldn't open config directory '%s'.\n", @@ -306,8 +320,6 @@ static void read_config(void) } while ((dent = readdir(conf_dir))) { - struct stat buf; - if (dent->d_name[0] == '.') continue; @@ -329,6 +341,13 @@ static void read_config(void) if (!S_ISREG(buf.st_mode)) goto next; + if (buf.st_uid != 0 || buf.st_gid != 0 || + (buf.st_mode & S_IWOTH) != 0) { + fprintf(stderr, PFX "Warning: unsecure config file '%s'.\n", + path); + goto next; + } + read_config_file(path); next: free(path);
libibverbs must refuse to load arbitrary shared objects. This patch check the configuration directory and files for - being owned by root; - not being writable by others. Signed-off-by: Yann Droneaud <ydroneaud@opteya.com> --- src/init.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)