* [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; 35+ 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] 35+ 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; 35+ 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] 35+ 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; 35+ 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] 35+ 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; 35+ 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] 35+ 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; 35+ 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] 35+ 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; 35+ 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] 35+ 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; 35+ 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] 35+ 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; 35+ 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] 35+ 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 0 siblings, 0 replies; 35+ 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] 35+ messages in thread
* Re: [PATCH ghak10 v5 1/2] audit: Add functions to log time adjustments @ 2018-08-27 16:38 ` Steve Grubb 0 siblings, 0 replies; 35+ messages in thread From: Steve Grubb @ 2018-08-27 16:38 UTC (permalink / raw) To: Ondrej Mosnacek Cc: Richard Guy Briggs, Linux kernel mailing list, Stephen Boyd, Miroslav Lichvar, Linux-Audit Mailing List, John Stultz, Thomas Gleixner 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] 35+ 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 -1 siblings, 2 replies; 35+ 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] 35+ 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; 35+ 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] 35+ 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; 35+ 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] 35+ 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; 35+ 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] 35+ 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; 35+ 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] 35+ 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; 35+ 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] 35+ 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; 35+ 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] 35+ 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; 35+ 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] 35+ 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; 35+ 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] 35+ 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; 35+ 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] 35+ 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; 35+ 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] 35+ 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; 35+ 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] 35+ 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; 35+ 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] 35+ 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; 35+ 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] 35+ 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; 35+ 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] 35+ messages in thread
* Re: [PATCH ghak10 v5 1/2] audit: Add functions to log time adjustments @ 2018-09-22 20:42 ` Paul Moore 0 siblings, 0 replies; 35+ messages in thread From: Paul Moore @ 2018-09-22 20:42 UTC (permalink / raw) To: omosnace Cc: rgb, linux-kernel, sboyd, mlichvar, linux-audit, john.stultz, tglx 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] 35+ 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; 35+ 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] 35+ 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; 35+ 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] 35+ 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; 35+ 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] 35+ messages in thread
* Re: [PATCH ghak10 v5 2/2] timekeeping/ntp: Audit clock/NTP params adjustments @ 2018-08-24 20:20 ` John Stultz 0 siblings, 0 replies; 35+ 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] 35+ 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; 35+ 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] 35+ 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; 35+ 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] 35+ 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; 35+ 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] 35+ 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; 35+ 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] 35+ 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; 35+ 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] 35+ messages in thread
end of thread, other threads:[~2018-09-22 20:42 UTC | newest] Thread overview: 35+ 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-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-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-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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.