Re: [PATCH ghak10 v5 1/2] audit: Add functions to log time adjustments

[Ondrej Mosnacek]

On Mon, Sep 17, 2018 at 4:51 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> On Mon, Sep 17, 2018 at 8:38 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> >
> > On Fri, Sep 14, 2018 at 5:19 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > > On Fri, Aug 24, 2018 at 8:00 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> 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.