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

2018-06-18 17:14 GMT+02:00 Richard Guy Briggs <rgb(a)redhat.com&gt;: > On 2018-06-18 09:35, Ondrej Mosnacek wrote: >> 2018-06-16 18:44 GMT+02:00 Richard Guy Briggs <rgb(a)redhat.com&gt;: >> > 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(a)redhat.com&gt; >> >> --- >> >> 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(a)redhat.com&gt; >> > 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(a)redhat.com&gt; > 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.