Re: [RFCv2 00/15] RFCv2: Consolidated userspace RDMA library repo

[Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

[-- Attachment #1.1: Type: text/plain, Size: 8378 bytes --]

On 9/1/2016 4:17 PM, Jason Gunthorpe wrote:
> On Thu, Sep 01, 2016 at 03:38:13PM -0400, Doug Ledford wrote:
>> On 9/1/2016 1:09 PM, Jason Gunthorpe wrote:
>>
>>> I'll write a patch to enable 'run from build' in a way which should
>>> make this even more straight forward.
>>>
>>> However, things are already fine today, you just drop your driver in
>>> /opt/intel-opa/lib/libhfi1.so and customize
>>> /etc/libverbs/hfi1verbs.driver with an absolute path to the .so (I
>>> wrote the absolute path patch for this years ago for exactly this
>>> reason)
>>
>> We do have to be a little bit careful here.
> 
> Nothing I describe requires any support beyond what we have today..
> 
> So if there is an actual security issue you need to make it clear and
> we need to deal with it right away.
> 
> However, I don't see your logic.
> 
>> 1)  The override directory needs to be a fixed place.
> 
> /etc/libibverbs.d/*.driver is the standard drop in place we used, and is
> already used by every provider driver.

I meant the .so's override directory.  I'm assuming the actual .so name
for an override driver will have the same name as the default .so, so
you need a second directory.  I would make that directory a fixed,
standard location.

>> 2)  The driver files need to only allow names.  The absolute path patch
>> sounds like an immediate security issue.
> 
> The patch for this has been in for years now.

It might be worth removing it, or at least analyzing it in more detail...

>> 3)  libibverbs needs to be modified to search the overrides directory
>> for <name>.so first, then the normal directory.  No other searches
>> should be performed.
> 
> libibverbs does not search paths. It dlopens "libfoo-rdmav2.so" and
> libdl searches the system search path for that name. AFAIK the user
> can override this search with LD_LIBRARY_PATH like any other dynamic
> link. (of course automatically prevented by libdl for setuid)
> 
> If *.driver specifies an absolute filename then libdl opens only that
> file.
> 
> We do not want libibverbs to search paths directly because that breaks
> biarch and other configurations. It is best to let libdl handle this.

It won't break biarch.  You can't load a 32bit .so with a 64bit
libibverbs and vice versa, so you can put the .so library path into your
binary based upon arch.  I would do this in preference to libdl searches.

>> 4)  The overrides directory and the normal directory need to be system
>> paths so that both file system permissions and SELinux context (if
>> enabled) prevent random files from being dropped there.
> 
> Of course. selinux/etc should apply the same protections to modifying
> the .driver files and the containing directory as it applies to
> modifying the system .so's. (ie only root can do it)
> 
> This is super critical.

It's not just the permissions to modify, it's the file's default
context.  Many files in SELinux parlance inherit their default context
based upon their location.  If you want to have a specific rdma_driver_t
context, you would need a directory specifically for them so you can
have an SELinux inheritance rule that sets the driver's context.
Historically, there hasn't been much in the way of SELinux rules related
to RDMA stuff, but I know that's actively changing as I've been involved
inside Red Hat with some of the very beginning stages of new rules and
contexts.

>> Basically, with the idea you outline above, you would have to change the
>> file type of the /etc/libibverbs.d/hfi1verbs.driver file (and all the
>> other driver files) to be a changeable config file, which prevents rpm
>> (and other) security scanners from being able to detect that it's
>> been
> 
> Really? You are shipping things in /etc/ and they are not marked config
> files? That doesn't violate your distro policies?

Generally all files in /etc are %config in their RPM tags.  But there
are two different %config markings in RPM, there is %config and
%config(noreplace).  On rpm upgrade of a package, any file marked
%config is overwritten with the new file.  If the old file had been
modified, it is saved as <filename>.rpmold.  If the file is
%config(noreplace), then on upgrade the new file will not overwrite the
old file, and if the new file varies from the old file in the previous
rpm, then the new file is saved as <filename>.rpmnew.  My comment above
is that in order to support modifying those driver files and not having
those modifications get wiped out on upgrade, the config files must be
marked as %config(noreplace) and that means we will preserve changes.
That's both good and bad in that the changes can be a user's desired
intent, or a nefarious user's subterfuge.  Now, if it nefarious, then
the admin could check for the presence of an .rpmnew file after each
upgrade, but as admin's are sometimes slack on that, it could slip under
the radar.

But really, with what I talked about above, with us creating two
directories, a standard and an override directory, and using that to
have SELinux file context inheritance, we can actually do away with all
of the /etc/libibverbs.d/* files entirely because we wouldn't need them
any more.  For proper SELinux support of a targeted subsystem, and the
RDMA stack really kinda deserves that because of the damage RDMA can do
in the wrong hands, this is almost mandatory.

> The entire point of these files, the only reason we have them at all,
> and the reason they are in /etc/, is *specifically* so users can
> configure the search path and customize the driver choice.
> 
> Otherwise we'd just dlopen("<kern-name>-rdmav2.so") and eliminate this
> configurability entirely. We certainly do not need the data stored in
> the .driver files to operate libibverbs. And indeed that
> auto-configurability is the basic approach I am thinking of to enable
> run-from-build.
> 
> So I view the lack of config marking as a packaging bug, sorry.
> 
>> anywhere.  And then you have something like opensm being run as root and
>> loading up this nefarious driver from wherever.  It's a big security issue.
> 
> This makes no sense. As above, access to the .driver files must
> require the same privledge to write as write to the .so libraries.

A text config file is easier to manage a hack on that a .so.  If your
library simply doesn't read any text config files and instead has
SELinux secured .so's in a well protected place, that is more secure.
Yes, you would need root in order to modify the driver config text files
if you are using vi, but the point is that the text config files are a
weak link in the security chain, at least weaker than the rest of it.

But, even if they do have root and use that to modify the text files,
that's still worse than just modifying the .so because an rpm upgrade
would not undo the damage so it's possible their subterfuge could exist
beyond measures that would otherwise render it null and void.  If the
admin found out that someone had hacked root and was attempting to
re-lock down the machine, that could be problematic (yes, I know they
should really be reinstalling from scratch, but admins do what admins do
when the shit hits the fan).

And in the intervening years, I doubt the configurability of these text
files has been used by more people than I can count on one hand.  And
those same people could, with no more than 5 minutes time, learn to
issue "make; make rpm; make install-rpm" in their git repo instead of
make install and get the same results with only slightly modified makefiles.

For all those reasons, I simply don't see the utility of those config
files being worth the added attack vector that they create.

> If an attacker can access the .driver files then they can access the
> .so, then the attacker can just replace libc.so, and game over.
> 
> I think any possible attack avenue would have to involve setuid. Eg
> trick a setuid program using libibverbs to dlopen a user controlled .so.
> 
> I don't have an avenue in mind though, that would require replacing
> /etc/ with something else, and AFAIK, the ability to do that would
> make  other setuid programs like su and sudo insecure??
> 
> Jason
> 


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG Key ID: 0E572FDD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]