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

[Steve Grubb <sgrubb@redhat.com>]

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@redhat.com>
> ---
>  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;