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

[Ondrej Mosnacek]

2018-06-18 20:39 GMT+02:00 Steve Grubb <sgrubb at 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.

This is still an open question for me... On the one hand, we might
want to know exactly what the bad guy was trying to do ("He changed
the offset to 500 ms." vs. just "He adjusted the clock."), on the
other hand, we may not really care and consider it yet another junk
data in the logs... A possible compromise could be to log only
relevant fields (see 'Possible improvements' in the commit message).
Assuming ntpd or other authorized applications would only modify
one/few variables at a time, this would add only a few fields to the
record each time.

Note that this new auxiliary record gets only logged on *modifying*
operations, which should not be that frequent, and thus it shouldn't
be a problem to output a bit of potentially useful information.

That said, I don't mind logging just an empty record if that is
preferred. At the end of the day it is up to Paul to decide what he
will accept.

>
> -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 at 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;
>
>
>
>



-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.