Bug 1550595 - Review Request: tpm2-abrmd-selinux - SELinux policies for tpm2-abrmd
Review Request: tpm2-abrmd-selinux - SELinux policies for tpm2-abrmd
Status: POST
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
Unspecified Unspecified
unspecified Severity unspecified
: ---
: ---
Assigned To: Robert-André Mauchin
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2018-03-01 09:51 EST by Javier Martinez Canillas
Modified: 2018-06-14 14:12 EDT (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed:
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
zebob.m: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Javier Martinez Canillas 2018-03-01 09:51:02 EST
Spec URL: https://raw.githubusercontent.com/martinezjavier/tpm2-abrmd-selinux/master/tpm2-abrmd-selinux.spec
SRPM URL: https://github.com/martinezjavier/tpm2-abrmd-selinux/raw/master/tpm2-abrmd-selinux-1.2.0-1.fc29.src.rpm

Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=25390371

Description:

The tpm2-abrmd (TPM2 access broker and Resource Manager) daemon is already included in Fedora. The latest 1.2.0 release contain some changes that makes the tpm2-abrmd to require a SELinux module in order to be used with the Fedora system wide SELinux policy.

This package is needed so the tpm2-abrmd SELinux module can be shipped and the daemon package updated to the latest release.

Fedora Account System Username: javierm
Comment 1 Lukas Vrabec 2018-04-09 07:56:24 EDT
Hi All,

I reviewed SELinux security policy for tpm2-abrmd and both spec file and
policy looks good to me, it reflects IndependentPolicy guidelines.

Thanks,
Lukas.
Comment 2 Robert-André Mauchin 2018-04-09 12:55:56 EDT
Thanks Lukas, I'm not a SELinux specialist so I didn't take this package, I''ll finish the review now.


 - These are not needed as this is the default:

%defattr(-,root,root,0755)
%attr(0644,root,root)
%attr(0644,root,root)


 - The latest version of tpm2-abrmd is 1.3.1, please bump your package.

 - The version in the header and the %changeloq are mismatched:

* Thu Mar 01 2018 Javier Martinez Canillas <javierm@redhat.com> - 0.0.1-1

  It should be 1.2.0-1 (or 1.3.1-1 when you update)
Comment 3 Robert-André Mauchin 2018-04-09 13:34:45 EDT
 - Add the LICENSE file with %license in %install

 - Own these directories:

[!]: Package must own all directories that it creates.
     Note: Directories without known owners:
     /usr/share/selinux/devel/include/contrib,
     /usr/share/selinux/devel/include, /usr/share/selinux/devel

 - Use %make_build instead of make for parallel build (unless it fails the build)

 
 Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed



===== MUST items =====

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[!]: If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %license.
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses
     found: "*No copyright* BSD (2 clause)", "BSD (2 clause)", "Unknown or
     generated". 30 files have unknown license. Detailed output of
     licensecheck in /home/bob/packaging/review/tpm2-abrmd-selinux/review-
     tpm2-abrmd-selinux/licensecheck.txt
[!]: Package must own all directories that it creates.
     Note: Directories without known owners:
     /usr/share/selinux/devel/include/contrib,
     /usr/share/selinux/devel/include, /usr/share/selinux/devel
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[!]: Each %files section contains %defattr if rpm < 4.4
     Note: %defattr present but not needed
[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Package is not known to require an ExcludeArch tag.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: Package requires other packages for directories it uses.
[x]: Package does not own files or directories owned by other packages.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 0 bytes in 0 files.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[!]: Uses parallel make %{?_smp_mflags} macro.
[-]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[?]: Package functions as described.
[!]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed
     files.
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: tpm2-abrmd-selinux-1.3.1-1.fc29.noarch.rpm
          tpm2-abrmd-selinux-1.3.1-1.fc29.src.rpm
tpm2-abrmd-selinux.noarch: E: explicit-lib-dependency libselinux-utils
tpm2-abrmd-selinux.noarch: W: no-documentation
tpm2-abrmd-selinux.noarch: W: dangerous-command-in-%pre cp
tpm2-abrmd-selinux.noarch: W: dangerous-command-in-%posttrans rm
2 packages and 0 specfiles checked; 1 errors, 3 warnings.
Comment 4 dac.override 2018-04-09 14:06:07 EDT
tpm2-abrmd-1.2.0/selinux/tabrmd.te:

allow tabrmd_t self:unix_dgram_socket { create_socket_perms };

redundant: provided by logging_send_syslog_msg(tabrmd_t)

https://github.com/fedora-selinux/selinux-policy/blob/rawhide/policy/modules/system/logging.te#L691

Questionable (can you reproduce this?): 

# This next bit doesn't belong here. It should be exposed through an
# interface likely from the dbus policy module.
gen_require(`
    type system_dbusd_t;
')
allow system_dbusd_t tabrmd_t:unix_stream_socket { read write };

If you can reproduce this then it should be inside the below optional block (no need to require type system_dbusd_t:

optional_policy(`
    dbus_system_domain(tabrmd_t, tabrmd_exec_t)
')

Your tabrmd.if file is useless (its like a library providing interfaces required to interact with your domain).
Comment 5 dac.override 2018-04-09 14:11:14 EDT
tabrmd.fc: arguably a bug in selinux-policy:

/usr/local/sbin/tpm2-abrmd      --      gen_context(system_u:object_r:tabrmd_exec_t,s0)

ideally an entry should be added to:

https://github.com/fedora-selinux/selinux-policy/blob/rawhide/config/file_contexts.subs_dist

/usr/local/sbin /usr/sbin
Comment 6 dac.override 2018-04-09 14:43:19 EDT
https://raw.githubusercontent.com/martinezjavier/tpm2-abrmd-selinux/master/tpm2-abrmd-selinux.spec

Excuse me but I believe that this spec is wrong:

The tabrmd.if file should be installed optionally seperately as part of a tpm2-abrmd-selinux-devel rpm, that requires selinux-policy-devel package (that owns /etc/selinux/targeted/devel

Look at these .if files as development headers
Comment 7 Javier Martinez Canillas 2018-04-09 14:49:49 EDT
(In reply to Robert-André Mauchin from comment #2)
> Thanks Lukas, I'm not a SELinux specialist so I didn't take this package,
> I''ll finish the review now.
>

Thanks a lot for your review!

> 
>  - These are not needed as this is the default:
> 
> %defattr(-,root,root,0755)
> %attr(0644,root,root)
> %attr(0644,root,root)
> 

Ok.

> 
>  - The latest version of tpm2-abrmd is 1.3.1, please bump your package.
>

That version wasn't released yet when I proposed the package for review more than a month ago (my original plan was to get reviewed so I could have this and update the tpm2-abrmd package to 1.3.0).
 
>  - The version in the header and the %changeloq are mismatched:
> 
> * Thu Mar 01 2018 Javier Martinez Canillas <javierm@redhat.com> - 0.0.1-1
>
>   It should be 1.2.0-1 (or 1.3.1-1 when you update)

Right, I thought that other packages were using their selinux_policyver in the %changelog but probably just got confused. I'll use the Version-Release instead.
Comment 8 Javier Martinez Canillas 2018-04-09 14:51:05 EDT
(In reply to Robert-André Mauchin from comment #3)
>  - Add the LICENSE file with %license in %install
> 
>  - Own these directories:
> 
> [!]: Package must own all directories that it creates.
>      Note: Directories without known owners:
>      /usr/share/selinux/devel/include/contrib,
>      /usr/share/selinux/devel/include, /usr/share/selinux/devel
> 
>  - Use %make_build instead of make for parallel build (unless it fails the
> build)
> 
>  
>  Package Review
> ==============

I'll fix all these and upload a new version. Thanks!
Comment 9 Javier Martinez Canillas 2018-04-09 14:57:09 EDT
(In reply to dac.override from comment #4)
> tpm2-abrmd-1.2.0/selinux/tabrmd.te:
> 
> allow tabrmd_t self:unix_dgram_socket { create_socket_perms };
> 
> redundant: provided by logging_send_syslog_msg(tabrmd_t)
> 
> https://github.com/fedora-selinux/selinux-policy/blob/rawhide/policy/modules/
> system/logging.te#L691
> 
> Questionable (can you reproduce this?): 
> 
> # This next bit doesn't belong here. It should be exposed through an
> # interface likely from the dbus policy module.
> gen_require(`
>     type system_dbusd_t;
> ')
> allow system_dbusd_t tabrmd_t:unix_stream_socket { read write };
> 
> If you can reproduce this then it should be inside the below optional block
> (no need to require type system_dbusd_t:
> 
> optional_policy(`
>     dbus_system_domain(tabrmd_t, tabrmd_exec_t)
> ')
>

Can you please take a look to the latest version of the policy module? Lukas already fixed tpm2-abrmd upstream:

https://github.com/tpm2-software/tpm2-abrmd/blob/1.x/selinux/tabrmd.te

 > Your tabrmd.if file is useless (its like a library providing interfaces
> required to interact with your domain).

Do you mean that it can just be removed? Sorry for the silly question but I'm not that familiar with SELinux.
Comment 10 dac.override 2018-04-09 15:00:42 EDT
redundant: https://github.com/tpm2-software/tpm2-abrmd/blob/1.x/selinux/tabrmd.te#L12

No i mean that you should probably populate that file with at least a minimal set of interfaces to interface with your domain.

Also thart .if file should ideally be installed with a seperate header devel-rpm that relies on the selinux-policy-devel rpm

This is a "header file" or a "devel" file
Comment 11 dac.override 2018-04-09 15:02:39 EDT
redudant: https://github.com/tpm2-software/tpm2-abrmd/blob/1.x/selinux/tabrmd.te#L18

the system_dbusd_t type is already enclosed with "dbus_system_domain()", no need to "import" it again with "dbus_stub()"
Comment 12 Javier Martinez Canillas 2018-04-09 15:03:39 EDT
(In reply to dac.override from comment #10)
> redundant:
> https://github.com/tpm2-software/tpm2-abrmd/blob/1.x/selinux/tabrmd.te#L12
> 
> No i mean that you should probably populate that file with at least a
> minimal set of interfaces to interface with your domain.
> 
> Also thart .if file should ideally be installed with a seperate header
> devel-rpm that relies on the selinux-policy-devel rpm
> 
> This is a "header file" or a "devel" file

Got it, I saw your other comment mentioned that too. I'll take care of all these when doing a re-spin of the package.

Thanks a lot for your review!
Comment 13 dac.override 2018-04-09 15:07:21 EDT
also this should be investigated reproduced:

https://github.com/tpm2-software/tpm2-abrmd/blob/1.x/selinux/tabrmd.te#L20

Its definitely not "rw_stream_socket_perms", if anything it is "unix_stream_socket { read write }" but even that should be clarified
Comment 14 Javier Martinez Canillas 2018-04-09 15:23:39 EDT
(In reply to dac.override from comment #13)
> also this should be investigated reproduced:
> 
> https://github.com/tpm2-software/tpm2-abrmd/blob/1.x/selinux/tabrmd.te#L20
> 
> Its definitely not "rw_stream_socket_perms", if anything it is
> "unix_stream_socket { read write }" but even that should be clarified

Ah, I see. The rw_stream_socket_perms it's actually much more than just read and write by looking at its definition in selinux-policy. I think you are correct and unix_stream_socket { read write } should be enough.

What do you mean by clarified? That's the reason why we need this policy in the first place, it's needed after the following tpm2-abrmd commit:

https://github.com/tpm2-software/tpm2-abrmd/commit/51a3c55d772b
Comment 15 dac.override 2018-04-09 15:34:02 EDT
it should be clarified because it is questionable.

If a "system_dbusd_domain" would need this permission then the permission would have been enclosed with "system_dbusd_domain()"

Looking at 
https://github.com/tpm2-software/tpm2-abrmd/commit/51a3c55d772b
it seems that this file descriptor gets passed to dbusd

So at least now that part is explained.

ideally the dbusd.if header would have exported an "dbus_rw_inherited_system_unix_stream_sockets()" interface for you to call, but there is not so just change line:

https://github.com/tpm2-software/tpm2-abrmd/blob/1.x/selinux/tabrmd.te#L20

to look like:

allow system_dbusd_t tabrmd_t:unix_stream_socket { read write};

Optionally add a comment: # TODO: add to dbus.if: dbus_rw_inherited_system_unix_stream_sockets() and call that instead
Comment 16 Javier Martinez Canillas 2018-04-09 15:37:15 EDT
(In reply to dac.override from comment #15)
> it should be clarified because it is questionable.
> 
> If a "system_dbusd_domain" would need this permission then the permission
> would have been enclosed with "system_dbusd_domain()"
> 
> Looking at 
> https://github.com/tpm2-software/tpm2-abrmd/commit/51a3c55d772b
> it seems that this file descriptor gets passed to dbusd
> 
> So at least now that part is explained.
> 
> ideally the dbusd.if header would have exported an
> "dbus_rw_inherited_system_unix_stream_sockets()" interface for you to call,
> but there is not so just change line:
> 
> https://github.com/tpm2-software/tpm2-abrmd/blob/1.x/selinux/tabrmd.te#L20
> 
> to look like:
> 
> allow system_dbusd_t tabrmd_t:unix_stream_socket { read write};
> 
> Optionally add a comment: # TODO: add to dbus.if:
> dbus_rw_inherited_system_unix_stream_sockets() and call that instead

I will, thanks again!
Comment 17 dac.override 2018-04-09 15:43:30 EDT
Oops i am wrong

You should add a tabrmd_rw_inherited_unix_stream_sockets() interface to tabrmd.if
and them call that in dbus.if instead....

########################################
## <summary>
##	Use and inherit system tabrmd file descriptors.
## </summary>
## <param name="domain">
##	<summary>
##	Domain allowed access.
##	</summary>
## </param>
#
interface(`tabrmd_use_fds',`
	gen_require(`
		type tabrmd_t;
	')

	allow $1 tabrmd_t:fd use;
')

########################################
## <summary>
##	Read and write inherited tabrmd DBUS unix stream sockets.
## </summary>
## <param name="domain">
##	<summary>
##	Domain allowed access.
##	</summary>
## </param>
#
interface(`tabrmd_rw_inherited_unix_stream_sockets',`
	gen_require(`
		type tabrmd_t;
	')

        tabrmd_use_fds($1)
	allow $1 tabrmd_t:unix_stream_socket { read write };
')
Comment 18 dac.override 2018-04-09 15:46:09 EDT
I other words this also demonstrates how the "selinux-policy modularization" effort lacks. Even now you have to ideally add changes to selinux-policy (dbus.te and file_contexts.subs_dist) to get it nice and tidy
Comment 19 dac.override 2018-04-09 15:47:41 EDT
typo's

########################################
## <summary>
##	Use and inherit tabrmd file descriptors.
## </summary>
## <param name="domain">
##	<summary>
##	Domain allowed access.
##	</summary>
## </param>
#
interface(`tabrmd_use_fds',`
	gen_require(`
		type tabrmd_t;
	')

	allow $1 tabrmd_t:fd use;
')

########################################
## <summary>
##	Read and write inherited tabrmd unix stream sockets.
## </summary>
## <param name="domain">
##	<summary>
##	Domain allowed access.
##	</summary>
## </param>
#
interface(`tabrmd_rw_inherited_unix_stream_sockets',`
	gen_require(`
		type tabrmd_t;
	')

        tabrmd_use_fds($1)
	allow $1 tabrmd_t:unix_stream_socket { read write };
')
Comment 20 dac.override 2018-04-09 15:50:12 EDT
So basically you export "tabrmd_rw_inherited_unix_stream_sockets()" in tabrmd.if and then you call "optional_policy(` tabrmd_rw_inherited_unix_stream_sockets(dbusd_system_t) ')" in dbus.te
Comment 21 Javier Martinez Canillas 2018-04-10 02:43:03 EDT
(In reply to dac.override from comment #20)
> So basically you export "tabrmd_rw_inherited_unix_stream_sockets()" in
> tabrmd.if and then you call "optional_policy(`
> tabrmd_rw_inherited_unix_stream_sockets(dbusd_system_t) ')" in dbus.te

I see, so then tpm2-abrmd-selinux will have to depend on a version of selinux-policy-contrib that contains the dbus.te changes, right?

I would also like Lukas opinion about this as well before doing the proposed change.
Comment 22 dac.override 2018-04-10 02:52:43 EDT
Yes.This is not going to work.
Comment 23 dac.override 2018-04-10 02:56:15 EDT
Indeed when the dbus module gets compiled it will be looking for the tabrmd_rw_inherited_unix_stream_sockets() interface that you export in tabrmd.if

If it is not there at build-time then it will just not include the rule.
Comment 24 dac.override 2018-04-10 02:59:24 EDT
In other words, you might get into a chicken and egg situation here.
Comment 25 dac.override 2018-04-10 03:04:21 EDT
Basically the way I see it is that this modularization effort requires that the headers are alway's installed if policy is installed. That then means that the various policy-devel packages need to alway's be installed.
Comment 26 Javier Martinez Canillas 2018-04-10 03:08:01 EDT
(In reply to dac.override from comment #25)
> Basically the way I see it is that this modularization effort requires that
> the headers are alway's installed if policy is installed. That then means
> that the various policy-devel packages need to alway's be installed.

Right, and then selinux-policy would need a BuildRequires dependency with tpm2-abrmd-selinux-devel (and all the -devel packages exporting interfaces). But then it won't be an independent SELinux policy module anymore as explained in the IndependentPolicy guideline...

So I think that we have these options:

a) Due as you propose and make selinux-policy-contrib to BuildRequires tpm2-abrmd-selinux-devel

b) Not having a tpm2-abrmd-selinux package and instead add the tpm2-abrmd AV rules to selinux-policy-contrib.

c) Just have "allow system_dbusd_t tabrmd_t:unix_stream_socket { read write }" in optional_policy as you first suggested.
Comment 27 dac.override 2018-04-10 03:15:32 EDT
Exactly.

a. Is in theory the most sane solution I Believe.
b. Is probably the most practical solution but that basically ignores modularization
c. Would be a short-term solution but is eventually probably a dead-end and sets a bad precendence. You see processes interact and operate. The purpose of the interfaces is to keep policy maintainable. If you start ignoring the interfaces that introduces inconsistencies and eventually part of the policy becomes hard to maintain
Comment 28 dac.override 2018-04-10 03:21:23 EDT
The CIL policy language would be a solution to this particular challenge. With the CIL language the interfaces are part of the modules. That means that there are no header packages. The interfaces are alway's available in the module store

CIL provides other benefits for this modularity scenario, The thing is that CIL is meant to be a intermediate layer, and there currently is no higher level language that leverages CIL.
Comment 29 Javier Martinez Canillas 2018-04-10 03:23:26 EDT
Got it. Thanks a lot for your explanations.

I think I'll probably go with (b) then. I like the idea of having independent modules for SELinux policies but now I understand that policies for the different components are more coupled than I thought.

I would like to go with (a), but my SELinux knowledge is close to non-existent so I'm by no means qualified to set a precedence on this.

I'm really just interested in updating tpm2-abrmd to the latest release to be honest.
Comment 30 dac.override 2018-04-10 03:41:50 EDT
Yes, It would have been less painful if your process did not pass fd's to dbus. That is really something I dislike about dbus. I think I like varlink a lot in that regard.

Nevertheless, I agree that currently b is probably the best way to go.
Comment 31 Javier Martinez Canillas 2018-05-08 11:23:19 EDT
So I finally found some time to work on this, as agreed I went with (b).

Following is the pull request for Fedora selinux-policy-contrib repo. Please let me know if I got something wrong:


https://github.com/fedora-selinux/selinux-policy-contrib/pull/57
Comment 32 Javier Martinez Canillas 2018-06-14 12:16:57 EDT
I've addressed all the issues pointed in the previous comments about the package. The new version is at:

Spec URL: https://raw.githubusercontent.com/martinezjavier/tpm2-abrmd-selinux/master/tpm2-abrmd-selinux.spec

SRPM URL: https://raw.githubusercontent.com/martinezjavier/tpm2-abrmd-selinux/master/tpm2-abrmd-selinux-1.3.1-1.fc29.src.rpm

Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=27617918
Comment 33 Robert-André Mauchin 2018-06-14 14:12:30 EDT
Good for me, package approved.

Note You need to log in before you can comment on or make changes to this bug.