Re: [RFC PATCH ghak10 2/3] audit: Add the audit_adjtime() function

On Friday, June 15, 2018 8:45:22 AM EDT Ondrej Mosnacek wrote: > This patch adds a new function that shall be used to log any > modification of the system's clock (via the adjtimex() syscall). > > The function logs an audit record of type AUDIT_TIME_ADJUSTED with the > following fields: > * txmodes (the 'modes' field of struct timex) > * txoffset (the 'offset' field of struct timex) > * txfreq (the 'freq' field of struct timex) > * txmaxerr (the 'maxerror' field of struct timex) > * txesterr (the 'esterror' field of struct timex) > * txstatus (the 'status' field of struct timex) > * txconst (the 'constant' field of struct timex) > * txsec, txusec (the 'tv_sec' and 'tv_usec' fields of the 'time' field > of struct timex (respectively)) > * txtick (the 'tick' field of struct timex) Are all of these fields security relevant? Primarily what we need to know is if time is being adjusted. This is relevant because a bad guy may adjust system time to make something appear to happen earlier or later than it really did which make correlation hard or impossible.

-Steve > These fields allow to reconstruct what exactly was changed by the > adjtimex syscall and to what value(s). Note that the values reported are > taken directly from the structure as received from userspace. The > syscall handling code may do some clamping of the values internally > before actually changing the kernel variables. Also, the fact that this > record has been logged does not necessarily mean that some variable was > changed (it may have been set to the same value as the old value). > > Quick overview of the 'struct timex' semantics: > The 'modes' field is a bitmask specifying which time variables (if > any) should be adjusted. The other fields (of those listed above) > contain the values of the respective variables that should be set. If > the corresponding bit is not set in the 'modes' field, then a field's > value is not significant (it may be some garbage value passed from > userspace). > > Note that after processing the input values from userspace, the > handler writes (all) the current (new) internal values into the struct > and hands it back to userspace. These values are not logged. > > Also note that 'txusec' may actually mean nanoseconds, not > microseconds, depending on whether ADJ_NSEC is set in 'modes'. > > Possible improvements: > * log only the fields that contain valid values (based on 'modes' field) > - this is not hard to implement, but will require some non-trivial > logic inside audit_adjtime() that will need to mirror the logic in > ntp.c -- do we want that? > * move the conditional for logging/not logging into audit_adjtime() > - (see also next patch in series) > - may not be desirable due to reasons above > - we should probably either do both this and above or neither > * log also the old values + the actual values that get set > - this would be rather difficult to do, probably not worth it > > Signed-off-by: Ondrej Mosnacek <omosnace(a)redhat.com&gt; > --- > include/linux/audit.h | 11 +++++++++++ > kernel/auditsc.c | 10 ++++++++++ > 2 files changed, 21 insertions(+) > > diff --git a/include/linux/audit.h b/include/linux/audit.h > index 69c78477590b..26e9db46293c 100644 > --- a/include/linux/audit.h > +++ b/include/linux/audit.h > @@ -26,6 +26,7 @@ > #include <linux/sched.h> > #include <linux/ptrace.h> > #include <uapi/linux/audit.h> > +#include <uapi/linux/timex.h> > > #define AUDIT_INO_UNSET ((unsigned long)-1) > #define AUDIT_DEV_UNSET ((dev_t)-1) > @@ -353,6 +354,7 @@ extern void __audit_log_capset(const struct cred *new, > const struct cred *old); extern void __audit_mmap_fd(int fd, int flags); > extern void __audit_log_kern_module(char *name); > extern void __audit_fanotify(unsigned int response); > +extern void __audit_adjtime(const struct timex *txc); > > static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp) > { > @@ -455,6 +457,12 @@ static inline void audit_fanotify(unsigned int > response) __audit_fanotify(response); > } > > +static inline void audit_adjtime(const struct timex *txc) > +{ > + if (!audit_dummy_context()) > + __audit_adjtime(txc); > +} > + > extern int audit_n_rules; > extern int audit_signals; > #else /* CONFIG_AUDITSYSCALL */ > @@ -581,6 +589,9 @@ static inline void audit_log_kern_module(char *name) > static inline void audit_fanotify(unsigned int response) > { } > > +static inline void audit_adjtime(const struct timex *txc) > +{ } > + > static inline void audit_ptrace(struct task_struct *t) > { } > #define audit_n_rules 0 > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > index ceb1c4596c51..927bf51a9968 100644 > --- a/kernel/auditsc.c > +++ b/kernel/auditsc.c > @@ -2422,6 +2422,16 @@ void __audit_fanotify(unsigned int response) > AUDIT_FANOTIFY, "resp=%u", response); > } > > +void __audit_adjtime(const struct timex *txc) > +{ > + audit_log(audit_context(), GFP_KERNEL, AUDIT_TIME_ADJUSTED, > + "txmodes=%u txoffset=%li txfreq=%li txmaxerr=%li txesterr=%li " > + "txstatus=%i txconst=%li txsec=%li txusec=%li txtick=%li", > + txc->modes, txc->offset, txc->freq, txc->maxerror, > + txc->esterror, txc->status, txc->constant, > + (long)txc->time.tv_sec, (long)txc->time.tv_usec, txc->tick); > +} > + > static void audit_log_task(struct audit_buffer *ab) > { > kuid_t auid, uid;