DTrace Audit Provider Prototype
ClosedPublic

Authored by rwatson on Mar 27 2017, 11:55 AM.

Details

Summary

Candidate patch to merge the prototype DTrace audit provider (dtaudit).
Proposed commit message:

Merge support for an experiment DTrace audit provider, which allows users
of DTrace to instrument security event auditing rather than relying on
conventional BSM trail files or audit pipes:

  • Add a set of per-event 'commit' probes, which provide access to particular auditable events at the time of commit in system-call return. These probes gain access to audit data via the in-kernel audit_record data structure.
  • Add a set of per-event 'bsm' probes, which provide access to particular auditable events at the time of BSM record generation in the audit worker thread. These probes have access to the in-kernel audit_record data structure and BSM representation as would be written to a trail file or audit pipe.

DTrace probe arguments consist of the name of the audit event (to support
future mechanisms of instrumenting multiple events via a single probe --
e.g., using classes), a pointer to the in-kernel audit record, and an
optional pointer to the BSM data and its length. For human convenience,
upper-case audit event names (AUE_...) are converted to lower case in
DTrace.

DTrace scripts can now cause additional audit-based data to be collected
on system calls, and inspect internal and BSM representations of the data.
They do not affect data captured in the audit trail or audit pipes
configured in the system. auditd(8) must be configured and running in
order to provide a database of event information, as well as other audit
configuration parameters (e.g., to capture command-line arguments or
environmental variables) for the provider to operate.

Sponsored by: DARPA, AFRL
MFC after: 3 weeks

Test Plan

Used extensively by the DARPA CADETS project at BAE/Cambridge/Memorial, and
also exercised in the DARPA TC testbed by various teams.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
rwatson created this revision.Mar 27 2017, 11:55 AM
Herald added 1 blocking reviewer(s): gnn. · View Herald TranscriptMar 27 2017, 11:55 AM
Herald added a subscriber: imp. · View Herald Transcript
rwatson updated this revision to Diff 26689.Mar 27 2017, 12:42 PM

Minor update: remove unneeded #include that snuck in.

gnn accepted this revision.Mar 28 2017, 11:36 PM
This revision is now accepted and ready to land.Mar 28 2017, 11:36 PM
markj accepted this revision.Mar 29 2017, 1:46 AM
markj added a subscriber: markj.
markj added inline comments.
sys/security/audit/audit_dtrace.c
128 ↗(On Diff #26689)

It looks like these can be const.

133 ↗(On Diff #26689)

Any reason not to use a C99 initializer?

497 ↗(On Diff #26689)

We should probably be unregistering the provider here.

530 ↗(On Diff #26689)

The module name needs to be updated.

sys/security/audit/audit_private.h
322 ↗(On Diff #26689)

Is this needed? I don't see any references to "evname_elem" before its definition.

Hi-ho,

Some of these questions may not make sense to those thoroughly versed in DTrace, but I'm new in this corner of the world. :)

Jon

sys/conf/files
4593 ↗(On Diff #26689)

This may be a bit of an elementary question, but is this line expressing that audit_dtrace.c is compiled when either or both dtaudit and audit are set? Will this correspond to an option called DTAUDIT alongside AUDIT from sys/conf/options? If so, should there be entries in sys/conf/NOTES and sys/conf/options as well?

Is the | dtraceall [...] part of the line declaring a dependency? Sorry again for a very basic question, but I haven't yet managed to find the documentation for this file's syntax (it's not in config(5) or config(8)).

sys/security/audit/audit.c
3 ↗(On Diff #26689)

And 2017 at all?

622 ↗(On Diff #26689)

I don't believe that the name ene has been used in this function: should the comment refer to dtaudit_state instead?

634 ↗(On Diff #26689)

allocated -> allocate?

638 ↗(On Diff #26689)

Ditto re: ene... perhaps referencing evname_elem would make sense here?

Is evname_elem likely to grow a reference count in the future? If so, might it make sense to introduce an EVNAME_PUT that #ifdefs away to nothing for now but will help prevent future foot-shooting? Perhaps not...

sys/security/audit/audit_dtrace.c
51 ↗(On Diff #26689)

Surely someone will rant about two (TWO!) blank lines here. :)

115 ↗(On Diff #26689)

It seems perhaps a bit early for DTRACE_STABILITY_STABLE... would DTRACE_STABILITY_EVOLVING or even DTRACE_STABILITY_UNSTABLE make sense?

237 ↗(On Diff #26689)

I suppose a DTrace script might want to inspect the commit flags? What's the cost of doing the pointer indirection in DTrace instead of this C code?

Also, what is a struct uthread? I don't see it anywhere under sys or include...

Thanks for these reviews; will make various changes and update the patch soon.

sys/conf/files
4593 ↗(On Diff #26689)

I've never really understood this syntax, and lifted it from other DTrace parts. There likely should be a NOTES entry. I'm not sure an options entry is required, as one appears not to be present for dtnfscl? If someone has a specific suggestion on how to improve the above I'm happy to do so -- but also find it quite opaque.

sys/security/audit/audit.c
3 ↗(On Diff #26689)

This code was written and distributed via GitHub in 2016, hence the date. If I make substantive changes (e.g., during review) I will be sure to update it :-).

622 ↗(On Diff #26689)

This reflects some evolution of the code towards trying to hide dtaudit details from the audit framework here. I'll update the comment.

634 ↗(On Diff #26689)

Will fix, thanks!

638 ↗(On Diff #26689)

I think it will, but I'm not quite sure how soon. I'm not sure I want to put in placeholders though, as they will probably prove to be incorrect. But keeping the comment here to warn future comers is definitely useful.

sys/security/audit/audit_dtrace.c
51 ↗(On Diff #26689)

Surely me if it were anyone else's patch :-).

115 ↗(On Diff #26689)

Will fix, thanks!

128 ↗(On Diff #26689)

Will fix, thanks!

133 ↗(On Diff #26689)

This was lifted from existing DTrace code style elsewhere -- but more than happy to upgrade.

237 ↗(On Diff #26689)

Nice catch -- this is a long-present "hanger on" from XNU, where there's a struct uthread. I'll fix this in a separate commit, however, so as not to entangle it with dtaudit.

497 ↗(On Diff #26689)

Indeed -- will fix!

530 ↗(On Diff #26689)

Will fix, thanks.

sys/security/audit/audit_private.h
322 ↗(On Diff #26689)

Left over from a prior iteration when I exposed that k_dtaudit_state was of a specific type; will fix.

rwatson added inline comments.Mar 29 2017, 10:31 AM
sys/conf/files
4593 ↗(On Diff #26689)

For whatever reason, it looks like DTrace modules generally don't appear in NOTES -- hence my not adding it after grepping around for things that modules normally do. This probably ought to be fixed more generally, but perhaps as a separate commit?

rwatson added inline comments.Mar 29 2017, 12:35 PM
sys/security/audit/audit_dtrace.c
128 ↗(On Diff #26689)

Unfortunately, I now remember why these can't be const: dtrace_probe_lookup() takes a non-const string argument for the probe name:

/usr/home/robert/freebsd/svncommit/base/head/sys/security/audit/audit_dtrace.c:380:39: error: 
      passing 'const char *' to parameter of type 'char *' discards qualifiers
      [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
            (dtrace_probe_lookup(dtaudit_id, dtaudit_module_str,
                                             ^~~~~~~~~~~~~~~~~~
/usr/home/robert/freebsd/svncommit/base/head/sys/cddl/contrib/opensolaris/uts/common/sys/dtrace.h:2151:68: note: 
      passing argument to parameter here
extern dtrace_id_t dtrace_probe_lookup(dtrace_provider_id_t, char *,
                                                                   ^

This is worth fixing .. but in a separate commit.

rwatson updated this revision to Diff 26768.EditedMar 29 2017, 12:59 PM

Address a number of reviewer comments from @jonathan, @markj for dtaudit.

This revision now requires review to proceed.Mar 29 2017, 12:59 PM
rwatson marked 14 inline comments as done.Mar 29 2017, 1:01 PM
rwatson added inline comments.Mar 29 2017, 1:19 PM
sys/security/audit/audit_dtrace.c
115 ↗(On Diff #26689)

This unfortunately proves to be a bad idea, as marking a probe as unstable leads to it being unusable via default DTrace command-line incantations:

robert@lemongrass-freebsd64:~/cadets/freebsd> sudo dtrace -n 'audit:::commit { trace(args[0]); }'
dtrace: invalid probe specifier audit:::commit { trace(args[0]); }: in action list: args[ ] may not be referenced because probe description audit:::commit matches an unstable set of probes

I will revert the change in an updated patch.

rwatson updated this revision to Diff 26779.Mar 29 2017, 3:20 PM

Restore higher stability level for DTrace probes, as otherwise the
DTrace command-line tool will reject use of the probes in its default
mode.

We are probably now at a commit candidate, if various reviewers wouldn't mind checking that they are happy with how the patch has ended up looking?

emaste added a subscriber: emaste.Mar 29 2017, 4:28 PM
markj accepted this revision.Mar 29 2017, 4:31 PM
markj added inline comments.
sys/security/audit/audit_dtrace.c
128 ↗(On Diff #26689)

Oh, right. This causes pain elsewhere and is indeed worth fixing.

This revision is now accepted and ready to land.Mar 29 2017, 4:31 PM
gnn accepted this revision.Mar 29 2017, 6:01 PM
This revision was automatically updated to reflect the committed changes.