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

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

[Ondrej Mosnacek]

I tried to implement separate records for each variable as suggested by
Richard and it turned out to be quite straightforward and results in
more compact and readable records (even though there is now a bit more
of them).

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)

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.

[RFC PATCH ghak10 v3 1/3] audit: Add AUDIT_TIME_* record types

[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 (if any).

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 include/uapi/linux/audit.h | 2 ++
 1 file changed, 2 insertions(+)

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 */
-- 
2.17.1

[RFC PATCH ghak10 v3 2/3] audit: Add functions to log time adjustments

[Ondrej Mosnacek]

This patch 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.

Syntax of records produced by these messages:
    AUDIT_TIME_INJOFFSET
        sec - the 'seconds' part of the offset
        nsec - the 'nanoseconds' part of the offset
    AUDIT_TIME_ADJNTPVAL
        type - 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 original value
        new - the new value

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 include/linux/audit.h | 21 +++++++++++++++++++++
 kernel/auditsc.c      | 15 +++++++++++++++
 2 files changed, 36 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/kernel/auditsc.c b/kernel/auditsc.c
index d762e0b8160e..078249e7444d 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,
+		  "type=%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;
-- 
2.17.1

[RFC PATCH ghak10 v3 3/3] timekeeping/ntp: Audit clock/NTP params adjustments

[Ondrej Mosnacek]

TODO ??split patch into two

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): type=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): type=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): type=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): type=status old=8256 new=8257
type=TIME_ADJNTPVAL msg=audit(1530616044.507:8): type=offset old=0 new=0
type=TIME_ADJNTPVAL msg=audit(1530616044.507:8): type=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): type=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): type=freq old=0 new=49180377088000
type=TIME_ADJNTPVAL msg=audit(1530616044.511:11): type=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): type=status old=64 new=64
type=TIME_ADJNTPVAL msg=audit(1530616044.521:12): type=maxerr old=16000000 new=16000000
type=TIME_ADJNTPVAL msg=audit(1530616044.521:12): type=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): type=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): type=freq old=49180377088000 new=49180377088000
type=TIME_ADJNTPVAL msg=audit(1530616033.783:14): type=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).

(Intentionally not sending to the timekeeping maintainers just yet,
let's settle on the record contents/format first.)

Signed-off-by: Ondrej Mosnacek <omosnace@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);
-- 
2.17.1

Re: [RFC PATCH ghak10 v3 3/3] timekeeping/ntp: Audit clock/NTP params adjustments

[Richard Guy Briggs]

On 2018-07-03 14:44, Ondrej Mosnacek wrote:
> TODO ??split patch into two
> 
> 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:

I like the format of the output below.  That is very clear and should be
much easier to drop certain types if they are determined to be
unimportant to log, or drop unchanged values, but that should be done in
consultation with the time folks.  Steve may have issue with
the field type "type", and I might suggest "op=" might be more
appropriate, but check with Steve.

I'd probably combine the header file definition of the record type with
the code that actually adds that record output.  I agree splitting the
patch along the lines of the two different record types.

Very nice!  (more below...)

> type=TIME_ADJNTPVAL msg=audit(1530616044.507:5): type=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): type=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): type=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): type=status old=8256 new=8257
> type=TIME_ADJNTPVAL msg=audit(1530616044.507:8): type=offset old=0 new=0
> type=TIME_ADJNTPVAL msg=audit(1530616044.507:8): type=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): type=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): type=freq old=0 new=49180377088000
> type=TIME_ADJNTPVAL msg=audit(1530616044.511:11): type=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): type=status old=64 new=64
> type=TIME_ADJNTPVAL msg=audit(1530616044.521:12): type=maxerr old=16000000 new=16000000
> type=TIME_ADJNTPVAL msg=audit(1530616044.521:12): type=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): type=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): type=freq old=49180377088000 new=49180377088000
> type=TIME_ADJNTPVAL msg=audit(1530616033.783:14): type=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).

Great.  This should be reflected in the syscall's "success=" value.

> (Intentionally not sending to the timekeeping maintainers just yet,
> let's settle on the record contents/format first.)
> 
> Signed-off-by: Ondrej Mosnacek <omosnace@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);
> -- 
> 2.17.1
> 
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

- RGB

--
Richard Guy Briggs <rgb@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

Re: [RFC PATCH ghak10 v3 3/3] timekeeping/ntp: Audit clock/NTP params adjustments

[Ondrej Mosnacek]

On Fri, Jul 13, 2018 at 9:24 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-07-03 14:44, Ondrej Mosnacek wrote:
> > TODO ??split patch into two
> >
> > 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:
>
> I like the format of the output below.  That is very clear and should be
> much easier to drop certain types if they are determined to be
> unimportant to log, or drop unchanged values, but that should be done in
> consultation with the time folks.  Steve may have issue with
> the field type "type", and I might suggest "op=" might be more
> appropriate, but check with Steve.

To me "op" sounds like it should name a verb, not a noun. But if we
want "op" for consistency with other record types and for easier
searching/filtering, I'm OK with that. Steve?

> I'd probably combine the header file definition of the record type with
> the code that actually adds that record output.  I agree splitting the
> patch along the lines of the two different record types.

OK, now that I think of it, I don't know why I chose to always
separate the record type definition... Indeed if I combine the two
patches I will make git blame-ing much easier. I will also split the
last patch in the final patchset.

>
> Very nice!  (more below...)
>
> > type=TIME_ADJNTPVAL msg=audit(1530616044.507:5): type=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): type=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): type=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): type=status old=8256 new=8257
> > type=TIME_ADJNTPVAL msg=audit(1530616044.507:8): type=offset old=0 new=0
> > type=TIME_ADJNTPVAL msg=audit(1530616044.507:8): type=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): type=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): type=freq old=0 new=49180377088000
> > type=TIME_ADJNTPVAL msg=audit(1530616044.511:11): type=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): type=status old=64 new=64
> > type=TIME_ADJNTPVAL msg=audit(1530616044.521:12): type=maxerr old=16000000 new=16000000
> > type=TIME_ADJNTPVAL msg=audit(1530616044.521:12): type=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): type=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): type=freq old=49180377088000 new=49180377088000
> > type=TIME_ADJNTPVAL msg=audit(1530616033.783:14): type=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).
>
> Great.  This should be reflected in the syscall's "success=" value.

I don't quite understand what you mean by this sentence. I should
clarify that I was talking about the case where the (modifying)
syscall was successful, but it happened to request a change to the
value that is already set (or to inject an offset of 0). IOW, it logs
also successful no-op changes. My reasoning behind that is that if you
have a policy that says, for example,  "Log every attempt to change
the owner of this file.", you would most likely just log all
modification attempts, even if the change is a no-op (i.e. an attempt
to change the file's owner is itself suspicious, even if less serious
than an actual modification).

>
> > (Intentionally not sending to the timekeeping maintainers just yet,
> > let's settle on the record contents/format first.)
> >
> > Signed-off-by: Ondrej Mosnacek <omosnace@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);
> > --
> > 2.17.1
> >
> > --
> > Linux-audit mailing list
> > Linux-audit@redhat.com
> > https://www.redhat.com/mailman/listinfo/linux-audit
>
> - RGB
>
> --
> Richard Guy Briggs <rgb@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.

Re: [RFC PATCH ghak10 v3 3/3] timekeeping/ntp: Audit clock/NTP params adjustments

[Richard Guy Briggs]

On 2018-07-16 10:15, Ondrej Mosnacek wrote:
> On Fri, Jul 13, 2018 at 9:24 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2018-07-03 14:44, Ondrej Mosnacek wrote:
> > > TODO ??split patch into two
> > >
> > > 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:
> >
> > I like the format of the output below.  That is very clear and should be
> > much easier to drop certain types if they are determined to be
> > unimportant to log, or drop unchanged values, but that should be done in
> > consultation with the time folks.  Steve may have issue with
> > the field type "type", and I might suggest "op=" might be more
> > appropriate, but check with Steve.
> 
> To me "op" sounds like it should name a verb, not a noun. But if we
> want "op" for consistency with other record types and for easier
> searching/filtering, I'm OK with that. Steve?
> 
> > I'd probably combine the header file definition of the record type with
> > the code that actually adds that record output.  I agree splitting the
> > patch along the lines of the two different record types.
> 
> OK, now that I think of it, I don't know why I chose to always
> separate the record type definition... Indeed if I combine the two
> patches I will make git blame-ing much easier. I will also split the
> last patch in the final patchset.
> 
> >
> > Very nice!  (more below...)
> >
> > > type=TIME_ADJNTPVAL msg=audit(1530616044.507:5): type=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): type=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): type=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): type=status old=8256 new=8257
> > > type=TIME_ADJNTPVAL msg=audit(1530616044.507:8): type=offset old=0 new=0
> > > type=TIME_ADJNTPVAL msg=audit(1530616044.507:8): type=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): type=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): type=freq old=0 new=49180377088000
> > > type=TIME_ADJNTPVAL msg=audit(1530616044.511:11): type=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): type=status old=64 new=64
> > > type=TIME_ADJNTPVAL msg=audit(1530616044.521:12): type=maxerr old=16000000 new=16000000
> > > type=TIME_ADJNTPVAL msg=audit(1530616044.521:12): type=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): type=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): type=freq old=49180377088000 new=49180377088000
> > > type=TIME_ADJNTPVAL msg=audit(1530616033.783:14): type=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).
> >
> > Great.  This should be reflected in the syscall's "success=" value.
> 
> I don't quite understand what you mean by this sentence. I should
> clarify that I was talking about the case where the (modifying)
> syscall was successful, but it happened to request a change to the
> value that is already set (or to inject an offset of 0). IOW, it logs
> also successful no-op changes. My reasoning behind that is that if you
> have a policy that says, for example,  "Log every attempt to change
> the owner of this file.", you would most likely just log all
> modification attempts, even if the change is a no-op (i.e. an attempt
> to change the file's owner is itself suspicious, even if less serious
> than an actual modification).

I would err on the side of logging *all* attempts to *set* a value,
whether the new value is the same as the existing value or not, and
whether the syscall is successful or not.  So I think we are in violent
agreement.

> > > (Intentionally not sending to the timekeeping maintainers just yet,
> > > let's settle on the record contents/format first.)
> > >
> > > Signed-off-by: Ondrej Mosnacek <omosnace@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);
> >
> > - RGB
> 
> Ondrej Mosnacek <omosnace at redhat dot com>

- RGB

--
Richard Guy Briggs <rgb@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

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

[Paul Moore]

On Tue, Jul 3, 2018 at 8:44 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> I tried to implement separate records for each variable as suggested by
> Richard and it turned out to be quite straightforward and results in
> more compact and readable records (even though there is now a bit more
> of them).
>
> 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)
>
> 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.

Looking at these new records, and trying to guess a bit at the
original intent of the feature request, I think we may be going a bit
overboard with the information we are logging.  I'm thinking all we
really need to capture in the audit log is the system time both before
and after the change (for the sake of simplicity I suggest using a
data format similar to the audit record timestamp).

While I created the GH issue for this, I believe the original request
came from a Red Hat BZ that Steve created; Steve, what sort of
certification requirements (if any?) are there for logging system time
changes?

-- 
paul moore
www.paul-moore.com

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

[Steve Grubb]

On Wednesday, July 18, 2018 2:36:11 PM EDT Paul Moore wrote:
> > 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.
> 
> Looking at these new records, and trying to guess a bit at the
> original intent of the feature request, I think we may be going a bit
> overboard with the information we are logging.  I'm thinking all we
> really need to capture in the audit log is the system time both before
> and after the change (for the sake of simplicity I suggest using a
> data format similar to the audit record timestamp).
> 
> While I created the GH issue for this, I believe the original request
> came from a Red Hat BZ that Steve created; Steve, what sort of
> certification requirements (if any?) are there for logging system time
> changes?

That we record any attempts to change the system time. The problem is that 
adjtimex passes a data structure that is opaque to user space. So, we can't 
tell if someone is setting time, adjusting a tolerance, or simply retrieving 
status.

With stime, we can clearly see the time that was sent into the kernel and it 
unconditionally sets time. With settimeofday, it uses a data structure that 
we cannot see, but whatever the contents are we are definitely setting time. 
Same goes for clock_settime. Only in 1 case do we actually see what the time 
is. So, that is not really needed. So, I think what we need to know is did 
the syscall do anything that adjusted the system's notion of time? And that's 
all.

-Steve

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

[Paul Moore]

On Wed, Jul 18, 2018 at 3:36 PM Steve Grubb <sgrubb@redhat.com> wrote:
> On Wednesday, July 18, 2018 2:36:11 PM EDT Paul Moore wrote:
> > > 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.
> >
> > Looking at these new records, and trying to guess a bit at the
> > original intent of the feature request, I think we may be going a bit
> > overboard with the information we are logging.  I'm thinking all we
> > really need to capture in the audit log is the system time both before
> > and after the change (for the sake of simplicity I suggest using a
> > data format similar to the audit record timestamp).
> >
> > While I created the GH issue for this, I believe the original request
> > came from a Red Hat BZ that Steve created; Steve, what sort of
> > certification requirements (if any?) are there for logging system time
> > changes?
>
> That we record any attempts to change the system time. The problem is that
> adjtimex passes a data structure that is opaque to user space. So, we can't
> tell if someone is setting time, adjusting a tolerance, or simply retrieving
> status.
>
> With stime, we can clearly see the time that was sent into the kernel and it
> unconditionally sets time. With settimeofday, it uses a data structure that
> we cannot see, but whatever the contents are we are definitely setting time.
> Same goes for clock_settime. Only in 1 case do we actually see what the time
> is. So, that is not really needed. So, I think what we need to know is did
> the syscall do anything that adjusted the system's notion of time? And that's
> all.

So presumably my above suggestion of simply recording the system time
both before and after the change would be sufficient, yes?

-- 
paul moore
www.paul-moore.com

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

[Steve Grubb]

On Wednesday, July 18, 2018 3:59:24 PM EDT Paul Moore wrote:
> On Wed, Jul 18, 2018 at 3:36 PM Steve Grubb <sgrubb@redhat.com> wrote:
> > On Wednesday, July 18, 2018 2:36:11 PM EDT Paul Moore wrote:
> > > > 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.
> > > 
> > > Looking at these new records, and trying to guess a bit at the
> > > original intent of the feature request, I think we may be going a bit
> > > overboard with the information we are logging.  I'm thinking all we
> > > really need to capture in the audit log is the system time both before
> > > and after the change (for the sake of simplicity I suggest using a
> > > data format similar to the audit record timestamp).
> > > 
> > > While I created the GH issue for this, I believe the original request
> > > came from a Red Hat BZ that Steve created; Steve, what sort of
> > > certification requirements (if any?) are there for logging system time
> > > changes?
> > 
> > That we record any attempts to change the system time. The problem is
> > that
> > adjtimex passes a data structure that is opaque to user space. So, we
> > can't tell if someone is setting time, adjusting a tolerance, or simply
> > retrieving status.
> > 
> > With stime, we can clearly see the time that was sent into the kernel and
> > it unconditionally sets time. With settimeofday, it uses a data
> > structure that we cannot see, but whatever the contents are we are
> > definitely setting time. Same goes for clock_settime. Only in 1 case do
> > we actually see what the time is. So, that is not really needed. So, I
> > think what we need to know is did the syscall do anything that adjusted
> > the system's notion of time? And that's all.
> 
> So presumably my above suggestion of simply recording the system time
> both before and after the change would be sufficient, yes?

Well, I think that we need to make it obvious that a time setting has changed 
as in a y/n answer. Doing a comparison of the time to get a y/n answer is 
less obvious if you are doing a grep. Also, what time will the event have? I 
presume that the event will have the new time since we are on the exit 
filter. So, do we really need to collect that?

-Steve

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

[Paul Moore]

On Wed, Jul 18, 2018 at 6:34 PM Steve Grubb <sgrubb@redhat.com> wrote:
> On Wednesday, July 18, 2018 3:59:24 PM EDT Paul Moore wrote:
> > On Wed, Jul 18, 2018 at 3:36 PM Steve Grubb <sgrubb@redhat.com> wrote:
> > > On Wednesday, July 18, 2018 2:36:11 PM EDT Paul Moore wrote:
> > > > > 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.
> > > >
> > > > Looking at these new records, and trying to guess a bit at the
> > > > original intent of the feature request, I think we may be going a bit
> > > > overboard with the information we are logging.  I'm thinking all we
> > > > really need to capture in the audit log is the system time both before
> > > > and after the change (for the sake of simplicity I suggest using a
> > > > data format similar to the audit record timestamp).
> > > >
> > > > While I created the GH issue for this, I believe the original request
> > > > came from a Red Hat BZ that Steve created; Steve, what sort of
> > > > certification requirements (if any?) are there for logging system time
> > > > changes?
> > >
> > > That we record any attempts to change the system time. The problem is
> > > that
> > > adjtimex passes a data structure that is opaque to user space. So, we
> > > can't tell if someone is setting time, adjusting a tolerance, or simply
> > > retrieving status.
> > >
> > > With stime, we can clearly see the time that was sent into the kernel and
> > > it unconditionally sets time. With settimeofday, it uses a data
> > > structure that we cannot see, but whatever the contents are we are
> > > definitely setting time. Same goes for clock_settime. Only in 1 case do
> > > we actually see what the time is. So, that is not really needed. So, I
> > > think what we need to know is did the syscall do anything that adjusted
> > > the system's notion of time? And that's all.
> >
> > So presumably my above suggestion of simply recording the system time
> > both before and after the change would be sufficient, yes?
>
> Well, I think that we need to make it obvious that a time setting has changed
> as in a y/n answer. Doing a comparison of the time to get a y/n answer is
> less obvious if you are doing a grep.

Presumably we would only be emitting this new record when the system
time changes as the result of a time setting change so the simple
presence of this record should be sufficient to indicate a time
change.

> Also, what time will the event have? I
> presume that the event will have the new time since we are on the exit
> filter. So, do we really need to collect that?

The audit event timestamp is not guaranteed to be the same as the user
supplied value so we should explicitly log the old/new system time.

-- 
paul moore
www.paul-moore.com

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

[Ondrej Mosnacek]

On Wed, Jul 18, 2018 at 9:59 PM Paul Moore <paul@paul-moore.com> wrote:
> On Wed, Jul 18, 2018 at 3:36 PM Steve Grubb <sgrubb@redhat.com> wrote:
> > On Wednesday, July 18, 2018 2:36:11 PM EDT Paul Moore wrote:
> > > > 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.
> > >
> > > Looking at these new records, and trying to guess a bit at the
> > > original intent of the feature request, I think we may be going a bit
> > > overboard with the information we are logging.  I'm thinking all we
> > > really need to capture in the audit log is the system time both before
> > > and after the change (for the sake of simplicity I suggest using a
> > > data format similar to the audit record timestamp).
> > >
> > > While I created the GH issue for this, I believe the original request
> > > came from a Red Hat BZ that Steve created; Steve, what sort of
> > > certification requirements (if any?) are there for logging system time
> > > changes?
> >
> > That we record any attempts to change the system time. The problem is that
> > adjtimex passes a data structure that is opaque to user space. So, we can't
> > tell if someone is setting time, adjusting a tolerance, or simply retrieving
> > status.
> >
> > With stime, we can clearly see the time that was sent into the kernel and it
> > unconditionally sets time. With settimeofday, it uses a data structure that
> > we cannot see, but whatever the contents are we are definitely setting time.
> > Same goes for clock_settime. Only in 1 case do we actually see what the time
> > is. So, that is not really needed. So, I think what we need to know is did
> > the syscall do anything that adjusted the system's notion of time? And that's
> > all.
>
> So presumably my above suggestion of simply recording the system time
> both before and after the change would be sufficient, yes?

I wouldn't say this is the right approach here, for two reasons that
I'll try to explain below.

adjtimex(2) can be basically used for two major types of adjustment:
1. for (immediately) injecting offset into system time,
2. for changing NTP status variables.

(1.) is the more obvious and simple adjustment, which *directly*
alters the system time. For this adjustment the current proposed
solution simply logs the delta, by which the time is adjusted (it can
be retrieved easily from the supplied timex struct). In my opinion,
logging of this delta is more explicit than logging the timestamps
(which might not give the precise offset anyway, since time might flow
between the two timestamps) and is also more easily grepped in the
logs, as Steve pointed out in his reply. For example, you can quickly
filter out adjustments of less than +/-1 second by simply comparing
the offset in the records.

(2.) is a lot more tricky... These adjustments modify the variables of
the NTP algorithm, which is quite complex and it is not obvious how it
influences the system time. From my understanding, the algorithm is
used for adjustments that require smooth transition over time - such
as (or maybe this is the only use case) inserting a leap second. Note
that in this case, just logging timestamp before and after the syscall
is not sufficient, since the change doesn't happen immediately, but in
small increments that are gradually added *after* the syscall already
finished. I don't know if (or which of) these adjustments can be used
for a real attack, but at least the leap second insertion sounds like
something that could be used to shift the time without being easily
detected. This is why I am wary of throwing things out from the
logging until we understand what is truly important and what is not.

So, to sum up, my two arguments against logging before/after timestamps are:
1. For the case of direct offset injection we can much more easily log
the actual offset and this is also more informative.
2. For the case of NTP adjustments it is just not sufficient, we need
to log some or all of the variable adjustments and let the people
analyzing the logs figure out how that influenced the clock later (NTP
algorithm is just too complex).

I hope this explanation makes the situation a bit more clear. Time is
hard [1] and this particular time issue just refuses to be any
different :)

[1] http://sflanders.net/2015/02/09/time-hard-proper-ntp-configuration/

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

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

[Paul Moore]

On Thu, Jul 19, 2018 at 3:36 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Wed, Jul 18, 2018 at 9:59 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Wed, Jul 18, 2018 at 3:36 PM Steve Grubb <sgrubb@redhat.com> wrote:
> > > On Wednesday, July 18, 2018 2:36:11 PM EDT Paul Moore wrote:
> > > > > 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.
> > > >
> > > > Looking at these new records, and trying to guess a bit at the
> > > > original intent of the feature request, I think we may be going a bit
> > > > overboard with the information we are logging.  I'm thinking all we
> > > > really need to capture in the audit log is the system time both before
> > > > and after the change (for the sake of simplicity I suggest using a
> > > > data format similar to the audit record timestamp).
> > > >
> > > > While I created the GH issue for this, I believe the original request
> > > > came from a Red Hat BZ that Steve created; Steve, what sort of
> > > > certification requirements (if any?) are there for logging system time
> > > > changes?
> > >
> > > That we record any attempts to change the system time. The problem is that
> > > adjtimex passes a data structure that is opaque to user space. So, we can't
> > > tell if someone is setting time, adjusting a tolerance, or simply retrieving
> > > status.
> > >
> > > With stime, we can clearly see the time that was sent into the kernel and it
> > > unconditionally sets time. With settimeofday, it uses a data structure that
> > > we cannot see, but whatever the contents are we are definitely setting time.
> > > Same goes for clock_settime. Only in 1 case do we actually see what the time
> > > is. So, that is not really needed. So, I think what we need to know is did
> > > the syscall do anything that adjusted the system's notion of time? And that's
> > > all.
> >
> > So presumably my above suggestion of simply recording the system time
> > both before and after the change would be sufficient, yes?
>
> I wouldn't say this is the right approach here, for two reasons that
> I'll try to explain below.
>
> adjtimex(2) can be basically used for two major types of adjustment:
> 1. for (immediately) injecting offset into system time,
> 2. for changing NTP status variables.
>
> (1.) is the more obvious and simple adjustment, which *directly*
> alters the system time. For this adjustment the current proposed
> solution simply logs the delta, by which the time is adjusted (it can
> be retrieved easily from the supplied timex struct). In my opinion,
> logging of this delta is more explicit than logging the timestamps
> (which might not give the precise offset anyway, since time might flow
> between the two timestamps) and is also more easily grepped in the
> logs, as Steve pointed out in his reply. For example, you can quickly
> filter out adjustments of less than +/-1 second by simply comparing
> the offset in the records.

I remain unconvinced for this case, I would argue that the fact that
the new system time is more useful than the delta, but it might not
matter considering the second case below.

> (2.) is a lot more tricky... These adjustments modify the variables of
> the NTP algorithm, which is quite complex and it is not obvious how it
> influences the system time. From my understanding, the algorithm is
> used for adjustments that require smooth transition over time - such
> as (or maybe this is the only use case) inserting a leap second. Note
> that in this case, just logging timestamp before and after the syscall
> is not sufficient, since the change doesn't happen immediately, but in
> small increments that are gradually added *after* the syscall already
> finished. I don't know if (or which of) these adjustments can be used
> for a real attack, but at least the leap second insertion sounds like
> something that could be used to shift the time without being easily
> detected. This is why I am wary of throwing things out from the
> logging until we understand what is truly important and what is not.

Okay, it sounds like we need to log some of these variables, but which
ones are relevant is still unknown (all?).  I would suggest a bit more
research on this so that we can say with some certainty which fields
are important, please include this information in the commit
descriptions so we have it in the git log for future use.

-- 
paul moore
www.paul-moore.com