[RFC PATCH ghak10 3/3] ntp: Audit valid attempts to adjust the clock

Tue Jun 19 16:22:32 UTC 2018

On 2018-06-19 10:30, Ondrej Mosnacek wrote:
> 2018-06-18 17:14 GMT+02:00 Richard Guy Briggs <rgb at redhat.com>:
> > On 2018-06-18 09:35, Ondrej Mosnacek wrote:
> >> 2018-06-16 18:44 GMT+02:00 Richard Guy Briggs <rgb at redhat.com>:
> >> > On 2018-06-15 14:45, Ondrej Mosnacek wrote:
> >> >> (Intentionally not sending to the timekeeping/ntp maintainers just yet,
> >> >> let's settle on the record contents/format first.)
> >> >>
> >> >> Signed-off-by: Ondrej Mosnacek <omosnace at redhat.com>
> >> >> ---
> >> >>  kernel/time/ntp.c | 11 +++++++++++
> >> >>  1 file changed, 11 insertions(+)
> >> >>
> >> >> diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
> >> >> index a09ded765f6c..78a01df0dbdb 100644
> >> >> --- a/kernel/time/ntp.c
> >> >> +++ b/kernel/time/ntp.c
> >> >> @@ -18,6 +18,7 @@
> >> >>  #include <linux/module.h>
> >> >>  #include <linux/rtc.h>
> >> >>  #include <linux/math64.h>
> >> >> +#include <linux/audit.h>
> >> >>
> >> >>  #include "ntp_internal.h"
> >> >>  #include "timekeeping_internal.h"
> >> >> @@ -722,6 +723,16 @@ int __do_adjtimex(struct timex *txc, struct timespec64 *ts, s32 *time_tai)
> >> >>  {
> >> >>       int result;
> >> >>
> >> >> +     /* Only log audit event if the clock was changed/attempted to be changed.
> >> >> +      * Based on the logic inside timekeeping_validate_timex().
> >> >> +      * NOTE: We need to log the event before any of the fields get
> >> >> +      * overwritten by the output values (the function will not fail, so it
> >> >> +      * is OK). */
> >> >> +     if (   ( (txc->modes & ADJ_ADJTIME) && !(txc->modes & ADJ_OFFSET_READONLY))
> >> >> +         || (!(txc->modes & ADJ_ADJTIME) &&   txc->modes)
> >> >> +         ||   (txc->modes & ADJ_SETOFFSET))
> >> >
> >> > Wouldn't the third condition be covered by the second?
> >>
> >> Not if txc->modes has ADJ_ADJTIME set, ADJ_OFFSET_READONLY unset, and
> >> ADJ_SETOFFSET set. Then it fails the first two conditions and only
> >> matches on the third one. I don't know if such combination can
> >> actually occur in practice, but timekeeping_validate_timex() doesn't
> >> reject it, so it's better to be on the safe side here.
> >
> > But the second condition would trigger on ADJ_OFFSET_READONLY when
> > ADJ_ADJTIME is not set (which may never happen together), which includes
> > ADJ_SETOFFSET alone or any other flag.
> 
> I think the misunderstanding is coming from the fact that
> ADJ_SETOFFSET is semantically unrelated to ADJ_OFFSET_READONLY (i.e.
> ADJ_OFFSET_READONLY does not necessarily mean we are not doing the
> "SETOFFSET" modification). From what I understand, it is a special
> value for the case when ADJ_ADJTIME is set, to override the default
> behavior which always modifies the offset. ADJ_SETOFFSET is checked in
> do_adjtimex (not to be confused with __do_adjtimex...) where it
> actually controls setting of some different "offset" than the "other
> offset" changed by ADJ_ADJTIME/ADJ_OFFSET.

Ok, fair enough, I'd figured they'd be related.  I suspect that its
logic is incomplete with respect to what interests us, in particular,
other exclusions that don't actually modify time.

> It is terribly confusing, but unfortunately it is what it is...
> Curiously, ADJ_ADJTIME (and its two related flags --
> ADJ_OFFSET_READONLY and ADJ_OFFSET_SINGLESHOT) are defined outside of
> the UAPI, but they are not referenced in the kernel anywhere outside
> of the do_adjtimex() handling code... So it might be some obsolete
> functionality or perhaps an undocumented API used by glibc to
> implement the adjtime(3) and/or ntp_adjtime(2) functions (these don't
> seem to be separate system calls, at least in the current kernel). I
> haven't looked at the glibc code though, so maybe I'm pointing to the
> wrong suspect here...

I did notice they were not user-visible like the rest.

> Either way, I would prefer the conditions to be spelled out like this,
> so that the logic is easy to compare to the
> timekeeping_validate_timex() function (just see where it checks for
> CAP_SYS_TIME, that's when there is a non-read-only operation
> happening). Right now it is quite easy to see that the logging
> condition is equivalent to the CAP_SYS_TIME checks in the validation
> function.

If that is the case, then that is a good argument for your last patch so
that the audit logic is updated when that function's logic is updated.

> To make things even simpler, it would be perhaps worth it to modify
> the timekeeping_validate_timex() function to signal whether the
> operation is readonly in the return code and use that in the
> condition...

Agreed.

> > Ok, for the second condition how about: txc->modes & ~(ADJ_ADJTIME | ADJ_OFFSET_READONLY)
> >
> > Or better yet if you only care about ADJ_ADJTIME and ADJ_SETOFFSET, how about only:
> >         txc->modes & (ADJ_ADJTIME | ADJ_SETOFFSET) && txc->modes != ADJ_OFFSET_READONLY
> > but I don't think that is what you want.
> >
> >> BTW, I just realized that the logging function can be called directly
> >> from inside the do_adjtimex() function in timekeeping.c (which is a
> >> wrapper around __do_adjtimex()), where it probably suits better (since
> >> this function contains the ADJ_SETOFFSET handling code and
> >> timekeeping.c also contains the timekeeping_validate_timex() function
> >> referenced in the comment). I will move it in v2.
> >>
> >> >
> >> >> +             audit_adjtime(txc);
> >> >> +
> >> >>       if (txc->modes & ADJ_ADJTIME) {
> >> >>               long save_adjust = time_adjust;
> >> >
> >> > - RGB
> >> >
> >> > --
> >> > Richard Guy Briggs <rgb at redhat.com>
> >> > Sr. S/W Engineer, Kernel Security, Base Operating Systems
> >> > Remote, Ottawa, Red Hat Canada
> >> > IRC: rgb, SunRaycer
> >> > Voice: +1.647.777.2635, Internal: (81) 32635
> >>
> >> --
> >> Ondrej Mosnacek <omosnace at redhat dot com>
> >> Associate Software Engineer, Security Technologies
> >> Red Hat, Inc.
> >
> > - RGB
> >
> > --
> > Richard Guy Briggs <rgb at redhat.com>
> > Sr. S/W Engineer, Kernel Security, Base Operating Systems
> > Remote, Ottawa, Red Hat Canada
> > IRC: rgb, SunRaycer
> > Voice: +1.647.777.2635, Internal: (81) 32635
> 
> 
> 
> -- 
> Ondrej Mosnacek <omosnace at redhat dot com>
> Associate Software Engineer, Security Technologies
> Red Hat, Inc.

- RGB

--
Richard Guy Briggs <rgb at redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635