[Rpm-maint] FSM hooks for rpm plugin

[Panu Matilainen]

On 01/09/2013 08:19 PM, Reshetova, Elena wrote:
>> Hmm, do you mean FSM_INIT hook is the "pre" hook and FSM_COMMIT the "post"
> hook, or that both INIT and COMMIT have their own separate pre- and
> post-hooks?
>
> Sorry for being unclear, I just meant that for FSM it is better to have two
> hooks before file is put to the filesystem and after as opposite to one
> hook.

Ah, yup. Hooks both before and after create/remove is not only better 
but necessary I think.

>
>> If the former, its not symmetric as fsmCommit() does not get called on
> removed files, and there are almost certainly other (at least error) paths
> where fsmCommit() wont get called. Of course we can make the removal-path
> call FSM_COMMIT hook despite not being in fsmCommit(), but ... perhaps the
>> hook names should follow the pre/post convention then, eg FSM_FILE_PRE and
> FSM_FILE_POST.
>> In either case, I think file removals should be covered by the hooks
>> too: you might want to strip security-related labels or such before
> actually removing, eg to prevent "leaks" through hardlinks (see
>> removeSBITS() in fsm.c).
>
> I didn't think of removal cases originally, this is really a good point that
> we should take them into account, too. The safe choice won't be to strip the
> security-related label in this case, because in hardlink case this might
> open a hole in a system. For example, if my package has file
> "sensitive_data" and plugin labelled it "Sensitive" upon installation.
> During run-time only processes that have access to "Sensitive" can
> read/write this file. But suppose attacker would make a hard link to the
> file and then somehow initiate a package uninstall. Plugin will remove the
> file (the first path to it), strip the label and then attacker can access
> the file via hardlink because file isn't protected with the correct label
> anymore. Moreover in MAC systems you can't just remove a label, since
> everything has to have a label, you can only change label. So, I guess the
> correct way in this case would be to change the label to some special
> "Isolated" label that no one has any rights to read/write/execute. Then even
> if hardlink is left, it is unusable after package uninstallation.

For MAC labels and the like, yes. For some other purposes (eg if we were 
to move the posix file capabilities handling into a plugin) you'd want 
to strip. While the immediate prime motivations for this stuff revolve 
around MAC systems, lets try to keep other possibilities in mind :)

>> Perhaps the hooks should take an additional argument to indicate the
> operation - create/remove at least, but depending on other things skip might
> be needed as well.
>
> OK, so then I will make two hooks FSM_FILE_PRE and FSM_FILE_POST that would
> be always called for any file (both install and remove). I will pass the
> info about the operation type to the hook together with path and mode.
> I guess FSM_FILE_PRE can be called just after FsmInit(), but what would be
> the correct place for FSM_FILE_POST? Seems like I would have to call it
> twice based on operation performed either in commit() or unlink().

Hmm. It might actually be better to have both install and erase case 
individually call the hooks where appropriate for better control over 
symmetry and such: for example in erase case, fsmInit() can and often 
will get called without it ever unlinking anything. The install case is 
likely to be quite tricky to get just right because there are all sorts 
of failure paths and whatnot, but fsmCommit() is probably where the post 
hook needs to be. Actually makes me wonder whether it should be the 
place for pre-hook as well.

There are all sorts of questions and issues here, depending what exactly 
we want. For the current in-rpm selinux implementation, having both PRE 
and POST hooks called from fsmCommit() would be sufficient I think. But 
if you want to use temporary labels while the files are getting 
unpacked... you might actually need yet another set of similar hooks, eg 
FSM_UNPACK_PRE|POST that get called when the temporary unpacking suffix 
is in use, and additional FSM_FILE_PRE|POST calls when the file is 
getting moved into its final place in fsmCommit().

>
>> Yup, path and mode is minimalism to the extreme :) Being able to sanely
>> pass rpmfi objects would be nice but it requires bunch of other changes
>> to happen first.
>
> Yes, I guess other arguments would have to wait for changes to come.

BTW it should be possible to pass rpmfi objects if we want even now. It 
just needs a bit of wrapping to deal with the iterator index madness to 
ensure the hooks get called with correct indexes and save + restore the 
former values on entry/exit to the hooks. IIRC there are some funny 
little quirks related to how rpmfiSetFX() and friends "work", so perhaps 
its better to concentrate on the "simple arguments only" version first.

>
>> Rpm checks signatures when reading packages/headers, so unless signature
>> checking is disabled, the headers that were fed to
>> rpmtsAddInstall/EraseElement() are already signature checked. With
>> caveats. Eg. while rpm generally assumes the re-opened package within
>> transaction is the same thing as initially added to the transaction,
>> technially the callback can hand us something else. In which case things
>> are likely to go not very well :) And then there's the fact that rpm
>> doesn't have a mechanism to actually enforce signed packages only, etc.
>
> This is smth I didn't know about rpm: never looked into that part before:
> was thinking that signature is checked per package one by one.

That's what many/most depsolvers (yum etc) do: they turn off the 
sigcheck-on-read functionality and then perform the signature check as a 
separate step. The reasons vary from historical (ancient rpm versions 
did not have sigcheck-on-read, so it had to be a separate step) to 
hysterical (misdesigned and/or missing API's to deal with stuff, 
especially bad in the python bindings) and anything in between...

>
>> But this reminds me...
>
>> The root issue with MSM delaying conflict checking until its basically
>> too late was insufficient information available during rpmtsPrepare():
>> at that point rpm has long since ditched the header object, which you'd
>> need to get the MSM-specific data to decide if something should be
>> allowed or not. And the next time the header is available is indeed only
>> inside the psm/fsm stage deep inside already running transaction.
>
>> I think we need to have additional hook(s) in the
>> rpmtsAddInstall/EraseElement() area where the full header is available
>> for the cases where additional data is needed by plugins. There's just a
>> slight problem: these hooks would be called long before the plugins are
>> currently even initialized, so the plugin initialization would have to
>> change quite significantly.
>
> Yes, I guess if we go this path, then plugin initialization would have to
> happen in rpmtsCreate(). Currently there is only memory for pointer
> allocated, but I guess nothing prevents us from doing the loading of plugins
> here too. What do you think?

The caveat with initializing from rpmtsCreate() is that transaction sets 
are used for all sorts of things that have absolutely nothing to do with 
installing or removing packages, but we have no clue whatsoever about 
the inteded use in rpmtsCreate(). In fact we dont really know anything 
at all at that point, as all the transaction parameters are set with 
other setter functions one by one later on. So it's not a very friendly 
place to do initialization from, but OTOH it would be rather obvious and 
central place for doing it. Needs more head-scratching I'm afraid.

Another possibility might be doing a lazy initialization on the first 
rpmtsAddInstall/EraseElement() call, at which point we know at least a 
little bit more about our surroundings.

> I can move the loading here from rpmtsRun() and also then add hooks in
> addinstall and EraseElement. So, these hooks will be called per each package
> in transaction and before even transaction starts, and then from header h I
> can pass the needed info to the hook, which would make the plugin to be
> aware of sw source earlier than now. So, then the conflict hook is called
> later, it has all the needed info and if there are some conflicts it can't
> allow, it can stop the transaction in advance.
>
> This sounds all good, but there is still small problem left. I still would
> like to enforce my own policy on what signatures are ok and not and what are
> the action if signature verification fails. Do you think this should be the
> part of new hook in addInstallElement? This would be logical since it again
> allows us to fail transaction way before it started (if needed) without
> installing some packages from it first. In this case verify hook isn't
> needed at all.

A good question (and a complex one too), unfortunately I dont have a 
clear answer to it. The thing is, something being added as a transaction 
element doesn't necessarily mean an install/erase will be attempted at 
all, it can be used eg just to test dependency closure of a package set 
that might be completely unrelated to the system itself. So it shouldn't 
be overly strict, and probably is not the place to try to enforce any 
policies.

It depends on what exactly you want to be able to enforce: the signature 
checking done in rpmReadPackageFile() is the most central place of them 
all, but that includes things like queries of non-installed packages 
too, for which you might want a rather different policy than what's 
allowed to be installed. Another possibility might be checkProblems() in 
the early rpmtsRun() stage.

>> If you're referring to the unified package object absraction I think I
>> mentioned at some point (related to the mess of stuff currently needed
>> for file conflict calculations), that's basically just a fuzzy cloud
>> dream at the moment. It'd require massive changes to rpm internals (and
>> API as well), and I dont have much of a clue when that might happen in
>> reality so we'll just have to get by what we got now, more or less.
>
> OK, thank you for explaining. It was hard to understand the transition to
> this object, but I guess it is normal if it still in "fuzzy cloud dream
> state" :)
>
>> Sure, just send what you have, even if only as a basis of further
>> discussion. Looking at an actual patch helps highlighting the bizarre
>> dark corners of rpm that need dealing with :)
>
> It seems that talking to you first is better than writing patches :)

To get a higher level idea of what might be needed etc before committing 
to a lot of work, sure. My point was perhaps just more generally that in 
cases where you have a rough-cut/work-in-progress patch(es) at hand 
already, there's no harm done sending those as a basis of discussion.

> I guess now we might have 3 different patches in a queue: FSM hooks,
> changing the plugin initialization, and new hooks for
> add/eraseElement. Do you have order preferences for this?

I'd prefer getting the FSM hooks done first, and for two others the 
order is dictated by practicalities: add/eraseElement hooks cant happen 
unless the plugin initialization is moved first.

> Btw, not related to hooks. I am using rpm from master on my host machine for
> testing hooks and etc. Also, I have been porting plugin to your latest dev.
> release since we are finally moving to a new rpm (happiness!).
> But then it comes to using rpm from master it seems that rpm2cpio gets
> broken on my machine: I get plenty of cpio: Malformed number <garbage>. I
> discover it when running gbs build for our Tizen environment with rpm from
> master. I think this is smth related to lzma, but I don't really understand
> what happens. Could it be that it gets disabled for some reason or I didn't
> use correct configure options when compiling rpm?

My first guess would be missing XZ development libraries+includes during 
compilation of rpm. If that's the case, when installing package with 
'rpm -U...' you'd get dependency errors on rpmlib(PayloadIsXz) or 
rpmlib(PayloadIsLzma), but rpm2cpio performs no such sanity checks.

	- Panu -

>
> Best Regards,
> Elena.
>
>