[RFC PATCH ghak10 v4 0/2] audit: Log modifying adjtimex(2) calls

[Ondrej Mosnacek]

Hi,

this patchset implements more detailed auditing of the adjtimex(2)
syscall in order to make it possible to:
a) distinguish modifying vs. read-only calls in the audit log
b) reconstruct from the audit log what changes were made and how they
have influenced the system clock

The main motivation is to be able to detect an adversary that tries to
confuse the audit timestamps by changing system time via adjtimex(2),
but at the same time avoid flooding the audit log with records of benign
read-only adjtimex(2) calls.

@John or other timekeeping/NTP folks: We had a discussion on the audit
ML on which of the internal timekeeping/NTP variables we should actually
log changes for. We are only interested in variables that can (directly
or indirectly) cause noticeable changes to the system clock, but since we
have only limited understanding of the NTP code, we would like to ask
you for advice on which variables are security relevant.

Right now, the patchset is conservative and logs all changes that can be
done via adjtimex(2):
- direct injection of timekeeping offset (obviously relevant)
- adjustment of timekeeping's TAI offset
- NTP value adjustments:
- time_offset (probably important)
- time_freq (maybe not important?)
- time_status (likely important, can cause leap second injection)
- time_maxerror (maybe not important?)
- time_esterror (maybe not important?)
- time_constant (???)
- time_adjust (sounds important)
- tick_usec (???)

Could you please give us some hints on the effect of changing these
variables and whether you think that it is important to log their
changes?

Thanks a lot!


GitHub issue: https://github.com/linux-audit/audit-kernel/issues/10

Changes in v4:
- Squashed first two patches into one
- Rename ADJNTPVAL's "type" field to "op" to align with audit record
conventions
- Minor commit message editing
- Cc timekeeping/NTP people for feedback

v3: https://www.redhat.com/archives/linux-audit/2018-July/msg00001.html
Changes in v3:
- Switched to separate records for each variable
- Both old and new value is now reported for each change
- Injecting offset is reported via a separate record (since this
offset consists of two values and is added directly to the clock,
i.e. it doesn't make sense to log old and new value)
- Added example records produced by chronyd -q (see the commit message
of the last patch)

v2: https://www.redhat.com/archives/linux-audit/2018-June/msg00114.html
Changes in v2:
- The audit_adjtime() function has been modified to only log those
fields that contain values that are actually used, resulting in more
compact records.
- The audit_adjtime() call has been moved to do_adjtimex() in
timekeeping.c
- Added an additional patch (for review) that simplifies the detection
if the syscall is read-only.

v1: https://www.redhat.com/archives/linux-audit/2018-June/msg00095.html

Ondrej Mosnacek (2):
audit: Add functions to log time adjustments
timekeeping/ntp: Audit clock/NTP params adjustments

include/linux/audit.h | 21 ++++++++++++++++
include/uapi/linux/audit.h | 2 ++
kernel/auditsc.c | 15 ++++++++++++
kernel/time/ntp.c | 50 ++++++++++++++++++++++++++++++--------
kernel/time/timekeeping.c | 3 +++
5 files changed, 81 insertions(+), 10 deletions(-)

[Ondrej Mosnacek]

This patch adds two auxiliary record types that will be used to annotate
the adjtimex SYSCALL records with the NTP/timekeeping values that have
been changed.

Next, it adds two functions to the audit interface:
- audit_tk_injoffset(), which will be called whenever a timekeeping
offset is injected by a syscall from userspace,
- audit_ntp_adjust(), which will be called whenever an NTP internal
variable is changed by a syscall from userspace.

Quick reference for the fields of the new records:
AUDIT_TIME_INJOFFSET
sec - the 'seconds' part of the offset
nsec - the 'nanoseconds' part of the offset
AUDIT_TIME_ADJNTPVAL
op - which value was adjusted:
offset - corresponding to the time_offset variable
freq - corresponding to the time_freq variable
status - corresponding to the time_status variable
maxerr - corresponding to the time_maxerror variable
esterr - corresponding to the time_esterror variable
const - corresponding to the time_constant variable
adjust - corresponding to the time_adjust variable
tick - corresponding to the tick_usec variable
tai - corresponding to the timekeeping's TAI offset
old - the old value
new - the new value

Signed-off-by: Ondrej Mosnacek <***@redhat.com>
---
include/linux/audit.h | 21 +++++++++++++++++++++
include/uapi/linux/audit.h | 2 ++
kernel/auditsc.c | 15 +++++++++++++++
3 files changed, 38 insertions(+)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 9334fbef7bae..0d084d4b4042 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)
@@ -356,6 +357,8 @@ 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_tk_injoffset(struct timespec64 offset);
+extern void __audit_ntp_adjust(const char *type, s64 oldval, s64 newval);

static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
{
@@ -458,6 +461,18 @@ static inline void audit_fanotify(unsigned int response)
__audit_fanotify(response);
}

+static inline void audit_tk_injoffset(struct timespec64 offset)
+{
+ if (!audit_dummy_context())
+ __audit_tk_injoffset(offset);
+}
+
+static inline void audit_ntp_adjust(const char *type, s64 oldval, s64 newval)
+{
+ if (!audit_dummy_context())
+ __audit_ntp_adjust(type, oldval, newval);
+}
+
extern int audit_n_rules;
extern int audit_signals;
#else /* CONFIG_AUDITSYSCALL */
@@ -584,6 +599,12 @@ static inline void audit_log_kern_module(char *name)
static inline void audit_fanotify(unsigned int response)
{ }

+static inline void audit_tk_injoffset(struct timespec64 offset)
+{ }
+
+static inline void audit_ntp_adjust(const char *type, s64 oldval, s64 newval)
+{ }
+
static inline void audit_ptrace(struct task_struct *t)
{ }
#define audit_n_rules 0
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 4e3eaba84175..242ce562b41a 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -114,6 +114,8 @@
#define AUDIT_REPLACE 1329 /* Replace auditd if this packet unanswerd */
#define AUDIT_KERN_MODULE 1330 /* Kernel Module events */
#define AUDIT_FANOTIFY 1331 /* Fanotify access decision */
+#define AUDIT_TIME_INJOFFSET 1332 /* Timekeeping offset injected */
+#define AUDIT_TIME_ADJNTPVAL 1333 /* NTP value adjustment */

#define AUDIT_AVC 1400 /* SE Linux avc denial or grant */
#define AUDIT_SELINUX_ERR 1401 /* Internal SE Linux Errors */
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index fb207466e99b..d355d32d9765 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2422,6 +2422,21 @@ void __audit_fanotify(unsigned int response)
AUDIT_FANOTIFY, "resp=%u", response);
}

+/* We need to allocate with GFP_ATOMIC here, since these two functions will be
+ * called while holding the timekeeping lock: */
+void __audit_tk_injoffset(struct timespec64 offset)
+{
+ audit_log(audit_context(), GFP_ATOMIC, AUDIT_TIME_INJOFFSET,
+ "sec=%lli nsec=%li", (long long)offset.tv_sec, offset.tv_nsec);
+}
+
+void __audit_ntp_adjust(const char *type, s64 oldval, s64 newval)
+{
+ audit_log(audit_context(), GFP_ATOMIC, AUDIT_TIME_ADJNTPVAL,
+ "op=%s old=%lli new=%lli", type,
+ (long long)oldval, (long long)newval);
+}
+
static void audit_log_task(struct audit_buffer *ab)
{
kuid_t auid, uid;

[Ondrej Mosnacek]

This patch adds logging of all attempts to either inject an offset into
the clock (producing an AUDIT_TIME_INJOFFSET record) or adjust an NTP
parameter (producing an AUDIT_TIME_ADJNTPVAL record).

For reference, running the following commands:

auditctl -D
auditctl -a exit,always -F arch=b64 -S adjtimex
chronyd -q

produces audit records like this:

type=TIME_ADJNTPVAL msg=audit(1530616044.507:5): op=adjust old=0 new=0
type=SYSCALL msg=audit(1530616044.507:5): arch=c000003e syscall=159 success=yes exit=5 a0=7fff57e78c00 a1=0 a2=4 a3=7f754ae28c0a items=0 ppid=626 pid=629 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=1 comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0 key=(null)
type=PROCTITLE msg=audit(1530616044.507:5): proctitle=6368726F6E7964002D71
type=TIME_ADJNTPVAL msg=audit(1530616044.507:6): op=maxerr old=16000000 new=0
type=SYSCALL msg=audit(1530616044.507:6): arch=c000003e syscall=159 success=yes exit=5 a0=7fff57e78c00 a1=1 a2=1 a3=7f754ae28c0a items=0 ppid=626 pid=629 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=1 comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0 key=(null)
type=PROCTITLE msg=audit(1530616044.507:6): proctitle=6368726F6E7964002D71
type=TIME_INJOFFSET msg=audit(1530616044.507:7): sec=0 nsec=0
type=TIME_ADJNTPVAL msg=audit(1530616044.507:7): op=status old=64 new=8256
type=SYSCALL msg=audit(1530616044.507:7): arch=c000003e syscall=159 success=yes exit=5 a0=7fff57e78c00 a1=1 a2=1 a3=7f754ae28c0a items=0 ppid=626 pid=629 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=1 comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0 key=(null)
type=PROCTITLE msg=audit(1530616044.507:7): proctitle=6368726F6E7964002D71
type=TIME_ADJNTPVAL msg=audit(1530616044.507:8): op=status old=8256 new=8257
type=TIME_ADJNTPVAL msg=audit(1530616044.507:8): op=offset old=0 new=0
type=TIME_ADJNTPVAL msg=audit(1530616044.507:8): op=freq old=0 new=0
type=SYSCALL msg=audit(1530616044.507:8): arch=c000003e syscall=159 success=yes exit=5 a0=7fff57e78ab0 a1=0 a2=55e129c850c0 a3=7f754ae28c0a items=0 ppid=626 pid=629 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=1 comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0 key=(null)
type=PROCTITLE msg=audit(1530616044.507:8): proctitle=6368726F6E7964002D71
type=TIME_ADJNTPVAL msg=audit(1530616044.507:9): op=status old=8257 new=64
type=SYSCALL msg=audit(1530616044.507:9): arch=c000003e syscall=159 success=yes exit=5 a0=7fff57e78ab0 a1=0 a2=55e129c850c0 a3=7f754ae28c0a items=0 ppid=626 pid=629 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=1 comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0 key=(null)
type=PROCTITLE msg=audit(1530616044.507:9): proctitle=6368726F6E7964002D71
type=SYSCALL msg=audit(1530616044.507:10): arch=c000003e syscall=159 success=yes exit=5 a0=7fff57e78a70 a1=0 a2=55e129c850c0 a3=7f754ae28c0a items=0 ppid=626 pid=629 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=1 comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0 key=(null)
type=PROCTITLE msg=audit(1530616044.507:10): proctitle=6368726F6E7964002D71
type=TIME_ADJNTPVAL msg=audit(1530616044.511:11): op=freq old=0 new=49180377088000
type=TIME_ADJNTPVAL msg=audit(1530616044.511:11): op=tick old=10000 new=10000
type=SYSCALL msg=audit(1530616044.511:11): arch=c000003e syscall=159 success=yes exit=5 a0=7fff57e78ad0 a1=0 a2=2710 a3=f42f82a800000 items=0 ppid=626 pid=629 auid=0 uid=385 gid=382 euid=385 suid=385 fsuid=385 egid=382 sgid=382 fsgid=382 tty=(none) ses=1 comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0 key=(null)
type=PROCTITLE msg=audit(1530616044.511:11): proctitle=6368726F6E7964002D71
type=TIME_ADJNTPVAL msg=audit(1530616044.521:12): op=status old=64 new=64
type=TIME_ADJNTPVAL msg=audit(1530616044.521:12): op=maxerr old=16000000 new=16000000
type=TIME_ADJNTPVAL msg=audit(1530616044.521:12): op=esterr old=16000000 new=16000000
type=SYSCALL msg=audit(1530616044.521:12): arch=c000003e syscall=159 success=yes exit=5 a0=7fff57e78b40 a1=1 a2=40 a3=f91f6ef84fbab items=0 ppid=626 pid=629 auid=0 uid=385 gid=382 euid=385 suid=385 fsuid=385 egid=382 sgid=382 fsgid=382 tty=(none) ses=1 comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0 key=(null)
type=PROCTITLE msg=audit(1530616044.521:12): proctitle=6368726F6E7964002D71
type=TIME_INJOFFSET msg=audit(1530616049.652:13): sec=-16 nsec=124887145
type=TIME_ADJNTPVAL msg=audit(1530616049.652:13): op=status old=64 new=8256
type=SYSCALL msg=audit(1530616049.652:13): arch=c000003e syscall=159 success=yes exit=5 a0=7fff57e78270 a1=1 a2=fffffffffffffff0 a3=137b828205ca12 items=0 ppid=626 pid=629 auid=0 uid=385 gid=382 euid=385 suid=385 fsuid=385 egid=382 sgid=382 fsgid=382 tty=(none) ses=1 comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0 key=(null)
type=PROCTITLE msg=audit(1530616049.652:13): proctitle=6368726F6E7964002D71
type=TIME_ADJNTPVAL msg=audit(1530616033.783:14): op=freq old=49180377088000 new=49180377088000
type=TIME_ADJNTPVAL msg=audit(1530616033.783:14): op=tick old=10000 new=10000
type=SYSCALL msg=audit(1530616033.783:14): arch=c000003e syscall=159 success=yes exit=5 a0=7fff57e78bc0 a1=0 a2=2710 a3=0 items=0 ppid=626 pid=629 auid=0 uid=385 gid=382 euid=385 suid=385 fsuid=385 egid=382 sgid=382 fsgid=382 tty=(none) ses=1 comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0 key=(null)
type=PROCTITLE msg=audit(1530616033.783:14): proctitle=6368726F6E7964002D71

The chronyd command that produced the above records executed the
following adjtimex(2) syscalls (as per strace output):

adjtimex({modes=ADJ_OFFSET|0x8000, offset=0, freq=0, maxerror=16000000, esterror=16000000, status=STA_UNSYNC, constant=2, precision=1, tolerance=32768000, time={tv_sec=1530616044, tv_usec=507215}, tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0, jitcnt=0, calcnt=0, errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)
adjtimex({modes=ADJ_MAXERROR, offset=0, freq=0, maxerror=0, esterror=16000000, status=STA_UNSYNC, constant=2, precision=1, tolerance=32768000, time={tv_sec=1530616044, tv_usec=507438}, tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0, jitcnt=0, calcnt=0, errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)
adjtimex({modes=ADJ_SETOFFSET|ADJ_NANO, offset=0, freq=0, maxerror=16000000, esterror=16000000, status=STA_UNSYNC|STA_NANO, constant=2, precision=1, tolerance=32768000, time={tv_sec=1530616044, tv_usec=507604737}, tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0, jitcnt=0, calcnt=0, errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)
adjtimex({modes=ADJ_OFFSET|ADJ_STATUS, offset=0, freq=0, maxerror=16000000, esterror=16000000, status=STA_PLL|STA_UNSYNC|STA_NANO, constant=2, precision=1, tolerance=32768000, time={tv_sec=1530616044, tv_usec=507698330}, tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0, jitcnt=0, calcnt=0, errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)
adjtimex({modes=ADJ_STATUS, offset=0, freq=0, maxerror=16000000, esterror=16000000, status=STA_UNSYNC, constant=2, precision=1, tolerance=32768000, time={tv_sec=1530616044, tv_usec=507792}, tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0, jitcnt=0, calcnt=0, errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)
adjtimex({modes=0, offset=0, freq=0, maxerror=16000000, esterror=16000000, status=STA_UNSYNC, constant=2, precision=1, tolerance=32768000, time={tv_sec=1530616044, tv_usec=508000}, tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0, jitcnt=0, calcnt=0, errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)
adjtimex({modes=ADJ_FREQUENCY|ADJ_TICK, offset=0, freq=750433, maxerror=16000000, esterror=16000000, status=STA_UNSYNC, constant=2, precision=1, tolerance=32768000, time={tv_sec=1530616044, tv_usec=512146}, tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0, jitcnt=0, calcnt=0, errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)
adjtimex({modes=ADJ_MAXERROR|ADJ_ESTERROR|ADJ_STATUS, offset=0, freq=750433, maxerror=16000000, esterror=16000000, status=STA_UNSYNC, constant=2, precision=1, tolerance=32768000, time={tv_sec=1530616044, tv_usec=522506}, tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0, jitcnt=0, calcnt=0, errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)
adjtimex({modes=ADJ_SETOFFSET|ADJ_NANO, offset=0, freq=750433, maxerror=16000000, esterror=16000000, status=STA_UNSYNC|STA_NANO, constant=2, precision=1, tolerance=32768000, time={tv_sec=1530616033, tv_usec=778717675}, tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0, jitcnt=0, calcnt=0, errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)
adjtimex({modes=ADJ_FREQUENCY|ADJ_TICK, offset=0, freq=750433, maxerror=16000000, esterror=16000000, status=STA_UNSYNC|STA_NANO, constant=2, precision=1, tolerance=32768000, time={tv_sec=1530616033, tv_usec=784644657}, tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0, jitcnt=0, calcnt=0, errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)

(The struct timex fields above are from *after* the syscall was
executed, so they contain the current (new) values as set from the
kernel, except of the 'modes' field, which contains the original value
sent by the caller.)

Note that the records are emitted even when the actual value does not
change (i.e. when there is an explicit attempt to change a value, but
the new value equals the old one).

Signed-off-by: Ondrej Mosnacek <***@redhat.com>
---
kernel/time/ntp.c | 50 +++++++++++++++++++++++++++++++--------
kernel/time/timekeeping.c | 3 +++
2 files changed, 43 insertions(+), 10 deletions(-)

diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index a09ded765f6c..cb2073a69ada 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -18,6 +18,7 @@
#include <linux/module.h>
#include <linux/rtc.h>
#include <linux/math64.h>
+#include <linux/audit.h>

#include "ntp_internal.h"
#include "timekeeping_internal.h"
@@ -294,6 +295,8 @@ static inline s64 ntp_update_offset_fll(s64 offset64, long secs)

static void ntp_update_offset(long offset)
{
+ s64 old_offset = time_offset;
+ s64 old_freq = time_freq;
s64 freq_adj;
s64 offset64;
long secs;
@@ -342,6 +345,9 @@ static void ntp_update_offset(long offset)
time_freq = max(freq_adj, -MAXFREQ_SCALED);

time_offset = div_s64(offset64 << NTP_SCALE_SHIFT, NTP_INTERVAL_FREQ);
+
+ audit_ntp_adjust("offset", old_offset, time_offset);
+ audit_ntp_adjust("freq", old_freq, time_freq);
}

/**
@@ -669,45 +675,67 @@ static inline void process_adjtimex_modes(struct timex *txc,
struct timespec64 *ts,
s32 *time_tai)
{
- if (txc->modes & ADJ_STATUS)
- process_adj_status(txc, ts);
+ if (txc->modes & (ADJ_STATUS | ADJ_NANO | ADJ_MICRO)) {
+ int old_status = time_status;
+
+ if (txc->modes & ADJ_STATUS)
+ process_adj_status(txc, ts);

- if (txc->modes & ADJ_NANO)
- time_status |= STA_NANO;
+ if (txc->modes & ADJ_NANO)
+ time_status |= STA_NANO;

- if (txc->modes & ADJ_MICRO)
- time_status &= ~STA_NANO;
+ if (txc->modes & ADJ_MICRO)
+ time_status &= ~STA_NANO;
+
+ audit_ntp_adjust("status", old_status, time_status);
+ }

if (txc->modes & ADJ_FREQUENCY) {
+ s64 old_freq = time_freq;
+
time_freq = txc->freq * PPM_SCALE;
time_freq = min(time_freq, MAXFREQ_SCALED);
time_freq = max(time_freq, -MAXFREQ_SCALED);
/* update pps_freq */
pps_set_freq(time_freq);
+
+ audit_ntp_adjust("freq", old_freq, time_freq);
}

- if (txc->modes & ADJ_MAXERROR)
+ if (txc->modes & ADJ_MAXERROR) {
+ audit_ntp_adjust("maxerr", time_maxerror, txc->maxerror);
time_maxerror = txc->maxerror;
+ }

- if (txc->modes & ADJ_ESTERROR)
+ if (txc->modes & ADJ_ESTERROR) {
+ audit_ntp_adjust("esterr", time_esterror, txc->esterror);
time_esterror = txc->esterror;
+ }

if (txc->modes & ADJ_TIMECONST) {
+ long old_constant = time_constant;
+
time_constant = txc->constant;
if (!(time_status & STA_NANO))
time_constant += 4;
time_constant = min(time_constant, (long)MAXTC);
time_constant = max(time_constant, 0l);
+
+ audit_ntp_adjust("const", old_constant, time_constant);
}

- if (txc->modes & ADJ_TAI && txc->constant > 0)
+ if (txc->modes & ADJ_TAI && txc->constant > 0) {
+ audit_ntp_adjust("tai", *time_tai, txc->constant);
*time_tai = txc->constant;
+ }

if (txc->modes & ADJ_OFFSET)
ntp_update_offset(txc->offset);

- if (txc->modes & ADJ_TICK)
+ if (txc->modes & ADJ_TICK) {
+ audit_ntp_adjust("tick", tick_usec, txc->tick);
tick_usec = txc->tick;
+ }

if (txc->modes & (ADJ_TICK|ADJ_FREQUENCY|ADJ_OFFSET))
ntp_update_frequency();
@@ -729,6 +757,8 @@ int __do_adjtimex(struct timex *txc, struct timespec64 *ts, s32 *time_tai)
/* adjtime() is independent from ntp_adjtime() */
time_adjust = txc->offset;
ntp_update_frequency();
+
+ audit_ntp_adjust("adjust", save_adjust, txc->offset);
}
txc->offset = save_adjust;
} else {
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 4786df904c22..9089ac329e69 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -25,6 +25,7 @@
#include <linux/stop_machine.h>
#include <linux/pvclock_gtod.h>
#include <linux/compiler.h>
+#include <linux/audit.h>

#include "tick-internal.h"
#include "ntp_internal.h"
@@ -2308,6 +2309,8 @@ int do_adjtimex(struct timex *txc)
ret = timekeeping_inject_offset(&delta);
if (ret)
return ret;
+
+ audit_tk_injoffset(delta);
}

getnstimeofday64(&ts);

[Thomas Gleixner]

On Mon, 20 Aug 2018, Ondrej Mosnacek wrote:

+ Miroslav Lichvar

Post by Ondrej Mosnacek
Hi,
this patchset implements more detailed auditing of the adjtimex(2)
a) distinguish modifying vs. read-only calls in the audit log
b) reconstruct from the audit log what changes were made and how they
have influenced the system clock
The main motivation is to be able to detect an adversary that tries to
confuse the audit timestamps by changing system time via adjtimex(2),
but at the same time avoid flooding the audit log with records of benign
read-only adjtimex(2) calls.
@John or other timekeeping/NTP folks: We had a discussion on the audit
ML on which of the internal timekeeping/NTP variables we should actually
log changes for. We are only interested in variables that can (directly
or indirectly) cause noticeable changes to the system clock, but since we
have only limited understanding of the NTP code, we would like to ask
you for advice on which variables are security relevant.
Right now, the patchset is conservative and logs all changes that can be
- direct injection of timekeeping offset (obviously relevant)
- adjustment of timekeeping's TAI offset
- time_offset (probably important)
- time_freq (maybe not important?)
- time_status (likely important, can cause leap second injection)
- time_maxerror (maybe not important?)
- time_esterror (maybe not important?)
- time_constant (???)
- time_adjust (sounds important)
- tick_usec (???)
Could you please give us some hints on the effect of changing these
variables and whether you think that it is important to log their
changes?
Thanks a lot!
GitHub issue: https://github.com/linux-audit/audit-kernel/issues/10
- Squashed first two patches into one
- Rename ADJNTPVAL's "type" field to "op" to align with audit record
conventions
- Minor commit message editing
- Cc timekeeping/NTP people for feedback
v3: https://www.redhat.com/archives/linux-audit/2018-July/msg00001.html
- Switched to separate records for each variable
- Both old and new value is now reported for each change
- Injecting offset is reported via a separate record (since this
offset consists of two values and is added directly to the clock,
i.e. it doesn't make sense to log old and new value)
- Added example records produced by chronyd -q (see the commit message
of the last patch)
v2: https://www.redhat.com/archives/linux-audit/2018-June/msg00114.html
- The audit_adjtime() function has been modified to only log those
fields that contain values that are actually used, resulting in more
compact records.
- The audit_adjtime() call has been moved to do_adjtimex() in
timekeeping.c
- Added an additional patch (for review) that simplifies the detection
if the syscall is read-only.
v1: https://www.redhat.com/archives/linux-audit/2018-June/msg00095.html
audit: Add functions to log time adjustments
timekeeping/ntp: Audit clock/NTP params adjustments
include/linux/audit.h | 21 ++++++++++++++++
include/uapi/linux/audit.h | 2 ++
kernel/auditsc.c | 15 ++++++++++++
kernel/time/ntp.c | 50 ++++++++++++++++++++++++++++++--------
kernel/time/timekeeping.c | 3 +++
5 files changed, 81 insertions(+), 10 deletions(-)
--
2.17.1

[Miroslav Lichvar]

Post by Ondrej Mosnacek
@John or other timekeeping/NTP folks: We had a discussion on the audit
ML on which of the internal timekeeping/NTP variables we should actually
log changes for. We are only interested in variables that can (directly
or indirectly) cause noticeable changes to the system clock, but since we
have only limited understanding of the NTP code, we would like to ask
you for advice on which variables are security relevant.

I guess that mostly depends on whether you consider setting the clock
to run faster or slower than real time to be an important event for
the audit.

Post by Ondrej Mosnacek
- time_offset (probably important)

This can adjust the clock by up to 0.5 seconds per call and also speed
it up or slow down by up to about 0.05% (43 seconds per day).

Post by Ondrej Mosnacek
- time_freq (maybe not important?)

This can speed up or slow down by up to about 0.05%.

Post by Ondrej Mosnacek
- time_status (likely important, can cause leap second injection)

Yes, it can insert/delete leap seconds and it also enables/disables
synchronization of the hardware real-time clock.

Post by Ondrej Mosnacek
- time_maxerror (maybe not important?)
- time_esterror (maybe not important?)

These two change the error estimates that are reported to applications
using ntp_gettime()/adjtimex(). If an application was periodically
checking that the clock is synchronized with some specified accuracy
and setting the maxerror to a larger value would cause the application
to abort, would it be an important event in the audit?

Post by Ondrej Mosnacek
- time_constant (???)

This controls the speed of the clock adjustments that are made when
time_offset is set. Probably not important for the audit.

Post by Ondrej Mosnacek
- time_adjust (sounds important)

This is similar to time_freq. It can temporarily speed up or slow down
the clock by up to 0.05%.

Post by Ondrej Mosnacek
- tick_usec (???)

This is a more extreme version of time_freq. It can speed up or slow
down the clock by up to 10%.

[Paul Moore]

Post by Miroslav Lichvar

Post by Ondrej Mosnacek
@John or other timekeeping/NTP folks: We had a discussion on the audit
ML on which of the internal timekeeping/NTP variables we should actually
log changes for. We are only interested in variables that can (directly
or indirectly) cause noticeable changes to the system clock, but since we
have only limited understanding of the NTP code, we would like to ask
you for advice on which variables are security relevant.

I guess that mostly depends on whether you consider setting the clock
to run faster or slower than real time to be an important event for
the audit.

Post by Ondrej Mosnacek
- time_offset (probably important)

This can adjust the clock by up to 0.5 seconds per call and also speed
it up or slow down by up to about 0.05% (43 seconds per day).

This seems worthwhile.

Post by Miroslav Lichvar

Post by Ondrej Mosnacek
- time_freq (maybe not important?)

This can speed up or slow down by up to about 0.05%.

This too.

Post by Miroslav Lichvar

Post by Ondrej Mosnacek
- time_status (likely important, can cause leap second injection)

Yes, it can insert/delete leap seconds and it also enables/disables
synchronization of the hardware real-time clock.

This one as well.

Post by Miroslav Lichvar

Post by Ondrej Mosnacek
- time_maxerror (maybe not important?)
- time_esterror (maybe not important?)

These two change the error estimates that are reported to applications
using ntp_gettime()/adjtimex(). If an application was periodically
checking that the clock is synchronized with some specified accuracy
and setting the maxerror to a larger value would cause the application
to abort, would it be an important event in the audit?

Since these don't really affect the time, just the expected error, I'm
not sure this is important.

Post by Miroslav Lichvar

Post by Ondrej Mosnacek
- time_constant (???)

This controls the speed of the clock adjustments that are made when
time_offset is set. Probably not important for the audit.

Agreed. I think we can skip this.

Post by Miroslav Lichvar

Post by Ondrej Mosnacek
- time_adjust (sounds important)

This is similar to time_freq. It can temporarily speed up or slow down
the clock by up to 0.05%.

Like time_freq, we should probably log this too.

Post by Miroslav Lichvar

Post by Ondrej Mosnacek
- tick_usec (???)

This is a more extreme version of time_freq. It can speed up or slow
down the clock by up to 10%.

Let's audit this one too.

[Ondrej Mosnacek]

Post by Paul Moore

Post by Miroslav Lichvar

Post by Ondrej Mosnacek
@John or other timekeeping/NTP folks: We had a discussion on the audit
ML on which of the internal timekeeping/NTP variables we should actually
log changes for. We are only interested in variables that can (directly
or indirectly) cause noticeable changes to the system clock, but since we
have only limited understanding of the NTP code, we would like to ask
you for advice on which variables are security relevant.

I guess that mostly depends on whether you consider setting the clock
to run faster or slower than real time to be an important event for
the audit.

Post by Ondrej Mosnacek
- time_offset (probably important)

This can adjust the clock by up to 0.5 seconds per call and also speed
it up or slow down by up to about 0.05% (43 seconds per day).

This seems worthwhile.

Post by Miroslav Lichvar

Post by Ondrej Mosnacek
- time_freq (maybe not important?)

This can speed up or slow down by up to about 0.05%.

This too.

Post by Miroslav Lichvar

Post by Ondrej Mosnacek
- time_status (likely important, can cause leap second injection)

Yes, it can insert/delete leap seconds and it also enables/disables
synchronization of the hardware real-time clock.

This one as well.

Post by Miroslav Lichvar

Post by Ondrej Mosnacek
- time_maxerror (maybe not important?)
- time_esterror (maybe not important?)

These two change the error estimates that are reported to applications
using ntp_gettime()/adjtimex(). If an application was periodically
checking that the clock is synchronized with some specified accuracy
and setting the maxerror to a larger value would cause the application
to abort, would it be an important event in the audit?

Since these don't really affect the time, just the expected error, I'm
not sure this is important.

Post by Miroslav Lichvar

Post by Ondrej Mosnacek
- time_constant (???)

This controls the speed of the clock adjustments that are made when
time_offset is set. Probably not important for the audit.

Agreed. I think we can skip this.

Post by Miroslav Lichvar

Post by Ondrej Mosnacek
- time_adjust (sounds important)

This is similar to time_freq. It can temporarily speed up or slow down
the clock by up to 0.05%.

Like time_freq, we should probably log this too.

Post by Miroslav Lichvar

Post by Ondrej Mosnacek
- tick_usec (???)

This is a more extreme version of time_freq. It can speed up or slow
down the clock by up to 10%.

Let's audit this one too.

I agree with Paul on all counts. I will go ahead and prepare a
patchset that logs everything except maxerror, esterror, and constant.

Thank you, Miroslav, for the explanations!

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

[Paul Moore]

Post by Ondrej Mosnacek

Post by Paul Moore

Post by Miroslav Lichvar

Post by Ondrej Mosnacek
@John or other timekeeping/NTP folks: We had a discussion on the audit
ML on which of the internal timekeeping/NTP variables we should actually
log changes for. We are only interested in variables that can (directly
or indirectly) cause noticeable changes to the system clock, but since we
have only limited understanding of the NTP code, we would like to ask
you for advice on which variables are security relevant.

I guess that mostly depends on whether you consider setting the clock
to run faster or slower than real time to be an important event for
the audit.

Post by Ondrej Mosnacek
- time_offset (probably important)

This can adjust the clock by up to 0.5 seconds per call and also speed
it up or slow down by up to about 0.05% (43 seconds per day).

This seems worthwhile.

Post by Miroslav Lichvar

Post by Ondrej Mosnacek
- time_freq (maybe not important?)

This can speed up or slow down by up to about 0.05%.

This too.

Post by Miroslav Lichvar

Post by Ondrej Mosnacek
- time_status (likely important, can cause leap second injection)

Yes, it can insert/delete leap seconds and it also enables/disables
synchronization of the hardware real-time clock.

This one as well.

Post by Miroslav Lichvar

Post by Ondrej Mosnacek
- time_maxerror (maybe not important?)
- time_esterror (maybe not important?)

These two change the error estimates that are reported to applications
using ntp_gettime()/adjtimex(). If an application was periodically
checking that the clock is synchronized with some specified accuracy
and setting the maxerror to a larger value would cause the application
to abort, would it be an important event in the audit?

Since these don't really affect the time, just the expected error, I'm
not sure this is important.

Post by Miroslav Lichvar

Post by Ondrej Mosnacek
- time_constant (???)

This controls the speed of the clock adjustments that are made when
time_offset is set. Probably not important for the audit.

Agreed. I think we can skip this.

Post by Miroslav Lichvar

Post by Ondrej Mosnacek
- time_adjust (sounds important)

This is similar to time_freq. It can temporarily speed up or slow down
the clock by up to 0.05%.

Like time_freq, we should probably log this too.

Post by Miroslav Lichvar

Post by Ondrej Mosnacek
- tick_usec (???)

This is a more extreme version of time_freq. It can speed up or slow
down the clock by up to 10%.

Let's audit this one too.

I agree with Paul on all counts. I will go ahead and prepare a
patchset that logs everything except maxerror, esterror, and constant.

Please make sure you include explanations similar to the above in the
patch descriptions; not just the cover letter, the goal is to have
this information captured in the git log.

Post by Ondrej Mosnacek
Thank you, Miroslav, for the explanations!

Yes, those explanations were helpful.

[Steve Grubb]

Post by Paul Moore

Post by Miroslav Lichvar

Post by Ondrej Mosnacek
@John or other timekeeping/NTP folks: We had a discussion on the audit
ML on which of the internal timekeeping/NTP variables we should actually
log changes for. We are only interested in variables that can (directly
or indirectly) cause noticeable changes to the system clock, but since we
have only limited understanding of the NTP code, we would like to ask
you for advice on which variables are security relevant.

I guess that mostly depends on whether you consider setting the clock
to run faster or slower than real time to be an important event for
the audit.

Post by Ondrej Mosnacek
- time_offset (probably important)

This can adjust the clock by up to 0.5 seconds per call and also speed
it up or slow down by up to about 0.05% (43 seconds per day).

This seems worthwhile.

Post by Miroslav Lichvar

Post by Ondrej Mosnacek
- time_freq (maybe not important?)

This can speed up or slow down by up to about 0.05%.

This too.

Post by Miroslav Lichvar

Post by Ondrej Mosnacek
- time_status (likely important, can cause leap second injection)

Yes, it can insert/delete leap seconds and it also enables/disables
synchronization of the hardware real-time clock.

This one as well.

Post by Miroslav Lichvar

Post by Ondrej Mosnacek
- time_maxerror (maybe not important?)
- time_esterror (maybe not important?)

These two change the error estimates that are reported to applications
using ntp_gettime()/adjtimex(). If an application was periodically
checking that the clock is synchronized with some specified accuracy
and setting the maxerror to a larger value would cause the application
to abort, would it be an important event in the audit?

Since these don't really affect the time, just the expected error, I'm
not sure this is important.

I don't think so.

-Steve

[Ondrej Mosnacek]

Post by Steve Grubb

Post by Paul Moore

Post by Miroslav Lichvar

Post by Ondrej Mosnacek
@John or other timekeeping/NTP folks: We had a discussion on the audit
ML on which of the internal timekeeping/NTP variables we should actually
log changes for. We are only interested in variables that can (directly
or indirectly) cause noticeable changes to the system clock, but since we
have only limited understanding of the NTP code, we would like to ask
you for advice on which variables are security relevant.

I guess that mostly depends on whether you consider setting the clock
to run faster or slower than real time to be an important event for
the audit.

Post by Ondrej Mosnacek
- time_offset (probably important)

This can adjust the clock by up to 0.5 seconds per call and also speed
it up or slow down by up to about 0.05% (43 seconds per day).

This seems worthwhile.

Post by Miroslav Lichvar

Post by Ondrej Mosnacek
- time_freq (maybe not important?)

This can speed up or slow down by up to about 0.05%.

This too.

Post by Miroslav Lichvar

Post by Ondrej Mosnacek
- time_status (likely important, can cause leap second injection)

Yes, it can insert/delete leap seconds and it also enables/disables
synchronization of the hardware real-time clock.

This one as well.

Post by Miroslav Lichvar

Post by Ondrej Mosnacek
- time_maxerror (maybe not important?)
- time_esterror (maybe not important?)

These two change the error estimates that are reported to applications
using ntp_gettime()/adjtimex(). If an application was periodically
checking that the clock is synchronized with some specified accuracy
and setting the maxerror to a larger value would cause the application
to abort, would it be an important event in the audit?

Since these don't really affect the time, just the expected error, I'm
not sure this is important.

I don't think so.

Sorry, just to make sure I understand it right - do you (also) not
think it is important or do you not think it is not important? :)

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

[Steve Grubb]

Post by Ondrej Mosnacek

Post by Steve Grubb

Post by Paul Moore

Post by Miroslav Lichvar

Post by Ondrej Mosnacek
@John or other timekeeping/NTP folks: We had a discussion on the audit
ML on which of the internal timekeeping/NTP variables we should
actually
log changes for. We are only interested in variables that can (directly
or indirectly) cause noticeable changes to the system clock, but
since we
have only limited understanding of the NTP code, we would like to ask
you for advice on which variables are security relevant.

I guess that mostly depends on whether you consider setting the clock
to run faster or slower than real time to be an important event for
the audit.

Post by Ondrej Mosnacek
- time_offset (probably important)

This can adjust the clock by up to 0.5 seconds per call and also speed
it up or slow down by up to about 0.05% (43 seconds per day).

This seems worthwhile.

Post by Miroslav Lichvar

Post by Ondrej Mosnacek
- time_freq (maybe not important?)

This can speed up or slow down by up to about 0.05%.

This too.

Post by Miroslav Lichvar

Post by Ondrej Mosnacek
- time_status (likely important, can cause leap second injection)

Yes, it can insert/delete leap seconds and it also enables/disables
synchronization of the hardware real-time clock.

This one as well.

Post by Miroslav Lichvar

Post by Ondrej Mosnacek
- time_maxerror (maybe not important?)
- time_esterror (maybe not important?)

These two change the error estimates that are reported to applications
using ntp_gettime()/adjtimex(). If an application was periodically
checking that the clock is synchronized with some specified accuracy
and setting the maxerror to a larger value would cause the
application to abort, would it be an important event in the audit?

Since these don't really affect the time, just the expected error, I'm
not sure this is important.

I don't think so.

Sorry, just to make sure I understand it right - do you (also) not
think it is important or do you not think it is not important? :)

I do not think its important to record the errors since the exit code tells
us there's a problem. IOW, I'm agreeing with Paul.

-Steve