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

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

[Ondrej Mosnacek]

Hi,

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

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

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

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

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

Thanks a lot!


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

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

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

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

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

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

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

-- 
2.17.1

[RFC PATCH ghak10 v4 1/2] audit: Add functions to log time adjustments

[Ondrej Mosnacek]

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

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

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

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

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 9334fbef7bae..0d084d4b4042 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -26,6 +26,7 @@
 #include <linux/sched.h>
 #include <linux/ptrace.h>
 #include <uapi/linux/audit.h>
+#include <uapi/linux/timex.h>
 
 #define AUDIT_INO_UNSET ((unsigned long)-1)
 #define AUDIT_DEV_UNSET ((dev_t)-1)
@@ -356,6 +357,8 @@ extern void __audit_log_capset(const struct cred *new, const struct cred *old);
 extern void __audit_mmap_fd(int fd, int flags);
 extern void __audit_log_kern_module(char *name);
 extern void __audit_fanotify(unsigned int response);
+extern void __audit_tk_injoffset(struct timespec64 offset);
+extern void __audit_ntp_adjust(const char *type, s64 oldval, s64 newval);
 
 static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
 {
@@ -458,6 +461,18 @@ static inline void audit_fanotify(unsigned int response)
 		__audit_fanotify(response);
 }
 
+static inline void audit_tk_injoffset(struct timespec64 offset)
+{
+	if (!audit_dummy_context())
+		__audit_tk_injoffset(offset);
+}
+
+static inline void audit_ntp_adjust(const char *type, s64 oldval, s64 newval)
+{
+	if (!audit_dummy_context())
+		__audit_ntp_adjust(type, oldval, newval);
+}
+
 extern int audit_n_rules;
 extern int audit_signals;
 #else /* CONFIG_AUDITSYSCALL */
@@ -584,6 +599,12 @@ static inline void audit_log_kern_module(char *name)
 static inline void audit_fanotify(unsigned int response)
 { }
 
+static inline void audit_tk_injoffset(struct timespec64 offset)
+{ }
+
+static inline void audit_ntp_adjust(const char *type, s64 oldval, s64 newval)
+{ }
+
 static inline void audit_ptrace(struct task_struct *t)
 { }
 #define audit_n_rules 0
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 4e3eaba84175..242ce562b41a 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -114,6 +114,8 @@
 #define AUDIT_REPLACE		1329	/* Replace auditd if this packet unanswerd */
 #define AUDIT_KERN_MODULE	1330	/* Kernel Module events */
 #define AUDIT_FANOTIFY		1331	/* Fanotify access decision */
+#define AUDIT_TIME_INJOFFSET	1332	/* Timekeeping offset injected */
+#define AUDIT_TIME_ADJNTPVAL	1333	/* NTP value adjustment */
 
 #define AUDIT_AVC		1400	/* SE Linux avc denial or grant */
 #define AUDIT_SELINUX_ERR	1401	/* Internal SE Linux Errors */
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index fb207466e99b..d355d32d9765 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2422,6 +2422,21 @@ void __audit_fanotify(unsigned int response)
 		AUDIT_FANOTIFY,	"resp=%u", response);
 }
 
+/* We need to allocate with GFP_ATOMIC here, since these two functions will be
+ * called while holding the timekeeping lock: */
+void __audit_tk_injoffset(struct timespec64 offset)
+{
+	audit_log(audit_context(), GFP_ATOMIC, AUDIT_TIME_INJOFFSET,
+		  "sec=%lli nsec=%li", (long long)offset.tv_sec, offset.tv_nsec);
+}
+
+void __audit_ntp_adjust(const char *type, s64 oldval, s64 newval)
+{
+	audit_log(audit_context(), GFP_ATOMIC, AUDIT_TIME_ADJNTPVAL,
+		  "op=%s old=%lli new=%lli", type,
+		  (long long)oldval, (long long)newval);
+}
+
 static void audit_log_task(struct audit_buffer *ab)
 {
 	kuid_t auid, uid;
-- 
2.17.1

[RFC PATCH ghak10 v4 2/2] timekeeping/ntp: Audit clock/NTP params adjustments

[Ondrej Mosnacek]

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

For reference, running the following commands:

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

produces audit records like this:

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

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

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

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

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

Signed-off-by: Ondrej Mosnacek <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 v4 0/2] audit: Log modifying adjtimex(2) calls

[Thomas Gleixner]

On Mon, 20 Aug 2018, Ondrej Mosnacek wrote:

+ Miroslav Lichvar

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

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

[Miroslav Lichvar]

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

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

> >   - NTP value adjustments:
> >     - time_offset (probably important)

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

> >     - time_freq (maybe not important?)

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

> >     - time_status (likely important, can cause leap second injection)

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

> >     - time_maxerror (maybe not important?)
> >     - time_esterror (maybe not important?)

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

> >     - time_constant (???)

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

> >     - time_adjust (sounds important)

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

> >     - tick_usec (???)

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

-- 
Miroslav Lichvar

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

[Paul Moore]

On Tue, Aug 21, 2018 at 3:21 AM Miroslav Lichvar <mlichvar@redhat.com> wrote:
> > On Mon, 20 Aug 2018, Ondrej Mosnacek wrote:
> > > @John or other timekeeping/NTP folks: We had a discussion on the audit
> > > ML on which of the internal timekeeping/NTP variables we should actually
> > > log changes for. We are only interested in variables that can (directly
> > > or indirectly) cause noticeable changes to the system clock, but since we
> > > have only limited understanding of the NTP code, we would like to ask
> > > you for advice on which variables are security relevant.
>
> I guess that mostly depends on whether you consider setting the clock
> to run faster or slower than real time to be an important event for
> the audit.
>
> > >   - NTP value adjustments:
> > >     - time_offset (probably important)
>
> This can adjust the clock by up to 0.5 seconds per call and also speed
> it up or slow down by up to about 0.05% (43 seconds per day).

This seems worthwhile.

> > >     - time_freq (maybe not important?)
>
> This can speed up or slow down by up to about 0.05%.

This too.

> > >     - time_status (likely important, can cause leap second injection)
>
> Yes, it can insert/delete leap seconds and it also enables/disables
> synchronization of the hardware real-time clock.

This one as well.

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

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

> > >     - time_constant (???)
>
> This controls the speed of the clock adjustments that are made when
> time_offset is set. Probably not important for the audit.

Agreed.  I think we can skip this.

> > >     - time_adjust (sounds important)
>
> This is similar to time_freq. It can temporarily speed up or slow down
> the clock by up to 0.05%.

Like time_freq, we should probably log this too.

> > >     - tick_usec (???)
>
> This is a more extreme version of time_freq. It can speed up or slow
> down the clock by up to 10%.

Let's audit this one too.

-- 
paul moore
www.paul-moore.com

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

[Ondrej Mosnacek]

On Wed, Aug 22, 2018 at 11:27 PM Paul Moore <paul@paul-moore.com> wrote:
> On Tue, Aug 21, 2018 at 3:21 AM Miroslav Lichvar <mlichvar@redhat.com> wrote:
> > > On Mon, 20 Aug 2018, Ondrej Mosnacek wrote:
> > > > @John or other timekeeping/NTP folks: We had a discussion on the audit
> > > > ML on which of the internal timekeeping/NTP variables we should actually
> > > > log changes for. We are only interested in variables that can (directly
> > > > or indirectly) cause noticeable changes to the system clock, but since we
> > > > have only limited understanding of the NTP code, we would like to ask
> > > > you for advice on which variables are security relevant.
> >
> > I guess that mostly depends on whether you consider setting the clock
> > to run faster or slower than real time to be an important event for
> > the audit.
> >
> > > >   - NTP value adjustments:
> > > >     - time_offset (probably important)
> >
> > This can adjust the clock by up to 0.5 seconds per call and also speed
> > it up or slow down by up to about 0.05% (43 seconds per day).
>
> This seems worthwhile.
>
> > > >     - time_freq (maybe not important?)
> >
> > This can speed up or slow down by up to about 0.05%.
>
> This too.
>
> > > >     - time_status (likely important, can cause leap second injection)
> >
> > Yes, it can insert/delete leap seconds and it also enables/disables
> > synchronization of the hardware real-time clock.
>
> This one as well.
>
> > > >     - time_maxerror (maybe not important?)
> > > >     - time_esterror (maybe not important?)
> >
> > These two change the error estimates that are reported to applications
> > using ntp_gettime()/adjtimex(). If an application was periodically
> > checking that the clock is synchronized with some specified accuracy
> > and setting the maxerror to a larger value would cause the application
> > to abort, would it be an important event in the audit?
>
> Since these don't really affect the time, just the expected error, I'm
> not sure this is important.
>
> > > >     - time_constant (???)
> >
> > This controls the speed of the clock adjustments that are made when
> > time_offset is set. Probably not important for the audit.
>
> Agreed.  I think we can skip this.
>
> > > >     - time_adjust (sounds important)
> >
> > This is similar to time_freq. It can temporarily speed up or slow down
> > the clock by up to 0.05%.
>
> Like time_freq, we should probably log this too.
>
> > > >     - tick_usec (???)
> >
> > This is a more extreme version of time_freq. It can speed up or slow
> > down the clock by up to 10%.
>
> Let's audit this one too.

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

Thank you, Miroslav, for the explanations!

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

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

[Paul Moore]

On Thu, Aug 23, 2018 at 5:14 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Wed, Aug 22, 2018 at 11:27 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Tue, Aug 21, 2018 at 3:21 AM Miroslav Lichvar <mlichvar@redhat.com> wrote:
> > > > On Mon, 20 Aug 2018, Ondrej Mosnacek wrote:
> > > > > @John or other timekeeping/NTP folks: We had a discussion on the audit
> > > > > ML on which of the internal timekeeping/NTP variables we should actually
> > > > > log changes for. We are only interested in variables that can (directly
> > > > > or indirectly) cause noticeable changes to the system clock, but since we
> > > > > have only limited understanding of the NTP code, we would like to ask
> > > > > you for advice on which variables are security relevant.
> > >
> > > I guess that mostly depends on whether you consider setting the clock
> > > to run faster or slower than real time to be an important event for
> > > the audit.
> > >
> > > > >   - NTP value adjustments:
> > > > >     - time_offset (probably important)
> > >
> > > This can adjust the clock by up to 0.5 seconds per call and also speed
> > > it up or slow down by up to about 0.05% (43 seconds per day).
> >
> > This seems worthwhile.
> >
> > > > >     - time_freq (maybe not important?)
> > >
> > > This can speed up or slow down by up to about 0.05%.
> >
> > This too.
> >
> > > > >     - time_status (likely important, can cause leap second injection)
> > >
> > > Yes, it can insert/delete leap seconds and it also enables/disables
> > > synchronization of the hardware real-time clock.
> >
> > This one as well.
> >
> > > > >     - time_maxerror (maybe not important?)
> > > > >     - time_esterror (maybe not important?)
> > >
> > > These two change the error estimates that are reported to applications
> > > using ntp_gettime()/adjtimex(). If an application was periodically
> > > checking that the clock is synchronized with some specified accuracy
> > > and setting the maxerror to a larger value would cause the application
> > > to abort, would it be an important event in the audit?
> >
> > Since these don't really affect the time, just the expected error, I'm
> > not sure this is important.
> >
> > > > >     - time_constant (???)
> > >
> > > This controls the speed of the clock adjustments that are made when
> > > time_offset is set. Probably not important for the audit.
> >
> > Agreed.  I think we can skip this.
> >
> > > > >     - time_adjust (sounds important)
> > >
> > > This is similar to time_freq. It can temporarily speed up or slow down
> > > the clock by up to 0.05%.
> >
> > Like time_freq, we should probably log this too.
> >
> > > > >     - tick_usec (???)
> > >
> > > This is a more extreme version of time_freq. It can speed up or slow
> > > down the clock by up to 10%.
> >
> > Let's audit this one too.
>
> I agree with Paul on all counts. I will go ahead and prepare a
> patchset that logs everything except maxerror, esterror, and constant.

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

> Thank you, Miroslav, for the explanations!

Yes, those explanations were helpful.

-- 
paul moore
www.paul-moore.com

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

[Steve Grubb]

On Wednesday, August 22, 2018 5:27:17 PM EDT Paul Moore wrote:
> On Tue, Aug 21, 2018 at 3:21 AM Miroslav Lichvar <mlichvar@redhat.com> 
wrote:
> > > On Mon, 20 Aug 2018, Ondrej Mosnacek wrote:
> > > > @John or other timekeeping/NTP folks: We had a discussion on the
> > > > audit
> > > > ML on which of the internal timekeeping/NTP variables we should
> > > > actually
> > > > log changes for. We are only interested in variables that can
> > > > (directly
> > > > or indirectly) cause noticeable changes to the system clock, but
> > > > since we
> > > > have only limited understanding of the NTP code, we would like to ask
> > > > you for advice on which variables are security relevant.
> > 
> > I guess that mostly depends on whether you consider setting the clock
> > to run faster or slower than real time to be an important event for
> > the audit.
> > 
> > > > - NTP value adjustments:
> > > > - time_offset (probably important)
> > 
> > This can adjust the clock by up to 0.5 seconds per call and also speed
> > it up or slow down by up to about 0.05% (43 seconds per day).
> 
> This seems worthwhile.
> 
> > > > - time_freq (maybe not important?)
> > 
> > This can speed up or slow down by up to about 0.05%.
> 
> This too.
> 
> > > > - time_status (likely important, can cause leap second injection)
> > 
> > Yes, it can insert/delete leap seconds and it also enables/disables
> > synchronization of the hardware real-time clock.
> 
> This one as well.
> 
> > > > - time_maxerror (maybe not important?)
> > > > - time_esterror (maybe not important?)
> > 
> > These two change the error estimates that are reported to applications
> > using ntp_gettime()/adjtimex(). If an application was periodically
> > checking that the clock is synchronized with some specified accuracy
> > and setting the maxerror to a larger value would cause the application
> > to abort, would it be an important event in the audit?
> 
> Since these don't really affect the time, just the expected error, I'm
> not sure this is important.

I don't think so.

-Steve

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

[Ondrej Mosnacek]

On Fri, Aug 24, 2018 at 4:56 PM Steve Grubb <sgrubb@redhat.com> wrote:
> On Wednesday, August 22, 2018 5:27:17 PM EDT Paul Moore wrote:
> > On Tue, Aug 21, 2018 at 3:21 AM Miroslav Lichvar <mlichvar@redhat.com>
> wrote:
> > > > On Mon, 20 Aug 2018, Ondrej Mosnacek wrote:
> > > > > @John or other timekeeping/NTP folks: We had a discussion on the
> > > > > audit
> > > > > ML on which of the internal timekeeping/NTP variables we should
> > > > > actually
> > > > > log changes for. We are only interested in variables that can
> > > > > (directly
> > > > > or indirectly) cause noticeable changes to the system clock, but
> > > > > since we
> > > > > have only limited understanding of the NTP code, we would like to ask
> > > > > you for advice on which variables are security relevant.
> > >
> > > I guess that mostly depends on whether you consider setting the clock
> > > to run faster or slower than real time to be an important event for
> > > the audit.
> > >
> > > > > - NTP value adjustments:
> > > > > - time_offset (probably important)
> > >
> > > This can adjust the clock by up to 0.5 seconds per call and also speed
> > > it up or slow down by up to about 0.05% (43 seconds per day).
> >
> > This seems worthwhile.
> >
> > > > > - time_freq (maybe not important?)
> > >
> > > This can speed up or slow down by up to about 0.05%.
> >
> > This too.
> >
> > > > > - time_status (likely important, can cause leap second injection)
> > >
> > > Yes, it can insert/delete leap seconds and it also enables/disables
> > > synchronization of the hardware real-time clock.
> >
> > This one as well.
> >
> > > > > - time_maxerror (maybe not important?)
> > > > > - time_esterror (maybe not important?)
> > >
> > > These two change the error estimates that are reported to applications
> > > using ntp_gettime()/adjtimex(). If an application was periodically
> > > checking that the clock is synchronized with some specified accuracy
> > > and setting the maxerror to a larger value would cause the application
> > > to abort, would it be an important event in the audit?
> >
> > Since these don't really affect the time, just the expected error, I'm
> > not sure this is important.
>
> I don't think so.

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

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

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

[Steve Grubb]

On Thursday, September 13, 2018 9:58:32 AM EDT Ondrej Mosnacek wrote:
> On Fri, Aug 24, 2018 at 4:56 PM Steve Grubb <sgrubb@redhat.com> wrote:
> > On Wednesday, August 22, 2018 5:27:17 PM EDT Paul Moore wrote:
> > > On Tue, Aug 21, 2018 at 3:21 AM Miroslav Lichvar <mlichvar@redhat.com>
> > 
> > wrote:
> > > > > On Mon, 20 Aug 2018, Ondrej Mosnacek wrote:
> > > > > > @John or other timekeeping/NTP folks: We had a discussion on the
> > > > > > audit
> > > > > > ML on which of the internal timekeeping/NTP variables we should
> > > > > > actually
> > > > > > log changes for. We are only interested in variables that can
> > > > > > (directly
> > > > > > or indirectly) cause noticeable changes to the system clock, but
> > > > > > since we
> > > > > > have only limited understanding of the NTP code, we would like to
> > > > > > ask
> > > > > > you for advice on which variables are security relevant.
> > > > 
> > > > I guess that mostly depends on whether you consider setting the clock
> > > > to run faster or slower than real time to be an important event for
> > > > the audit.
> > > > 
> > > > > > - NTP value adjustments:
> > > > > > - time_offset (probably important)
> > > > 
> > > > This can adjust the clock by up to 0.5 seconds per call and also
> > > > speed
> > > > it up or slow down by up to about 0.05% (43 seconds per day).
> > > 
> > > This seems worthwhile.
> > > 
> > > > > > - time_freq (maybe not important?)
> > > > 
> > > > This can speed up or slow down by up to about 0.05%.
> > > 
> > > This too.
> > > 
> > > > > > - time_status (likely important, can cause leap second injection)
> > > > 
> > > > Yes, it can insert/delete leap seconds and it also enables/disables
> > > > synchronization of the hardware real-time clock.
> > > 
> > > This one as well.
> > > 
> > > > > > - time_maxerror (maybe not important?)
> > > > > > - time_esterror (maybe not important?)
> > > > 
> > > > These two change the error estimates that are reported to
> > > > applications
> > > > using ntp_gettime()/adjtimex(). If an application was periodically
> > > > checking that the clock is synchronized with some specified accuracy
> > > > and setting the maxerror to a larger value would cause the
> > > > application to abort, would it be an important event in the audit?
> > > 
> > > Since these don't really affect the time, just the expected error, I'm
> > > not sure this is important.
> > 
> > I don't think so.
> 
> Sorry, just to make sure I understand it right - do you (also) not
> think it is important or do you not think it is not important? :)

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

-Steve