Hi Panu,
Thank you for such fast reply given that you are so busy!
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.
>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.
>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.
>...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.
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.
> +
> + /* 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.
> 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.
> 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.
> @@ -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.
>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. 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 :(
Best Regards,
Elena.
Attachment:
smime.p7s
Description: S/MIME cryptographic signature
