* [PATCH ghak10 v5 0/2] audit: Log modifying adjtimex(2) calls
@ 2018-08-24 11:59 Ondrej Mosnacek
2018-08-24 12:00 ` [PATCH ghak10 v5 1/2] audit: Add functions to log time adjustments Ondrej Mosnacek
2018-08-24 12:00 ` [PATCH ghak10 v5 2/2] timekeeping/ntp: Audit clock/NTP params adjustments Ondrej Mosnacek
0 siblings, 2 replies; 32+ messages in thread
From: Ondrej Mosnacek @ 2018-08-24 11:59 UTC (permalink / raw)
To: linux-audit
Cc: Paul Moore, Richard Guy Briggs, Steve Grubb, Miroslav Lichvar,
John Stultz, Thomas Gleixner, Stephen Boyd, linux-kernel,
Ondrej Mosnacek
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.
The current version of the patchset logs the following changes:
- direct injection of timekeeping offset
- adjustment of timekeeping's TAI offset
- NTP value adjustments:
- time_offset
- time_freq
- time_status
- time_adjust
- tick_usec
Changes to the following NTP values are not logged, as they are not
important for security:
- time_maxerror
- time_esterror
- time_constant
Audit kernel GitHub issue: https://github.com/linux-audit/audit-kernel/issues/10
Changes in v5:
- Dropped logging of some less important changes and update commit messages
- No longer mark the patchset as RFC
v4: https://www.redhat.com/archives/linux-audit/2018-August/msg00023.html
Changes in v4:
- Squashed first two patches into one
- Renamed 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 | 38 ++++++++++++++++++++++++++++++--------
kernel/time/timekeeping.c | 3 +++
5 files changed, 71 insertions(+), 8 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH ghak10 v5 1/2] audit: Add functions to log time adjustments
2018-08-24 11:59 [PATCH ghak10 v5 0/2] audit: Log modifying adjtimex(2) calls Ondrej Mosnacek
@ 2018-08-24 12:00 ` Ondrej Mosnacek
2018-08-24 18:33 ` John Stultz
` (2 more replies)
2018-08-24 12:00 ` [PATCH ghak10 v5 2/2] timekeeping/ntp: Audit clock/NTP params adjustments Ondrej Mosnacek
1 sibling, 3 replies; 32+ messages in thread
From: Ondrej Mosnacek @ 2018-08-24 12:00 UTC (permalink / raw)
To: linux-audit
Cc: Paul Moore, Richard Guy Briggs, Steve Grubb, Miroslav Lichvar,
John Stultz, Thomas Gleixner, Stephen Boyd, linux-kernel,
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
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
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH ghak10 v5 2/2] timekeeping/ntp: Audit clock/NTP params adjustments
2018-08-24 11:59 [PATCH ghak10 v5 0/2] audit: Log modifying adjtimex(2) calls Ondrej Mosnacek
2018-08-24 12:00 ` [PATCH ghak10 v5 1/2] audit: Add functions to log time adjustments Ondrej Mosnacek
@ 2018-08-24 12:00 ` Ondrej Mosnacek
2018-08-24 19:47 ` Richard Guy Briggs
1 sibling, 1 reply; 32+ messages in thread
From: Ondrej Mosnacek @ 2018-08-24 12:00 UTC (permalink / raw)
To: linux-audit
Cc: Paul Moore, Richard Guy Briggs, Steve Grubb, Miroslav Lichvar,
John Stultz, Thomas Gleixner, Stephen Boyd, linux-kernel,
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=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=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.)
The changes to the time_maxerror, time_esterror, and time_constant
variables are not logged, as these are not important for security.
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).
An overview of changes that can be done via adjtimex(2) (based on
information from Miroslav Lichvar) and whether they are audited:
timekeeping_inject_offset() -- injects offset directly into system
time (AUDITED)
__timekeeping_set_tai_offset() -- sets the offset from the
International Atomic Time
(AUDITED)
NTP variables:
time_offset -- 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) (AUDITED)
time_freq -- can speed up or slow down by up to about 0.05%
time_status -- can insert/delete leap seconds and it also enables/
disables synchronization of the hardware real-time
clock (AUDITED)
time_maxerror, time_esterror -- change error estimates used to
inform userspace applications
(NOT AUDITED)
time_constant -- controls the speed of the clock adjustments that
are made when time_offset is set (NOT AUDITED)
time_adjust -- can temporarily speed up or slow down the clock by up
to 0.05% (AUDITED)
tick_usec -- a more extreme version of time_freq; can speed up or
slow down the clock by up to 10% (AUDITED)
Cc: Miroslav Lichvar <mlichvar@redhat.com>
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
kernel/time/ntp.c | 38 ++++++++++++++++++++++++++++++--------
kernel/time/timekeeping.c | 3 +++
2 files changed, 33 insertions(+), 8 deletions(-)
diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index a09ded765f6c..f96c6d326aae 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,21 +675,31 @@ 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)
@@ -700,14 +716,18 @@ static inline void process_adjtimex_modes(struct timex *txc,
time_constant = max(time_constant, 0l);
}
- 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 +749,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
^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH ghak10 v5 1/2] audit: Add functions to log time adjustments
2018-08-24 12:00 ` [PATCH ghak10 v5 1/2] audit: Add functions to log time adjustments Ondrej Mosnacek
@ 2018-08-24 18:33 ` John Stultz
2018-08-27 8:28 ` Ondrej Mosnacek
2018-08-27 7:50 ` Miroslav Lichvar
2018-09-14 3:18 ` Paul Moore
2 siblings, 1 reply; 32+ messages in thread
From: John Stultz @ 2018-08-24 18:33 UTC (permalink / raw)
To: Ondrej Mosnacek
Cc: linux-audit, Paul Moore, Richard Guy Briggs, Steve Grubb,
Miroslav Lichvar, Thomas Gleixner, Stephen Boyd, lkml
On Fri, Aug 24, 2018 at 5:00 AM, Ondrej Mosnacek <omosnace@redhat.com> wrote:
> 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
> 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);
> +}
So it looks like you've been careful here, but just want to make sure,
nothing in the audit_log path calls anything that might possibly call
back into timekeeping code? We've had a number of issues over time
where debug calls out to other subsystems end up getting tweaked to
add some timestamping and we deadlock. :)
One approach we've done to make sure we don't create a trap for future
changes in other subsystems, is avoid calling into other subsystems
until later when we've dropped the locks, (see clock_was_set). Its a
little rough for some of the things done deep in the ntp code, but
might be an extra cautious approach to try.
thanks
-john
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH ghak10 v5 2/2] timekeeping/ntp: Audit clock/NTP params adjustments
2018-08-24 12:00 ` [PATCH ghak10 v5 2/2] timekeeping/ntp: Audit clock/NTP params adjustments Ondrej Mosnacek
@ 2018-08-24 19:47 ` Richard Guy Briggs
2018-08-24 20:20 ` John Stultz
2018-08-27 11:35 ` Ondrej Mosnacek
0 siblings, 2 replies; 32+ messages in thread
From: Richard Guy Briggs @ 2018-08-24 19:47 UTC (permalink / raw)
To: Ondrej Mosnacek
Cc: linux-audit, linux-kernel, Stephen Boyd, Miroslav Lichvar,
John Stultz, Thomas Gleixner
On 2018-08-24 14:00, Ondrej Mosnacek wrote:
> 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).
I thought I saw it suggested earlier in one of the replies to a previous
revision of the patchset to separate the two types of records with their
calling circumstances. The inj-offset bits could stand alone in their
own patch leaving all the rest in its own patch. The record numbers and
examples are easier to offer when given together, but they aren't as
clear they are indepnendent records and callers. That way, each patch
stands on its own. (more below)
> 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=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=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.)
>
> The changes to the time_maxerror, time_esterror, and time_constant
> variables are not logged, as these are not important for security.
>
> 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).
>
> An overview of changes that can be done via adjtimex(2) (based on
> information from Miroslav Lichvar) and whether they are audited:
> timekeeping_inject_offset() -- injects offset directly into system
> time (AUDITED)
> __timekeeping_set_tai_offset() -- sets the offset from the
> International Atomic Time
> (AUDITED)
> NTP variables:
> time_offset -- 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) (AUDITED)
> time_freq -- can speed up or slow down by up to about 0.05%
> time_status -- can insert/delete leap seconds and it also enables/
> disables synchronization of the hardware real-time
> clock (AUDITED)
> time_maxerror, time_esterror -- change error estimates used to
> inform userspace applications
> (NOT AUDITED)
> time_constant -- controls the speed of the clock adjustments that
> are made when time_offset is set (NOT AUDITED)
> time_adjust -- can temporarily speed up or slow down the clock by up
> to 0.05% (AUDITED)
> tick_usec -- a more extreme version of time_freq; can speed up or
> slow down the clock by up to 10% (AUDITED)
>
> Cc: Miroslav Lichvar <mlichvar@redhat.com>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
> kernel/time/ntp.c | 38 ++++++++++++++++++++++++++++++--------
> kernel/time/timekeeping.c | 3 +++
> 2 files changed, 33 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
> index a09ded765f6c..f96c6d326aae 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,21 +675,31 @@ 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)
> @@ -700,14 +716,18 @@ static inline void process_adjtimex_modes(struct timex *txc,
> time_constant = max(time_constant, 0l);
> }
>
> - 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;
> + }
It appears this time_tai use of "constant" is different than
time_constant, the former not mentioned by Miroslav Lichvar. What is it
and is it important to log for security? It sounds like it is
important.
> 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 +749,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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH ghak10 v5 2/2] timekeeping/ntp: Audit clock/NTP params adjustments
2018-08-24 19:47 ` Richard Guy Briggs
@ 2018-08-24 20:20 ` John Stultz
2018-08-27 11:35 ` Ondrej Mosnacek
1 sibling, 0 replies; 32+ messages in thread
From: John Stultz @ 2018-08-24 20:20 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: Ondrej Mosnacek, linux-audit, lkml, Stephen Boyd,
Miroslav Lichvar, Thomas Gleixner
On Fri, Aug 24, 2018 at 12:47 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-08-24 14:00, Ondrej Mosnacek wrote:
>> 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).
>
> I thought I saw it suggested earlier in one of the replies to a previous
> revision of the patchset to separate the two types of records with their
> calling circumstances. The inj-offset bits could stand alone in their
> own patch leaving all the rest in its own patch. The record numbers and
> examples are easier to offer when given together, but they aren't as
> clear they are indepnendent records and callers. That way, each patch
> stands on its own. (more below)
>
>> 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=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=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.)
>>
>> The changes to the time_maxerror, time_esterror, and time_constant
>> variables are not logged, as these are not important for security.
>>
>> 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).
>>
>> An overview of changes that can be done via adjtimex(2) (based on
>> information from Miroslav Lichvar) and whether they are audited:
>> timekeeping_inject_offset() -- injects offset directly into system
>> time (AUDITED)
>> __timekeeping_set_tai_offset() -- sets the offset from the
>> International Atomic Time
>> (AUDITED)
>> NTP variables:
>> time_offset -- 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) (AUDITED)
>> time_freq -- can speed up or slow down by up to about 0.05%
>> time_status -- can insert/delete leap seconds and it also enables/
>> disables synchronization of the hardware real-time
>> clock (AUDITED)
>> time_maxerror, time_esterror -- change error estimates used to
>> inform userspace applications
>> (NOT AUDITED)
>> time_constant -- controls the speed of the clock adjustments that
>> are made when time_offset is set (NOT AUDITED)
>> time_adjust -- can temporarily speed up or slow down the clock by up
>> to 0.05% (AUDITED)
>> tick_usec -- a more extreme version of time_freq; can speed up or
>> slow down the clock by up to 10% (AUDITED)
>>
>> Cc: Miroslav Lichvar <mlichvar@redhat.com>
>> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
>> ---
>> kernel/time/ntp.c | 38 ++++++++++++++++++++++++++++++--------
>> kernel/time/timekeeping.c | 3 +++
>> 2 files changed, 33 insertions(+), 8 deletions(-)
>>
>> diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
>> index a09ded765f6c..f96c6d326aae 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,21 +675,31 @@ 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)
>> @@ -700,14 +716,18 @@ static inline void process_adjtimex_modes(struct timex *txc,
>> time_constant = max(time_constant, 0l);
>> }
>>
>> - 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;
>> + }
>
> It appears this time_tai use of "constant" is different than
> time_constant, the former not mentioned by Miroslav Lichvar. What is it
> and is it important to log for security? It sounds like it is
> important.
From the adjtimex man page:
ADJ_TAI (since Linux 2.6.26)
Set TAI (Atomic International Time) offset from buf->constant.
thanks
-john
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH ghak10 v5 1/2] audit: Add functions to log time adjustments
2018-08-24 12:00 ` [PATCH ghak10 v5 1/2] audit: Add functions to log time adjustments Ondrej Mosnacek
2018-08-24 18:33 ` John Stultz
@ 2018-08-27 7:50 ` Miroslav Lichvar
2018-08-27 9:13 ` Ondrej Mosnacek
2018-09-14 3:18 ` Paul Moore
2 siblings, 1 reply; 32+ messages in thread
From: Miroslav Lichvar @ 2018-08-27 7:50 UTC (permalink / raw)
To: Ondrej Mosnacek
Cc: linux-audit, Paul Moore, Richard Guy Briggs, Steve Grubb,
John Stultz, Thomas Gleixner, Stephen Boyd, linux-kernel
On Fri, Aug 24, 2018 at 02:00:00PM +0200, Ondrej Mosnacek wrote:
> 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.
It seems the "adjust" function intentionally logs also calls/modes
that don't actually change anything. Can you please explain it a bit
in the message?
NTP/PTP daemons typically don't read the adjtimex values in a normal
operation and overwrite them on each update, even if they don't
change. If the audit function checked that oldval != newval, the
number of messages would be reduced and it might be easier to follow.
--
Miroslav Lichvar
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH ghak10 v5 1/2] audit: Add functions to log time adjustments
2018-08-24 18:33 ` John Stultz
@ 2018-08-27 8:28 ` Ondrej Mosnacek
2018-09-13 15:54 ` Richard Guy Briggs
0 siblings, 1 reply; 32+ messages in thread
From: Ondrej Mosnacek @ 2018-08-27 8:28 UTC (permalink / raw)
To: John Stultz
Cc: Linux-Audit Mailing List, Paul Moore, Richard Guy Briggs,
Steve Grubb, Miroslav Lichvar, Thomas Gleixner, Stephen Boyd,
Linux kernel mailing list
On Fri, Aug 24, 2018 at 8:33 PM John Stultz <john.stultz@linaro.org> wrote:
> On Fri, Aug 24, 2018 at 5:00 AM, Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > 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
> > 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);
> > +}
>
> So it looks like you've been careful here, but just want to make sure,
> nothing in the audit_log path calls anything that might possibly call
> back into timekeeping code? We've had a number of issues over time
> where debug calls out to other subsystems end up getting tweaked to
> add some timestamping and we deadlock. :)
Hm, that's a good point... AFAIK, the only place where audit_log()
might call into timekeeping is when it captures the timestamp for the
record (via ktime_get_coarse_real_ts64()), but this is only called
when the functions are called outside of a syscall and that should
never happen here. I'm thinking I could harden this more by returning
early from these functions if WARN_ON_ONCE(!audit_context() ||
!audit_context()->in_syscall)... It is not a perfect solution, but at
least there will be something in the code reminding us about this
issue.
>
> One approach we've done to make sure we don't create a trap for future
> changes in other subsystems, is avoid calling into other subsystems
> until later when we've dropped the locks, (see clock_was_set). Its a
> little rough for some of the things done deep in the ntp code, but
> might be an extra cautious approach to try.
I'm afraid that delaying the audit_* calls would complicate things too
much here... Paul/Richard, any thoughts?
--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH ghak10 v5 1/2] audit: Add functions to log time adjustments
2018-08-27 7:50 ` Miroslav Lichvar
@ 2018-08-27 9:13 ` Ondrej Mosnacek
2018-08-27 16:38 ` Steve Grubb
0 siblings, 1 reply; 32+ messages in thread
From: Ondrej Mosnacek @ 2018-08-27 9:13 UTC (permalink / raw)
To: Miroslav Lichvar
Cc: Linux-Audit Mailing List, Paul Moore, Richard Guy Briggs,
Steve Grubb, John Stultz, Thomas Gleixner, Stephen Boyd,
Linux kernel mailing list
On Mon, Aug 27, 2018 at 9:50 AM Miroslav Lichvar <mlichvar@redhat.com> wrote:
> On Fri, Aug 24, 2018 at 02:00:00PM +0200, Ondrej Mosnacek wrote:
> > 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.
>
> It seems the "adjust" function intentionally logs also calls/modes
> that don't actually change anything. Can you please explain it a bit
> in the message?
>
> NTP/PTP daemons typically don't read the adjtimex values in a normal
> operation and overwrite them on each update, even if they don't
> change. If the audit function checked that oldval != newval, the
> number of messages would be reduced and it might be easier to follow.
We actually want to log any attempt to change a value, as even an
intention to set/change something could be a hint that the process is
trying to do something bad (see discussion at [1]). There are valid
arguments both for and against this choice, but we have to pick one in
the end... Anyway, I should explain the reasoning in the commit
message better, right now it just states the fact without explanation
(in the second patch), thank you for pointing my attention to it.
[1] https://www.redhat.com/archives/linux-audit/2018-July/msg00061.html
--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH ghak10 v5 2/2] timekeeping/ntp: Audit clock/NTP params adjustments
2018-08-24 19:47 ` Richard Guy Briggs
2018-08-24 20:20 ` John Stultz
@ 2018-08-27 11:35 ` Ondrej Mosnacek
2018-08-27 11:45 ` Miroslav Lichvar
2018-09-13 15:35 ` Richard Guy Briggs
1 sibling, 2 replies; 32+ messages in thread
From: Ondrej Mosnacek @ 2018-08-27 11:35 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: Linux-Audit Mailing List, Linux kernel mailing list,
Stephen Boyd, Miroslav Lichvar, John Stultz, Thomas Gleixner
On Fri, Aug 24, 2018 at 9:51 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-08-24 14:00, Ondrej Mosnacek wrote:
> > 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).
>
> I thought I saw it suggested earlier in one of the replies to a previous
> revision of the patchset to separate the two types of records with their
> calling circumstances. The inj-offset bits could stand alone in their
> own patch leaving all the rest in its own patch. The record numbers and
> examples are easier to offer when given together, but they aren't as
> clear they are indepnendent records and callers. That way, each patch
> stands on its own. (more below)
Well, the idea of current split-up is to separate changes in different
subsystems. I would argue that the two record types are related enough
(and the diffs short enough) that it is worth keeping them together.
>
> > 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=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=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.)
> >
> > The changes to the time_maxerror, time_esterror, and time_constant
> > variables are not logged, as these are not important for security.
> >
> > 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).
> >
> > An overview of changes that can be done via adjtimex(2) (based on
> > information from Miroslav Lichvar) and whether they are audited:
> > timekeeping_inject_offset() -- injects offset directly into system
> > time (AUDITED)
> > __timekeeping_set_tai_offset() -- sets the offset from the
> > International Atomic Time
> > (AUDITED)
> > NTP variables:
> > time_offset -- 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) (AUDITED)
> > time_freq -- can speed up or slow down by up to about 0.05%
> > time_status -- can insert/delete leap seconds and it also enables/
> > disables synchronization of the hardware real-time
> > clock (AUDITED)
> > time_maxerror, time_esterror -- change error estimates used to
> > inform userspace applications
> > (NOT AUDITED)
> > time_constant -- controls the speed of the clock adjustments that
> > are made when time_offset is set (NOT AUDITED)
> > time_adjust -- can temporarily speed up or slow down the clock by up
> > to 0.05% (AUDITED)
> > tick_usec -- a more extreme version of time_freq; can speed up or
> > slow down the clock by up to 10% (AUDITED)
> >
> > Cc: Miroslav Lichvar <mlichvar@redhat.com>
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
> > kernel/time/ntp.c | 38 ++++++++++++++++++++++++++++++--------
> > kernel/time/timekeeping.c | 3 +++
> > 2 files changed, 33 insertions(+), 8 deletions(-)
> >
> > diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
> > index a09ded765f6c..f96c6d326aae 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,21 +675,31 @@ 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)
> > @@ -700,14 +716,18 @@ static inline void process_adjtimex_modes(struct timex *txc,
> > time_constant = max(time_constant, 0l);
> > }
> >
> > - 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;
> > + }
>
> It appears this time_tai use of "constant" is different than
> time_constant, the former not mentioned by Miroslav Lichvar. What is it
> and is it important to log for security? It sounds like it is
> important.
I believe ADJ_TIMECONST and ADJ_TAI are completely different things
and just reuse the same struct field (I would guess that ADJ_TAI
support was added later and it was decided like this to keep the ABI).
The TAI offset is the offset of the clock from the International
Atomic Time, so basically the time zone offset. I suppose it can't
influence the audit timestamps, but changing timezones can still cause
all sorts of confusion throughout the system, so intuitively I would
say we should log it.
>
> > 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 +749,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.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH ghak10 v5 2/2] timekeeping/ntp: Audit clock/NTP params adjustments
2018-08-27 11:35 ` Ondrej Mosnacek
@ 2018-08-27 11:45 ` Miroslav Lichvar
2018-08-27 12:02 ` Ondrej Mosnacek
2018-08-27 21:42 ` Thomas Gleixner
2018-09-13 15:35 ` Richard Guy Briggs
1 sibling, 2 replies; 32+ messages in thread
From: Miroslav Lichvar @ 2018-08-27 11:45 UTC (permalink / raw)
To: Ondrej Mosnacek
Cc: Richard Guy Briggs, Linux-Audit Mailing List,
Linux kernel mailing list, Stephen Boyd, John Stultz,
Thomas Gleixner
On Mon, Aug 27, 2018 at 01:35:09PM +0200, Ondrej Mosnacek wrote:
> On Fri, Aug 24, 2018 at 9:51 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > It appears this time_tai use of "constant" is different than
> > time_constant, the former not mentioned by Miroslav Lichvar. What is it
> > and is it important to log for security? It sounds like it is
> > important.
> The TAI offset is the offset of the clock from the International
> Atomic Time, so basically the time zone offset. I suppose it can't
> influence the audit timestamps, but changing timezones can still cause
> all sorts of confusion throughout the system, so intuitively I would
> say we should log it.
It's not related to timezones. ADJ_TAI sets the offset of the system
TAI clock (CLOCK_TAI) relative to the standard UTC clock
(CLOCK_REALTIME). CLOCK_TAI is rarely used by applications. Setting
the TAI offset effectively injects a whole-second offset to the TAI
time.
--
Miroslav Lichvar
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH ghak10 v5 2/2] timekeeping/ntp: Audit clock/NTP params adjustments
2018-08-27 11:45 ` Miroslav Lichvar
@ 2018-08-27 12:02 ` Ondrej Mosnacek
2018-08-27 21:42 ` Thomas Gleixner
1 sibling, 0 replies; 32+ messages in thread
From: Ondrej Mosnacek @ 2018-08-27 12:02 UTC (permalink / raw)
To: Miroslav Lichvar
Cc: Richard Guy Briggs, Linux-Audit Mailing List,
Linux kernel mailing list, Stephen Boyd, John Stultz,
Thomas Gleixner
On Mon, Aug 27, 2018 at 1:46 PM Miroslav Lichvar <mlichvar@redhat.com> wrote:
> On Mon, Aug 27, 2018 at 01:35:09PM +0200, Ondrej Mosnacek wrote:
> > On Fri, Aug 24, 2018 at 9:51 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > It appears this time_tai use of "constant" is different than
> > > time_constant, the former not mentioned by Miroslav Lichvar. What is it
> > > and is it important to log for security? It sounds like it is
> > > important.
>
> > The TAI offset is the offset of the clock from the International
> > Atomic Time, so basically the time zone offset. I suppose it can't
> > influence the audit timestamps, but changing timezones can still cause
> > all sorts of confusion throughout the system, so intuitively I would
> > say we should log it.
>
> It's not related to timezones. ADJ_TAI sets the offset of the system
> TAI clock (CLOCK_TAI) relative to the standard UTC clock
> (CLOCK_REALTIME). CLOCK_TAI is rarely used by applications. Setting
> the TAI offset effectively injects a whole-second offset to the TAI
> time.
Ah, I stand corrected then. In that case I think we don't actually
need to audit it, since it just changes how the time reported by the
CLOCK_TAI clock type will be different from CLOCK_REALTIME. Playing
with this value could cause some strange jumps in time for
applications that use it, but this seems like a very unlikely
situation. It really depends on how careful we want to be (vs. how
eagerly we want to reduce unimportant records), but this already looks
like logging it would be overdoing it...
Thanks for the correction, I really should RTFM more :)
--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH ghak10 v5 1/2] audit: Add functions to log time adjustments
2018-08-27 9:13 ` Ondrej Mosnacek
@ 2018-08-27 16:38 ` Steve Grubb
2018-09-13 13:59 ` Ondrej Mosnacek
0 siblings, 1 reply; 32+ messages in thread
From: Steve Grubb @ 2018-08-27 16:38 UTC (permalink / raw)
To: Ondrej Mosnacek
Cc: Miroslav Lichvar, Linux-Audit Mailing List, Paul Moore,
Richard Guy Briggs, John Stultz, Thomas Gleixner, Stephen Boyd,
Linux kernel mailing list
On Monday, August 27, 2018 5:13:17 AM EDT Ondrej Mosnacek wrote:
> On Mon, Aug 27, 2018 at 9:50 AM Miroslav Lichvar <mlichvar@redhat.com>
wrote:
> > On Fri, Aug 24, 2018 at 02:00:00PM +0200, Ondrej Mosnacek wrote:
> > > 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.
> >
> > It seems the "adjust" function intentionally logs also calls/modes
> > that don't actually change anything. Can you please explain it a bit
> > in the message?
> >
> > NTP/PTP daemons typically don't read the adjtimex values in a normal
> > operation and overwrite them on each update, even if they don't
> > change. If the audit function checked that oldval != newval, the
> > number of messages would be reduced and it might be easier to follow.
>
> We actually want to log any attempt to change a value, as even an
> intention to set/change something could be a hint that the process is
> trying to do something bad (see discussion at [1]).
One of the problems is that these applications can flood the logs very
quickly. An attempt to change is not needed unless it fails for permissions
reasons. So, limiting to actual changes is probably a good thing.
-Steve
> There are valid
> arguments both for and against this choice, but we have to pick one in
> the end... Anyway, I should explain the reasoning in the commit
> message better, right now it just states the fact without explanation
> (in the second patch), thank you for pointing my attention to it.
>
> [1] https://www.redhat.com/archives/linux-audit/2018-July/msg00061.html
>
> --
> Ondrej Mosnacek <omosnace at redhat dot com>
> Associate Software Engineer, Security Technologies
> Red Hat, Inc.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH ghak10 v5 2/2] timekeeping/ntp: Audit clock/NTP params adjustments
2018-08-27 11:45 ` Miroslav Lichvar
2018-08-27 12:02 ` Ondrej Mosnacek
@ 2018-08-27 21:42 ` Thomas Gleixner
1 sibling, 0 replies; 32+ messages in thread
From: Thomas Gleixner @ 2018-08-27 21:42 UTC (permalink / raw)
To: Miroslav Lichvar
Cc: Ondrej Mosnacek, Richard Guy Briggs, Linux-Audit Mailing List,
Linux kernel mailing list, Stephen Boyd, John Stultz
On Mon, 27 Aug 2018, Miroslav Lichvar wrote:
> On Mon, Aug 27, 2018 at 01:35:09PM +0200, Ondrej Mosnacek wrote:
> > On Fri, Aug 24, 2018 at 9:51 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > It appears this time_tai use of "constant" is different than
> > > time_constant, the former not mentioned by Miroslav Lichvar. What is it
> > > and is it important to log for security? It sounds like it is
> > > important.
>
> > The TAI offset is the offset of the clock from the International
> > Atomic Time, so basically the time zone offset. I suppose it can't
> > influence the audit timestamps, but changing timezones can still cause
> > all sorts of confusion throughout the system, so intuitively I would
> > say we should log it.
>
> It's not related to timezones. ADJ_TAI sets the offset of the system
> TAI clock (CLOCK_TAI) relative to the standard UTC clock
> (CLOCK_REALTIME). CLOCK_TAI is rarely used by applications. Setting
> the TAI offset effectively injects a whole-second offset to the TAI
> time.
Rarely used today, but with TSN it's going to get more usage in not so
distant future.
Thanks,
tglx
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH ghak10 v5 1/2] audit: Add functions to log time adjustments
2018-08-27 16:38 ` Steve Grubb
@ 2018-09-13 13:59 ` Ondrej Mosnacek
2018-09-13 15:14 ` Richard Guy Briggs
2018-09-14 3:09 ` Paul Moore
0 siblings, 2 replies; 32+ messages in thread
From: Ondrej Mosnacek @ 2018-09-13 13:59 UTC (permalink / raw)
To: Steve Grubb
Cc: Miroslav Lichvar, Linux-Audit Mailing List, Paul Moore,
Richard Guy Briggs, John Stultz, Thomas Gleixner, Stephen Boyd,
Linux kernel mailing list
On Mon, Aug 27, 2018 at 6:38 PM Steve Grubb <sgrubb@redhat.com> wrote:
> On Monday, August 27, 2018 5:13:17 AM EDT Ondrej Mosnacek wrote:
> > On Mon, Aug 27, 2018 at 9:50 AM Miroslav Lichvar <mlichvar@redhat.com>
> wrote:
> > > On Fri, Aug 24, 2018 at 02:00:00PM +0200, Ondrej Mosnacek wrote:
> > > > 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.
> > >
> > > It seems the "adjust" function intentionally logs also calls/modes
> > > that don't actually change anything. Can you please explain it a bit
> > > in the message?
> > >
> > > NTP/PTP daemons typically don't read the adjtimex values in a normal
> > > operation and overwrite them on each update, even if they don't
> > > change. If the audit function checked that oldval != newval, the
> > > number of messages would be reduced and it might be easier to follow.
> >
> > We actually want to log any attempt to change a value, as even an
> > intention to set/change something could be a hint that the process is
> > trying to do something bad (see discussion at [1]).
>
> One of the problems is that these applications can flood the logs very
> quickly. An attempt to change is not needed unless it fails for permissions
> reasons. So, limiting to actual changes is probably a good thing.
Well, Richard seemed to "violently" agree with the opposite, so now I
don't know which way to go... Paul, you are the official tie-breaker
here, which do you prefer?
>
> -Steve
>
> > There are valid
> > arguments both for and against this choice, but we have to pick one in
> > the end... Anyway, I should explain the reasoning in the commit
> > message better, right now it just states the fact without explanation
> > (in the second patch), thank you for pointing my attention to it.
> >
> > [1] https://www.redhat.com/archives/linux-audit/2018-July/msg00061.html
> >
> > --
> > Ondrej Mosnacek <omosnace at redhat dot com>
> > Associate Software Engineer, Security Technologies
> > Red Hat, Inc.
>
>
>
>
--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH ghak10 v5 1/2] audit: Add functions to log time adjustments
2018-09-13 13:59 ` Ondrej Mosnacek
@ 2018-09-13 15:14 ` Richard Guy Briggs
2018-09-17 12:32 ` Ondrej Mosnacek
2018-09-14 3:09 ` Paul Moore
1 sibling, 1 reply; 32+ messages in thread
From: Richard Guy Briggs @ 2018-09-13 15:14 UTC (permalink / raw)
To: Ondrej Mosnacek
Cc: Steve Grubb, Miroslav Lichvar, Linux-Audit Mailing List,
Paul Moore, John Stultz, Thomas Gleixner, Stephen Boyd,
Linux kernel mailing list
On 2018-09-13 15:59, Ondrej Mosnacek wrote:
> On Mon, Aug 27, 2018 at 6:38 PM Steve Grubb <sgrubb@redhat.com> wrote:
> > On Monday, August 27, 2018 5:13:17 AM EDT Ondrej Mosnacek wrote:
> > > On Mon, Aug 27, 2018 at 9:50 AM Miroslav Lichvar <mlichvar@redhat.com>
> > wrote:
> > > > On Fri, Aug 24, 2018 at 02:00:00PM +0200, Ondrej Mosnacek wrote:
> > > > > 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.
> > > >
> > > > It seems the "adjust" function intentionally logs also calls/modes
> > > > that don't actually change anything. Can you please explain it a bit
> > > > in the message?
> > > >
> > > > NTP/PTP daemons typically don't read the adjtimex values in a normal
> > > > operation and overwrite them on each update, even if they don't
> > > > change. If the audit function checked that oldval != newval, the
> > > > number of messages would be reduced and it might be easier to follow.
> > >
> > > We actually want to log any attempt to change a value, as even an
> > > intention to set/change something could be a hint that the process is
> > > trying to do something bad (see discussion at [1]).
> >
> > One of the problems is that these applications can flood the logs very
> > quickly. An attempt to change is not needed unless it fails for permissions
> > reasons. So, limiting to actual changes is probably a good thing.
>
> Well, Richard seemed to "violently" agree with the opposite, so now I
> don't know which way to go... Paul, you are the official tie-breaker
> here, which do you prefer?
The circumstances have changed with new information being added. I
recall violently agreeing several iterations ago with your previous
assessment, which has also changed with this new information. I'd agree
with Steve that a flood of information about something that did not
change value could hide important information.
(BTW: The expression "to violoently agree with" is generally used in a
situation where two parties appear to have been arguing two different
sides of an issue and then realize they have much more in common than
initially apparent.)
> > -Steve
> >
> > > There are valid
> > > arguments both for and against this choice, but we have to pick one in
> > > the end... Anyway, I should explain the reasoning in the commit
> > > message better, right now it just states the fact without explanation
> > > (in the second patch), thank you for pointing my attention to it.
> > >
> > > [1] https://www.redhat.com/archives/linux-audit/2018-July/msg00061.html
> > >
> > > --
> > > Ondrej Mosnacek <omosnace at redhat dot com>
>
> 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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH ghak10 v5 2/2] timekeeping/ntp: Audit clock/NTP params adjustments
2018-08-27 11:35 ` Ondrej Mosnacek
2018-08-27 11:45 ` Miroslav Lichvar
@ 2018-09-13 15:35 ` Richard Guy Briggs
1 sibling, 0 replies; 32+ messages in thread
From: Richard Guy Briggs @ 2018-09-13 15:35 UTC (permalink / raw)
To: Ondrej Mosnacek
Cc: Linux-Audit Mailing List, Linux kernel mailing list,
Stephen Boyd, Miroslav Lichvar, John Stultz, Thomas Gleixner
On 2018-08-27 13:35, Ondrej Mosnacek wrote:
> On Fri, Aug 24, 2018 at 9:51 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2018-08-24 14:00, Ondrej Mosnacek wrote:
> > > 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).
> >
> > I thought I saw it suggested earlier in one of the replies to a previous
> > revision of the patchset to separate the two types of records with their
> > calling circumstances. The inj-offset bits could stand alone in their
> > own patch leaving all the rest in its own patch. The record numbers and
> > examples are easier to offer when given together, but they aren't as
> > clear they are indepnendent records and callers. That way, each patch
> > stands on its own. (more below)
>
> Well, the idea of current split-up is to separate changes in different
> subsystems. I would argue that the two record types are related enough
> (and the diffs short enough) that it is worth keeping them together.
I would group the introduction of the macro with its usage, not
splitting across sub-systems. If you feel that the two are similar
enough, then all this should be in one patch. The record code patch
depends on the record macro, so they should be in the same patch. The
two records don't depend on each other and could be in seperate patches
with one cover letter to introduce and tie them together.
> > > 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=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=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.)
> > >
> > > The changes to the time_maxerror, time_esterror, and time_constant
> > > variables are not logged, as these are not important for security.
> > >
> > > 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).
> > >
> > > An overview of changes that can be done via adjtimex(2) (based on
> > > information from Miroslav Lichvar) and whether they are audited:
> > > timekeeping_inject_offset() -- injects offset directly into system
> > > time (AUDITED)
> > > __timekeeping_set_tai_offset() -- sets the offset from the
> > > International Atomic Time
> > > (AUDITED)
> > > NTP variables:
> > > time_offset -- 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) (AUDITED)
> > > time_freq -- can speed up or slow down by up to about 0.05%
> > > time_status -- can insert/delete leap seconds and it also enables/
> > > disables synchronization of the hardware real-time
> > > clock (AUDITED)
> > > time_maxerror, time_esterror -- change error estimates used to
> > > inform userspace applications
> > > (NOT AUDITED)
> > > time_constant -- controls the speed of the clock adjustments that
> > > are made when time_offset is set (NOT AUDITED)
> > > time_adjust -- can temporarily speed up or slow down the clock by up
> > > to 0.05% (AUDITED)
> > > tick_usec -- a more extreme version of time_freq; can speed up or
> > > slow down the clock by up to 10% (AUDITED)
> > >
> > > Cc: Miroslav Lichvar <mlichvar@redhat.com>
> > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > > ---
> > > kernel/time/ntp.c | 38 ++++++++++++++++++++++++++++++--------
> > > kernel/time/timekeeping.c | 3 +++
> > > 2 files changed, 33 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
> > > index a09ded765f6c..f96c6d326aae 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,21 +675,31 @@ 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)
> > > @@ -700,14 +716,18 @@ static inline void process_adjtimex_modes(struct timex *txc,
> > > time_constant = max(time_constant, 0l);
> > > }
> > >
> > > - 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;
> > > + }
> >
> > It appears this time_tai use of "constant" is different than
> > time_constant, the former not mentioned by Miroslav Lichvar. What is it
> > and is it important to log for security? It sounds like it is
> > important.
>
> I believe ADJ_TIMECONST and ADJ_TAI are completely different things
> and just reuse the same struct field (I would guess that ADJ_TAI
> support was added later and it was decided like this to keep the ABI).
>
> The TAI offset is the offset of the clock from the International
> Atomic Time, so basically the time zone offset. I suppose it can't
> influence the audit timestamps, but changing timezones can still cause
> all sorts of confusion throughout the system, so intuitively I would
> say we should log it.
>
> >
> > > 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 +749,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.
- 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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH ghak10 v5 1/2] audit: Add functions to log time adjustments
2018-08-27 8:28 ` Ondrej Mosnacek
@ 2018-09-13 15:54 ` Richard Guy Briggs
2018-09-17 12:33 ` Ondrej Mosnacek
0 siblings, 1 reply; 32+ messages in thread
From: Richard Guy Briggs @ 2018-09-13 15:54 UTC (permalink / raw)
To: Ondrej Mosnacek
Cc: John Stultz, Linux-Audit Mailing List, Paul Moore, Steve Grubb,
Miroslav Lichvar, Thomas Gleixner, Stephen Boyd,
Linux kernel mailing list
On 2018-08-27 10:28, Ondrej Mosnacek wrote:
> On Fri, Aug 24, 2018 at 8:33 PM John Stultz <john.stultz@linaro.org> wrote:
> > On Fri, Aug 24, 2018 at 5:00 AM, Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > 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
> > > 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);
> > > +}
> >
> > So it looks like you've been careful here, but just want to make sure,
> > nothing in the audit_log path calls anything that might possibly call
> > back into timekeeping code? We've had a number of issues over time
> > where debug calls out to other subsystems end up getting tweaked to
> > add some timestamping and we deadlock. :)
>
> Hm, that's a good point... AFAIK, the only place where audit_log()
> might call into timekeeping is when it captures the timestamp for the
> record (via ktime_get_coarse_real_ts64()), but this is only called
> when the functions are called outside of a syscall and that should
> never happen here. I'm thinking I could harden this more by returning
> early from these functions if WARN_ON_ONCE(!audit_context() ||
> !audit_context()->in_syscall)... It is not a perfect solution, but at
> least there will be something in the code reminding us about this
> issue.
It looks like this should be safe already since if there is no context,
it won't call into the real audit logging function and as you point out,
this should never happen outside of a syscall.
> > One approach we've done to make sure we don't create a trap for future
> > changes in other subsystems, is avoid calling into other subsystems
> > until later when we've dropped the locks, (see clock_was_set). Its a
> > little rough for some of the things done deep in the ntp code, but
> > might be an extra cautious approach to try.
>
> I'm afraid that delaying the audit_* calls would complicate things too
> much here... Paul/Richard, any thoughts?
The other way to address this would be to have your timekeeping audit
logging functions save the parameters you want to log somewhere in the
struct audit_context union and have it print the record on syscall exit
based on the presence of this type and applicable filters.
> 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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH ghak10 v5 1/2] audit: Add functions to log time adjustments
2018-09-13 13:59 ` Ondrej Mosnacek
2018-09-13 15:14 ` Richard Guy Briggs
@ 2018-09-14 3:09 ` Paul Moore
2018-09-17 12:33 ` Ondrej Mosnacek
1 sibling, 1 reply; 32+ messages in thread
From: Paul Moore @ 2018-09-14 3:09 UTC (permalink / raw)
To: omosnace
Cc: sgrubb, mlichvar, linux-audit, rgb, john.stultz, tglx, sboyd,
linux-kernel
On Thu, Sep 13, 2018 at 9:59 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Mon, Aug 27, 2018 at 6:38 PM Steve Grubb <sgrubb@redhat.com> wrote:
> > On Monday, August 27, 2018 5:13:17 AM EDT Ondrej Mosnacek wrote:
> > > On Mon, Aug 27, 2018 at 9:50 AM Miroslav Lichvar <mlichvar@redhat.com>
> > wrote:
> > > > On Fri, Aug 24, 2018 at 02:00:00PM +0200, Ondrej Mosnacek wrote:
> > > > > 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.
> > > >
> > > > It seems the "adjust" function intentionally logs also calls/modes
> > > > that don't actually change anything. Can you please explain it a bit
> > > > in the message?
> > > >
> > > > NTP/PTP daemons typically don't read the adjtimex values in a normal
> > > > operation and overwrite them on each update, even if they don't
> > > > change. If the audit function checked that oldval != newval, the
> > > > number of messages would be reduced and it might be easier to follow.
> > >
> > > We actually want to log any attempt to change a value, as even an
> > > intention to set/change something could be a hint that the process is
> > > trying to do something bad (see discussion at [1]).
> >
> > One of the problems is that these applications can flood the logs very
> > quickly. An attempt to change is not needed unless it fails for permissions
> > reasons. So, limiting to actual changes is probably a good thing.
>
> Well, Richard seemed to "violently" agree with the opposite, so now I
> don't know which way to go... Paul, you are the official tie-breaker
> here, which do you prefer?
The general idea is that we only care about *changes* to the system
state, so if a process is setting a variable to with a value that
matches it's current value I see no reason why we need to generate a
change record.
Another thing to keep in mind, we can always change the behavior to be
more verbose (*always* generate a record, regardless of value) without
likely causing a regression, but limiting records is more difficult
and more likely to cause regressions.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH ghak10 v5 1/2] audit: Add functions to log time adjustments
2018-08-24 12:00 ` [PATCH ghak10 v5 1/2] audit: Add functions to log time adjustments Ondrej Mosnacek
2018-08-24 18:33 ` John Stultz
2018-08-27 7:50 ` Miroslav Lichvar
@ 2018-09-14 3:18 ` Paul Moore
2018-09-14 15:16 ` Richard Guy Briggs
2018-09-17 12:38 ` Ondrej Mosnacek
2 siblings, 2 replies; 32+ messages in thread
From: Paul Moore @ 2018-09-14 3:18 UTC (permalink / raw)
To: omosnace
Cc: linux-audit, rgb, sgrubb, mlichvar, john.stultz, tglx, sboyd,
linux-kernel
On Fri, Aug 24, 2018 at 8:00 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> 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
> adjust - corresponding to the time_adjust variable
> tick - corresponding to the tick_usec variable
> tai - corresponding to the timekeeping's TAI offset
I understand that reusing "op" is tempting, but the above aren't
really operations, they are state variables which are being changed.
Using the CONFIG_CHANGE record as a basis, I wonder if we are better
off with something like the following:
type=TIME_CHANGE <var>=<value_new> old=<value_old>
... you might need to preface the variable names with something like
"ntp_" or "offset_". You'll notice I'm also suggesting we use a
single record type here; is there any reason why two records types are
required?
> 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(+)
A reminder that we need tests for these new records and a RFE page on the wiki:
* https://github.com/linux-audit/audit-testsuite
* https://github.com/linux-audit/audit-kernel/wiki
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH ghak10 v5 1/2] audit: Add functions to log time adjustments
2018-09-14 3:18 ` Paul Moore
@ 2018-09-14 15:16 ` Richard Guy Briggs
2018-09-14 15:34 ` Steve Grubb
2018-09-17 14:36 ` Paul Moore
2018-09-17 12:38 ` Ondrej Mosnacek
1 sibling, 2 replies; 32+ messages in thread
From: Richard Guy Briggs @ 2018-09-14 15:16 UTC (permalink / raw)
To: Paul Moore
Cc: omosnace, linux-audit, sgrubb, mlichvar, john.stultz, tglx,
sboyd, linux-kernel
On 2018-09-13 23:18, Paul Moore wrote:
> On Fri, Aug 24, 2018 at 8:00 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > 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
> > adjust - corresponding to the time_adjust variable
> > tick - corresponding to the tick_usec variable
> > tai - corresponding to the timekeeping's TAI offset
>
> I understand that reusing "op" is tempting, but the above aren't
> really operations, they are state variables which are being changed.
> Using the CONFIG_CHANGE record as a basis, I wonder if we are better
> off with something like the following:
>
> type=TIME_CHANGE <var>=<value_new> old=<value_old>
>
> ... you might need to preface the variable names with something like
> "ntp_" or "offset_". You'll notice I'm also suggesting we use a
> single record type here; is there any reason why two records types are
> required?
Why not do something like:
type=TIME_CHANGE var=<var> new=<value_new> old=<value_old>
So that we don't pollute the field namespace *and* create 8 variants on
the same record format? This shouldn't be much of a concern with binary
record formats, but we're stuck with the current parsing scheme for now.
> > 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(+)
>
> A reminder that we need tests for these new records and a RFE page on the wiki:
>
> * https://github.com/linux-audit/audit-testsuite
> * https://github.com/linux-audit/audit-kernel/wiki
>
> --
> paul moore
> www.paul-moore.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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH ghak10 v5 1/2] audit: Add functions to log time adjustments
2018-09-14 15:16 ` Richard Guy Briggs
@ 2018-09-14 15:34 ` Steve Grubb
2018-09-14 16:24 ` Richard Guy Briggs
2018-09-17 14:36 ` Paul Moore
1 sibling, 1 reply; 32+ messages in thread
From: Steve Grubb @ 2018-09-14 15:34 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: Paul Moore, omosnace, linux-audit, mlichvar, john.stultz, tglx,
sboyd, linux-kernel
On Friday, September 14, 2018 11:16:43 AM EDT Richard Guy Briggs wrote:
> On 2018-09-13 23:18, Paul Moore wrote:
> > On Fri, Aug 24, 2018 at 8:00 AM Ondrej Mosnacek <omosnace@redhat.com>
wrote:
> > > 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
> > > adjust - corresponding to the time_adjust variable
> > > tick - corresponding to the tick_usec variable
> > > tai - corresponding to the timekeeping's TAI offset
> >
> > I understand that reusing "op" is tempting, but the above aren't
> > really operations, they are state variables which are being changed.
> > Using the CONFIG_CHANGE record as a basis, I wonder if we are better
> >
> > off with something like the following:
> > type=TIME_CHANGE <var>=<value_new> old=<value_old>
> >
> > ... you might need to preface the variable names with something like
> > "ntp_" or "offset_". You'll notice I'm also suggesting we use a
> > single record type here; is there any reason why two records types are
> > required?
>
> Why not do something like:
>
> type=TIME_CHANGE var=<var> new=<value_new> old=<value_old>
>
> So that we don't pollute the field namespace *and* create 8 variants on
> the same record format? This shouldn't be much of a concern with binary
> record formats, but we're stuck with the current parsing scheme for now.
Something like this or the other format is fine. Neither hurt parsing because
these are not searchable fields. We only have issues when it involves a
searchable field name.
HTH...
-Steve
> > > 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(+)
> >
> > A reminder that we need tests for these new records and a RFE page on the
> > wiki:
> >
> > * https://github.com/linux-audit/audit-testsuite
> > * https://github.com/linux-audit/audit-kernel/wiki
>
> - 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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH ghak10 v5 1/2] audit: Add functions to log time adjustments
2018-09-14 15:34 ` Steve Grubb
@ 2018-09-14 16:24 ` Richard Guy Briggs
0 siblings, 0 replies; 32+ messages in thread
From: Richard Guy Briggs @ 2018-09-14 16:24 UTC (permalink / raw)
To: Steve Grubb; +Cc: sboyd, linux-kernel, mlichvar, linux-audit, john.stultz, tglx
On 2018-09-14 11:34, Steve Grubb wrote:
> On Friday, September 14, 2018 11:16:43 AM EDT Richard Guy Briggs wrote:
> > On 2018-09-13 23:18, Paul Moore wrote:
> > > On Fri, Aug 24, 2018 at 8:00 AM Ondrej Mosnacek <omosnace@redhat.com>
> wrote:
> > > > 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
> > > > adjust - corresponding to the time_adjust variable
> > > > tick - corresponding to the tick_usec variable
> > > > tai - corresponding to the timekeeping's TAI offset
> > >
> > > I understand that reusing "op" is tempting, but the above aren't
> > > really operations, they are state variables which are being changed.
> > > Using the CONFIG_CHANGE record as a basis, I wonder if we are better
> > >
> > > off with something like the following:
> > > type=TIME_CHANGE <var>=<value_new> old=<value_old>
> > >
> > > ... you might need to preface the variable names with something like
> > > "ntp_" or "offset_". You'll notice I'm also suggesting we use a
> > > single record type here; is there any reason why two records types are
> > > required?
> >
> > Why not do something like:
> >
> > type=TIME_CHANGE var=<var> new=<value_new> old=<value_old>
> >
> > So that we don't pollute the field namespace *and* create 8 variants on
> > the same record format? This shouldn't be much of a concern with binary
> > record formats, but we're stuck with the current parsing scheme for now.
>
> Something like this or the other format is fine. Neither hurt parsing because
> these are not searchable fields. We only have issues when it involves a
> searchable field name.
Ok, fair enough. Thanks Steve.
> HTH...
>
> -Steve
>
> > > > 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(+)
> > >
> > > A reminder that we need tests for these new records and a RFE page on the
> > > wiki:
> > >
> > > * https://github.com/linux-audit/audit-testsuite
> > > * https://github.com/linux-audit/audit-kernel/wiki
> >
> > - RGB
- 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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH ghak10 v5 1/2] audit: Add functions to log time adjustments
2018-09-13 15:14 ` Richard Guy Briggs
@ 2018-09-17 12:32 ` Ondrej Mosnacek
0 siblings, 0 replies; 32+ messages in thread
From: Ondrej Mosnacek @ 2018-09-17 12:32 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: Steve Grubb, Miroslav Lichvar, Linux-Audit Mailing List,
Paul Moore, John Stultz, Thomas Gleixner, Stephen Boyd,
Linux kernel mailing list
On Thu, Sep 13, 2018 at 5:19 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-09-13 15:59, Ondrej Mosnacek wrote:
> > On Mon, Aug 27, 2018 at 6:38 PM Steve Grubb <sgrubb@redhat.com> wrote:
> > > On Monday, August 27, 2018 5:13:17 AM EDT Ondrej Mosnacek wrote:
> > > > On Mon, Aug 27, 2018 at 9:50 AM Miroslav Lichvar <mlichvar@redhat.com>
> > > wrote:
> > > > > On Fri, Aug 24, 2018 at 02:00:00PM +0200, Ondrej Mosnacek wrote:
> > > > > > 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.
> > > > >
> > > > > It seems the "adjust" function intentionally logs also calls/modes
> > > > > that don't actually change anything. Can you please explain it a bit
> > > > > in the message?
> > > > >
> > > > > NTP/PTP daemons typically don't read the adjtimex values in a normal
> > > > > operation and overwrite them on each update, even if they don't
> > > > > change. If the audit function checked that oldval != newval, the
> > > > > number of messages would be reduced and it might be easier to follow.
> > > >
> > > > We actually want to log any attempt to change a value, as even an
> > > > intention to set/change something could be a hint that the process is
> > > > trying to do something bad (see discussion at [1]).
> > >
> > > One of the problems is that these applications can flood the logs very
> > > quickly. An attempt to change is not needed unless it fails for permissions
> > > reasons. So, limiting to actual changes is probably a good thing.
> >
> > Well, Richard seemed to "violently" agree with the opposite, so now I
> > don't know which way to go... Paul, you are the official tie-breaker
> > here, which do you prefer?
>
> The circumstances have changed with new information being added. I
> recall violently agreeing several iterations ago with your previous
> assessment, which has also changed with this new information. I'd agree
> with Steve that a flood of information about something that did not
> change value could hide important information.
OK, understood.
> (BTW: The expression "to violoently agree with" is generally used in a
> situation where two parties appear to have been arguing two different
> sides of an issue and then realize they have much more in common than
> initially apparent.)
I see, thanks for the explanation! I didn't know that expression
before, so I think I took it a bit too literally :)
>
> > > -Steve
> > >
> > > > There are valid
> > > > arguments both for and against this choice, but we have to pick one in
> > > > the end... Anyway, I should explain the reasoning in the commit
> > > > message better, right now it just states the fact without explanation
> > > > (in the second patch), thank you for pointing my attention to it.
> > > >
> > > > [1] https://www.redhat.com/archives/linux-audit/2018-July/msg00061.html
> > > >
> > > > --
> > > > Ondrej Mosnacek <omosnace at redhat dot com>
> >
> > 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
--
Ondrej Mosnacek <omosnace at redhat dot com>Associate Software
Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH ghak10 v5 1/2] audit: Add functions to log time adjustments
2018-09-13 15:54 ` Richard Guy Briggs
@ 2018-09-17 12:33 ` Ondrej Mosnacek
0 siblings, 0 replies; 32+ messages in thread
From: Ondrej Mosnacek @ 2018-09-17 12:33 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: John Stultz, Linux-Audit Mailing List, Paul Moore, Steve Grubb,
Miroslav Lichvar, Thomas Gleixner, Stephen Boyd,
Linux kernel mailing list
On Thu, Sep 13, 2018 at 5:59 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-08-27 10:28, Ondrej Mosnacek wrote:
> > On Fri, Aug 24, 2018 at 8:33 PM John Stultz <john.stultz@linaro.org> wrote:
> > > On Fri, Aug 24, 2018 at 5:00 AM, Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > > 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
> > > > 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);
> > > > +}
> > >
> > > So it looks like you've been careful here, but just want to make sure,
> > > nothing in the audit_log path calls anything that might possibly call
> > > back into timekeeping code? We've had a number of issues over time
> > > where debug calls out to other subsystems end up getting tweaked to
> > > add some timestamping and we deadlock. :)
> >
> > Hm, that's a good point... AFAIK, the only place where audit_log()
> > might call into timekeeping is when it captures the timestamp for the
> > record (via ktime_get_coarse_real_ts64()), but this is only called
> > when the functions are called outside of a syscall and that should
> > never happen here. I'm thinking I could harden this more by returning
> > early from these functions if WARN_ON_ONCE(!audit_context() ||
> > !audit_context()->in_syscall)... It is not a perfect solution, but at
> > least there will be something in the code reminding us about this
> > issue.
>
> It looks like this should be safe already since if there is no context,
> it won't call into the real audit logging function and as you point out,
> this should never happen outside of a syscall.
>
> > > One approach we've done to make sure we don't create a trap for future
> > > changes in other subsystems, is avoid calling into other subsystems
> > > until later when we've dropped the locks, (see clock_was_set). Its a
> > > little rough for some of the things done deep in the ntp code, but
> > > might be an extra cautious approach to try.
> >
> > I'm afraid that delaying the audit_* calls would complicate things too
> > much here... Paul/Richard, any thoughts?
>
> The other way to address this would be to have your timekeeping audit
> logging functions save the parameters you want to log somewhere in the
> struct audit_context union and have it print the record on syscall exit
> based on the presence of this type and applicable filters.
Yes, I thought about that, but it doesn't seem to be worth the hassle.
I think I'll just add the paranoid WARN_ON_ONCE(...) unless there are
strong objections to that.
>
> > 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
--
Ondrej Mosnacek <omosnace at redhat dot com>Associate Software
Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH ghak10 v5 1/2] audit: Add functions to log time adjustments
2018-09-14 3:09 ` Paul Moore
@ 2018-09-17 12:33 ` Ondrej Mosnacek
0 siblings, 0 replies; 32+ messages in thread
From: Ondrej Mosnacek @ 2018-09-17 12:33 UTC (permalink / raw)
To: Paul Moore
Cc: Steve Grubb, Miroslav Lichvar, Linux-Audit Mailing List,
Richard Guy Briggs, John Stultz, Thomas Gleixner, Stephen Boyd,
Linux kernel mailing list
On Fri, Sep 14, 2018 at 5:09 AM Paul Moore <paul@paul-moore.com> wrote:
> On Thu, Sep 13, 2018 at 9:59 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > On Mon, Aug 27, 2018 at 6:38 PM Steve Grubb <sgrubb@redhat.com> wrote:
> > > On Monday, August 27, 2018 5:13:17 AM EDT Ondrej Mosnacek wrote:
> > > > On Mon, Aug 27, 2018 at 9:50 AM Miroslav Lichvar <mlichvar@redhat.com>
> > > wrote:
> > > > > On Fri, Aug 24, 2018 at 02:00:00PM +0200, Ondrej Mosnacek wrote:
> > > > > > 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.
> > > > >
> > > > > It seems the "adjust" function intentionally logs also calls/modes
> > > > > that don't actually change anything. Can you please explain it a bit
> > > > > in the message?
> > > > >
> > > > > NTP/PTP daemons typically don't read the adjtimex values in a normal
> > > > > operation and overwrite them on each update, even if they don't
> > > > > change. If the audit function checked that oldval != newval, the
> > > > > number of messages would be reduced and it might be easier to follow.
> > > >
> > > > We actually want to log any attempt to change a value, as even an
> > > > intention to set/change something could be a hint that the process is
> > > > trying to do something bad (see discussion at [1]).
> > >
> > > One of the problems is that these applications can flood the logs very
> > > quickly. An attempt to change is not needed unless it fails for permissions
> > > reasons. So, limiting to actual changes is probably a good thing.
> >
> > Well, Richard seemed to "violently" agree with the opposite, so now I
> > don't know which way to go... Paul, you are the official tie-breaker
> > here, which do you prefer?
>
> The general idea is that we only care about *changes* to the system
> state, so if a process is setting a variable to with a value that
> matches it's current value I see no reason why we need to generate a
> change record.
>
> Another thing to keep in mind, we can always change the behavior to be
> more verbose (*always* generate a record, regardless of value) without
> likely causing a regression, but limiting records is more difficult
> and more likely to cause regressions.
OK, that makes sense. I'll limit logging to actual changes in the next revision.
>
> --
> paul moore
> www.paul-moore.com
--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH ghak10 v5 1/2] audit: Add functions to log time adjustments
2018-09-14 3:18 ` Paul Moore
2018-09-14 15:16 ` Richard Guy Briggs
@ 2018-09-17 12:38 ` Ondrej Mosnacek
2018-09-17 14:20 ` Richard Guy Briggs
2018-09-17 14:50 ` Paul Moore
1 sibling, 2 replies; 32+ messages in thread
From: Ondrej Mosnacek @ 2018-09-17 12:38 UTC (permalink / raw)
To: Paul Moore
Cc: Linux-Audit Mailing List, Richard Guy Briggs, Steve Grubb,
Miroslav Lichvar, John Stultz, Thomas Gleixner, Stephen Boyd,
Linux kernel mailing list
On Fri, Sep 14, 2018 at 5:19 AM Paul Moore <paul@paul-moore.com> wrote:
> On Fri, Aug 24, 2018 at 8:00 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > 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
> > adjust - corresponding to the time_adjust variable
> > tick - corresponding to the tick_usec variable
> > tai - corresponding to the timekeeping's TAI offset
>
> I understand that reusing "op" is tempting, but the above aren't
> really operations, they are state variables which are being changed.
I remember Steve (or was it Richard?) convincing me at one of the
meetings that "op" is the right filed name to use, despite it not
being a name for an operation... But I don't really care, I'm okay
with changing it to e.g. "var" as Richard suggests later in this
thread.
> Using the CONFIG_CHANGE record as a basis, I wonder if we are better
> off with something like the following:
>
> type=TIME_CHANGE <var>=<value_new> old=<value_old>
>
> ... you might need to preface the variable names with something like
> "ntp_" or "offset_". You'll notice I'm also suggesting we use a
> single record type here; is there any reason why two records types are
> required?
There are actually two reasons:
1. The injected offset is a timespec64, so it consists of two integer
values (and it would be weird to produce two records for it, since IMO
it is conceptually still a single variable).
2. In all other cases the variable is reset to the (possibly
transformed) input value, while in this case the input value is added
directly to the system time. This can be viewed as a kind of variable
too, but it would be weird to report old and new value for it, since
its value flows with time.
Plus, when I look at:
type=TIME_INJOFFSET [...]: sec=-16 nsec=124887145
I can immediately see that the time was shifted back by 16-something
seconds, while when I look at something like:
type=TIME_CHANGE [...]: var=time_sec new=1537185685 old=1537185701
type=TIME_CHANGE [...]: var=time_nsec new=664373417 old=789260562
I can just see some big numbers that I need to do math with before I
get an idea of what is the magnitude (or sign) of the change.
>
> > 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(+)
>
> A reminder that we need tests for these new records and a RFE page on the wiki:
>
> * https://github.com/linux-audit/audit-testsuite
I was going to start working on this once the format issues are
settled. (Although I probably should have kept the RFC in the subject
until then...)
> * https://github.com/linux-audit/audit-kernel/wiki
I admit I forgot about this duty, but again I would like to wait for
the discussions to settle before writing that up.
>
> --
> paul moore
> www.paul-moore.com--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH ghak10 v5 1/2] audit: Add functions to log time adjustments
2018-09-17 12:38 ` Ondrej Mosnacek
@ 2018-09-17 14:20 ` Richard Guy Briggs
2018-09-17 14:50 ` Paul Moore
1 sibling, 0 replies; 32+ messages in thread
From: Richard Guy Briggs @ 2018-09-17 14:20 UTC (permalink / raw)
To: Ondrej Mosnacek
Cc: Paul Moore, Linux-Audit Mailing List, Steve Grubb,
Miroslav Lichvar, John Stultz, Thomas Gleixner, Stephen Boyd,
Linux kernel mailing list
On 2018-09-17 14:38, Ondrej Mosnacek wrote:
> On Fri, Sep 14, 2018 at 5:19 AM Paul Moore <paul@paul-moore.com> wrote:
> > On Fri, Aug 24, 2018 at 8:00 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > 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
> > > adjust - corresponding to the time_adjust variable
> > > tick - corresponding to the tick_usec variable
> > > tai - corresponding to the timekeeping's TAI offset
> >
> > I understand that reusing "op" is tempting, but the above aren't
> > really operations, they are state variables which are being changed.
>
> I remember Steve (or was it Richard?) convincing me at one of the
> meetings that "op" is the right filed name to use, despite it not
> being a name for an operation... But I don't really care, I'm okay
> with changing it to e.g. "var" as Richard suggests later in this
> thread.
>
> > Using the CONFIG_CHANGE record as a basis, I wonder if we are better
> > off with something like the following:
> >
> > type=TIME_CHANGE <var>=<value_new> old=<value_old>
> >
> > ... you might need to preface the variable names with something like
> > "ntp_" or "offset_". You'll notice I'm also suggesting we use a
> > single record type here; is there any reason why two records types are
> > required?
>
> There are actually two reasons:
> 1. The injected offset is a timespec64, so it consists of two integer
> values (and it would be weird to produce two records for it, since IMO
> it is conceptually still a single variable).
> 2. In all other cases the variable is reset to the (possibly
> transformed) input value, while in this case the input value is added
> directly to the system time. This can be viewed as a kind of variable
> too, but it would be weird to report old and new value for it, since
> its value flows with time.
>
> Plus, when I look at:
> type=TIME_INJOFFSET [...]: sec=-16 nsec=124887145
>
> I can immediately see that the time was shifted back by 16-something
> seconds, while when I look at something like:
>
> type=TIME_CHANGE [...]: var=time_sec new=1537185685 old=1537185701
> type=TIME_CHANGE [...]: var=time_nsec new=664373417 old=789260562
>
> I can just see some big numbers that I need to do math with before I
> get an idea of what is the magnitude (or sign) of the change.
>
> > > 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(+)
> >
> > A reminder that we need tests for these new records and a RFE page on the wiki:
> >
> > * https://github.com/linux-audit/audit-testsuite
>
> I was going to start working on this once the format issues are
> settled. (Although I probably should have kept the RFC in the subject
> until then...)
There are more iterative development methods (to which we appear to be
trying to steer) where the test is written first, partly helping clarify
what the goal is. Refining the test is incremental.
> > * https://github.com/linux-audit/audit-kernel/wiki
>
> I admit I forgot about this duty, but again I would like to wait for
> the discussions to settle before writing that up.
While tempting to leave it to the end, documenting it initially can help
clarify in your own mind what you are trying to accomplish and can help
others understand what you are trying to do.
> > paul moore
> 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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH ghak10 v5 1/2] audit: Add functions to log time adjustments
2018-09-14 15:16 ` Richard Guy Briggs
2018-09-14 15:34 ` Steve Grubb
@ 2018-09-17 14:36 ` Paul Moore
1 sibling, 0 replies; 32+ messages in thread
From: Paul Moore @ 2018-09-17 14:36 UTC (permalink / raw)
To: rgb
Cc: omosnace, linux-audit, sgrubb, mlichvar, john.stultz, tglx,
sboyd, linux-kernel
On Fri, Sep 14, 2018 at 11:21 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-09-13 23:18, Paul Moore wrote:
> > On Fri, Aug 24, 2018 at 8:00 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > 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
> > > adjust - corresponding to the time_adjust variable
> > > tick - corresponding to the tick_usec variable
> > > tai - corresponding to the timekeeping's TAI offset
> >
> > I understand that reusing "op" is tempting, but the above aren't
> > really operations, they are state variables which are being changed.
> > Using the CONFIG_CHANGE record as a basis, I wonder if we are better
> > off with something like the following:
> >
> > type=TIME_CHANGE <var>=<value_new> old=<value_old>
> >
> > ... you might need to preface the variable names with something like
> > "ntp_" or "offset_". You'll notice I'm also suggesting we use a
> > single record type here; is there any reason why two records types are
> > required?
>
> Why not do something like:
>
> type=TIME_CHANGE var=<var> new=<value_new> old=<value_old>
>
> So that we don't pollute the field namespace *and* create 8 variants on
> the same record format? This shouldn't be much of a concern with binary
> record formats, but we're stuck with the current parsing scheme for now.
Since there is already some precedence with the "<var>=<value_new>"
format, and the field namespace is already a bit of a mess IMHO, I'd
like us to stick with the style used by CONFIG_CHANGE.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH ghak10 v5 1/2] audit: Add functions to log time adjustments
2018-09-17 12:38 ` Ondrej Mosnacek
2018-09-17 14:20 ` Richard Guy Briggs
@ 2018-09-17 14:50 ` Paul Moore
2018-09-21 11:21 ` Ondrej Mosnacek
1 sibling, 1 reply; 32+ messages in thread
From: Paul Moore @ 2018-09-17 14:50 UTC (permalink / raw)
To: omosnace
Cc: linux-audit, rgb, sgrubb, mlichvar, john.stultz, tglx, sboyd,
linux-kernel
On Mon, Sep 17, 2018 at 8:38 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> On Fri, Sep 14, 2018 at 5:19 AM Paul Moore <paul@paul-moore.com> wrote:
> > On Fri, Aug 24, 2018 at 8:00 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > 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
> > > adjust - corresponding to the time_adjust variable
> > > tick - corresponding to the tick_usec variable
> > > tai - corresponding to the timekeeping's TAI offset
> >
> > I understand that reusing "op" is tempting, but the above aren't
> > really operations, they are state variables which are being changed.
>
> I remember Steve (or was it Richard?) convincing me at one of the
> meetings that "op" is the right filed name to use, despite it not
> being a name for an operation... But I don't really care, I'm okay
> with changing it to e.g. "var" as Richard suggests later in this
> thread.
As I said before, this seems like an abuse of the "op" field.
> > Using the CONFIG_CHANGE record as a basis, I wonder if we are better
> > off with something like the following:
> >
> > type=TIME_CHANGE <var>=<value_new> old=<value_old>
> >
> > ... you might need to preface the variable names with something like
> > "ntp_" or "offset_". You'll notice I'm also suggesting we use a
> > single record type here; is there any reason why two records types are
> > required?
>
> There are actually two reasons:
> 1. The injected offset is a timespec64, so it consists of two integer
> values (and it would be weird to produce two records for it, since IMO
> it is conceptually still a single variable).
> 2. In all other cases the variable is reset to the (possibly
> transformed) input value, while in this case the input value is added
> directly to the system time. This can be viewed as a kind of variable
> too, but it would be weird to report old and new value for it, since
> its value flows with time.
>
> Plus, when I look at:
> type=TIME_INJOFFSET [...]: sec=-16 nsec=124887145
>
> I can immediately see that the time was shifted back by 16-something
> seconds, while when I look at something like:
>
> type=TIME_CHANGE [...]: var=time_sec new=1537185685 old=1537185701
> type=TIME_CHANGE [...]: var=time_nsec new=664373417 old=789260562
>
> I can just see some big numbers that I need to do math with before I
> get an idea of what is the magnitude (or sign) of the change.
Okay, with that in mind, perhaps when recording the offset values we
omit the "old" values (arguably that doesn't make much sense here) and
keep the sec/nsec split:
type=TIME_CHANGE [...]: offset_sec=<X> offset_nsec=<Y>
... and for all others we stick with:
type=TIME_CHANGE [...]: ntp_<VAR>=<NEWVAL> old=<OLD_VAL>
... and if that results in multiple TIME_CHANGE records for a given
event, that's fine with me.
> > A reminder that we need tests for these new records and a RFE page on the wiki:
> >
> > * https://github.com/linux-audit/audit-testsuite
>
> I was going to start working on this once the format issues are
> settled. (Although I probably should have kept the RFC in the subject
> until then...)
>
> > * https://github.com/linux-audit/audit-kernel/wiki
>
> I admit I forgot about this duty, but again I would like to wait for
> the discussions to settle before writing that up.
That is fine, do it in whatever order works best for you, just
understand that I'm probably not going to merge patches like this
until I see both documentation and tests.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH ghak10 v5 1/2] audit: Add functions to log time adjustments
2018-09-17 14:50 ` Paul Moore
@ 2018-09-21 11:21 ` Ondrej Mosnacek
2018-09-22 20:42 ` Paul Moore
0 siblings, 1 reply; 32+ messages in thread
From: Ondrej Mosnacek @ 2018-09-21 11:21 UTC (permalink / raw)
To: Paul Moore
Cc: Linux-Audit Mailing List, Richard Guy Briggs, Steve Grubb,
Miroslav Lichvar, John Stultz, Thomas Gleixner, Stephen Boyd,
Linux kernel mailing list
On Mon, Sep 17, 2018 at 4:51 PM Paul Moore <paul@paul-moore.com> wrote:
> On Mon, Sep 17, 2018 at 8:38 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > On Fri, Sep 14, 2018 at 5:19 AM Paul Moore <paul@paul-moore.com> wrote:
> > > On Fri, Aug 24, 2018 at 8:00 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > > 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
> > > > adjust - corresponding to the time_adjust variable
> > > > tick - corresponding to the tick_usec variable
> > > > tai - corresponding to the timekeeping's TAI offset
> > >
> > > I understand that reusing "op" is tempting, but the above aren't
> > > really operations, they are state variables which are being changed.
> >
> > I remember Steve (or was it Richard?) convincing me at one of the
> > meetings that "op" is the right filed name to use, despite it not
> > being a name for an operation... But I don't really care, I'm okay
> > with changing it to e.g. "var" as Richard suggests later in this
> > thread.
>
> As I said before, this seems like an abuse of the "op" field.
>
> > > Using the CONFIG_CHANGE record as a basis, I wonder if we are better
> > > off with something like the following:
> > >
> > > type=TIME_CHANGE <var>=<value_new> old=<value_old>
> > >
> > > ... you might need to preface the variable names with something like
> > > "ntp_" or "offset_". You'll notice I'm also suggesting we use a
> > > single record type here; is there any reason why two records types are
> > > required?
> >
> > There are actually two reasons:
> > 1. The injected offset is a timespec64, so it consists of two integer
> > values (and it would be weird to produce two records for it, since IMO
> > it is conceptually still a single variable).
> > 2. In all other cases the variable is reset to the (possibly
> > transformed) input value, while in this case the input value is added
> > directly to the system time. This can be viewed as a kind of variable
> > too, but it would be weird to report old and new value for it, since
> > its value flows with time.
> >
> > Plus, when I look at:
> > type=TIME_INJOFFSET [...]: sec=-16 nsec=124887145
> >
> > I can immediately see that the time was shifted back by 16-something
> > seconds, while when I look at something like:
> >
> > type=TIME_CHANGE [...]: var=time_sec new=1537185685 old=1537185701
> > type=TIME_CHANGE [...]: var=time_nsec new=664373417 old=789260562
> >
> > I can just see some big numbers that I need to do math with before I
> > get an idea of what is the magnitude (or sign) of the change.
>
> Okay, with that in mind, perhaps when recording the offset values we
> omit the "old" values (arguably that doesn't make much sense here) and
> keep the sec/nsec split:
>
> type=TIME_CHANGE [...]: offset_sec=<X> offset_nsec=<Y>
>
> ... and for all others we stick with:
>
> type=TIME_CHANGE [...]: ntp_<VAR>=<NEWVAL> old=<OLD_VAL>
Alright, that format would work. However, I would still like to have a
separate type for the offset injection, since it has different field
structure and semantics (difference vs. new+old). I don't see any
reason to sacrifice the distinction for just one record type slot
(AFAIK we technically still have about 2 billion left...).
(Maybe you just duplicated the record type by mistake, in that case
please disregard the last sentence.)
>
> ... and if that results in multiple TIME_CHANGE records for a given
> event, that's fine with me.
>
> > > A reminder that we need tests for these new records and a RFE page on the wiki:
> > >
> > > * https://github.com/linux-audit/audit-testsuite
> >
> > I was going to start working on this once the format issues are
> > settled. (Although I probably should have kept the RFC in the subject
> > until then...)
> >
> > > * https://github.com/linux-audit/audit-kernel/wiki
> >
> > I admit I forgot about this duty, but again I would like to wait for
> > the discussions to settle before writing that up.
>
> That is fine, do it in whatever order works best for you, just
> understand that I'm probably not going to merge patches like this
> until I see both documentation and tests.
>
> --
> paul moore
> www.paul-moore.com
--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH ghak10 v5 1/2] audit: Add functions to log time adjustments
2018-09-21 11:21 ` Ondrej Mosnacek
@ 2018-09-22 20:42 ` Paul Moore
0 siblings, 0 replies; 32+ messages in thread
From: Paul Moore @ 2018-09-22 20:42 UTC (permalink / raw)
To: omosnace
Cc: linux-audit, rgb, sgrubb, mlichvar, john.stultz, tglx, sboyd,
linux-kernel
On Fri, Sep 21, 2018 at 7:21 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Mon, Sep 17, 2018 at 4:51 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Mon, Sep 17, 2018 at 8:38 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > On Fri, Sep 14, 2018 at 5:19 AM Paul Moore <paul@paul-moore.com> wrote:
> > > > On Fri, Aug 24, 2018 at 8:00 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
...
> > Okay, with that in mind, perhaps when recording the offset values we
> > omit the "old" values (arguably that doesn't make much sense here) and
> > keep the sec/nsec split:
> >
> > type=TIME_CHANGE [...]: offset_sec=<X> offset_nsec=<Y>
> >
> > ... and for all others we stick with:
> >
> > type=TIME_CHANGE [...]: ntp_<VAR>=<NEWVAL> old=<OLD_VAL>
>
> Alright, that format would work. However, I would still like to have a
> separate type for the offset injection, since it has different field
> structure and semantics (difference vs. new+old). I don't see any
> reason to sacrifice the distinction for just one record type slot
> (AFAIK we technically still have about 2 billion left...).
>
> (Maybe you just duplicated the record type by mistake, in that case
> please disregard the last sentence.)
A reasonable guess on the typo, but no that was intentional :)
As described above, there is no set field ordering for the TIME_CHANGE
record, just like there is not set field ordering for the
CONFIG_CHANGE record. Why? We only include the state variables that
are being changed instead of including all of the available state
variables. Yes, historically there are the "new" and "old" fields,
but I don't see that as a strong convention; the special "old=" field
name helps prevent confusion.
Yes, we aren't really at risk of running out of record types, but why
do we *need* two types here? I don't believe the ordering/structure
argument is significant in this particular case, and I would much
rather see all the time related state changes included in one
TIME_CHANGE record.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2018-09-22 20:42 UTC | newest]
Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-24 11:59 [PATCH ghak10 v5 0/2] audit: Log modifying adjtimex(2) calls Ondrej Mosnacek
2018-08-24 12:00 ` [PATCH ghak10 v5 1/2] audit: Add functions to log time adjustments Ondrej Mosnacek
2018-08-24 18:33 ` John Stultz
2018-08-27 8:28 ` Ondrej Mosnacek
2018-09-13 15:54 ` Richard Guy Briggs
2018-09-17 12:33 ` Ondrej Mosnacek
2018-08-27 7:50 ` Miroslav Lichvar
2018-08-27 9:13 ` Ondrej Mosnacek
2018-08-27 16:38 ` Steve Grubb
2018-09-13 13:59 ` Ondrej Mosnacek
2018-09-13 15:14 ` Richard Guy Briggs
2018-09-17 12:32 ` Ondrej Mosnacek
2018-09-14 3:09 ` Paul Moore
2018-09-17 12:33 ` Ondrej Mosnacek
2018-09-14 3:18 ` Paul Moore
2018-09-14 15:16 ` Richard Guy Briggs
2018-09-14 15:34 ` Steve Grubb
2018-09-14 16:24 ` Richard Guy Briggs
2018-09-17 14:36 ` Paul Moore
2018-09-17 12:38 ` Ondrej Mosnacek
2018-09-17 14:20 ` Richard Guy Briggs
2018-09-17 14:50 ` Paul Moore
2018-09-21 11:21 ` Ondrej Mosnacek
2018-09-22 20:42 ` Paul Moore
2018-08-24 12:00 ` [PATCH ghak10 v5 2/2] timekeeping/ntp: Audit clock/NTP params adjustments Ondrej Mosnacek
2018-08-24 19:47 ` Richard Guy Briggs
2018-08-24 20:20 ` John Stultz
2018-08-27 11:35 ` Ondrej Mosnacek
2018-08-27 11:45 ` Miroslav Lichvar
2018-08-27 12:02 ` Ondrej Mosnacek
2018-08-27 21:42 ` Thomas Gleixner
2018-09-13 15:35 ` Richard Guy Briggs
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).