[RFC PATCH ghak10 1/3] audit: Add AUDIT_TIME_ADJUSTED record type

[RFC PATCH ghak10 1/3] audit: Add AUDIT_TIME_ADJUSTED record type

[Ondrej Mosnacek]

This auxiliary record type will be used to annotate the adjtimex SYSCALL
records with the information that the clock has been adjusted. This
record shall be emitted only when the clock is modified (including
changes that have no effect, e.g. adjust by zero offset, etc.).

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

diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 04f9bd249094..d7dab9e94932 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -114,6 +114,7 @@
 #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_ADJUSTED	1332	/* Clock adjustment event (aux for SYSCALL) */
 
 #define AUDIT_AVC		1400	/* SE Linux avc denial or grant */
 #define AUDIT_SELINUX_ERR	1401	/* Internal SE Linux Errors */
-- 
2.17.1

[RFC PATCH ghak10 2/3] audit: Add the audit_adjtime() function

[Ondrej Mosnacek]

This patch adds a new function that shall be used to log any
modification of the system's clock (via the adjtimex() syscall).

The function logs an audit record of type AUDIT_TIME_ADJUSTED with the
following fields:
* txmodes  (the 'modes' field of struct timex)
* txoffset (the 'offset' field of struct timex)
* txfreq   (the 'freq' field of struct timex)
* txmaxerr (the 'maxerror' field of struct timex)
* txesterr (the 'esterror' field of struct timex)
* txstatus (the 'status' field of struct timex)
* txconst  (the 'constant' field of struct timex)
* txsec, txusec (the 'tv_sec' and 'tv_usec' fields of the 'time' field
  of struct timex (respectively))
* txtick   (the 'tick' field of struct timex)

These fields allow to reconstruct what exactly was changed by the
adjtimex syscall and to what value(s). Note that the values reported are
taken directly from the structure as received from userspace. The
syscall handling code may do some clamping of the values internally
before actually changing the kernel variables. Also, the fact that this
record has been logged does not necessarily mean that some variable was
changed (it may have been set to the same value as the old value).

Quick overview of the 'struct timex' semantics:
  The 'modes' field is a bitmask specifying which time variables (if
  any) should be adjusted. The other fields (of those listed above)
  contain the values of the respective variables that should be set. If
  the corresponding bit is not set in the 'modes' field, then a field's
  value is not significant (it may be some garbage value passed from
  userspace).

  Note that after processing the input values from userspace, the
  handler writes (all) the current (new) internal values into the struct
  and hands it back to userspace. These values are not logged.

  Also note that 'txusec' may actually mean nanoseconds, not
  microseconds, depending on whether ADJ_NSEC is set in 'modes'.

Possible improvements:
* log only the fields that contain valid values (based on 'modes' field)
  - this is not hard to implement, but will require some non-trivial
    logic inside audit_adjtime() that will need to mirror the logic in
    ntp.c -- do we want that?
* move the conditional for logging/not logging into audit_adjtime()
  - (see also next patch in series)
  - may not be desirable due to reasons above
  - we should probably either do both this and above or neither
* log also the old values + the actual values that get set
  - this would be rather difficult to do, probably not worth it

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

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 69c78477590b..26e9db46293c 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)
@@ -353,6 +354,7 @@ 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_adjtime(const struct timex *txc);
 
 static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
 {
@@ -455,6 +457,12 @@ static inline void audit_fanotify(unsigned int response)
 		__audit_fanotify(response);
 }
 
+static inline void audit_adjtime(const struct timex *txc)
+{
+	if (!audit_dummy_context())
+		__audit_adjtime(txc);
+}
+
 extern int audit_n_rules;
 extern int audit_signals;
 #else /* CONFIG_AUDITSYSCALL */
@@ -581,6 +589,9 @@ static inline void audit_log_kern_module(char *name)
 static inline void audit_fanotify(unsigned int response)
 { }
 
+static inline void audit_adjtime(const struct timex *txc)
+{ }
+
 static inline void audit_ptrace(struct task_struct *t)
 { }
 #define audit_n_rules 0
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index ceb1c4596c51..927bf51a9968 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2422,6 +2422,16 @@ void __audit_fanotify(unsigned int response)
 		AUDIT_FANOTIFY,	"resp=%u", response);
 }
 
+void __audit_adjtime(const struct timex *txc)
+{
+	audit_log(audit_context(), GFP_KERNEL, AUDIT_TIME_ADJUSTED,
+		  "txmodes=%u txoffset=%li txfreq=%li txmaxerr=%li txesterr=%li "
+		  "txstatus=%i txconst=%li txsec=%li txusec=%li txtick=%li",
+		  txc->modes, txc->offset, txc->freq, txc->maxerror,
+		  txc->esterror, txc->status, txc->constant,
+		  (long)txc->time.tv_sec, (long)txc->time.tv_usec, txc->tick);
+}
+
 static void audit_log_task(struct audit_buffer *ab)
 {
 	kuid_t auid, uid;
-- 
2.17.1

[RFC PATCH ghak10 3/3] ntp: Audit valid attempts to adjust the clock

[Ondrej Mosnacek]

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

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 kernel/time/ntp.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index a09ded765f6c..78a01df0dbdb 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"
@@ -722,6 +723,16 @@ int __do_adjtimex(struct timex *txc, struct timespec64 *ts, s32 *time_tai)
 {
 	int result;
 
+	/* Only log audit event if the clock was changed/attempted to be changed.
+	 * Based on the logic inside timekeeping_validate_timex().
+	 * NOTE: We need to log the event before any of the fields get
+	 * overwritten by the output values (the function will not fail, so it
+	 * is OK). */
+	if (   ( (txc->modes & ADJ_ADJTIME) && !(txc->modes & ADJ_OFFSET_READONLY))
+	    || (!(txc->modes & ADJ_ADJTIME) &&   txc->modes)
+	    ||   (txc->modes & ADJ_SETOFFSET))
+		audit_adjtime(txc);
+
 	if (txc->modes & ADJ_ADJTIME) {
 		long save_adjust = time_adjust;
 
-- 
2.17.1

Re: [RFC PATCH ghak10 3/3] ntp: Audit valid attempts to adjust the clock

[Richard Guy Briggs]

On 2018-06-15 14:45, Ondrej Mosnacek wrote:
> (Intentionally not sending to the timekeeping/ntp maintainers just yet,
> let's settle on the record contents/format first.)
> 
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  kernel/time/ntp.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
> index a09ded765f6c..78a01df0dbdb 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"
> @@ -722,6 +723,16 @@ int __do_adjtimex(struct timex *txc, struct timespec64 *ts, s32 *time_tai)
>  {
>  	int result;
>  
> +	/* Only log audit event if the clock was changed/attempted to be changed.
> +	 * Based on the logic inside timekeeping_validate_timex().
> +	 * NOTE: We need to log the event before any of the fields get
> +	 * overwritten by the output values (the function will not fail, so it
> +	 * is OK). */
> +	if (   ( (txc->modes & ADJ_ADJTIME) && !(txc->modes & ADJ_OFFSET_READONLY))
> +	    || (!(txc->modes & ADJ_ADJTIME) &&   txc->modes)
> +	    ||   (txc->modes & ADJ_SETOFFSET))

Wouldn't the third condition be covered by the second?

> +		audit_adjtime(txc);
> +
>  	if (txc->modes & ADJ_ADJTIME) {
>  		long save_adjust = time_adjust;

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [RFC PATCH ghak10 3/3] ntp: Audit valid attempts to adjust the clock

[Ondrej Mosnacek]

2018-06-16 18:44 GMT+02:00 Richard Guy Briggs <rgb@redhat.com>:
> On 2018-06-15 14:45, Ondrej Mosnacek wrote:
>> (Intentionally not sending to the timekeeping/ntp maintainers just yet,
>> let's settle on the record contents/format first.)
>>
>> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
>> ---
>>  kernel/time/ntp.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
>> index a09ded765f6c..78a01df0dbdb 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"
>> @@ -722,6 +723,16 @@ int __do_adjtimex(struct timex *txc, struct timespec64 *ts, s32 *time_tai)
>>  {
>>       int result;
>>
>> +     /* Only log audit event if the clock was changed/attempted to be changed.
>> +      * Based on the logic inside timekeeping_validate_timex().
>> +      * NOTE: We need to log the event before any of the fields get
>> +      * overwritten by the output values (the function will not fail, so it
>> +      * is OK). */
>> +     if (   ( (txc->modes & ADJ_ADJTIME) && !(txc->modes & ADJ_OFFSET_READONLY))
>> +         || (!(txc->modes & ADJ_ADJTIME) &&   txc->modes)
>> +         ||   (txc->modes & ADJ_SETOFFSET))
>
> Wouldn't the third condition be covered by the second?

Not if txc->modes has ADJ_ADJTIME set, ADJ_OFFSET_READONLY unset, and
ADJ_SETOFFSET set. Then it fails the first two conditions and only
matches on the third one. I don't know if such combination can
actually occur in practice, but timekeeping_validate_timex() doesn't
reject it, so it's better to be on the safe side here.

BTW, I just realized that the logging function can be called directly
from inside the do_adjtimex() function in timekeeping.c (which is a
wrapper around __do_adjtimex()), where it probably suits better (since
this function contains the ADJ_SETOFFSET handling code and
timekeeping.c also contains the timekeeping_validate_timex() function
referenced in the comment). I will move it in v2.

>
>> +             audit_adjtime(txc);
>> +
>>       if (txc->modes & ADJ_ADJTIME) {
>>               long save_adjust = time_adjust;
>
> - RGB
>
> --
> Richard Guy Briggs <rgb@redhat.com>
> Sr. S/W Engineer, Kernel Security, Base Operating Systems
> Remote, Ottawa, Red Hat Canada
> IRC: rgb, SunRaycer
> Voice: +1.647.777.2635, Internal: (81) 32635

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

Re: [RFC PATCH ghak10 3/3] ntp: Audit valid attempts to adjust the clock

[Richard Guy Briggs]

On 2018-06-18 09:35, Ondrej Mosnacek wrote:
> 2018-06-16 18:44 GMT+02:00 Richard Guy Briggs <rgb@redhat.com>:
> > On 2018-06-15 14:45, Ondrej Mosnacek wrote:
> >> (Intentionally not sending to the timekeeping/ntp maintainers just yet,
> >> let's settle on the record contents/format first.)
> >>
> >> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> >> ---
> >>  kernel/time/ntp.c | 11 +++++++++++
> >>  1 file changed, 11 insertions(+)
> >>
> >> diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
> >> index a09ded765f6c..78a01df0dbdb 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"
> >> @@ -722,6 +723,16 @@ int __do_adjtimex(struct timex *txc, struct timespec64 *ts, s32 *time_tai)
> >>  {
> >>       int result;
> >>
> >> +     /* Only log audit event if the clock was changed/attempted to be changed.
> >> +      * Based on the logic inside timekeeping_validate_timex().
> >> +      * NOTE: We need to log the event before any of the fields get
> >> +      * overwritten by the output values (the function will not fail, so it
> >> +      * is OK). */
> >> +     if (   ( (txc->modes & ADJ_ADJTIME) && !(txc->modes & ADJ_OFFSET_READONLY))
> >> +         || (!(txc->modes & ADJ_ADJTIME) &&   txc->modes)
> >> +         ||   (txc->modes & ADJ_SETOFFSET))
> >
> > Wouldn't the third condition be covered by the second?
> 
> Not if txc->modes has ADJ_ADJTIME set, ADJ_OFFSET_READONLY unset, and
> ADJ_SETOFFSET set. Then it fails the first two conditions and only
> matches on the third one. I don't know if such combination can
> actually occur in practice, but timekeeping_validate_timex() doesn't
> reject it, so it's better to be on the safe side here.

But the second condition would trigger on ADJ_OFFSET_READONLY when
ADJ_ADJTIME is not set (which may never happen together), which includes
ADJ_SETOFFSET alone or any other flag.

Ok, for the second condition how about: txc->modes & ~(ADJ_ADJTIME | ADJ_OFFSET_READONLY)

Or better yet if you only care about ADJ_ADJTIME and ADJ_SETOFFSET, how about only:
	txc->modes & (ADJ_ADJTIME | ADJ_SETOFFSET) && txc->modes != ADJ_OFFSET_READONLY
but I don't think that is what you want.

> BTW, I just realized that the logging function can be called directly
> from inside the do_adjtimex() function in timekeeping.c (which is a
> wrapper around __do_adjtimex()), where it probably suits better (since
> this function contains the ADJ_SETOFFSET handling code and
> timekeeping.c also contains the timekeeping_validate_timex() function
> referenced in the comment). I will move it in v2.
> 
> >
> >> +             audit_adjtime(txc);
> >> +
> >>       if (txc->modes & ADJ_ADJTIME) {
> >>               long save_adjust = time_adjust;
> >
> > - 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

Re: [RFC PATCH ghak10 2/3] audit: Add the audit_adjtime() function

[Steve Grubb]

On Friday, June 15, 2018 8:45:22 AM EDT Ondrej Mosnacek wrote:
> This patch adds a new function that shall be used to log any
> modification of the system's clock (via the adjtimex() syscall).
> 
> The function logs an audit record of type AUDIT_TIME_ADJUSTED with the
> following fields:
> * txmodes  (the 'modes' field of struct timex)
> * txoffset (the 'offset' field of struct timex)
> * txfreq   (the 'freq' field of struct timex)
> * txmaxerr (the 'maxerror' field of struct timex)
> * txesterr (the 'esterror' field of struct timex)
> * txstatus (the 'status' field of struct timex)
> * txconst  (the 'constant' field of struct timex)
> * txsec, txusec (the 'tv_sec' and 'tv_usec' fields of the 'time' field
>   of struct timex (respectively))
> * txtick   (the 'tick' field of struct timex)

Are all of these fields security relevant? Primarily what we need to know is 
if time is being adjusted. This is relevant because a bad guy may adjust 
system time to make something appear to happen earlier or later than it 
really did which make correlation hard or impossible.

-Steve

> These fields allow to reconstruct what exactly was changed by the
> adjtimex syscall and to what value(s). Note that the values reported are
> taken directly from the structure as received from userspace. The
> syscall handling code may do some clamping of the values internally
> before actually changing the kernel variables. Also, the fact that this
> record has been logged does not necessarily mean that some variable was
> changed (it may have been set to the same value as the old value).
> 
> Quick overview of the 'struct timex' semantics:
>   The 'modes' field is a bitmask specifying which time variables (if
>   any) should be adjusted. The other fields (of those listed above)
>   contain the values of the respective variables that should be set. If
>   the corresponding bit is not set in the 'modes' field, then a field's
>   value is not significant (it may be some garbage value passed from
>   userspace).
> 
>   Note that after processing the input values from userspace, the
>   handler writes (all) the current (new) internal values into the struct
>   and hands it back to userspace. These values are not logged.
> 
>   Also note that 'txusec' may actually mean nanoseconds, not
>   microseconds, depending on whether ADJ_NSEC is set in 'modes'.
> 
> Possible improvements:
> * log only the fields that contain valid values (based on 'modes' field)
>   - this is not hard to implement, but will require some non-trivial
>     logic inside audit_adjtime() that will need to mirror the logic in
>     ntp.c -- do we want that?
> * move the conditional for logging/not logging into audit_adjtime()
>   - (see also next patch in series)
>   - may not be desirable due to reasons above
>   - we should probably either do both this and above or neither
> * log also the old values + the actual values that get set
>   - this would be rather difficult to do, probably not worth it
> 
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  include/linux/audit.h | 11 +++++++++++
>  kernel/auditsc.c      | 10 ++++++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 69c78477590b..26e9db46293c 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)
> @@ -353,6 +354,7 @@ 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_adjtime(const struct timex *txc);
> 
>  static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
>  {
> @@ -455,6 +457,12 @@ static inline void audit_fanotify(unsigned int
> response) __audit_fanotify(response);
>  }
> 
> +static inline void audit_adjtime(const struct timex *txc)
> +{
> +	if (!audit_dummy_context())
> +		__audit_adjtime(txc);
> +}
> +
>  extern int audit_n_rules;
>  extern int audit_signals;
>  #else /* CONFIG_AUDITSYSCALL */
> @@ -581,6 +589,9 @@ static inline void audit_log_kern_module(char *name)
>  static inline void audit_fanotify(unsigned int response)
>  { }
> 
> +static inline void audit_adjtime(const struct timex *txc)
> +{ }
> +
>  static inline void audit_ptrace(struct task_struct *t)
>  { }
>  #define audit_n_rules 0
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index ceb1c4596c51..927bf51a9968 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2422,6 +2422,16 @@ void __audit_fanotify(unsigned int response)
>  		AUDIT_FANOTIFY,	"resp=%u", response);
>  }
> 
> +void __audit_adjtime(const struct timex *txc)
> +{
> +	audit_log(audit_context(), GFP_KERNEL, AUDIT_TIME_ADJUSTED,
> +		  "txmodes=%u txoffset=%li txfreq=%li txmaxerr=%li txesterr=%li "
> +		  "txstatus=%i txconst=%li txsec=%li txusec=%li txtick=%li",
> +		  txc->modes, txc->offset, txc->freq, txc->maxerror,
> +		  txc->esterror, txc->status, txc->constant,
> +		  (long)txc->time.tv_sec, (long)txc->time.tv_usec, txc->tick);
> +}
> +
>  static void audit_log_task(struct audit_buffer *ab)
>  {
>  	kuid_t auid, uid;

Re: [RFC PATCH ghak10 3/3] ntp: Audit valid attempts to adjust the clock

[Ondrej Mosnacek]

2018-06-18 17:14 GMT+02:00 Richard Guy Briggs <rgb@redhat.com>:
> On 2018-06-18 09:35, Ondrej Mosnacek wrote:
>> 2018-06-16 18:44 GMT+02:00 Richard Guy Briggs <rgb@redhat.com>:
>> > On 2018-06-15 14:45, Ondrej Mosnacek wrote:
>> >> (Intentionally not sending to the timekeeping/ntp maintainers just yet,
>> >> let's settle on the record contents/format first.)
>> >>
>> >> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
>> >> ---
>> >>  kernel/time/ntp.c | 11 +++++++++++
>> >>  1 file changed, 11 insertions(+)
>> >>
>> >> diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
>> >> index a09ded765f6c..78a01df0dbdb 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"
>> >> @@ -722,6 +723,16 @@ int __do_adjtimex(struct timex *txc, struct timespec64 *ts, s32 *time_tai)
>> >>  {
>> >>       int result;
>> >>
>> >> +     /* Only log audit event if the clock was changed/attempted to be changed.
>> >> +      * Based on the logic inside timekeeping_validate_timex().
>> >> +      * NOTE: We need to log the event before any of the fields get
>> >> +      * overwritten by the output values (the function will not fail, so it
>> >> +      * is OK). */
>> >> +     if (   ( (txc->modes & ADJ_ADJTIME) && !(txc->modes & ADJ_OFFSET_READONLY))
>> >> +         || (!(txc->modes & ADJ_ADJTIME) &&   txc->modes)
>> >> +         ||   (txc->modes & ADJ_SETOFFSET))
>> >
>> > Wouldn't the third condition be covered by the second?
>>
>> Not if txc->modes has ADJ_ADJTIME set, ADJ_OFFSET_READONLY unset, and
>> ADJ_SETOFFSET set. Then it fails the first two conditions and only
>> matches on the third one. I don't know if such combination can
>> actually occur in practice, but timekeeping_validate_timex() doesn't
>> reject it, so it's better to be on the safe side here.
>
> But the second condition would trigger on ADJ_OFFSET_READONLY when
> ADJ_ADJTIME is not set (which may never happen together), which includes
> ADJ_SETOFFSET alone or any other flag.

I think the misunderstanding is coming from the fact that
ADJ_SETOFFSET is semantically unrelated to ADJ_OFFSET_READONLY (i.e.
ADJ_OFFSET_READONLY does not necessarily mean we are not doing the
"SETOFFSET" modification). From what I understand, it is a special
value for the case when ADJ_ADJTIME is set, to override the default
behavior which always modifies the offset. ADJ_SETOFFSET is checked in
do_adjtimex (not to be confused with __do_adjtimex...) where it
actually controls setting of some different "offset" than the "other
offset" changed by ADJ_ADJTIME/ADJ_OFFSET.

It is terribly confusing, but unfortunately it is what it is...
Curiously, ADJ_ADJTIME (and its two related flags --
ADJ_OFFSET_READONLY and ADJ_OFFSET_SINGLESHOT) are defined outside of
the UAPI, but they are not referenced in the kernel anywhere outside
of the do_adjtimex() handling code... So it might be some obsolete
functionality or perhaps an undocumented API used by glibc to
implement the adjtime(3) and/or ntp_adjtime(2) functions (these don't
seem to be separate system calls, at least in the current kernel). I
haven't looked at the glibc code though, so maybe I'm pointing to the
wrong suspect here...

Either way, I would prefer the conditions to be spelled out like this,
so that the logic is easy to compare to the
timekeeping_validate_timex() function (just see where it checks for
CAP_SYS_TIME, that's when there is a non-read-only operation
happening). Right now it is quite easy to see that the logging
condition is equivalent to the CAP_SYS_TIME checks in the validation
function.

To make things even simpler, it would be perhaps worth it to modify
the timekeeping_validate_timex() function to signal whether the
operation is readonly in the return code and use that in the
condition...

>
> Ok, for the second condition how about: txc->modes & ~(ADJ_ADJTIME | ADJ_OFFSET_READONLY)
>
> Or better yet if you only care about ADJ_ADJTIME and ADJ_SETOFFSET, how about only:
>         txc->modes & (ADJ_ADJTIME | ADJ_SETOFFSET) && txc->modes != ADJ_OFFSET_READONLY
> but I don't think that is what you want.
>
>> BTW, I just realized that the logging function can be called directly
>> from inside the do_adjtimex() function in timekeeping.c (which is a
>> wrapper around __do_adjtimex()), where it probably suits better (since
>> this function contains the ADJ_SETOFFSET handling code and
>> timekeeping.c also contains the timekeeping_validate_timex() function
>> referenced in the comment). I will move it in v2.
>>
>> >
>> >> +             audit_adjtime(txc);
>> >> +
>> >>       if (txc->modes & ADJ_ADJTIME) {
>> >>               long save_adjust = time_adjust;
>> >
>> > - 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



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

Re: [RFC PATCH ghak10 2/3] audit: Add the audit_adjtime() function

[Ondrej Mosnacek]

2018-06-18 20:39 GMT+02:00 Steve Grubb <sgrubb@redhat.com>:
> On Friday, June 15, 2018 8:45:22 AM EDT Ondrej Mosnacek wrote:
>> This patch adds a new function that shall be used to log any
>> modification of the system's clock (via the adjtimex() syscall).
>>
>> The function logs an audit record of type AUDIT_TIME_ADJUSTED with the
>> following fields:
>> * txmodes  (the 'modes' field of struct timex)
>> * txoffset (the 'offset' field of struct timex)
>> * txfreq   (the 'freq' field of struct timex)
>> * txmaxerr (the 'maxerror' field of struct timex)
>> * txesterr (the 'esterror' field of struct timex)
>> * txstatus (the 'status' field of struct timex)
>> * txconst  (the 'constant' field of struct timex)
>> * txsec, txusec (the 'tv_sec' and 'tv_usec' fields of the 'time' field
>>   of struct timex (respectively))
>> * txtick   (the 'tick' field of struct timex)
>
> Are all of these fields security relevant? Primarily what we need to know is
> if time is being adjusted. This is relevant because a bad guy may adjust
> system time to make something appear to happen earlier or later than it
> really did which make correlation hard or impossible.

This is still an open question for me... On the one hand, we might
want to know exactly what the bad guy was trying to do ("He changed
the offset to 500 ms." vs. just "He adjusted the clock."), on the
other hand, we may not really care and consider it yet another junk
data in the logs... A possible compromise could be to log only
relevant fields (see 'Possible improvements' in the commit message).
Assuming ntpd or other authorized applications would only modify
one/few variables at a time, this would add only a few fields to the
record each time.

Note that this new auxiliary record gets only logged on *modifying*
operations, which should not be that frequent, and thus it shouldn't
be a problem to output a bit of potentially useful information.

That said, I don't mind logging just an empty record if that is
preferred. At the end of the day it is up to Paul to decide what he
will accept.

>
> -Steve
>
>> These fields allow to reconstruct what exactly was changed by the
>> adjtimex syscall and to what value(s). Note that the values reported are
>> taken directly from the structure as received from userspace. The
>> syscall handling code may do some clamping of the values internally
>> before actually changing the kernel variables. Also, the fact that this
>> record has been logged does not necessarily mean that some variable was
>> changed (it may have been set to the same value as the old value).
>>
>> Quick overview of the 'struct timex' semantics:
>>   The 'modes' field is a bitmask specifying which time variables (if
>>   any) should be adjusted. The other fields (of those listed above)
>>   contain the values of the respective variables that should be set. If
>>   the corresponding bit is not set in the 'modes' field, then a field's
>>   value is not significant (it may be some garbage value passed from
>>   userspace).
>>
>>   Note that after processing the input values from userspace, the
>>   handler writes (all) the current (new) internal values into the struct
>>   and hands it back to userspace. These values are not logged.
>>
>>   Also note that 'txusec' may actually mean nanoseconds, not
>>   microseconds, depending on whether ADJ_NSEC is set in 'modes'.
>>
>> Possible improvements:
>> * log only the fields that contain valid values (based on 'modes' field)
>>   - this is not hard to implement, but will require some non-trivial
>>     logic inside audit_adjtime() that will need to mirror the logic in
>>     ntp.c -- do we want that?
>> * move the conditional for logging/not logging into audit_adjtime()
>>   - (see also next patch in series)
>>   - may not be desirable due to reasons above
>>   - we should probably either do both this and above or neither
>> * log also the old values + the actual values that get set
>>   - this would be rather difficult to do, probably not worth it
>>
>> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
>> ---
>>  include/linux/audit.h | 11 +++++++++++
>>  kernel/auditsc.c      | 10 ++++++++++
>>  2 files changed, 21 insertions(+)
>>
>> diff --git a/include/linux/audit.h b/include/linux/audit.h
>> index 69c78477590b..26e9db46293c 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)
>> @@ -353,6 +354,7 @@ 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_adjtime(const struct timex *txc);
>>
>>  static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
>>  {
>> @@ -455,6 +457,12 @@ static inline void audit_fanotify(unsigned int
>> response) __audit_fanotify(response);
>>  }
>>
>> +static inline void audit_adjtime(const struct timex *txc)
>> +{
>> +     if (!audit_dummy_context())
>> +             __audit_adjtime(txc);
>> +}
>> +
>>  extern int audit_n_rules;
>>  extern int audit_signals;
>>  #else /* CONFIG_AUDITSYSCALL */
>> @@ -581,6 +589,9 @@ static inline void audit_log_kern_module(char *name)
>>  static inline void audit_fanotify(unsigned int response)
>>  { }
>>
>> +static inline void audit_adjtime(const struct timex *txc)
>> +{ }
>> +
>>  static inline void audit_ptrace(struct task_struct *t)
>>  { }
>>  #define audit_n_rules 0
>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> index ceb1c4596c51..927bf51a9968 100644
>> --- a/kernel/auditsc.c
>> +++ b/kernel/auditsc.c
>> @@ -2422,6 +2422,16 @@ void __audit_fanotify(unsigned int response)
>>               AUDIT_FANOTIFY, "resp=%u", response);
>>  }
>>
>> +void __audit_adjtime(const struct timex *txc)
>> +{
>> +     audit_log(audit_context(), GFP_KERNEL, AUDIT_TIME_ADJUSTED,
>> +               "txmodes=%u txoffset=%li txfreq=%li txmaxerr=%li txesterr=%li "
>> +               "txstatus=%i txconst=%li txsec=%li txusec=%li txtick=%li",
>> +               txc->modes, txc->offset, txc->freq, txc->maxerror,
>> +               txc->esterror, txc->status, txc->constant,
>> +               (long)txc->time.tv_sec, (long)txc->time.tv_usec, txc->tick);
>> +}
>> +
>>  static void audit_log_task(struct audit_buffer *ab)
>>  {
>>       kuid_t auid, uid;
>
>
>
>



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

Re: [RFC PATCH ghak10 3/3] ntp: Audit valid attempts to adjust the clock

[Richard Guy Briggs]

On 2018-06-19 10:30, Ondrej Mosnacek wrote:
> 2018-06-18 17:14 GMT+02:00 Richard Guy Briggs <rgb@redhat.com>:
> > On 2018-06-18 09:35, Ondrej Mosnacek wrote:
> >> 2018-06-16 18:44 GMT+02:00 Richard Guy Briggs <rgb@redhat.com>:
> >> > On 2018-06-15 14:45, Ondrej Mosnacek wrote:
> >> >> (Intentionally not sending to the timekeeping/ntp maintainers just yet,
> >> >> let's settle on the record contents/format first.)
> >> >>
> >> >> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> >> >> ---
> >> >>  kernel/time/ntp.c | 11 +++++++++++
> >> >>  1 file changed, 11 insertions(+)
> >> >>
> >> >> diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
> >> >> index a09ded765f6c..78a01df0dbdb 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"
> >> >> @@ -722,6 +723,16 @@ int __do_adjtimex(struct timex *txc, struct timespec64 *ts, s32 *time_tai)
> >> >>  {
> >> >>       int result;
> >> >>
> >> >> +     /* Only log audit event if the clock was changed/attempted to be changed.
> >> >> +      * Based on the logic inside timekeeping_validate_timex().
> >> >> +      * NOTE: We need to log the event before any of the fields get
> >> >> +      * overwritten by the output values (the function will not fail, so it
> >> >> +      * is OK). */
> >> >> +     if (   ( (txc->modes & ADJ_ADJTIME) && !(txc->modes & ADJ_OFFSET_READONLY))
> >> >> +         || (!(txc->modes & ADJ_ADJTIME) &&   txc->modes)
> >> >> +         ||   (txc->modes & ADJ_SETOFFSET))
> >> >
> >> > Wouldn't the third condition be covered by the second?
> >>
> >> Not if txc->modes has ADJ_ADJTIME set, ADJ_OFFSET_READONLY unset, and
> >> ADJ_SETOFFSET set. Then it fails the first two conditions and only
> >> matches on the third one. I don't know if such combination can
> >> actually occur in practice, but timekeeping_validate_timex() doesn't
> >> reject it, so it's better to be on the safe side here.
> >
> > But the second condition would trigger on ADJ_OFFSET_READONLY when
> > ADJ_ADJTIME is not set (which may never happen together), which includes
> > ADJ_SETOFFSET alone or any other flag.
> 
> I think the misunderstanding is coming from the fact that
> ADJ_SETOFFSET is semantically unrelated to ADJ_OFFSET_READONLY (i.e.
> ADJ_OFFSET_READONLY does not necessarily mean we are not doing the
> "SETOFFSET" modification). From what I understand, it is a special
> value for the case when ADJ_ADJTIME is set, to override the default
> behavior which always modifies the offset. ADJ_SETOFFSET is checked in
> do_adjtimex (not to be confused with __do_adjtimex...) where it
> actually controls setting of some different "offset" than the "other
> offset" changed by ADJ_ADJTIME/ADJ_OFFSET.

Ok, fair enough, I'd figured they'd be related.  I suspect that its
logic is incomplete with respect to what interests us, in particular,
other exclusions that don't actually modify time.

> It is terribly confusing, but unfortunately it is what it is...
> Curiously, ADJ_ADJTIME (and its two related flags --
> ADJ_OFFSET_READONLY and ADJ_OFFSET_SINGLESHOT) are defined outside of
> the UAPI, but they are not referenced in the kernel anywhere outside
> of the do_adjtimex() handling code... So it might be some obsolete
> functionality or perhaps an undocumented API used by glibc to
> implement the adjtime(3) and/or ntp_adjtime(2) functions (these don't
> seem to be separate system calls, at least in the current kernel). I
> haven't looked at the glibc code though, so maybe I'm pointing to the
> wrong suspect here...

I did notice they were not user-visible like the rest.

> Either way, I would prefer the conditions to be spelled out like this,
> so that the logic is easy to compare to the
> timekeeping_validate_timex() function (just see where it checks for
> CAP_SYS_TIME, that's when there is a non-read-only operation
> happening). Right now it is quite easy to see that the logging
> condition is equivalent to the CAP_SYS_TIME checks in the validation
> function.

If that is the case, then that is a good argument for your last patch so
that the audit logic is updated when that function's logic is updated.

> To make things even simpler, it would be perhaps worth it to modify
> the timekeeping_validate_timex() function to signal whether the
> operation is readonly in the return code and use that in the
> condition...

Agreed.

> > Ok, for the second condition how about: txc->modes & ~(ADJ_ADJTIME | ADJ_OFFSET_READONLY)
> >
> > Or better yet if you only care about ADJ_ADJTIME and ADJ_SETOFFSET, how about only:
> >         txc->modes & (ADJ_ADJTIME | ADJ_SETOFFSET) && txc->modes != ADJ_OFFSET_READONLY
> > but I don't think that is what you want.
> >
> >> BTW, I just realized that the logging function can be called directly
> >> from inside the do_adjtimex() function in timekeeping.c (which is a
> >> wrapper around __do_adjtimex()), where it probably suits better (since
> >> this function contains the ADJ_SETOFFSET handling code and
> >> timekeeping.c also contains the timekeeping_validate_timex() function
> >> referenced in the comment). I will move it in v2.
> >>
> >> >
> >> >> +             audit_adjtime(txc);
> >> >> +
> >> >>       if (txc->modes & ADJ_ADJTIME) {
> >> >>               long save_adjust = time_adjust;
> >> >
> >> > - 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
> 
> 
> 
> -- 
> 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

Re: [RFC PATCH ghak10 2/3] audit: Add the audit_adjtime() function

[Steve Grubb]

On Tuesday, June 19, 2018 4:57:37 AM EDT Ondrej Mosnacek wrote:
> 2018-06-18 20:39 GMT+02:00 Steve Grubb <sgrubb@redhat.com>:
> > On Friday, June 15, 2018 8:45:22 AM EDT Ondrej Mosnacek wrote:
> >> This patch adds a new function that shall be used to log any
> >> modification of the system's clock (via the adjtimex() syscall).
> >> 
> >> The function logs an audit record of type AUDIT_TIME_ADJUSTED with the
> >> following fields:
> >> * txmodes  (the 'modes' field of struct timex)
> >> * txoffset (the 'offset' field of struct timex)
> >> * txfreq   (the 'freq' field of struct timex)
> >> * txmaxerr (the 'maxerror' field of struct timex)
> >> * txesterr (the 'esterror' field of struct timex)
> >> * txstatus (the 'status' field of struct timex)
> >> * txconst  (the 'constant' field of struct timex)
> >> * txsec, txusec (the 'tv_sec' and 'tv_usec' fields of the 'time' field
> >> of struct timex (respectively))
> >> * txtick   (the 'tick' field of struct timex)
> > 
> > Are all of these fields security relevant? Primarily what we need to know
> > is if time is being adjusted. This is relevant because a bad guy may
> > adjust system time to make something appear to happen earlier or later
> > than it really did which make correlation hard or impossible.
> 
> This is still an open question for me... On the one hand, we might
> want to know exactly what the bad guy was trying to do ("He changed
> the offset to 500 ms." vs. just "He adjusted the clock."), on the
> other hand, we may not really care and consider it yet another junk
> data in the logs... A possible compromise could be to log only
> relevant fields (see 'Possible improvements' in the commit message).
> Assuming ntpd or other authorized applications would only modify
> one/few variables at a time, this would add only a few fields to the
> record each time.

I think we want the modes field so that we know what was changed. But do we 
really need to know maxerror or status? I think we should limit this to the 
modes and any value of a time adjustment. 

> Note that this new auxiliary record gets only logged on *modifying*
> operations, which should not be that frequent, and thus it shouldn't
> be a problem to output a bit of potentially useful information.

We're after just security information.

> That said, I don't mind logging just an empty record if that is
> preferred.

An empty buffer doesn't tell us what was adjusted.

Thanks,
-Steve

Re: [RFC PATCH ghak10 2/3] audit: Add the audit_adjtime() function

[Lenny Bruzenak]

[-- Attachment #1.1: Type: text/plain, Size: 578 bytes --]

On 06/19/2018 03:52 PM, Steve Grubb wrote:

> I think we want the modes field so that we know what was changed. But do we 
> really need to know maxerror or status? I think we should limit this to the 
> modes and any value of a time adjustment. 
>
>> Note that this new auxiliary record gets only logged on *modifying*
>> operations, which should not be that frequent, and thus it shouldn't
>> be a problem to output a bit of potentially useful information.
> We're after just security information.

+1. Sometimes more isn't merrier.
:-)
 
LCB

-- 
Lenny Bruzenak
MagitekLTD


[-- Attachment #1.2: Type: text/html, Size: 1208 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC PATCH ghak10 2/3] audit: Add the audit_adjtime() function

[Ondrej Mosnacek]

ut 19. 6. 2018 o 22:52 Steve Grubb <sgrubb@redhat.com> napísal(a):
> On Tuesday, June 19, 2018 4:57:37 AM EDT Ondrej Mosnacek wrote:
> > 2018-06-18 20:39 GMT+02:00 Steve Grubb <sgrubb@redhat.com>:
> > > On Friday, June 15, 2018 8:45:22 AM EDT Ondrej Mosnacek wrote:
> > >> This patch adds a new function that shall be used to log any
> > >> modification of the system's clock (via the adjtimex() syscall).
> > >>
> > >> The function logs an audit record of type AUDIT_TIME_ADJUSTED with the
> > >> following fields:
> > >> * txmodes  (the 'modes' field of struct timex)
> > >> * txoffset (the 'offset' field of struct timex)
> > >> * txfreq   (the 'freq' field of struct timex)
> > >> * txmaxerr (the 'maxerror' field of struct timex)
> > >> * txesterr (the 'esterror' field of struct timex)
> > >> * txstatus (the 'status' field of struct timex)
> > >> * txconst  (the 'constant' field of struct timex)
> > >> * txsec, txusec (the 'tv_sec' and 'tv_usec' fields of the 'time' field
> > >> of struct timex (respectively))
> > >> * txtick   (the 'tick' field of struct timex)
> > >
> > > Are all of these fields security relevant? Primarily what we need to know
> > > is if time is being adjusted. This is relevant because a bad guy may
> > > adjust system time to make something appear to happen earlier or later
> > > than it really did which make correlation hard or impossible.
> >
> > This is still an open question for me... On the one hand, we might
> > want to know exactly what the bad guy was trying to do ("He changed
> > the offset to 500 ms." vs. just "He adjusted the clock."), on the
> > other hand, we may not really care and consider it yet another junk
> > data in the logs... A possible compromise could be to log only
> > relevant fields (see 'Possible improvements' in the commit message).
> > Assuming ntpd or other authorized applications would only modify
> > one/few variables at a time, this would add only a few fields to the
> > record each time.
>
> I think we want the modes field so that we know what was changed. But do we
> really need to know maxerror or status? I think we should limit this to the
> modes and any value of a time adjustment.

Well, the status field can be at the very least used to set the
STA_INS/STA_DEL status flags [1], which seem to coordinate the
insertion of leap second. Now this may be a bit far-fetched, but I can
imagine how bogus insertions of leap seconds could be used to
gradually shift the clock to a bad value. I think we can safely drop
maxerror/esterror (I believe these are just informational of how much
the clock is 'out of sync'), but the rest seems like it could at least
indirectly influence the time (IOW, I wasn't able to find a proof that
it can't...).

[1] https://elixir.bootlin.com/linux/v4.17/source/include/uapi/linux/timex.h#L128

>
> > Note that this new auxiliary record gets only logged on *modifying*
> > operations, which should not be that frequent, and thus it shouldn't
> > be a problem to output a bit of potentially useful information.
>
> We're after just security information.
>
> > That said, I don't mind logging just an empty record if that is
> > preferred.
>
> An empty buffer doesn't tell us what was adjusted.

Yes, sorry, that should have been "a record with only modes field".

>
> Thanks,
> -Steve

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

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit