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@nemebean.com <mailto:openstack@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)