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

[Ondrej Mosnacek]

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.

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...

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.

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...

>
> 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.