Re: [Rpm-maint] Plugin ponderings

[Reshetova, Elena]

>Like said in some earlier email, there's not much hope getting the details
100% right the first time. The best way of finding out what works and what
doesn't is to try it out, so...
>I started looking into creating a syslog plugin for rpm. One could argue
that logging is fundamental enough to be in rpm core, but then having it in
a plugin allows nicely for alternatives (such as *gasp* native systemd
journal support) and is a nice and harmless way to test the plugin system.
Lets just say I ran >into issues real fast. No fingerpointing implied - I
didn't catch these on review either and some of the issues are direct
consequences from my own suggestions, doh :)

Yes, I have warned that I am a security person, nothing more :) So my head
is full of security use cases which are unfortunately far from covering all
potential use cases :( I think I will keep learning a lot about package
management for a long while still!

>For logging installs and erasures, one of the primary interests is
PLUGINHOOK_PSM_POST. As it is now, the hook doesn't get called if the
package failed to install, which is a case you'll certainly want to log:
>
> /* Run post transaction element hook for all plugins */
> if (!rc) rc = rpmpluginsCallPsmPost(ts->plugins, te);

>The other problem here is that the PSM_POST hook doesn't get passed the
result code of the install (otoh it doesn't get called on failure to begin
with so...). One could get the status from rpmteFailed(), but that element
failure is set only after rpmpsmRun() returns so its not available here.
>Similar issues are present in the TSM hooks: TSM_POST doesn't get called on
some error situations (file conflicts etc) where TSM_PRE is called, and the
transaction result code is not passed to plugins.

Yes, I guess it would make sense to make them all symmetric, like we are now
trying to get for scripts. And in this case of course return code is must. 

>Me thinks we seriously need to define a consistent convention for all
plugin hooks and then scrutinize the existing and future hook paths for
correctness in all possible situations.
Fully agree, just it will be probably also work in progress constantly as
getting hooks right :)

>I'd think part of the convention needs to be that whenever PRE-hooks get
called, the pairing POST hooks are guaranteed to get called too. Even if
some of the PRE-hooks returned errors: some plugin(s) might return an error
from its PRE-hook, but there could be other plugins which succeeded in their
own >business and might have allocated resources they need to free up in
POST.
Makes sense. 

>Then there's the issue of return codes: maybe all the POST hooks should
take an added 'rc' argument for the return code of the actual operation. 
I think this is good, we would just need to define for plugins what return
code means. Wonder if we can do it generic enough after all without going to
define a bunch of "InstallationSucededWithWarningsType1",
"InstallationSucededWithWarningsType2", "InstallationFailedReasonA",
"InstallationFailedReasonB", Like to tell that this hook will always get
RPMRC_OK, if package is installed (somehow, even with warnings) and FAIL, if
installation badly failed and package traces aren't present in filesystem. 
>That seems simple enough in principle, but there be dragons in the details:

>For example rpmpsmRun() returns RPMRC_FAIL only if the package didn't get
installed (or removed) at all. %pre and %preun scriptlets can prevent
install/erase from taking place, %post and %postun cannot (as the operations
cannot be rolled back) so RPMRC_OK is returned from
>rpmpsmRun() even if they fail: basically the operation succeeded with
warnings. This semantic needs to be taken into account with plugin hook
results too, so rpmpluginsCallPsmPost() can't really cause RPMRC_FAIL to be
returned from rpmpsmRun(). Or we need to revamp the whole damn return code
>system (which might well be the sanest option really, especially since its
just an internal API)
>Similar quirks are present in rpmtsRun() return codes, but that's a more
difficult case as its part of the public API (which everybody loves to hate
because of the nutty and useless return codes :)
>Another question is whether rpmRC codes are sufficient for the task. 
>It's used for a lot of things in rpm, but many of those uses are ugly
abuses in reality.

Return codes are really hard. Suppose we define some new ones for plugin
purposes, so that hooks can return more sane codes and potentially indicate
even type of failure. I would love to see some kind of security failure as
return code in this case and treat it more seriously :) But then for hooks
like rpmpluginsCallPsmPost() it doesn't matter anyway like you said: we
can't revert installation at this point, so even if we return
"EverythingIsReallyReallyBad", what can rpm do? I have noticed this at the
beginning when our initial patch was labelling the filesystem in
rpmpluginsCallPsmPost(), which made me think what if la

Re: [Rpm-maint] Script hooks patch

[Reshetova, Elena]

>I see a couple of further problems with this:

>- runLuaScript() will leak memory and a file descriptor if
>rpmpluginsCallScriptPre() fails.

Sorry, will fix. 

>- For external scripts the hooks run on different sides of the fork(): 
>pre-hook runs in the forked child, post-hook in the parent, so parent will
be unaware of anything occurring in the pre-hook.

Yes, but we can't run it in the parent, because how then we setup the
security context on forked process?

>I'd prefer the hooks called from rpmScriptRun() already so there would be
exactly one place where to deal with them, but whether that would actually
work is a different question entirely...
Would look nice from code point of view for sure, but I can't figure out a
way to do it currently...

>External scripts have a lengthy setup sequence in the child between the
>fork() and exec(), including a call to rpmExpand() on
%_install_script_path. Which should not, but technically could, have a
shell-invocation in it, which would cause a fork() + exec() of its own which
would cause (at least in case of SELinux) the exec context setup to go to
wrong address. It should be possible >to move the rpmExpand() into parent
and just pass the path as an argument to doScriptExec() though but just goes
to show how the kind of minefield this is.

Oh, shell invocation isn't good at all in this case: it will be invoked with
all rpm privileges :( Is it an intended feature or smth that we can try to
limit? I have been thinking of this just as a way to load configuration, but
it seems to be a worse case. 

>Internal scripts present different problems: as it is now, there's not much
difference between calling hooks from runLuaScript() and rpmScriptRun(), but
if we assume they'll get forked in the future, any security context switch
would have to happen after the fork. Which inevitably causes the same
>asymmetry with pre- and post-hooks as there is now with external scripts.
True, but how much does the parent needs to know about it? 

>One way to address it would be having an "unpaired" post-fork (or pre-exec
if you like) hook that explicitly runs in the child process, which basically
is what we have now in the script setup hook. Which then begs the question:
what exactly are the new script pre- and post-hooks supposed to be used for?
>Ok, I can imagine eg logging plugin wanting to do something in both of
those, but...

Yes, I am starting to lean towards 3 set of hooks for scripts: symmetrical
pre-post in parent (nicely combined in rpmScriptRun) and separate Setup hook
in child. The purpose of child hook would be to setup security context, the
pre-hook can be used for initial screening (IMO important part too), like if
we even want this particular  script from this particular package to run (I
can even think of passing script content to it, if we want to scan it for
malware patterns, but this is probably too futuristic), and post hook can be
used... hm... (trying to think of security reason)  maybe just simply for
verifying how did it go and cleaning up (can't find really any specific
reason now)

What do you think? Should I then change the patch to add two hooks in
addition to current one and move them to rpmScriptRun() for clean view?

Best Regards,
Elena.


smime.p7s
Description: S/MIME cryptographic signature
___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint