[RFC PATCH ghak10 v2 2/4] audit: Add the audit_adjtime() function

Wed Jun 27 07:59:16 UTC 2018

ut 19. 6. 2018 o 17:04 Richard Guy Briggs <rgb at redhat.com> 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 <omosnace at redhat.com>
> > ---
> >  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 <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.