[Rpm-maint] First attempt for the patch on extending the plugin interface for rpm

Panu Matilainen pmatilai at laiskiainen.org
Wed Oct 10 14:21:19 UTC 2012


On 10/09/2012 03:26 PM, Reshetova, Elena wrote:
> Hi Panu,
>
> Thank you for such fast reply given that you are so busy!

Just trying to make up a bit for all the past delays ;)

> My comments are below.
>
>> Note that the existing hooks are by no means "sacred" for other than
>> documenting the needs of the SELinux collection. AFAIK the collections
>> aren't actually being used anywhere yet (its still marked experimental), and
>> the interfaces are internal-only so if there's something that needs/wants to
>> be changed, >they can be changed.
>
> Sure, I was first planning to substitute the OpenTE hook with generic pre
> transaction hook,
> but then I started to think that there is not enough reasons to touch the
> collections hooks because they are making sense
> for collections and there is even a collection plugin defined as a special
> plugin case. Also, the hooks that I try to introduce
> are not conflicting with collection hooks, so I didn't touch them so far.

Yup. My point was just that it IS still ok to change (rather than 
painfully work around) the existing hooks if the need rises.

>> Macros the means through which rpm is generally configured, and that kind
>> of approach should make it reasonably simple to allow selective
>> enabling/disabling of them from cli (annoying but inevitable necessity).
>> Might be more annoying for other API users - currently disabling SELinux is
>> a simple matter of setting RPMTRANS_FLAG_NOCONTEXTS flag for the
>> transaction, with plugins it would be something ... quite different.
>
> Yes, it would be different and I think it is good not to allow to disable
> the security plugin simply from command line for some transaction. This
> would allow the distribution to specify and control what kind of rpm
> plugins must be present.

Actually, the ability to sanely disable these things from cli and API is 
in practise a hard requirement. The reason is that rpm is used in a 
large variety of situations beyond the basic "system package management" 
task: creation of chroot (build-)environments that are nothing like what 
the system might be running, install/live images, certain per-user 
tasks, workarounds for weird setups etc. Plus all the things we never 
even imagined - by now I hope to have learned (the hard way) that no 
matter how unlikely it seems, the unforeseen oddball (yet legal) cases 
always exist :)

I do realize that needs on an embedded / handheld systems can be 
drastically different from  a general purpose system. How to support 
"full lockdown" where needed but allow disabling for other uses is 
something to think about.

>
>> I think this was one of the prime reasons I had that look into the
>> "security manager" object thingie: to avoid having to pass the transaction
>> set to places where it really does not belong to.
>> OTOH it would seem to me (most of) the places could simply be passed a
>> handle to the plugins instead of ts, as the ts is pretty much only used for
>> ts->plugins.
>
> True, I think this is that I will do: it still requires adding the plugins
> as parameter to functions, but avoids adding  ts_internal and ts itself. I
> will change my patch.

Ok.

>> ...and this is related to what was one of the issues the "security manager"
>> thing ran into. Assuming there can be more than one present:
>> how to meaningfully deal with errors? Returning an error if one or more of
>> them failed is a simple answer, but whether that's sufficient for the
>> callee(s) I dont know, I seem to recall some things wanting a better idea
>> of what failed and how (but that might well be just from staring too much at
>> the way things are currently done)
>
> Currently I am just returning an error is any of the plugins have returned
> an error. This is quite typical approach for security, when multi-layer
> security modules are present.

Yup, it's a simple and straightforward rule. Whether it's sufficient 
remains to be seen I guess.

> I think in this case it would be a responsibility of the plugin to indicate
> why an error condition was returned. For MSM plugin an error with
> explanation will be printed when smth fails due to plugin.
> It might not be the same thing as returning a concrete error, but there is
> no way to generalize the errors for different types of plugins (even
> security) that it actually makes much more sense.

What I was thinking was more in the direction of "if plugin X returned 
failure, should we even call the other hooks" (such as falling back to 
something else if one of the involved filesystem doesn't support feature 
Y), but this is now something that neither the plugin or rpm has any way 
of actually knowing. The point perhaps being, the exact semantics for 
the plugin hooks needs to be far more clearly defined sooner or later.

>> +
>> +        /* Run file updated hook for all plugins */
>> +        rc = rpmpluginsCallFileUpdated(ts->plugins, fsm->path,
>> + fsm->buf,
>> len);
>
>> Hmm. Looking at the MSM plugin (from
>> git://github.com/ereshetova/rpm.git), this is primarily used for
>> calculating yet another digest for the file being processed. This should be
>> avoidable, the rpmio FD_t type supports calculating more than one on the
>> digest on the fly, only the interfaces to use it are currently buried in
>> rpmio/rpmio_internal.h >(but not actually hidden from ABI).
>
> Yes, it can be used to calculate any digest or hash for the file to be
> installed and then used for example to calculate the RSA signature over the
> hash and write it to the xattrs for IMA. I will take a look if I can redo
> this part with the interface you mentioned.

FWIW, I've been considering exporting the fd digest API for quite some 
time. That stuff probably needs a bit of polishing/sanitizing before 
exporting, but I'm not at all opposed to doing it.

>
>>    		    rc = fsmSetSELabel(sehandle, dn, mode);
>> -
>> +                    if (!rc)
>> +                        rc = rpmpluginsCallDirCreated(ts->plugins,
>> + dn,
>> mode);
>
>> This is a pretty ugly special case hack :) Part of the reason for
>> fsmSetSELabel() was to isolate the whole labeling thing into something
>> fairly generic that could be as well in a plugin. So this should be more of
>> a "post file/dir-creation hook" I think . Also access to such a thing (or a
>> way to avoid the need there completely...) would be needed in
>> rpmdbMoveDatabase() too.
>
> Yes, special case hack indeed :) Actually I was thinking of simply calling
> the FILE_CLOSED hook (and maybe renaming it to smth better in this case) at
> this point, but then I guess I would need to add a special argument to
> indicate that this was for dir that wasn't included in the package (it is
> important to distinguish this for security reasons since you would likely
> label such directory in a different way). I will address this in the next
> patch version, too.

Right. For SELinux it hasn't mattered whether a directory was owned or 
not. Perhaps it could... although unowned directories are not exactly 
infrequent occurence in the real world non-perfect packaging. All it 
takes is a simple dependency loop (that might be hard to solve in 
packaging) to cause unowned directory to be created before the package 
actually owning the directory gets installed.

>
>>    	if (xx == 0) {
>> -	    xx = execv(argv[0], argv);
>> +                /* Run script exec hook for all plugins */
>> +                if (rpmpluginsCallScriptExec(ts->plugins, argv) !=
>> RPMRC_FAIL) {
>> +		        xx = execv(argv[0], argv);
>> +                }
>
>> Ah, this is one of the places where I remember pondering over with the
>> "security manager" experiment: my idea was to bury the execv() call itself
>> into the manager so it could be execv(), rpm_execcon() or whatever depending
>> on the manager type/what's loaded, but that didn't fly at all :) This seems
>> like a >much more feasible approach, but the hook name wants clarification
>> as the hook MUST NOT actually perform the exec, only setup any preliminaries
>> it wants for the exec (eg setexeccon() in case of selinux)
>
> Yes, this is what I had in mind now when I wrote this patch. I have noticed
> that actually executing the script in plugin won't be possible unless you
> want your scripts to be executed n times :) The purpose of the hook now is
> only to setup the security context.
> I will rename it in the next version to avoid confusion.

Ok.

>
>> @@ -409,6 +411,9 @@ static void handleInstInstalledFile(const rpmts
>> ts, rpmte p, rpmfi fi, int fx,
>>    	if (rpmtsFilterFlags(ts) & RPMPROB_FILTER_REPLACEOLDFILES)
>>    	    rConflicts = 0;
>>
>> +        /* run file conflict hook for all plugins */
>> +        rpmpluginsCallFileConflict(ts->plugins, ts, p, fi, otherFi,
>> otherHeader);
>> +
>
>> This wont work in rpm >= 4.10 which no longer uses the rpmfi
>> "self-iterator" here - random access to the file info sets is needed and it
>> can even end up accessing the same file info set at two places
>> simultaneously (its possible for a package to have file conflicts within
>> itself via directory symlinks). So you'd >have to actually pass indexes for
>> both fi and otherFi, but then the indexed accessors to rpmfi aren't exported
>> in the API currently and...
>
> Ok, sorry that I have missed this from the latest release. I will take a
> look on this and try to think what can be done. As minimum we would need
> just to know the path of conflicting file and previous package name that
> have installed this file.

For now, go for the absolute minimum exposure of rpm "objects". The 
problem here is different from the other internals (such as fsm in the 
previous versions) exposure, the issue here is that it exposes a ad-hoc 
set of things that rpm currently happens to internally use to handle 
this stuff. Which is not entirely unlike Lucas Arts adventure games of 
old: you have a rope, two bananas and a fish, and you need to somehow 
use those to cross the ravine to get the umbrella from the other side 
that you need to... :)

The fact that the hook breaks in rpm >= 4.10 despite arguments remaining 
the same kinda underlines the issue. Rpm internals are going through 
some pretty fundamental changes at the moment, and this is one of the 
places that will hopefully become saner when its all done (my wishful 
thinking is to have at least the basics around in rpm 4.11 already). 
When that happens we can export more, but right now its better to keep 
things to minimum you can get by with.

The signature verification hook is another place where some weird 
internal bits and pieces get passed around. That might be unavoidable, 
dunno... but then (and I think I've said this before), signature 
enforcing mechanism should really be in rpm core (and is sorely missed 
currently). Plugins might further enhance / implement policy around 
that, or something. Oh and I know this is hand-wavy and vague :-/

>> Overall I'm still not really comfortable of letting plugins mess around
>> with such a core functionality of rpm (I do remember the previous discussion
>> around this though)
>
> In previous discussion you have mentioned that plugins should not get any
> rpm internal data structs when it can be avoided (like fsm and etc.) and I
> have tried to address this.

Yes, noticed and appreciated :) The current version of the patch is a 
vast improvement over the previous one(s) from that POV.

What I referred to above is specifically the file conflict resolution 
thing (and our previous discussion about that). Eg looking at the MSM 
plugin, AFAICT it can let conflicts pass at this point, yet deny it deep 
down from inside when the transaction is already in progress. There 
always will be cases where a failure cannot be detected before actually 
trying to install (ie, in fsm), but that should be the absolute last 
resort, not something you'd ever see in "normal" operation.

> I am also willing to work further on this unless
> it becomes acceptable for upstream. The actual reason why a security plugin
> needs all these hooks is to get enough control over the installation
> process.
> Like being able to control if a certain file can be substituted from another
> package or if a certain package signed with a certain key should get
> installed and obtain all requested resources. It is very hard to address any
> of these things through rpm itself without starting to ugly strip and add
> the code here and there or arrange a bunch of ifdefs. And this of course
> makes the maintenance of rpm for a distribution with strict security
> requirements a nightmare :(
>
> I understand that for the PC world with root given to the user these things
> aren't really that needed.  But then you take a look on cases in mobile or
> IVI (In-vehicle-infotainment), they are not only applicable, but required by
> operators, car manufacturers and even safety requirements. How do we prevent
> a user from installing a new cool release of Angry Birds (that just happens
> to be malicious) that actually substitutes some of our system binaries,
> makes the car to lose the reversing back video stream and makes the user to
> reverse over the neighbour's dog? Maybe a bit too harsh example, but with
> modern systems it might be possible soon unless we put proper security
> mechanisms in place :(

I understand these concerns (I think :), and tightening rpm security I 
certainly have nothing against. I'm just perhaps looking at this from 
slightly different perspective:

Many of these issues actually exist on general purpose systems as well: 
for example you wouldn't generally want a 3rd party package from 
somewhere to be able to replace (whether through updating or obsoleting) 
a "system" package or its files. Or have that 3rd party package prevent 
an important update that happens to conflict with a file that the 3rd 
party package has already "claimed". So the kind of package priorization 
by its source / signature and related policies is a much more generic 
need than specialized handheld/embedded systems, the allowability (and 
need) of overriding is what differs more (BTW, how does the user of an 
embedded IVI or handheld device specify an rpm conflict override to 
begin with?)

That's why I'm a bit doubtful over some things I see here: plugins 
should not (have to) be by design doing strange things behind rpms back 
to achieve what are actually common needs.

	- Panu -


More information about the Rpm-maint mailing list