Skip to content

expose improved kernel logging features through libseccomp-golang #29

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
tyhicks wants to merge 5 commits into from
Closed

expose improved kernel logging features through libseccomp-golang #29

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7e015ff
golang: Add API level bindings
tyhicks Oct 25, 2017
05ae4c0
golang: Add support for SCMP_FLTATR_CTL_LOG
tyhicks Sep 21, 2017
503d08c
golang: Add filterAttrLog getter/setters test
tyhicks Sep 21, 2017
97bab4b
golang: Add support for SCMP_ACT_LOG
tyhicks Sep 21, 2017
a722f44
golang: Add ActLog test
tyhicks Sep 21, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 71 additions & 0 deletions seccomp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,10 @@ const (
ActTrace ScmpAction = iota
// ActAllow permits the syscall to continue execution
ActAllow ScmpAction = iota
// ActLog permits the syscall to continue execution after logging it.
// This action is only usable when libseccomp API level 3 or higher is
// supported.
ActLog ScmpAction = iota
Copy link
Contributor

@mheon mheon Oct 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you put this after ActAllow? By placing it between ActTrace and ActAllow, ActAllow's constant is renumbered versus current versions of the library

)

const (
Expand Down Expand Up @@ -295,6 +299,8 @@ func (a ScmpAction) String() string {
case ActTrace:
return fmt.Sprintf("Action: Notify tracing processes with code %d",
(a >> 16))
case ActLog:
return "Action: Log system call"
case ActAllow:
return "Action: Allow system call"
default:
Expand Down Expand Up @@ -328,6 +334,25 @@ func GetLibraryVersion() (major, minor, micro uint) {
return verMajor, verMinor, verMicro
}

// GetApi returns the API level supported by the system.
Copy link
Contributor

@mheon mheon Oct 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give a brief description of what API levels there are in the comments for this and SetAPI?

Copy link
Contributor Author

@tyhicks tyhicks Oct 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I sure can. My reasoning for not including API level descriptions in libseccomp-golang was that they may become outdated because developers could easily forget to keep the descriptions updated when new API levels are added. The seccomp_api_get man page is probably going to be the most authoritative location for API level descriptions but I don't know how you feel about pointing to that man page from the function documentation.

Copy link
Contributor

@mheon mheon Oct 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be fine with directing people to a manpage. A link to the libseccomp github (if it's documented anywhere on there) would also be fine. I'd just like to give people a pointer as to what valid API levels look like.

// Returns a positive int containing the API level, or 0 with an error if the
// API level could not be detected due to the library being older than v2.4.0.
// See the seccomp_api_get(3) man page for details on available API levels:
// https://github.com/seccomp/libseccomp/blob/master/doc/man/man3/seccomp_api_get.3
func GetApi() (uint, error) {
return getApi()
}

// SetApi forcibly sets the API level. General use of this function is strongly
// discouraged.
// Returns an error if the API level could not be set. An error is always
// returned if the library is older than v2.4.0
// See the seccomp_api_get(3) man page for details on available API levels:
// https://github.com/seccomp/libseccomp/blob/master/doc/man/man3/seccomp_api_get.3
func SetApi(api uint) error {
return setApi(api)
Copy link
Contributor

@mheon mheon Oct 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see us calling this internally (and if we do, the public version has no locking). As such, I don't think we need the internal helper - you can just move the logic out into here.

Copy link
Contributor Author

@tyhicks tyhicks Oct 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! That means that I'd have to move the cgo glue code to handle old versions of libseccomp into seccomp.go. The seccomp.go file doesn't have any cgo glue code like that currently which is why I opted to stick it into seccomp_internal.go. I'll move everything to seccomp.go.

Copy link
Contributor

@mheon mheon Oct 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is an excellent reason to keep it exactly where it is. Didn't look closely enough, sorry! If it depends on CGo in seccomp_internal.go I'm fine with keeping the helpers.

}

// Syscall functions

// GetName retrieves the name of a syscall from its number.
Expand Down Expand Up @@ -730,6 +755,30 @@ func (f *ScmpFilter) GetNoNewPrivsBit() (bool, error) {
return true, nil
}

// GetLogBit returns the current state the Log bit will be set to on the filter
// being loaded, or an error if an issue was encountered retrieving the value.
// The Log bit tells the kernel that all actions taken by the filter, with the
// exception of ActAllow, should be logged.
Copy link
Contributor

@mheon mheon Sep 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add something in the comment regarding a minimum required kernel version for this working?

Copy link
Contributor Author

@tyhicks tyhicks Sep 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is a good idea. There's, unfortunately, now a little uncertainty around which kernel release it'll be present in due to an unrelated issue. I'll update the comment as soon as there's some clarity.

Copy link
Contributor Author

@tyhicks tyhicks Oct 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the update to this PR, I've added a comment about the minimum API level required (3).

// The Log bit is only usable when libseccomp API level 3 or higher is
// supported.
func (f *ScmpFilter) GetLogBit() (bool, error) {
log, err := f.getFilterAttr(filterAttrLog)
if err != nil {
api, apiErr := getApi()
if (apiErr != nil && api == 0) || (apiErr == nil && api < 3) {
return false, fmt.Errorf("getting the log bit is only supported in libseccomp 2.4.0 and newer with API level 3 or higher")
}

return false, err
}

if log == 0 {
return false, nil
}

return true, nil
}

// SetBadArchAction sets the default action taken on a syscall for an
// architecture not in the filter, or an error if an issue was encountered
// setting the value.
Expand All @@ -756,6 +805,28 @@ func (f *ScmpFilter) SetNoNewPrivsBit(state bool) error {
return f.setFilterAttr(filterAttrNNP, toSet)
}

// SetLogBit sets the state of the Log bit, which will be applied on filter
// load, or an error if an issue was encountered setting the value.
Copy link
Contributor

@mheon mheon Sep 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, please add something about minimum kernel version where this will take effect. And/or minimum libseccomp version.

Copy link
Contributor Author

@tyhicks tyhicks Sep 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

Copy link
Contributor Author

@tyhicks tyhicks Oct 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the update to this PR, I've added a comment about the minimum API level required (3).

// The Log bit is only usable when libseccomp API level 3 or higher is
// supported.
func (f *ScmpFilter) SetLogBit(state bool) error {
var toSet C.uint32_t = 0x0

if state {
toSet = 0x1
}

err := f.setFilterAttr(filterAttrLog, toSet)
if err != nil {
api, apiErr := getApi()
if (apiErr != nil && api == 0) || (apiErr == nil && api < 3) {
return fmt.Errorf("setting the log bit is only supported in libseccomp 2.4.0 and newer with API level 3 or higher")
}
}

return err
}

// SetSyscallPriority sets a syscall's priority.
// This provides a hint to the filter generator in libseccomp about the
// importance of this syscall. High-priority syscalls are placed
Expand Down
65 changes: 64 additions & 1 deletion seccomp_internal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (

// #cgo pkg-config: libseccomp
/*
#include <errno.h>
#include <stdlib.h>
#include <seccomp.h>

Expand Down Expand Up @@ -67,16 +68,29 @@ const uint32_t C_ARCH_PPC64LE = SCMP_ARCH_PPC64LE;
const uint32_t C_ARCH_S390 = SCMP_ARCH_S390;
const uint32_t C_ARCH_S390X = SCMP_ARCH_S390X;

#ifndef SCMP_ACT_LOG
#define SCMP_ACT_LOG 0x7ffc0000U
#endif
Copy link
Contributor Author

@tyhicks tyhicks Sep 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the right thing to do here? This is to allow a new libseccomp-golang to build with an older libseccomp which doesn't have SCMP_ACT_LOG support. Is it acceptable to fail the build if the installed libseccomp is not new enough or should I leave this #define here?

Copy link
Contributor

@mheon mheon Sep 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This matches what we're been doing elsewhere - we need to support building against older libseccomp, so defining if it's undefined is necessary. So yes, this is the correct thing to do, though it's not very convenient or good practice in general.


const uint32_t C_ACT_KILL = SCMP_ACT_KILL;
const uint32_t C_ACT_TRAP = SCMP_ACT_TRAP;
const uint32_t C_ACT_ERRNO = SCMP_ACT_ERRNO(0);
const uint32_t C_ACT_TRACE = SCMP_ACT_TRACE(0);
const uint32_t C_ACT_LOG = SCMP_ACT_LOG;
const uint32_t C_ACT_ALLOW = SCMP_ACT_ALLOW;

// The libseccomp SCMP_FLTATR_CTL_LOG member of the scmp_filter_attr enum was
// added in v2.4.0
#if (SCMP_VER_MAJOR < 2) || \
(SCMP_VER_MAJOR == 2 && SCMP_VER_MINOR < 4)
#define SCMP_FLTATR_CTL_LOG _SCMP_FLTATR_MIN
#endif
Copy link
Contributor Author

@tyhicks tyhicks Sep 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar question here regarding what to do if the installed libseccomp is not new enough to know about SCMP_FLTATR_CTL_LOG.

Copy link
Contributor

@mheon mheon Sep 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also be correct.

Copy link
Contributor Author

@tyhicks tyhicks Oct 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ended up not working since SCMP_FLTATR_CTL_LOG is a member of an enum and the C preprocessor isn't able to handle enum members in ifndef conditionals. In the update to this PR, I conditionalized based on the (upcoming) libseccomp version of 2.3.3.


const uint32_t C_ATTRIBUTE_DEFAULT = (uint32_t)SCMP_FLTATR_ACT_DEFAULT;
const uint32_t C_ATTRIBUTE_BADARCH = (uint32_t)SCMP_FLTATR_ACT_BADARCH;
const uint32_t C_ATTRIBUTE_NNP = (uint32_t)SCMP_FLTATR_CTL_NNP;
const uint32_t C_ATTRIBUTE_TSYNC = (uint32_t)SCMP_FLTATR_CTL_TSYNC;
const uint32_t C_ATTRIBUTE_LOG = (uint32_t)SCMP_FLTATR_CTL_LOG;

const int C_CMP_NE = (int)SCMP_CMP_NE;
const int C_CMP_LT = (int)SCMP_CMP_LT;
Expand Down Expand Up @@ -122,6 +136,25 @@ unsigned int get_micro_version()
}
#endif

// The libseccomp API level functions were added in v2.4.0
#if (SCMP_VER_MAJOR < 2) || \
(SCMP_VER_MAJOR == 2 && SCMP_VER_MINOR < 4)
const unsigned int seccomp_api_get(void)
{
// libseccomp-golang requires libseccomp v2.2.0, at a minimum, which
// supported API level 2. However, the kernel may not support API level
// 2 constructs which are the seccomp() system call and the TSYNC
// filter flag. Return the "reserved" value of 0 here to indicate that
// proper API level support is not available in libseccomp.
return 0;
}

int seccomp_api_set(unsigned int level)
{
return -EOPNOTSUPP;
}
#endif

typedef struct scmp_arg_cmp* scmp_cast_t;

void* make_arg_cmp_array(unsigned int length)
Expand Down Expand Up @@ -159,6 +192,7 @@ const (
filterAttrActBadArch scmpFilterAttr = iota
filterAttrNNP scmpFilterAttr = iota
filterAttrTsync scmpFilterAttr = iota
filterAttrLog scmpFilterAttr = iota
)

const (
Expand All @@ -169,7 +203,7 @@ const (
archEnd ScmpArch = ArchS390X
// Comparison boundaries to check for action validity
actionStart ScmpAction = ActKill
actionEnd ScmpAction = ActAllow
actionEnd ScmpAction = ActLog
// Comparison boundaries to check for comparison operator validity
compareOpStart ScmpCompareOp = CompareNotEqual
compareOpEnd ScmpCompareOp = CompareMaskedEqual
Expand Down Expand Up @@ -201,6 +235,29 @@ func ensureSupportedVersion() error {
return nil
}

// Get the API level
func getApi() (uint, error) {
api := C.seccomp_api_get()
if api == 0 {
return 0, fmt.Errorf("API level operations are not supported")
}

return uint(api), nil
}

// Set the API level
func setApi(api uint) error {
if retCode := C.seccomp_api_set(C.uint(api)); retCode != 0 {
if syscall.Errno(-1*retCode) == syscall.EOPNOTSUPP {
return fmt.Errorf("API level operations are not supported")
}

return fmt.Errorf("could not set API level: %v", retCode)
}

return nil
}

// Filter helpers

// Filter finalizer - ensure that kernel context for filters is freed
Expand Down Expand Up @@ -466,6 +523,8 @@ func actionFromNative(a C.uint32_t) (ScmpAction, error) {
return ActErrno.SetReturnCode(int16(aTmp)), nil
case C.C_ACT_TRACE:
return ActTrace.SetReturnCode(int16(aTmp)), nil
case C.C_ACT_LOG:
return ActLog, nil
case C.C_ACT_ALLOW:
return ActAllow, nil
default:
Expand All @@ -484,6 +543,8 @@ func (a ScmpAction) toNative() C.uint32_t {
return C.C_ACT_ERRNO | (C.uint32_t(a) >> 16)
case ActTrace:
return C.C_ACT_TRACE | (C.uint32_t(a) >> 16)
case ActLog:
return C.C_ACT_LOG
case ActAllow:
return C.C_ACT_ALLOW
default:
Expand All @@ -502,6 +563,8 @@ func (a scmpFilterAttr) toNative() uint32 {
return uint32(C.C_ATTRIBUTE_NNP)
case filterAttrTsync:
return uint32(C.C_ATTRIBUTE_TSYNC)
case filterAttrLog:
return uint32(C.C_ATTRIBUTE_LOG)
default:
return 0x0
}
Expand Down
Loading