ut 19. 6. 2018 o 17:04 Richard Guy Briggs <[email protected]> napísal(a): > > On 2018-06-19 15:58, 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; this is the only > > mandatory field) > > * txsec, txusec (the 'tv_sec' and 'tv_usec' fields of the 'time' field > > of struct timex (respectively)) > > * 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) > > * txtick (the 'tick' field of struct timex) > > I see below that you conditionally log these fields if the flag is > present. There has been concern of fields coming and going > (disappearing) so this could cause problems for parsers looking for > specific fields that aren't present. The first version of the patch > listed them all, which may be overkill. As has been suggested, limiting > it to security-affecting fields would be a first step.
I assumed that it was normal for records to have some fields that are optional (i.e. it is known and documented that they may be missing). But I understand that it is quite inconvenient to handle such fields and the desire to have all fields always there, even if with some special 'empty' values... > Another possibility would be to list one changed field per record. This > could work if multiple flags are infrequent in one call. A format I'd > suggest might be txfield=mode old=<value> new=<value>. (Note only the > single flag value rather than all modes flags.) There would be more > overhead if there was more than one field present in a call, but would > address the disappearing fields issue. Indeed this would be nicer, although more verbose... Also, detecting old and new values would require a bit more intrusive code changes, but it should be doable. > > > 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). > > If the value is the same, that may still be relevant for reporting that > someone tried to set/change it. > > > 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: > > * 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 <[email protected]> > > --- > > include/linux/audit.h | 11 +++++++++++ > > kernel/auditsc.c | 38 ++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 49 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..9a1f3fcc377a 100644 > > --- a/kernel/auditsc.c > > +++ b/kernel/auditsc.c > > @@ -2422,6 +2422,44 @@ void __audit_fanotify(unsigned int response) > > AUDIT_FANOTIFY, "resp=%u", response); > > } > > > > +void __audit_adjtime(const struct timex *txc) > > +{ > > + struct audit_buffer *ab; > > + > > + ab = audit_log_start(audit_context(), GFP_KERNEL, > > AUDIT_TIME_ADJUSTED); > > + if (unlikely(!ab)) > > + return; > > + > > + audit_log_format(ab, "txmodes=%u", txc->modes); > > + > > + if (txc->modes & ADJ_SETOFFSET) > > + audit_log_format(ab, " txsec=%li txusec=%li", > > + (long)txc->time.tv_sec, (long)txc->time.tv_usec); > > + > > + if (txc->modes & ADJ_OFFSET) > > + audit_log_format(ab, " txoffset=%li", txc->offset); > > + > > + if (txc->modes & ADJ_FREQUENCY) > > + audit_log_format(ab, " txfreq=%li", txc->freq); > > + > > + if (txc->modes & ADJ_MAXERROR) > > + audit_log_format(ab, " txmaxerr=%li", txc->maxerror); > > + > > + if (txc->modes & ADJ_ESTERROR) > > + audit_log_format(ab, " txesterr=%li", txc->esterror); > > + > > + if (txc->modes & ADJ_STATUS) > > + audit_log_format(ab, " txstatus=%li", txc->status); > > + > > + if (txc->modes & (ADJ_TIMECONST | ADJ_TAI)) > > + audit_log_format(ab, " txconst=%li", txc->constant); > > + > > + if (txc->modes & ADJ_TICK) > > + audit_log_format(ab, " txtick=%li", txc->tick); > > + > > + audit_log_end(ab); > > +} > > + > > static void audit_log_task(struct audit_buffer *ab) > > { > > kuid_t auid, uid; > > -- > > 2.17.1 > > > > - RGB > > -- > Richard Guy Briggs <[email protected]> > 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. -- Linux-audit mailing list [email protected] https://www.redhat.com/mailman/listinfo/linux-audit
