Bug 1076247 - ntpd apparmor profile is overconstrained for certain setups
ntpd apparmor profile is overconstrained for certain setups
Status: RESOLVED FIXED
Classification: openSUSE
Product: openSUSE Tumbleweed
Classification: openSUSE
Component: AppArmor
Current
Other All
: P5 - None : Normal (vote)
: ---
Assigned To: Christian Boltz
E-mail List
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2018-01-16 18:37 UTC by Achim Gratz
Modified: 2020-10-25 20:53 UTC (History)
1 user (show)

See Also:
Found By: ---
Services Priority:
Business Priority:
Blocker: ---
Marketing QA Status: ---
IT Deployment: ---


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Achim Gratz 2018-01-16 18:37:01 UTC
When ntpd is configured to have a stratum-0 refclock and ntpstats logging is on, it will create clockstat files.  The apparmor profile does not allow this to happen, which seems to lead to strange delays of approximately 1 minute when shutting down ntpd.  Adding

 /var/log/ntpstats/clockstats* lrw,

in the appropriate place in the usr.sbin.ntpd apparmor profile fixed both issues for me.  In addition, since the location and name of the log file and statistics directory is in fact configurable from ntp.conf, it seems prudent to add a hint there that any changes there need to be accompanied by edits to the apparmor profile.
Comment 1 Christian Boltz 2018-01-16 20:25:09 UTC
Thanks for the report - patch submitted upstream.

For ntp.conf - I assume you have
    statsdir /var/log/ntpstats/
Right?

That will need a change in the ntp.conf we ship in the ntp package, but it's probably a better idea than more or less recommending /tmp/ (which is there in a comment).

If you are familiar with the OBS [1], feel free to send a SR to the ntp package ;-) Otherwise please tell me if my guess for statsdir was right and who needs write access to that directory (root or the 'ntp' user?)

[1] Actually such a small change is a good way to get started ;-)
Comment 2 Christian Boltz 2018-01-23 20:28:25 UTC
One of the upstream developers doubts the 'l' (link) permission is really needed, and since I don't have a stratum-0 refclock, I'd like to ask you to test this ;-)

Can you please change your added rule to

 /var/log/ntpstats/clockstats* rw,

Then run "rcapparmor reload" and report back if ntpd causes any log events (ALLOWED or DENIED) in /var/log/audit/audit.log? (If you don't have auditd running, check /var/log/messages or journalctl.)

For bonus points, also temporarily remove the 'l' permission from the other /var/log/ntpstats/loopstats* and peerstats* rules, run
  aa-complain /etc/apparmor.d/usr.sbin.ntpd
to switch the profile into complain mode and then provide the audit.log entries ntpd triggers.


Note: complain mode allows everything and logs what would be denied, so even if something is missing in the profile, ntpd will work.

To switch the profile back to enforce mode, run
  aa-enforce /etc/apparmor.d/usr.sbin.ntpd
Comment 3 Achim Gratz 2018-01-23 20:54:58 UTC
Yes, I have
 statsdir /var/log/ntpstats
as is standard.

(In reply to Christian Boltz from comment #2)
> One of the upstream developers doubts the 'l' (link) permission is really
> needed, and since I don't have a stratum-0 refclock, I'd like to ask you to
> test this ;-)

Huh?  What upstream developer was that?  If you care to look, the *stat files in that directory are always hardlinked to the *stat20180123 files for the same date and unlinked/relinked on date rollover.  So you do need to be able to create hardlinks.

> Can you please change your added rule to
> 
>  /var/log/ntpstats/clockstats* rw,

I can, but it's not much use I think since I don't usually run that machine while the date is rolling over.  Also, I think it _will_ fail.  But that problem should show on a daemon restart also… let me try.

> Then run "rcapparmor reload" and report back if ntpd causes any log events
> (ALLOWED or DENIED) in /var/log/audit/audit.log? (If you don't have auditd
> running, check /var/log/messages or journalctl.)
> 
> For bonus points, also temporarily remove the 'l' permission from the other
> /var/log/ntpstats/loopstats* and peerstats* rules, run
>   aa-complain /etc/apparmor.d/usr.sbin.ntpd
> to switch the profile into complain mode and then provide the audit.log
> entries ntpd triggers.

As suspected:

type=AVC msg=audit(1516740267.681:127): apparmor="ALLOWED" operation="link" profile="/usr/sbin/ntpd" name="/var/log/ntpstats/peerstats" pid=14450 comm="ntpd" requested_mask="l" denied_mask="l" fsuid=74 ouid=74 target="/var/log/ntpstats/peerstats.20180123"                                                                                                 
type=AVC msg=audit(1516740267.681:128): apparmor="ALLOWED" operation="link" profile="/usr/sbin/ntpd" name="/var/log/ntpstats/clockstats" pid=14450 comm="ntpd" requested_mask="l" denied_mask="l" fsuid=74 ouid=74 target="/var/log/ntpstats/clockstats.20180123"                                                                                               

If you want to simplify the rules you might use a glob there and require that everything is owned by ntp/ntp, that should have the same effect.

Another thing to add as comment to ntp.conf: mention NTPD_DEVICE and how to add any devices configured for refclocks in /etc/apparmor.d/tunables/ntpd.
Comment 4 Christian Boltz 2018-01-23 22:45:40 UTC
(In reply to Achim Gratz from comment #3)
> Yes, I have
>  statsdir /var/log/ntpstats
> as is standard.

Good to know, thanks!

> (In reply to Christian Boltz from comment #2)
> > One of the upstream developers doubts the 'l' (link) permission is really
> > needed, and since I don't have a stratum-0 refclock, I'd like to ask you to
> > test this ;-)
> 
> Huh?  What upstream developer was that?  

An upstream AppArmor developer with the goal to keep the profile as restrictive as possible ;-)

> If you care to look, the *stat
> files in that directory are always hardlinked to the *stat20180123 files for
> the same date and unlinked/relinked on date rollover.  So you do need to be
> able to create hardlinks.

Thanks for the explanation and the additional testing. That makes it obvious that 'l' permissions are really needed.

> If you want to simplify the rules you might use a glob there and require
> that everything is owned by ntp/ntp, that should have the same effect.

That would mean to prefix those rules with the owner keyword:

  owner /var/log/ntpstats/clockstats* lrw,
  owner /var/log/ntpstats/loopstats* lrw,
  owner /var/log/ntpstats/peerstats* lrw,

Can you please test if ntpd still works with the owner keyword added?

> Another thing to add as comment to ntp.conf: mention NTPD_DEVICE and how to
> add any devices configured for refclocks in /etc/apparmor.d/tunables/ntpd.

Indeed, that makes sense.
Comment 5 Achim Gratz 2018-01-24 18:35:33 UTC
(In reply to Christian Boltz from comment #4)
> > If you want to simplify the rules you might use a glob there and require
> > that everything is owned by ntp/ntp, that should have the same effect.
> 
> That would mean to prefix those rules with the owner keyword:
> 
>   owner /var/log/ntpstats/clockstats* lrw,
>   owner /var/log/ntpstats/loopstats* lrw,
>   owner /var/log/ntpstats/peerstats* lrw,
> 
> Can you please test if ntpd still works with the owner keyword added?

type=AVC msg=audit(1516818577.469:120): apparmor="STATUS" operation="profile_replace" profile="unconfined" name="/usr/sbin/ntpd" pid=4485 comm="apparmor_parser"

So it seems to work.  However, I thought to simplify those rules along the lines of

  owner /var/log/ntpstats/*stats* lrw,

or even
  owner /var/log/ntpstats/* lrw,

(if the number of rules is the objective here).  The directory already is "drwxr-xr-x 1 ntp ntp", so I don't think there'd be an appreciable loss in security with that slightly widened rule.
Comment 7 Christian Boltz 2018-12-21 14:39:41 UTC
Updated packages for Tumbleweed are on their way (SR 660561), Leap updates will follow in the next days.

These packages will add the "l" permission for the stats files, but won't add the owner restriction. Also, a SR for the ntp.conf comment is still on my TODO list.
Comment 8 Christian Boltz 2020-10-25 20:53:30 UTC
Looks like I forgot to close this bug ;-)