Open Stack

Ben Nemec wrote:
> On 4/1/19 2:59 PM, Michael Still wrote:
>> On Tue, Apr 2, 2019 at 5:06 AM Ben Nemec <openstack at nemebean.com 
>> <mailto:openstack at nemebean.com>> wrote:
>>
>> [snip]
>>     This matches my understanding of the intent behind privsep. 
>> Instead of
>>     trying to figure out how to filter out dangerous paths or 
>> whatever, you
>>     have the privileged code build the path out of sanitized data passed
>>     in.
>>     You still have to be careful about preventing injection attacks 
>> (say I
>>     pass in [uuid]/../../..), but at least you don't have to try to 
>> write a
>>     rootwrap filter for every possible path in every possible driver. The
>>     privileged driver code itself knows the correct path and won't allow
>>     anything else.
>>
>> I don't think anyone disputes that theory.
>>
>> The issue here is that the calling code in nova makes that very hard 
>> in some cases. For example, nova.privsep.path.readfile exists because 
>> it is called by nova.virt.disk.localfs.read_file [1], which takes a 
>> raw path to read. That is in turn used by calling code to read at 
>> least two paths that are generated in calling code. Now all of that is 
>> fixable, and that's a relatively simple example, but it requires a 
>> call by call analysis that is quite expensive to do and will cause a 
>> lot of code churn to fix.
>>
>> No one is saying we shouldn't do it. However, the current privsep code 
>> took the approach of an incremental improvement on top of what we 
>> already had, instead of attempting to boil the ocean in a single patch 
>> series. The code for nova.virt.disk.localfs.read_file used to call a 
>> method called read_file_as_root that was just as bad as what we have 
>> now, but used rootwrap to cat arbitrary files in the filesystem (git 
>> sha f535e8bb9905b5632416135af5789704db6d2867 is the original transition).
>>
>> My point is that we need to start somewhere, and proposing that we 
>> abandon privsep entirely because its not immediately perfect isn't 
>> very helpful.
> 
> For the record, that was not at all my intent. It doesn't sound to me 
> like the current privsep implementation is meaningfully worse than using 
> rootwrap, and it seems to me that it should be much easier (note: 
> easier, not easy) to move forward now that everything is using privsep. 
> I was just +1'ing the desired end state of privsep calls not taking 
> paths as input anymore.

Also, the reason why the calling Nova code has generic things like 
nova.virt.disk.localfs.read_file is because rootwrap /encouraged/ 
reusing generic filters across calling code. I see the privsep 
transition as having 3 steps:

1- introduce privsep
2- change rootwrap calls into generic privsep functions
3- start refactoring calling code so that generic privsep functions can 
be replaced by narrow, context-aware functions

You can tackle (2) and (3) at the same time. You get performance gains 
as soon as you start doing (2) bits, but you won't really gain security 
until (3) bits are completed for all the actually-this-means-root 
generic functions.

So I'm not sure we should "immediately stop adding or using generic 
privsep functions" as they don't really make the situation worse than it 
already is security-wise. For *new* features we should definitely avoid 
the privsep generic functions antipattern -- but transforming a generic 
rootwrap call into a generic privsep function still sounds like a gain.

It would be good to describe the antipattern and how to write "good" 
privsep functions though, if only to be able to point developers and 
reviewers to that. Suggestions on where we could do that?

-- 
Thierry Carrez (ttx)