by Kees Cook

On 08/04/2017 05:24 PM, Tyler Hicks wrote:
> On 08/03/2017 11:33 AM, Kees Cook wrote:
>> On Fri, Jul 28, 2017 at 1:55 PM, Tyler Hicks <[email protected]> wrote:
>>> Adminstrators can write to this sysctl to set the seccomp actions that
>>> are allowed to be logged. Any actions not found in this sysctl will not
>>> be logged.
>>>
>>> For example, all SECCOMP_RET_KILL, SECCOMP_RET_TRAP, and
>>> SECCOMP_RET_ERRNO actions would be loggable if "kill trap errno" were
>>> written to the sysctl. SECCOMP_RET_TRACE actions would not be logged
>>> since its string representation ("trace") wasn't present in the sysctl
>>> value.
>>
>> Just to make sure I'm clear on this, the key word above is "loggable",
>> in that filters requesting logging will be seen.
>>
>> i.e. at the end of the series, the final state of "will it be logged?" is:
>>
>> if action==RET_ALLOW:
>> do not log
>> else if action==RET_KILL || audit-enabled:
>> log
>> else if filter-requests-logging && action in actions_logged:
>> log
>> else:
>> do not log
>
> Not quite. You didn't mention RET_LOG, RET_KILL (and RET_LOG) can be
> quieted by the admin, and the audit behavior is different. It is like this:
>
> if action == RET_ALLOW:
> do not log
> else if action == RET_KILL && RET_KILL in actions_logged:
> log
> else if action == RET_LOG && RET_LOG in actions_logged:
> log
> else if filter-requests-logging && action in actions_logged:
> log
> else if audit_enabled && process-is-being-audited:
> log
> else:
> do not log
>
> Writing that up made me realize that there is a behavior change that my
> patch set introduces when the process is being audited. Before my patch
> set, this was the behavior:
>
> ...
> else if action == RET_KILL && audit_enabled && process-is-being-audited:
> log
> ...
>
> Now it is:
>
> ...
> else if audit_enabled && process-is-being-audited:
> log
> ...
>
> Should I go back to only logging RET_KILL actions in that situation?

After going back and manually testing, I was wrong about this. There's
no change in behavior for tasks that are being audited. The original
behavior is preserved and I even say that in this patch's commit
message. Sorry for the noise here.

Tyler

>
>>
>>> The path to the sysctl is:
>>>
>>> /proc/sys/kernel/seccomp/actions_logged
>>>
>>> The actions_avail sysctl can be read to discover the valid action names
>>> that can be written to the actions_logged sysctl with the exception of
>>> SECCOMP_RET_ALLOW. It cannot be configured for logging.
>>>
>>> The default setting for the sysctl is to allow all actions to be logged
>>> except SECCOMP_RET_ALLOW.
>>>
>>> There's one important exception to this sysctl. If a task is
>>> specifically being audited, meaning that an audit context has been
>>> allocated for the task, seccomp will log all actions other than
>>> SECCOMP_RET_ALLOW despite the value of actions_logged. This exception
>>> preserves the existing auditing behavior of tasks with an allocated
>>> audit context.
>>>
>>> Signed-off-by: Tyler Hicks <[email protected]>
>>> ---
>>>
>>> * Changes since v4:
>>> - the sysctl is now a list of actions that are allowed by the admin to be
>>> logged rather than a list of actions that should be logged
>>> + a follow up patch will let applications have a say in what should be
>>> logged but the admin has the final say with this sysctl
>>> + RET_ALLOW cannot be allowed to be logged
>>> - fix comment style
>>> - mark the seccomp_action_names array as const
>>> - adjust for new reStructuredText format
>>>
>>> Documentation/userspace-api/seccomp_filter.rst | 18 +++
>>> include/linux/audit.h | 6 +-
>>> kernel/seccomp.c | 180 ++++++++++++++++++++++++-
>>> 3 files changed, 196 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/Documentation/userspace-api/seccomp_filter.rst b/Documentation/userspace-api/seccomp_filter.rst
>>> index 35fc7cb..2d1d8ab 100644
>>> --- a/Documentation/userspace-api/seccomp_filter.rst
>>> +++ b/Documentation/userspace-api/seccomp_filter.rst
>>> @@ -187,6 +187,24 @@ directory. Here's a description of each file in that directory:
>>> program was built, differs from the set of actions actually
>>> supported in the current running kernel.
>>>
>>> +``actions_logged``:
>>> + A read-write ordered list of seccomp return values (refer to the
>>> + ``SECCOMP_RET_*`` macros above) that are allowed to be logged. Writes
>>> + to the file do not need to be in ordered form but reads from the file
>>> + will be ordered in the same way as the actions_avail sysctl.
>>> +
>>> + It is important to note that the value of ``actions_logged`` does not
>>> + prevent certain actions from being logged when the audit subsystem is
>>> + configured to audit a task. If the action is not found in
>>> + ``actions_logged`` list, the final decision on whether to audit the
>>> + action for that task is ultimately left up to the audit subsystem to
>>> + decide for all seccomp return values other than ``SECCOMP_RET_ALLOW``.
>>> +
>>> + The ``allow`` string is not accepted in the ``actions_logged`` sysctl
>>> + as it is not possible to log ``SECCOMP_RET_ALLOW`` actions. Attempting
>>> + to write ``allow`` to the sysctl will result in an EINVAL being
>>> + returned.
>>> +
>>> Adding architecture support
>>> ===========================
>>>
>>> diff --git a/include/linux/audit.h b/include/linux/audit.h
>>> index 2150bdc..8c30f06 100644
>>> --- a/include/linux/audit.h
>>> +++ b/include/linux/audit.h
>>> @@ -314,11 +314,7 @@ void audit_core_dumps(long signr);
>>>
>>> static inline void audit_seccomp(unsigned long syscall, long signr, int code)
>>> {
>>> - if (!audit_enabled)
>>> - return;
>>> -
>>> - /* Force a record to be reported if a signal was delivered. */
>>> - if (signr || unlikely(!audit_dummy_context()))
>>> + if (audit_enabled && unlikely(!audit_dummy_context()))
>>> __audit_seccomp(syscall, signr, code);
>>> }
>>>
>>> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
>>> index 6bff068..87257f2 100644
>>> --- a/kernel/seccomp.c
>>> +++ b/kernel/seccomp.c
>>> @@ -516,6 +516,52 @@ static void seccomp_send_sigsys(int syscall, int reason)
>>> }
>>> #endif /* CONFIG_SECCOMP_FILTER */
>>>
>>> +/* For use with seccomp_actions_logged */
>>> +#define SECCOMP_LOG_KILL (1 << 0)
>>> +#define SECCOMP_LOG_TRAP (1 << 2)
>>> +#define SECCOMP_LOG_ERRNO (1 << 3)
>>> +#define SECCOMP_LOG_TRACE (1 << 4)
>>> +#define SECCOMP_LOG_ALLOW (1 << 5)
>>> +
>>> +static u32 seccomp_actions_logged = SECCOMP_LOG_KILL | SECCOMP_LOG_TRAP |
>>> + SECCOMP_LOG_ERRNO | SECCOMP_LOG_TRACE;
>>> +
>>> +static inline void seccomp_log(unsigned long syscall, long signr, u32 action)
>>> +{
>>> + bool log;
>>> +
>>> + switch (action) {
>>> + case SECCOMP_RET_TRAP:
>>> + log = seccomp_actions_logged & SECCOMP_LOG_TRAP;
>>> + break;
>>> + case SECCOMP_RET_ERRNO:
>>> + log = seccomp_actions_logged & SECCOMP_LOG_ERRNO;
>>> + break;
>>> + case SECCOMP_RET_TRACE:
>>> + log = seccomp_actions_logged & SECCOMP_LOG_TRACE;
>>> + break;
>>> + case SECCOMP_RET_ALLOW:
>>> + log = false;
>>> + break;
>>> + case SECCOMP_RET_KILL:
>>> + default:
>>> + log = seccomp_actions_logged & SECCOMP_LOG_KILL;
>>> + }
>>> +
>>> + /*
>>> + * Force an audit message to be emitted when the action is allowed to
>>> + * be logged by the admin.
>>> + */
>>> + if (log)
>>> + return __audit_seccomp(syscall, signr, action);
>>
>> At this point in the patch series, there's no filter-requested-logging
>> flag, so I think the above logic isn't needed until later in the
>> series (or rather, only RET_KILL should be checked).
>
> Hmmm... you're right. This slipped in since the sysctl's purpose morphed
> from configuring what actions should be logged to configuring what
> actions are allowed to be logged.
>
>>
>>> +
>>> + /*
>>> + * Let the audit subsystem decide if the action should be audited based
>>> + * on whether the current task itself is being audited.
>>> + */
>>> + return audit_seccomp(syscall, signr, action);
>>
>> With audit_seccomp() being a single if, maybe it should just be
>> collapsed into this function?
>>
>> if (log || (audit_enabled && unlikely(!audit_dummy_context()))
>> audit_seccomp(...)
>
> I think that would be fine. Unless you say otherwise, I'll also rename
> __audit_seccomp() to audit_seccomp() after doing that.
>
> Tyler
>
>>
>> I do like the change in name, though: this new function is correctly
>> named seccomp_log().
>>
>> -Kees
>>
>
>

Attachments:

signature.asc (801.00 B)
OpenPGP digital signature