Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933977AbdDGXqo (ORCPT ); Fri, 7 Apr 2017 19:46:44 -0400 Received: from youngberry.canonical.com ([91.189.89.112]:56019 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751981AbdDGXqh (ORCPT ); Fri, 7 Apr 2017 19:46:37 -0400 Subject: Re: [PATCH v3 0/4] Improved seccomp logging To: Kees Cook References: <1487043928-5982-1-git-send-email-tyhicks@canonical.com> Cc: Andy Lutomirski , Paul Moore , Eric Paris , Will Drewry , linux-audit@redhat.com, "linux-kernel@vger.kernel.org" , John Crispin , Linux API From: Tyler Hicks Message-ID: Date: Fri, 7 Apr 2017 18:46:17 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="oXf4p8U0QFL4QGjmX9wStXjDmb0xGsLpS" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 10856 Lines: 251 This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --oXf4p8U0QFL4QGjmX9wStXjDmb0xGsLpS Content-Type: multipart/mixed; boundary="1fAO2773SpKvDc76ECgS79rw1FPh5Lgw7"; protected-headers="v1" From: Tyler Hicks To: Kees Cook Cc: Andy Lutomirski , Paul Moore , Eric Paris , Will Drewry , linux-audit@redhat.com, "linux-kernel@vger.kernel.org" , John Crispin , Linux API Message-ID: Subject: Re: [PATCH v3 0/4] Improved seccomp logging References: <1487043928-5982-1-git-send-email-tyhicks@canonical.com> In-Reply-To: --1fAO2773SpKvDc76ECgS79rw1FPh5Lgw7 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 04/07/2017 05:46 PM, Kees Cook wrote: > On Fri, Apr 7, 2017 at 3:16 PM, Tyler Hicks wro= te: >> On 02/22/2017 12:46 PM, Kees Cook wrote: >>> On Thu, Feb 16, 2017 at 3:29 PM, Kees Cook wr= ote: >>>> On Wed, Feb 15, 2017 at 7:24 PM, Andy Lutomirski wrote: >>>>> On Mon, Feb 13, 2017 at 7:45 PM, Tyler Hicks wrote: >>>>>> This patch set is the third revision of the following two previous= ly >>>>>> submitted patch sets: >>>>>> >>>>>> v1: http://lkml.kernel.org/r/1483375990-14948-1-git-send-email-tyh= icks@canonical.com >>>>>> v1: http://lkml.kernel.org/r/1483377999-15019-2-git-send-email-tyh= icks@canonical.com >>>>>> >>>>>> v2: http://lkml.kernel.org/r/1486100262-32391-1-git-send-email-tyh= icks@canonical.com >>>>>> >>>>>> The patch set aims to address some known deficiencies in seccomp's= current >>>>>> logging capabilities: >>>>>> >>>>>> 1. Inability to log all filter actions. >>>>>> 2. Inability to selectively enable filtering; e.g. devs want noi= sy logging, >>>>>> users want relative quiet. >>>>>> 3. Consistent behavior with audit enabled and disabled. >>>>>> 4. Inability to easily develop a filter due to the lack of a >>>>>> permissive/complain mode. >>>>> >>>>> I think I dislike this, but I think my dislikes may be fixable with= >>>>> minor changes. >>>>> >>>>> What I dislike is that this mixes app-specific built-in configurati= on >>>>> (seccomp) with global privileged stuff (audit). The result is a >>>>> potentially difficult to use situation in which you need to modify = an >>>>> app to make it loggable (using RET_LOG) and then fiddle with >>>>> privileged config (auditctl, etc) to actually see the logs. >>>> >>>> You make a good point about RET_LOG vs log_max_action. I think makin= g >>>> RET_LOG the default value would work for 99% of the cases. >>> >>> Actually, I take this back: making "log" the default means that >>> everything else gets logged too, include "expected" return values lik= e >>> errno, trap, etc. I think that would be extremely noisy as a default >>> (for upstream or Ubuntu). >>> >>> Perhaps RET_LOG should unconditionally log? Or maybe the logged >>> actions should be a bit field instead of a single value? Then the >>> default could be "RET_KILL and RET_LOG", but an admin could switch it= >>> to just RET_KILL, or even nothing at all? Hmmm... >> >> Hi Kees - my apologies for going quiet on this topic after we spoke >> about it more in IRC. I needed to tend to other work but now I'm able = to >> return to this seccomp logging patch set. >> >> To summarize what we discussed in IRC, the Chrome browser makes >> extensive use of RET_ERRNO, RET_TRACE, etc., to sandbox code that may >> not ever be adjusted to keep from bump into the sandbox walls. >> Therefore, it is unreasonable to enable logging of something like >> RET_ERRNO on a system-wide level where Chrome browser is in use. >> >> In contrast, snapd wants to set up "noisier" sandboxes for application= s >> to make it clear to the developers and the users that the sandboxed >> application is bumping into the sandbox walls. Developers will know wh= y >> their code may not be working as intended and users will know that the= >> application is doing things that the platform doesn't agree with. Thes= e >> sandboxes will end up using RET_ERRNO in the majority of cases. >> >> This means that with the current design of this patch set, Chrome >> browser will either be unintentionally spamming the logs or snapd's >> sandboxes will be helplessly silent when both Chrome and snapd is >> installed at the same time, depending on the admin's preferences. >> >> To bring it back up a level, two applications may have a very differen= t >> outlook on how acceptable a given seccomp action is and they may >> disagree on whether or not the user/administrator should be informed. >> >> I've been giving thought to the idea of providing a way for the >> application setting up the filter to opt into logging of certain >> actions. Here's a high level breakdown: >> >> - The administrator will have control of system-wide seccomp logging >> and the application will have control of how its seccomp actions ar= e >> logged >> - Both the administrator's and application's logging preferences are >> bitmasks of actions that should be logged >> - The default administrator bitmask is set to log all actions except >> RET_ALLOW >> + Configurable via a sysctl >> + Very similar to the log_max_action syscall that was proposed in >> this patch set but a bitmask instead of a max action >> - The default application bitmask is set to log only RET_KILL and >> RET_LOG >> + Configurable via the seccomp(2) syscall (details TBD) >> - Actions are only logged when the application has requested the >> action to be logged and the administrator has allowed the action to= >> be logged >> + By default, this is only RET_KILL and RET_LOG actions >> >> Let me know what you think about this and I can turn out another patch= >> set next week if it sounds reasonable. >=20 > This seems good to me. With a bitmask we don't have to play crazy > games with levels, and with the app able to declare what it wants > logged, we get the flexibility needed by app developers. >=20 > One change I think I'd like to make is that an app can't block > RET_KILL from being logged (the sysadmin can, though). What do think > of that minor tweak? That's fine from my point of view. >=20 > Is RET_ALLOW allowed to be logged by an application? I guess with it > default-off for an admin, it should be okay. As you say, it should be fine to allow but I don't know if there's any real use in it. I can go either way here. >=20 > Does the app-controlled bitmask apply to the filter, the process, the > process tree, or something else? e.g. systemd launches an app with a > filter, leaving the defaults alone, then later process installs a > filter and wants everything logged -- will things from the earlier > filter get logged? I think implementation preferences may decide many of these questions. As I see it, here are the options in order of my preference: A) Claim the MSB of the filter return value and make the app logging preference per-rule - If the bit is set, log the rule - Provides very fine-grained logging control at the "high cost" of the remaining free bit in the filter return bitmask - The bit can be ignored in the case of RET_KILL - Can be synced across all threads in the calling task with the SECCOMP_FILTER_FLAG_TSYNC filter flag B) Claim a few bits in the filter flags and make the app logging preference per-filter - Something like SECCOMP_FILTER_FLAG_LOG_TRAP, SECCOMP_FILTER_FLAG_LOG_ERRNO, and SECCOMP_FILTER_FLAG_LOG_TRACE - Logging for RET_KILL and RET_LOG can't be turned off - I'd prefer not to waste a bit for RET_ALLOW in this case so it simply won't be loggable - Works with the SECCOMP_FILTER_FLAG_TSYNC filter flag - Doesn't scale well if many new actions are added in the future C) A simplified version of 'B' where only a single mode bit is claimed to enable logging for all actions except RET_ALLOW - Something like SECCOMP_FILTER_FLAG_LOG_ACTIONS - Filters without this flag only log RET_KILL and RET_LOG - Scales much better than 'B' at the expense of less flexibility - Works with the SECCOMP_FILTER_FLAG_TSYNC filter flag D) Claim a bit in the filter mode and make the app logging preference per-process - This new SECCOMP_MODE_ENABLE_LOGGING mode would take a bitmask of actions that should be logged - Incurs a small per-task increase in memory footprint in the form of an additional member in 'struct seccomp' - Has odd behavior you described above where launchers may set the logging preference and then launched application may want something different I think 'A' is the cleanest design but I don't know if highly configurable logging is deserving of the MSB bit in the filter return. I'd like to hear your thoughts there. I _barely_ prefer 'B' over 'C'. They're essential equal in my use case. To be honest, I haven't completely wrapped my head around how 'D' would actually work in practice so I may be writing it off prematurely. Am I missing any more clever options that you can think of? Let me know what you think of the possibilities. Tyler --1fAO2773SpKvDc76ECgS79rw1FPh5Lgw7-- --oXf4p8U0QFL4QGjmX9wStXjDmb0xGsLpS Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBCgAGBQJY6CTJAAoJENaSAD2qAscKsQMP/27s7LwqZn330UNnBeYqffhU 9mB5NVqs7h8hmWWGjTPGNI8IipXVQzsbveG9KBBg5y161Bjco90tlBdVZ9WQ2o+z VNDjQ4zXXGkF28KtqHOHIAbdHhR0enr7LR2TIXtTuCF15+Zck6H+1TqhUV6r8FcP 3c0nLoWob2eYPsI5wo1h1JxefFW523KMp3C6Wh2dZpzKtTTD42NRjpLNKrxbNUsS 2Ab9FbFEHqLTk8+6hUUWoO7dUoYC1L4YvTqIInsnRyyvFdx6J3KDuuZGa916YHtP nA6ocWq1D6CACimCKeQLQcUU4xP1rATq+lhnh+T9kNK8gAx2iDTr8/kOlJUApqR7 NUA31TzaP3cWuYU8iVl6nMS5tToqPpDQQ5ehJh+j6rGjQbaGLeqHvbUZrto0oDr3 6Ephi4L70owj4BoroL4dAi+5ZElC7byW1F9bcH8/2WyC0bRxW2d/WfaK9Ji0snh/ 0Qk8yF5b+wF5zQ2n9XT+0/V45g27HqKpQhkzIlp76Rj8BbszejMcvyWVWvv6bLsM 1P7hNrqg66wZ92CObvqJxuxtuSuxFIC7TaBdl4Ml0vaySh3XOUmK3dExPxsUpeea meWSPhzEGd2IO9DLSUcxFuuPw6q3AyRVR18o0JPADK12sncrlLhpqtldBWKSdvP1 WbzwyNse9iHTx4CqnF8V =e3IM -----END PGP SIGNATURE----- --oXf4p8U0QFL4QGjmX9wStXjDmb0xGsLpS--