Page MenuHomeFreeBSD

DTrace Audit Provider Prototype
ClosedPublic

Authored by rwatson on Mar 27 2017, 11:55 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mar 18 2024, 7:57 PM
Unknown Object (File)
Mar 18 2024, 7:38 PM
Unknown Object (File)
Mar 18 2024, 3:08 AM
Unknown Object (File)
Mar 18 2024, 3:08 AM
Unknown Object (File)
Mar 18 2024, 3:08 AM
Unknown Object (File)
Mar 18 2024, 3:08 AM
Unknown Object (File)
Mar 18 2024, 3:08 AM
Unknown Object (File)
Mar 18 2024, 3:08 AM
Subscribers

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

Event Timeline

Minor update: remove unneeded #include that snuck in.

This revision is now accepted and ready to land.Mar 28 2017, 11:36 PM
markj added a subscriber: markj.
markj added inline comments.
sys/security/audit/audit_dtrace.c
129

It looks like these can be const.

134

Any reason not to use a C99 initializer?

498

We should probably be unregistering the provider here.

531

The module name needs to be updated.

sys/security/audit/audit_private.h
322

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
4596

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

And 2017 at all?

622

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

634

allocated -> allocate?

638

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
52

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

116

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

238

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
4596

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

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

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

634

Will fix, thanks!

638

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
52

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

116

Will fix, thanks!

129

Will fix, thanks!

134

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

238

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.

498

Indeed -- will fix!

531

Will fix, thanks.

sys/security/audit/audit_private.h
322

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

sys/conf/files
4596

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?

sys/security/audit/audit_dtrace.c
129

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 edited edge metadata.

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

This revision now requires review to proceed.Mar 29 2017, 12:59 PM
sys/security/audit/audit_dtrace.c
116

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.

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?

markj added inline comments.
sys/security/audit/audit_dtrace.c
129

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
This revision was automatically updated to reflect the committed changes.