* Re: [PATCH bpf-next v13 4/7] landlock: Add ptrace LSM hooks
From: KP Singh @ 2019-11-06 10:15 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Alexei Starovoitov, linux-kernel, Alexei Starovoitov,
Andy Lutomirski, Casey Schaufler, Daniel Borkmann, David Drysdale,
Florent Revest, James Morris, Jann Horn, John Johansen,
Jonathan Corbet, Kees Cook, Michael Kerrisk,
Mickaël Salaün, Paul Moore, Sargun Dhillon,
Serge E . Hallyn, Shuah Khan, Stephen Smalley, Tejun Heo,
Tetsuo Handa, Tycho Andersen, Will Drewry, bpf, kernel-hardening,
linux-api, linux-security-module, netdev
In-Reply-To: <23acf523-dbc4-855b-ca49-2bbfa5e7117e@digikod.net>
On 05-Nov 19:01, Mickaël Salaün wrote:
>
> On 05/11/2019 18:18, Alexei Starovoitov wrote:
> > On Mon, Nov 04, 2019 at 06:21:43PM +0100, Mickaël Salaün wrote:
> >> Add a first Landlock hook that can be used to enforce a security policy
> >> or to audit some process activities. For a sandboxing use-case, it is
> >> needed to inform the kernel if a task can legitimately debug another.
> >> ptrace(2) can also be used by an attacker to impersonate another task
> >> and remain undetected while performing malicious activities.
> >>
> >> Using ptrace(2) and related features on a target process can lead to a
> >> privilege escalation. A sandboxed task must then be able to tell the
> >> kernel if another task is more privileged, via ptrace_may_access().
> >>
> >> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > ...
> >> +static int check_ptrace(struct landlock_domain *domain,
> >> + struct task_struct *tracer, struct task_struct *tracee)
> >> +{
> >> + struct landlock_hook_ctx_ptrace ctx_ptrace = {
> >> + .prog_ctx = {
> >> + .tracer = (uintptr_t)tracer,
> >> + .tracee = (uintptr_t)tracee,
> >> + },
> >> + };
> >
> > So you're passing two kernel pointers obfuscated as u64 into bpf program
> > yet claiming that the end goal is to make landlock unprivileged?!
> > The most basic security hole in the tool that is aiming to provide security.
>
> How could you used these pointers without dedicated BPF helpers? This
> context items are typed as PTR_TO_TASK and can't be used without a
> dedicated helper able to deal with ARG_PTR_TO_TASK. Moreover, pointer
> arithmetic is explicitly forbidden (and I added tests for that). Did I
> miss something?
>
> >
> > I think the only way bpf-based LSM can land is both landlock and KRSI
> > developers work together on a design that solves all use cases.
>
> As I said in a previous cover letter [1], that would be great. I think
> that the current Landlock bases (almost everything from this series
> except the seccomp interface) should meet both needs, but I would like
> to have the point of view of the KRSI developers.
As I mentioned we are willing to collaborate but the current landlock
patches does not meet the needs for KRSI:
* One program type per use-case (eg. LANDLOCK_PROG_PTRACE) as opposed to
a single program type. This is something that KRSI proposed in it's
initial design [1] and the new common "eBPF + LSM" based approach
[2] would maintain as well.
* Landlock chooses to have multiple LSM hooks per landlock hook which is
more restrictive. It's not easy to write precise MAC and Audit
policies for a privileged LSM based on this and this ends up bloating
the context that needs to be maintained and requires avoidable
boilerplate work in the kernel.
[1] https://lore.kernel.org/patchwork/project/lkml/list/?series=410101
[2] https://lore.kernel.org/bpf/20191106100655.GA18815@chromium.org/T/#u
- KP Singh
>
> [1] https://lore.kernel.org/lkml/20191029171505.6650-1-mic@digikod.net/
>
> > BPF is capable
> > to be a superset of all existing LSMs whereas landlock and KRSI propsals today
> > are custom solutions to specific security concerns. BPF subsystem was extended
> > with custom things in the past. In networking we have lwt, skb, tc, xdp, sk
> > program types with a lot of overlapping functionality. We couldn't figure out
> > how to generalize them into single 'networking' program. Now we can and we
> > should. Accepting two partially overlapping bpf-based LSMs would be repeating
> > the same mistake again.
>
> I'll let the LSM maintainers comment on whether BPF could be a superset
> of all LSM, but given the complexity of an access-control system, I have
> some doubts though. Anyway, we need to start somewhere and then iterate.
> This patch series is a first step.
^ permalink raw reply
* Re: [PATCH v10 00/25] LSM: Module stacking for AppArmor
From: James Morris @ 2019-11-06 10:40 UTC (permalink / raw)
To: Casey Schaufler
Cc: Stephen Smalley, casey.schaufler, linux-security-module, selinux,
keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <19e2696d-ab07-21e5-ba22-4ffdcae3c97c@schaufler-ca.com>
[-- Attachment #1: Type: text/plain, Size: 616 bytes --]
On Tue, 29 Oct 2019, Casey Schaufler wrote:
> > Can you re-base on something more recent than v5.1-rc2 (that's the base for that branch currently)?
> > At present it won't even boot for me on modern Fedora. Two key missing commits are:
>
> Sigh. It's based on James' next-general. As it's going up through James,
> and he hasn't updated that branch, I'm sort of stuck. BTW, I have a re-based
> version, but don't see how to get it into my git tree without mucking up
> the eventual merge.
Don't use next-general for this. Use the most recent released kernel if
that works.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH] security/keyring: avoid pagefaults in keyring_read_iterator
From: Chris von Recklinghausen @ 2019-11-06 15:25 UTC (permalink / raw)
To: David Howells
Cc: Jarkko Sakkinen, James Morris, Serge E . Hallyn, keyrings,
linux-security-module, linux-kernel, Waiman Long
In-Reply-To: <3c87bfba-9dc9-665f-17e8-0656e87c658b@redhat.com>
On 10/25/2019 07:10 AM, Chris von Recklinghausen wrote:
> On 10/21/2019 11:46 AM, Chris von Recklinghausen wrote:
>> On 10/21/2019 10:21 AM, David Howells wrote:
>>> Chris von Recklinghausen <crecklin@redhat.com> wrote:
>>>
>>>> The put_user call from keyring_read_iterator caused a page fault which
>>>> attempts to lock mm->mmap_sem and type->lock_class (key->sem) in the reverse
>>>> order that keyring_read_iterator did, thus causing the circular locking
>>>> dependency.
>>>>
>>>> Remedy this by using access_ok and __put_user instead of put_user so we'll
>>>> return an error instead of faulting in the page.
>>> I wonder if it's better to create a kernel buffer outside of the lock in
>>> keyctl_read_key(). Hmmm... The reason I didn't want to do that is that
>>> keyrings have don't have limits on the size. Maybe that's not actually a
>>> problem, since 1MiB would be able to hold a list of a quarter of a million
>>> keys.
>>>
>>> David
>>>
>> Hi David,
>>
>> Thanks for the feedback.
>>
>> I can try to prototype that, but regardless of where the kernel buffer
>> is allocated, the important part is causing the initial pagefault in the
>> read path outside the lock so __put_user won't fail due to a valid user
>> address but page backing the user address isn't in-core.
>>
>> I'll start work on v2.
> Actually I'm going to back off on a v2 effort at this point and request
> that folks comment on the code as-is. Changing keyctl_read_key to use
> its own kernel buffer might be a worthwhile effort, but it doesn't
> appear to me to have any effects on preventing pagefaults on user pages
> at inopportune points of the code.
Does anyone have any more feedback on v1 of this patch?
Thanks,
Chris
>
> Thanks,
>
> Chris
>
>> Thanks,
>>
>> Chris
>>
^ permalink raw reply
* Re: [PATCH v10 00/25] LSM: Module stacking for AppArmor
From: Casey Schaufler @ 2019-11-06 16:11 UTC (permalink / raw)
To: James Morris
Cc: Stephen Smalley, casey.schaufler, linux-security-module, selinux,
keescook, john.johansen, penguin-kernel, paul, casey
In-Reply-To: <alpine.LRH.2.21.1911060238410.30342@namei.org>
On 11/6/2019 2:40 AM, James Morris wrote:
> On Tue, 29 Oct 2019, Casey Schaufler wrote:
>
>>> Can you re-base on something more recent than v5.1-rc2 (that's the base for that branch currently)?
>>> At present it won't even boot for me on modern Fedora. Two key missing commits are:
>> Sigh. It's based on James' next-general. As it's going up through James,
>> and he hasn't updated that branch, I'm sort of stuck. BTW, I have a re-based
>> version, but don't see how to get it into my git tree without mucking up
>> the eventual merge.
> Don't use next-general for this. Use the most recent released kernel if
> that works.
On 9/24/2019 10:56 AM, James Morris wrote:
> I'd probably create a new branch (next-stacking) from v5.4 for this work.
I'm planning to switch over once next-stacking gets created. I've stuck
with next-general because my understanding has been to not rebase unless
necessary, and to not get ahead of what where you're expected to land.
I'll rebase now, but fear I may end up with git tree issues moving from
security#next-general.
I am counting on getting next-stacking shortly after 5.4. Let me know
if that plan changes.
^ permalink raw reply
* Re: [PATCH bpf-next v13 4/7] landlock: Add ptrace LSM hooks
From: Mickaël Salaün @ 2019-11-06 16:55 UTC (permalink / raw)
To: KP Singh, Alexei Starovoitov
Cc: linux-kernel, Alexei Starovoitov, Andy Lutomirski,
Casey Schaufler, Daniel Borkmann, David Drysdale, Florent Revest,
James Morris, Jann Horn, John Johansen, Jonathan Corbet,
Kees Cook, Michael Kerrisk, Mickaël Salaün, Paul Moore,
Sargun Dhillon, Serge E . Hallyn, Shuah Khan, Stephen Smalley,
Tejun Heo, Tetsuo Handa, Tycho Andersen, Will Drewry, bpf,
kernel-hardening, linux-api, linux-security-module, netdev,
Casey Schaufler
In-Reply-To: <20191106100655.GA18815@chromium.org>
On 06/11/2019 11:06, KP Singh wrote:
> On 05-Nov 11:34, Alexei Starovoitov wrote:
>> On Tue, Nov 05, 2019 at 07:01:41PM +0100, Mickaël Salaün wrote:
>>> On 05/11/2019 18:18, Alexei Starovoitov wrote:
[...]
>>>> I think the only way bpf-based LSM can land is both landlock and KRSI
>>>> developers work together on a design that solves all use cases.
>>>
>>> As I said in a previous cover letter [1], that would be great. I think
>>> that the current Landlock bases (almost everything from this series
>>> except the seccomp interface) should meet both needs, but I would like
>>> to have the point of view of the KRSI developers.
>>>
>>> [1] https://lore.kernel.org/lkml/20191029171505.6650-1-mic@digikod.net/
>>>
>>>> BPF is capable
>>>> to be a superset of all existing LSMs whereas landlock and KRSI propsals today
>>>> are custom solutions to specific security concerns. BPF subsystem was extended
>>>> with custom things in the past. In networking we have lwt, skb, tc, xdp, sk
>>>> program types with a lot of overlapping functionality. We couldn't figure out
>>>> how to generalize them into single 'networking' program. Now we can and we
>>>> should. Accepting two partially overlapping bpf-based LSMs would be repeating
>>>> the same mistake again.
>>>
>>> I'll let the LSM maintainers comment on whether BPF could be a superset
>>> of all LSM, but given the complexity of an access-control system, I have
>>> some doubts though. Anyway, we need to start somewhere and then iterate.
>>> This patch series is a first step.
>>
>> I would like KRSI folks to speak up. So far I don't see any sharing happening
>> between landlock and KRSI. You're claiming this set is a first step. They're
>> claiming the same about their patches. I'd like to set a patchset that was
>> jointly developed.
>
> We are willing to collaborate with the Landlock developers and come up
> with a common approach that would work for Landlock and KRSI. I want
> to mention that this collaboration and the current Landlock approach
> of using an eBPF based LSM for unprivileged sandboxing only makes sense
> if unprivileged usage of eBPF is going to be ever allowed.
The ability to *potentially* do unprivileged sandboxing is definitely
not tied nor a blocker to the unprivileged usage of eBPF. As explained
in the documentation [1] (cf. Guiding principles / Unprivileged use),
Landlock is designed to be as safe as possible (from a security point of
view). The impact is more complex and important than just using
unprivileged eBPF, which may not be required. Unprivileged use of eBPF
would be nice, but I think the current direction is to extend the Linux
capabilities with one or multiple dedicated to eBPF [2] (e.g. CAP_BPF +
something else), which may be even better (and a huge difference with
CAP_SYS_ADMIN, a.k.a. privileged mode or root). Landlock is designed to
deal with unprivileged (i.e. non-root) use cases, but of course, if the
Landlock architecture may enable to do unprivileged stuff, it definitely
can do privileged stuff too. However, having an architecture designed
with safe unprivileged use in mind can't be achieve afterwards.
[1] https://lore.kernel.org/lkml/20191104172146.30797-8-mic@digikod.net/
[2] https://lore.kernel.org/bpf/20190827205213.456318-1-ast@kernel.org/
>
> Purely from a technical standpoint, both the current designs for
> Landlock and KRSI target separate use cases and it would not be
> possible to build "one on top of the other". We've tried to identify
> the lowest denominator ("eBPF+LSM") requirements for both Landlock
> (unprivileged sandboxing / Discretionary Access Control) and KRSI
> (flexibility and unification of privileged MAC and Audit) and
> prototyped an implementation based on the newly added / upcoming
> features in BPF.
This is not as binary as that. Sandboxing can be seen as DAC but also as
MAC, depending on the subject which apply the security policy and the
subjects which are enforced by this policy. If the sandboxing is applied
system-wide, it is what we usually call MAC. DAC, in the Linux world,
enables any user to restrict access to their files to other users.
With Landlock it is not the same because a process can restrict itself
but also enforce these restrictions on all its future children (which
may be malicious, whatever their UID/GID). The threat and the definition
of the attacker are not the same in both cases. With the Linux DAC the
potentially malicious subjects are the other users, whereas with
Landlock the potentially malicious subjects are (for now) the current
process and all its children. Another way to explain it, and how
Landlock is designed, is that a specific enforcement (i.e. a set of BPF
programs) is tied to a domain, in which a set of subject are. From this
perspective, this approach (subjects/tasks in a domain) is orthogonal to
the DAC system (subjects/users). This design may apply to a system-wide
MAC system by putting all the system tasks in one domain, and managing
restrictions (by subject) with other means (e.g. task's UID,
command-line strings, environment variables). In short, Landlock (in
this patch series) is closer to a (potentially scoped) MAC system. But
thanks to eBPF, Landlock is firstly a programmatic access-control, which
means that the one who write the programs and tie them to a set of
tasks, can implement their own access-control system (e.g. RBAC,
time-based…), or something else (e.g. an audit system).
The audit part can simply be achieve with dedicated helpers and programs
that always allow accesses.
Landlock evolved over multiple iterations and is now designed to be very
flexible. The current way to enforce a security policy is to go through
the seccomp syscall (which makes sense for multiple reasons explained
and accepted before). But Landlock is designed to enable similar
enforcements (or audit) with other ways to define a domain (e.g. cgroups
[3], or system-wide securityfs as done in KRSI). Indeed, the only part
tied to this scoped enforcement is in the domain_syscall.c file. A new
file domain_fs.c could be added to implement a securityfs for a
system-wide enforcement (and have other features as KRSI does).
[3] https://lore.kernel.org/lkml/20160914072415.26021-17-mic@digikod.net/
One possible important difference between Landlock and KRSI right now is
the BPF program management. Both manage a list of programs per hook.
However KRSI needs to be able to replace a program in these lists. This
is not implemented in this Landlock patch series, first because it is
not the main use-case and it is safer to have an append-only way to add
restrictions (cf. seccomp-bpf model), and second because it is simpler
to deal with immutable lists. However, it is worth considering extending
the Landlock domain management with the ability to update the program
lists. One challenge may be to identify which program should be replaced
(which KRSI does with the program name). I think it would be wiser to
implement this in a second step though, maybe not for the syscall
interface (thanks to a new seccomp operation), but first with the
securityfs one.
>
> We've been successfully able to prototype the use cases for KRSI
> (privileged MAC and Audit) using this "eBPF+LSM" and shared our
> approach at the Linux Security Summit [1]:
>
> * Use the new in-kernel BTF (CO-RE eBPF programs) [2] and the ability
> of the BPF verifier to use the BTF information for access validation
> to provide a more generic way to attach to the various LSM hooks.
> This potentially saves a lot of redundant work:
>
> - Creation of new program types.
> - Multiple types of contexts (or a single context with Unions).
> - Changes to the verifier and creation of new BPF argument types
> (eg. PTR_TO_TASK)
As I understood from the LSS talk, KRSI's approach is to use the same
hooks as LSM (cf. the securityfs). As said Alexei [4] "It must not make
LSM hooks into stable ABI". Moveover, the LSM hooks may change
according to internal kernel evolution, and their semantic may not make
sense from a user space point of view. This is one reason for which
Lanlock abstract those hooks into something that is simpler and designed
to fit well with eBPF (program contexts and their attached types, as
explained in the documentation).
[4]
https://lore.kernel.org/lkml/20191105215453.szhdkrvuekwfz6le@ast-mbp.dhcp.thefacebook.com/
How does KRSI plan to deal with one LSM hook being split in two hooks in
a next version of the kernel (cf. [5])?
[5] https://lore.kernel.org/lkml/20190910115527.5235-6-kpsingh@chromium.org/
Another reason to have multiple different attach types/contexts (cf.
landlock_domain->programs[]) is to limit useless BPF program
interpretation (in addition to the non-system-wide scoped of programs).
It also enables to handle and verify strict context use (which is also
explain in the Guiding principles). It would be a huge wast of time to
run every BPF programs for all LSM hooks. KRSI does the same but instead
of relying on the program type it rely on the list tied to the
securityfs file.
BTF is great, but as far as I know, it's goal is to easily deal with the
moving kernel ABI (e.g. task_struct layout, config/constant variables),
and it is definitely useful to programs using bpf_probe_read() and
similar accessors. However, I don't see how KRSI would avoid BPF types
thanks to BTF.
There is only one program type for Landlock (i.e.
BPF_PROG_TYPE_LANDLOCK_HOOK), and I don't see why adding new program
*attach* types (e.g. BPF_LANDLOCK_PTRACE) may be an issue. The kernel
will still need to be modified to implement new hooks and the new BPF
helpers anyway, BTF will not change that, except maybe if the internal
LSM API is exposed in a way or another to BPF (thanks to BTF), which
does not seem acceptable. Am I missing something?
The current KRSI approach is to allow a common set of helpers to be
called by all programs (because there is no way to differentiate them
with their type).
How KRSI would deal with kernel objects other than the current task
(e.g. a ptrace hook with a tracer and a tracee, a file open/read) with
the struct krsi_ctx unions [6]?
[6] https://lore.kernel.org/lkml/20190910115527.5235-7-kpsingh@chromium.org/
How does KRSI plan to deal with security blobs?
>
> * These new BPF features also alleviate the original concerns that we
> raised when initially proposing KRSI and designing for precise BPF
> helpers. We have some patches coming up which incorporate these new
> changes and will be sharing something on the mailing list after some
> cleanup.
>
> We can use the common "eBPF+LSM" for both privileged MAC and Audit and
> unprivileged sandboxing i.e. Discretionary Access Control.
> Here's what it could look like:
>
> * Common infrastructure allows attachment to all hooks which works well
> for privileged MAC and Audit. This could be extended to provide
> another attachment type for unprivileged DAC, which can restrict the
> hooks that can be attached to, and also the information that is
> exposed to the eBPF programs which is something that Landlock could
> build.
I agree that the "privileged-only" hooks should be a superset of the
"security-safe-and-potentially-unprivileged" hooks. :)
However, as said previously, I'm convinced it is a requirement to have
abstract hooks (and associated program attach types) as defined by Landlock.
I'm not sure what you mean by "the information that is exposed to the
eBPF program". Is it the current Landlock implementation of specific
contexts and attach types?
>
> * This attachment could use the proposed landlock domains and attach to
> the task_struct providing the discretionary access control semantics.
Not task_struct but creds, yes. This is a characteristic of sandboxing,
which may not be useful for the KRSI use case. It makes sense for KRSI
to attach program sets (or Landlock domains) to the whole system, then
using the creds does not make sense here. This difference is small and a
previous version of Landlock already validated this use case with
cgroups [3] (which is postponed to simplify the patch series).
[3] https://lore.kernel.org/lkml/20160914072415.26021-17-mic@digikod.net/
>
> [1] https://static.sched.com/hosted_files/lsseu2019/a2/Kernel%20Runtime%20Security%20Instrumentation.pdf
> [2] http://vger.kernel.org/bpfconf2019_talks/bpf-core.pdf
>
> - KP Singh
I think it should be OK to first land something close to this Landlock
patch series and then we could extend the domain management features and
add the securityfs support that KRSI needs. The main concern seems to be
about hook definitions.
Another approach would be to land Landlock and KRSI as distinct LSM
while trying as much as possible to mutualize code/helpers.
^ permalink raw reply
* Re: [PATCH bpf-next v13 4/7] landlock: Add ptrace LSM hooks
From: Mickaël Salaün @ 2019-11-06 16:58 UTC (permalink / raw)
To: KP Singh
Cc: Alexei Starovoitov, linux-kernel, Alexei Starovoitov,
Andy Lutomirski, Casey Schaufler, Daniel Borkmann, David Drysdale,
Florent Revest, James Morris, Jann Horn, John Johansen,
Jonathan Corbet, Kees Cook, Michael Kerrisk,
Mickaël Salaün, Paul Moore, Sargun Dhillon,
Serge E . Hallyn, Shuah Khan, Stephen Smalley, Tejun Heo,
Tetsuo Handa, Tycho Andersen, Will Drewry, bpf, kernel-hardening,
linux-api, linux-security-module, netdev
In-Reply-To: <20191106101558.GA19467@chromium.org>
On 06/11/2019 11:15, KP Singh wrote:
> On 05-Nov 19:01, Mickaël Salaün wrote:
>> On 05/11/2019 18:18, Alexei Starovoitov wrote:
[...]
>>>
>>> I think the only way bpf-based LSM can land is both landlock and KRSI
>>> developers work together on a design that solves all use cases.
>>
>> As I said in a previous cover letter [1], that would be great. I think
>> that the current Landlock bases (almost everything from this series
>> except the seccomp interface) should meet both needs, but I would like
>> to have the point of view of the KRSI developers.
>
> As I mentioned we are willing to collaborate but the current landlock
> patches does not meet the needs for KRSI:
>
> * One program type per use-case (eg. LANDLOCK_PROG_PTRACE) as opposed to
> a single program type. This is something that KRSI proposed in it's
> initial design [1] and the new common "eBPF + LSM" based approach
> [2] would maintain as well.
As ask in my previous email [1], I don't see how KRSI would efficiently
deal with other LSM hooks with a unique program (attach) type.
[1]
https://lore.kernel.org/lkml/813cedde-8ed7-2d3b-883d-909efa978d41@digikod.net/
>
> * Landlock chooses to have multiple LSM hooks per landlock hook which is
> more restrictive. It's not easy to write precise MAC and Audit
> policies for a privileged LSM based on this and this ends up bloating
> the context that needs to be maintained and requires avoidable
> boilerplate work in the kernel.
Why do you think it is more restrictive or it adds boilerplate work? How
does KRSI will deal with more complex hooks than execve-like with
multiple kernel objects?
>
> [1] https://lore.kernel.org/patchwork/project/lkml/list/?series=410101
> [2] https://lore.kernel.org/bpf/20191106100655.GA18815@chromium.org/T/#u
>
> - KP Singh
^ permalink raw reply
* Re: [PATCH linux-kselftest/test v2] apparmor: add AppArmor KUnit tests for policy unpack
From: Kees Cook @ 2019-11-06 17:18 UTC (permalink / raw)
To: Brendan Higgins
Cc: shuah, john.johansen, jmorris, serge, alan.maguire, yzaikin,
davidgow, mcgrof, tytso, linux-kernel, linux-security-module,
kunit-dev, linux-kselftest, Mike Salvatore
In-Reply-To: <20191106004329.16991-1-brendanhiggins@google.com>
On Tue, Nov 05, 2019 at 04:43:29PM -0800, Brendan Higgins wrote:
> From: Mike Salvatore <mike.salvatore@canonical.com>
>
> Add KUnit tests to test AppArmor unpacking of userspace policies.
> AppArmor uses a serialized binary format for loading policies. To find
> policy format documentation see
> Documentation/admin-guide/LSM/apparmor.rst.
>
> In order to write the tests against the policy unpacking code, some
> static functions needed to be exposed for testing purposes. One of the
> goals of this patch is to establish a pattern for which testing these
> kinds of functions should be done in the future.
>
> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> Signed-off-by: Mike Salvatore <mike.salvatore@canonical.com>
> ---
> security/apparmor/Kconfig | 16 +
> security/apparmor/policy_unpack.c | 4 +
> security/apparmor/policy_unpack_test.c | 607 +++++++++++++++++++++++++
> 3 files changed, 627 insertions(+)
> create mode 100644 security/apparmor/policy_unpack_test.c
>
> diff --git a/security/apparmor/Kconfig b/security/apparmor/Kconfig
> index d8b1a360a6368..78a33ccac2574 100644
> --- a/security/apparmor/Kconfig
> +++ b/security/apparmor/Kconfig
> @@ -66,3 +66,19 @@ config SECURITY_APPARMOR_DEBUG_MESSAGES
> Set the default value of the apparmor.debug kernel parameter.
> When enabled, various debug messages will be logged to
> the kernel message buffer.
> +
> +config SECURITY_APPARMOR_KUNIT_TEST
> + bool "Build KUnit tests for policy_unpack.c"
> + depends on KUNIT && SECURITY_APPARMOR
> + help
> + This builds the AppArmor KUnit tests.
> +
> + KUnit tests run during boot and output the results to the debug log
> + in TAP format (http://testanything.org/). Only useful for kernel devs
> + running KUnit test harness and are not for inclusion into a
> + production build.
> +
> + For more information on KUnit and unit tests in general please refer
> + to the KUnit documentation in Documentation/dev-tools/kunit/.
> +
> + If unsure, say N.
> diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
> index 8cfc9493eefc7..37c1dd3178fc0 100644
> --- a/security/apparmor/policy_unpack.c
> +++ b/security/apparmor/policy_unpack.c
> @@ -1120,3 +1120,7 @@ int aa_unpack(struct aa_loaddata *udata, struct list_head *lh,
>
> return error;
> }
> +
> +#ifdef CONFIG_SECURITY_APPARMOR_KUNIT_TEST
> +#include "policy_unpack_test.c"
> +#endif /* CONFIG_SECURITY_APPARMOR_KUNIT_TEST */
To make this even LESS intrusive, the ifdefs could live in ..._test.c.
Also, while I *think* the kernel build system will correctly track this
dependency, can you double-check that changes to ..._test.c correctly
trigger a recompile of policy_unpack.c?
--
Kees Cook
^ permalink raw reply
* [PATCH v4 08/10] IMA: Defined functions to queue and dequeue keys for measurement
From: Lakshmi Ramasubramanian @ 2019-11-06 19:01 UTC (permalink / raw)
To: zohar, dhowells, matthewgarrett, sashal, jamorris,
linux-integrity, linux-security-module, keyrings, linux-kernel
In-Reply-To: <20191106190116.2578-1-nramas@linux.microsoft.com>
A key can be measured right away only if IMA is initialized.
Otherwise, the key should be queued up and processed when IMA
initialization is completed.
This patch defines functions to queue and dequeue keys for
measurement and a config to enable these functions.
These functions are defined in a new file namely
ima_asymmetric_keys.c
Note that currently IMA subsystem can be enabled without
enabling KEYS subsystem.
Adding support for measuring asymmetric keys in IMA requires KEYS
subsystem to be enabled. To handle this dependency a new config
namely CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS has been added. Enabling
this config requires the following configs to be enabled:
CONFIG_IMA, CONFIG_KEYS, CONFIG_ASYMMETRIC_KEY_TYPE, and
CONFIG_ASYMMETRIC_PUBLIC_KEY_SUBTYPE.
The new file ima_asymmetric_keys.c is built only if
CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS is enabled.
This config is turned off by default.
Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
security/integrity/ima/Kconfig | 14 ++
security/integrity/ima/Makefile | 1 +
security/integrity/ima/ima.h | 24 ++++
security/integrity/ima/ima_asymmetric_keys.c | 136 +++++++++++++++++++
4 files changed, 175 insertions(+)
create mode 100644 security/integrity/ima/ima_asymmetric_keys.c
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 838476d780e5..c6d14884bc19 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -310,3 +310,17 @@ config IMA_APPRAISE_SIGNED_INIT
default n
help
This option requires user-space init to be signed.
+
+config IMA_MEASURE_ASYMMETRIC_KEYS
+ bool "Enable measuring asymmetric keys on key create or update"
+ depends on IMA
+ depends on KEYS
+ depends on ASYMMETRIC_KEY_TYPE
+ depends on ASYMMETRIC_PUBLIC_KEY_SUBTYPE
+ default n
+ help
+ This option enables measuring asymmetric keys when
+ the key is created or updated. Additionally, IMA policy
+ needs to be configured to either measure keys linked to
+ any keyring or only measure keys linked to the keyrings
+ specified in the IMA policy through the keyrings= option.
diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
index 31d57cdf2421..3e9d0ad68c7b 100644
--- a/security/integrity/ima/Makefile
+++ b/security/integrity/ima/Makefile
@@ -12,3 +12,4 @@ ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o
ima-$(CONFIG_IMA_APPRAISE_MODSIG) += ima_modsig.o
ima-$(CONFIG_HAVE_IMA_KEXEC) += ima_kexec.o
obj-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o
+obj-$(CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS) += ima_asymmetric_keys.o
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 6a86daa62c5b..872883520ea6 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -206,6 +206,30 @@ extern const char *const func_tokens[];
struct modsig;
+#ifdef CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS
+/*
+ * To track keys that need to be measured.
+ */
+struct ima_measure_key_entry {
+ struct list_head list;
+ void *public_key;
+ u32 public_key_len;
+ char *keyring_name;
+};
+
+int ima_queue_or_process_key_for_measurement(struct key *keyring,
+ struct key *key);
+void ima_measure_queued_keys(void);
+#else
+static inline int ima_queue_or_process_key_for_measurement(
+ struct key *keyring,
+ struct key *key)
+{
+ return 0;
+}
+static inline void ima_measure_queued_keys(void) {}
+#endif /* CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS */
+
/* LIM API function definitions */
int ima_get_action(struct inode *inode, const struct cred *cred, u32 secid,
int mask, enum ima_hooks func, int *pcr,
diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c
new file mode 100644
index 000000000000..fa3d9bf8fcbe
--- /dev/null
+++ b/security/integrity/ima/ima_asymmetric_keys.c
@@ -0,0 +1,136 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2019 Microsoft Corporation
+ *
+ * Author: Lakshmi Ramasubramanian (nramas@linux.microsoft.com)
+ *
+ * File: ima_asymmetric_keys.c
+ * Queue and de-queue functions for measuring asymmetric keys.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <crypto/public_key.h>
+#include <keys/asymmetric-type.h>
+#include "ima.h"
+
+/*
+ * To synchronize access to the list of keys that need to be measured
+ */
+static DEFINE_MUTEX(ima_measure_keys_mutex);
+static LIST_HEAD(ima_measure_keys);
+
+static void ima_free_measure_key_entry(struct ima_measure_key_entry *entry)
+{
+ if (entry != NULL) {
+ if (entry->public_key != NULL)
+ kzfree(entry->public_key);
+ if (entry->keyring_name != NULL)
+ kzfree(entry->keyring_name);
+ kzfree(entry);
+ }
+}
+
+static struct ima_measure_key_entry *ima_alloc_measure_key_entry(
+ struct key *keyring,
+ struct key *key)
+{
+ int rc = 0;
+ const struct public_key *pk;
+ size_t keyring_name_len;
+ struct ima_measure_key_entry *entry = NULL;
+
+ pk = key->payload.data[asym_crypto];
+ keyring_name_len = strlen(keyring->description) + 1;
+ entry = kzalloc(sizeof(*entry), GFP_KERNEL);
+ if (entry != NULL) {
+ entry->public_key = kzalloc(pk->keylen, GFP_KERNEL);
+ entry->keyring_name =
+ kzalloc(keyring_name_len, GFP_KERNEL);
+ }
+
+ if ((entry == NULL) || (entry->public_key == NULL) ||
+ (entry->keyring_name == NULL)) {
+ rc = -ENOMEM;
+ goto out;
+ }
+
+ strcpy(entry->keyring_name, keyring->description);
+ memcpy(entry->public_key, pk->key, pk->keylen);
+ entry->public_key_len = pk->keylen;
+ rc = 0;
+
+out:
+ if (rc) {
+ ima_free_measure_key_entry(entry);
+ entry = NULL;
+ }
+
+ return entry;
+}
+
+int ima_queue_or_process_key_for_measurement(struct key *keyring,
+ struct key *key)
+{
+ int rc = 0;
+ struct ima_measure_key_entry *entry = NULL;
+ const struct public_key *pk;
+
+ if (key->type != &key_type_asymmetric)
+ return 0;
+
+ mutex_lock(&ima_measure_keys_mutex);
+
+ if (ima_initialized) {
+ /*
+ * keyring->description points to the name of the keyring
+ * (such as ".builtin_trusted_keys", ".ima", etc.) to
+ * which the given key is linked to.
+ *
+ * The name of the keyring is passed in the "eventname"
+ * parameter to process_buffer_measurement() and is set
+ * in the "eventname" field in ima_event_data for
+ * the key measurement IMA event.
+ *
+ * The name of the keyring is also passed in the "keyring"
+ * parameter to process_buffer_measurement() to check
+ * if the IMA policy is configured to measure a key linked
+ * to the given keyring.
+ */
+ pk = key->payload.data[asym_crypto];
+ process_buffer_measurement(pk->key, pk->keylen,
+ keyring->description,
+ KEYRING_CHECK, 0,
+ keyring->description);
+ } else {
+ entry = ima_alloc_measure_key_entry(keyring, key);
+ if (entry != NULL) {
+ INIT_LIST_HEAD(&entry->list);
+ list_add_tail(&entry->list, &ima_measure_keys);
+ } else
+ rc = -ENOMEM;
+ }
+
+ mutex_unlock(&ima_measure_keys_mutex);
+
+ return rc;
+}
+
+void ima_measure_queued_keys(void)
+{
+ struct ima_measure_key_entry *entry, *tmp;
+
+ mutex_lock(&ima_measure_keys_mutex);
+
+ list_for_each_entry_safe(entry, tmp, &ima_measure_keys, list) {
+ process_buffer_measurement(entry->public_key,
+ entry->public_key_len,
+ entry->keyring_name,
+ KEYRING_CHECK, 0,
+ entry->keyring_name);
+ list_del(&entry->list);
+ ima_free_measure_key_entry(entry);
+ }
+
+ mutex_unlock(&ima_measure_keys_mutex);
+}
--
2.17.1
^ permalink raw reply related
* [PATCH v4 09/10] IMA: Call queue and dequeue functions to measure keys
From: Lakshmi Ramasubramanian @ 2019-11-06 19:01 UTC (permalink / raw)
To: zohar, dhowells, matthewgarrett, sashal, jamorris,
linux-integrity, linux-security-module, keyrings, linux-kernel
In-Reply-To: <20191106190116.2578-1-nramas@linux.microsoft.com>
Keys should be queued for measurement if IMA is not yet initialized.
Keys queued for measurement, if any, need to be processed when IMA
initialization is completed.
This patch updates the IMA hook for key_create_or_update
to call ima_queue_or_process_key_for_measurement() and
adds the call to process queued keys upon IMA initialization
completion.
Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
security/integrity/ima/ima_init.c | 1 +
security/integrity/ima/ima_main.c | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index a810af6df587..74817a9f78e5 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -137,6 +137,7 @@ int __init ima_init(void)
return rc;
ima_initialized = true;
+ ima_measure_queued_keys();
return 0;
}
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 56540357c854..8733990867f2 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -757,7 +757,7 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
unsigned long flags, bool create)
{
if ((keyring != NULL) && (key != NULL))
- return;
+ ima_queue_or_process_key_for_measurement(keyring, key);
}
static int __init init_ima(void)
--
2.17.1
^ permalink raw reply related
* [PATCH v4 0/10] KEYS: Measure keys when they are created or updated
From: Lakshmi Ramasubramanian @ 2019-11-06 19:01 UTC (permalink / raw)
To: zohar, dhowells, matthewgarrett, sashal, jamorris,
linux-integrity, linux-security-module, keyrings, linux-kernel
Problem Statement:
Keys created or updated in the system are currently not being measured.
Therefore an attestation service, for instance, would not be able to
attest whether or not the trusted keys keyring(s), for instance, contain
only known good (trusted) keys.
IMA measures system files, command line arguments passed to kexec,
boot aggregate, etc. It can be used to measure keys as well.
But there is no mechanism available in the kernel for IMA to
know when a key is created or updated.
This change aims to address measuring keys created or updated
in the system.
To achieve the above the following changes have been made:
- Added a new IMA hook namely, ima_post_key_create_or_update, which
measures the key. This IMA hook is called from key_create_or_update
function. The key measurement can be controlled through IMA policy.
In this change set a new IMA policy function KEYRING_CHECK has been
added to measure keys. The policy can optionally specify a set of
keyrings to measure. By default all keyrings are included in
the measurement when KEYRING_CHECK policy is specified.
# measure all keys
measure func=KEYRING_CHECK
# measure keys on the IMA keyring
measure func=KEYRING_CHECK keyring=".ima"
# measure keys on the BUILTIN and IMA keyrings into a different PCR
measure func=KEYRING_CHECK keyring=".builtin_trusted_keys|.ima" pcr=11
Testing performed:
* Booted the kernel with this change.
* Executed keyctl tests from the Linux Test Project (LTP)
* All keys are measured when only KEYRING_CHECK is set.
* Only keys added to the given keyrings are measured
when keyrings option is set.
* Keys are not measured when KEYRING_CHECK is not set.
* Key is queued for measurement if IMA is not yet initialized
and processed when IMA is initialized.
* Key is measured rightaway when IMA is initialized.
* Added a new key to a keyring and verified "key create" code path.
=> In this case added a key to .ima keyring.
* Added the same key again and verified "key update" code path.
=> Add the same key to .ima keyring.
Change Log:
v4:
=> Rebased the changes to v5.4-rc3
=> Applied the following dependent patch set first
and then added new changes.
https://lore.kernel.org/linux-integrity/1572492694-6520-1-git-send-email-zohar@linux.ibm.com
=> Refactored the patch set to separate out changes related to
func KEYRING_CHECK and options keyrings into different patches.
=> Moved the functions to queue and dequeue keys for measurement
from ima_queue.c to a new file ima_asymmetric_keys.c.
=> Added a new config namely CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS
to enable asymmetric key measurement. When this config option is
enabled ima_asymmetric_keys.c is compiled.
v3:
=> Added KEYRING_CHECK for measuring keys. This can optionally specify
keyrings to measure.
=> Updated ima_get_action() and related functions to return
the keyrings if specified in the policy.
=> process_buffer_measurement() function is updated to take keyring
as a parameter. The key will be measured if the policy includes
the keyring in the list of measured keyrings. If the policy does not
specify any keyrings then all keys are measured.
v2:
=> Per suggestion from Mimi reordered the patch set to first
enable measuring keys added or updated in the system.
And, then scope the measurement to keys added to
builtin_trusted_keys keyring through ima policy.
=> Removed security_key_create_or_update function and instead
call ima hook, to measure the key, directly from
key_create_or_update function.
v1:
=> LSM function for key_create_or_update. It calls ima.
=> Added ima hook for measuring keys
=> ima measures keys based on ima policy.
v0:
=> Added LSM hook for key_create_or_update.
=> Measure keys added to builtin or secondary trusted keys keyring.
Background:
Currently ima measures file hashes and .ima signatures. ima signatures
are validated against keys in the ".ima" keyring. If the kernel is built
with CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY enabled,
then all keys in ".ima" keyring must be signed by a key in
".builtin_trusted_keys" or ".secondary_trusted_keys" keyrings.
Although ima supports the above configuration, not having an insight
into what keys are present in these trusted keys keyrings would prevent
an attestation service from validating a client machine.
On systems with CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
enabled, measuring keys in the ".builtin_trusted_keys" keyring provides
a mechanism to attest that the client's system binaries are indeed signed
by signers that chain to known trusted keys.
Without this change, to attest the clients one needs to maintain
an "allowed list" of file hashes of all versions of all client binaries
that are deployed on the clients in the enterprise. That is a huge
operational challenge in a large scale environment of clients with
heterogenous builds. This also limits scalability and agility of
rolling out frequent client binary updates.
Questions and concerns raised by reviewers on this patch set:
Question 1:
Is "Signed with a trusted key" equal to "Trusted file"?
Doesn't the service need the hashes of the system files to determine
whether a file is trusted or not?
"Signed with a trusted key" does not equal "Trusted"
Answer:
Agree "Signed with a trusted key" may not equal "Trusted".
To address this, the attesting service can maintain a small
manageable set of bad hashes (a "Blocked list") and a list of
trusted keys expected in client's .builtin_trusted_keys" keyring.
Using this data, the service can detect the presence of
"Disallowed (untrusted) version of client binaries".
Question 2:
Providing more data to the service (such as ".builtin_trusted_keys"),
empowers the service to deny access to clients (block clients).
IMA walks a fine line in enforcing and measuring file integrity.
This patchset breaches that fine line and in doing so brings back
the fears of trusted computing.
Answer:
Any new measurement we add in IMA will provide more data to service
and can enable it to deny access to clients. It is not clear why this patch
set would breach the fine line between measuring and enforcing.
Since this patch set is disabled by default and enabled through
CONFIG_IMA_MEASURE_TRUSTED_KEYS, only those enterprises that
require this new measurement can opt-in for it. Since it is disabled
by default, it does not restrict the autonomy of independent users
who are unaffected by attestation.
Question 3:
IMA log already contains a pointer to the IMA keys used for signature
verification. Why does the service need to care what keys were used
to sign (install) the IMA keys? What is gained by measuring the keys
in the ".builtin_trusted_keys"
Answer:
To attest the clients using the current IMA log, service needs to maintain
hashes of all the deployed versions of all the system binaries for their
enterprise. This will introduce a very high operational overhead in
a large scale environment of clients with heterogenous builds.
This limits scalability and agility of rolling out frequent client
binary updates.
On the other hand, with the current patch set, we will have IMA
validate the file signature on the clients and the service validate
that the IMA keys were installed using trusted keys.
This provides a chain of trust:
=> IMA Key validates file signature on the client
=> Built-In trusted key attests IMA key on the client
=> Attestation service attests the Built-In trusted keys
reported by the client in the IMA log
This approach, therefore, would require the service to maintain
a manageble set of trusted keys that it receives from a trusted source.
And, verify if the clients only have keys from that set of trusted keys.
Question 4:
Where will the attestation service receive the keys to validate against?
Answer:
Attestation service will receive the keys from a trusted source such as
the enterprise build services that provides the client builds.
The service will use this set of keys to verify that the keys reported by
the clients in the IMA log contains only keys from this trusted list.
Question 5:
What is changing in the IMA log through this patch set?
Answer:
This patch set does not remove any data that is currently included
in the IMA log. It only adds more data to the IMA log - the data on
".builtin_trusted_keys"
Lakshmi Ramasubramanian (10):
KEYS: Defined an IMA hook and a func to measure keys on key create or
update
KEYS: Added KEYRING_CHECK func in IMA policy to measure keys
KEYS: Added keyrings= option in IMA policy to only measure keys added
to the specified keyrings.
KEYS: Read keyrings= option from the IMA policy into ima_rule_entry
KEYS: Updated IMA policy functions to return keyrings option read from
the policy
KEYS: Measure key if the IMA policy allows measurement for the keyring
to which the key is linked to
KEYS: Added a boolean flag to track IMA initialization status
KEYS: Defined functions to queue and dequeue keys for measurement
KEYS: Call queue and dequeue functions to measure keys
KEYS: Call the IMA hook to measure key when a new key is created or an
existing key is updated
Documentation/ABI/testing/ima_policy | 16 ++-
include/linux/ima.h | 8 ++
security/integrity/ima/Kconfig | 14 ++
security/integrity/ima/Makefile | 1 +
security/integrity/ima/ima.h | 34 ++++-
security/integrity/ima/ima_api.c | 8 +-
security/integrity/ima/ima_appraise.c | 4 +-
security/integrity/ima/ima_asymmetric_keys.c | 136 +++++++++++++++++++
security/integrity/ima/ima_init.c | 10 +-
security/integrity/ima/ima_main.c | 47 ++++++-
security/integrity/ima/ima_policy.c | 39 +++++-
security/keys/key.c | 9 ++
12 files changed, 309 insertions(+), 17 deletions(-)
create mode 100644 security/integrity/ima/ima_asymmetric_keys.c
--
2.17.1
^ permalink raw reply
* [PATCH v4 04/10] IMA: Read keyrings= option from the IMA policy into ima_rule_entry
From: Lakshmi Ramasubramanian @ 2019-11-06 19:01 UTC (permalink / raw)
To: zohar, dhowells, matthewgarrett, sashal, jamorris,
linux-integrity, linux-security-module, keyrings, linux-kernel
In-Reply-To: <20191106190116.2578-1-nramas@linux.microsoft.com>
"keyrings=" option, if specified in the IMA policy, needs to be
stored in the list of IMA rules when the configured IMA policy is read.
This patch defines a new policy token enum namely Opt_keyrings
for reading "keyrings=" option from the IMA policy.
Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
security/integrity/ima/ima_policy.c | 28 +++++++++++++++++++++++++++-
1 file changed, 27 insertions(+), 1 deletion(-)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 4d68ad8ed91c..74a727dc6030 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -768,7 +768,8 @@ enum {
Opt_uid_gt, Opt_euid_gt, Opt_fowner_gt,
Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt,
Opt_appraise_type, Opt_appraise_flag,
- Opt_permit_directio, Opt_pcr, Opt_template, Opt_err
+ Opt_permit_directio, Opt_pcr, Opt_template, Opt_keyrings,
+ Opt_err
};
static const match_table_t policy_tokens = {
@@ -804,6 +805,7 @@ static const match_table_t policy_tokens = {
{Opt_permit_directio, "permit_directio"},
{Opt_pcr, "pcr=%s"},
{Opt_template, "template=%s"},
+ {Opt_keyrings, "keyrings=%s"},
{Opt_err, NULL}
};
@@ -1051,6 +1053,23 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
result = 0;
entry->flags |= IMA_FSNAME;
break;
+ case Opt_keyrings:
+ ima_log_string(ab, "keyrings", args[0].from);
+
+ if ((entry->keyrings) ||
+ (entry->action != MEASURE) ||
+ (entry->func != KEYRING_CHECK)) {
+ result = -EINVAL;
+ break;
+ }
+ entry->keyrings = kstrdup(args[0].from, GFP_KERNEL);
+ if (!entry->keyrings) {
+ result = -ENOMEM;
+ break;
+ }
+ result = 0;
+ entry->flags |= IMA_KEYRINGS;
+ break;
case Opt_fsuuid:
ima_log_string(ab, "fsuuid", args[0].from);
@@ -1426,6 +1445,13 @@ int ima_policy_show(struct seq_file *m, void *v)
seq_puts(m, " ");
}
+ if (entry->flags & IMA_KEYRINGS) {
+ if (entry->keyrings != NULL)
+ snprintf(tbuf, sizeof(tbuf), "%s", entry->keyrings);
+ seq_printf(m, pt(Opt_keyrings), tbuf);
+ seq_puts(m, " ");
+ }
+
if (entry->flags & IMA_PCR) {
snprintf(tbuf, sizeof(tbuf), "%d", entry->pcr);
seq_printf(m, pt(Opt_pcr), tbuf);
--
2.17.1
^ permalink raw reply related
* [PATCH v4 07/10] IMA: Added a boolean flag to track IMA initialization status
From: Lakshmi Ramasubramanian @ 2019-11-06 19:01 UTC (permalink / raw)
To: zohar, dhowells, matthewgarrett, sashal, jamorris,
linux-integrity, linux-security-module, keyrings, linux-kernel
In-Reply-To: <20191106190116.2578-1-nramas@linux.microsoft.com>
IMA initialization status need to be checked before attempting to
determine the action (measure, appraise, etc.) and any related options
specified in the IMA policy.
This patch defines a flag namely ima_initialized to track
IMA initialization status.
ima_policy_flag cannot be relied upon for knowing IMA initialization
status because ima_policy_flag will be set to 0 when either IMA
is not initialized or the IMA policy itself is empty.
Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
security/integrity/ima/ima.h | 1 +
security/integrity/ima/ima_init.c | 9 ++++++++-
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index f15199f7ff2a..6a86daa62c5b 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -52,6 +52,7 @@ extern int ima_policy_flag;
extern int ima_hash_algo;
extern int ima_appraise;
extern struct tpm_chip *ima_tpm_chip;
+extern bool ima_initialized;
/* IMA event related data */
struct ima_event_data {
diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index 5d55ade5f3b9..a810af6df587 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -23,6 +23,7 @@
/* name for boot aggregate entry */
static const char boot_aggregate_name[] = "boot_aggregate";
struct tpm_chip *ima_tpm_chip;
+bool ima_initialized;
/* Add the boot aggregate to the IMA measurement list and extend
* the PCR register.
@@ -131,5 +132,11 @@ int __init ima_init(void)
ima_init_policy();
- return ima_fs_init();
+ rc = ima_fs_init();
+ if (rc != 0)
+ return rc;
+
+ ima_initialized = true;
+
+ return 0;
}
--
2.17.1
^ permalink raw reply related
* [PATCH v4 10/10] KEYS: Call the IMA hook to measure key when a new key is created or an existing key is updated
From: Lakshmi Ramasubramanian @ 2019-11-06 19:01 UTC (permalink / raw)
To: zohar, dhowells, matthewgarrett, sashal, jamorris,
linux-integrity, linux-security-module, keyrings, linux-kernel
In-Reply-To: <20191106190116.2578-1-nramas@linux.microsoft.com>
key_create_or_update function needs to call the IMA hook to measure
the key when a new key is created or an existing key is updated.
This patch adds the call to the IMA hook from key_create_or_update
function.
Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
include/linux/ima.h | 8 ++++++++
security/keys/key.c | 9 +++++++++
2 files changed, 17 insertions(+)
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 6d904754d858..069879242a15 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -24,6 +24,9 @@ extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
enum kernel_read_file_id id);
extern void ima_post_path_mknod(struct dentry *dentry);
extern void ima_kexec_cmdline(const void *buf, int size);
+extern void ima_post_key_create_or_update(struct key *keyring,
+ struct key *key,
+ unsigned long flags, bool create);
#ifdef CONFIG_IMA_KEXEC
extern void ima_add_kexec_buffer(struct kimage *image);
@@ -92,6 +95,11 @@ static inline void ima_post_path_mknod(struct dentry *dentry)
}
static inline void ima_kexec_cmdline(const void *buf, int size) {}
+
+static inline void ima_post_key_create_or_update(struct key *keyring,
+ struct key *key,
+ unsigned long flags,
+ bool create) {}
#endif /* CONFIG_IMA */
#ifndef CONFIG_IMA_KEXEC
diff --git a/security/keys/key.c b/security/keys/key.c
index 764f4c57913e..9782d4d046fd 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -13,6 +13,7 @@
#include <linux/security.h>
#include <linux/workqueue.h>
#include <linux/random.h>
+#include <linux/ima.h>
#include <linux/err.h>
#include "internal.h"
@@ -936,6 +937,9 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
goto error_link_end;
}
+ /* let the ima module know about the created key. */
+ ima_post_key_create_or_update(keyring, key, flags, true);
+
key_ref = make_key_ref(key, is_key_possessed(keyring_ref));
error_link_end:
@@ -965,6 +969,11 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
}
key_ref = __key_update(key_ref, &prep);
+
+ /* let the ima module know about the updated key. */
+ if (!IS_ERR(key_ref))
+ ima_post_key_create_or_update(keyring, key, flags, false);
+
goto error_free_prep;
}
EXPORT_SYMBOL(key_create_or_update);
--
2.17.1
^ permalink raw reply related
* [PATCH v4 05/10] IMA: Updated IMA policy functions to return keyrings option read from the policy
From: Lakshmi Ramasubramanian @ 2019-11-06 19:01 UTC (permalink / raw)
To: zohar, dhowells, matthewgarrett, sashal, jamorris,
linux-integrity, linux-security-module, keyrings, linux-kernel
In-Reply-To: <20191106190116.2578-1-nramas@linux.microsoft.com>
keyrings option read from the IMA policy needs to be provided to
the callers that determine the action to be performed.
This patch updates ima_get_action() and ima_match_policy() functions
to return the keyrings option specified in the IMA policy.
Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
security/integrity/ima/ima.h | 6 ++++--
security/integrity/ima/ima_api.c | 8 +++++---
security/integrity/ima/ima_appraise.c | 2 +-
security/integrity/ima/ima_main.c | 5 +++--
security/integrity/ima/ima_policy.c | 7 ++++++-
5 files changed, 19 insertions(+), 9 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 7f23405b2718..387829afb9a2 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -208,7 +208,8 @@ struct modsig;
/* LIM API function definitions */
int ima_get_action(struct inode *inode, const struct cred *cred, u32 secid,
int mask, enum ima_hooks func, int *pcr,
- struct ima_template_desc **template_desc);
+ struct ima_template_desc **template_desc,
+ char **keyrings);
int ima_must_measure(struct inode *inode, int mask, enum ima_hooks func);
int ima_collect_measurement(struct integrity_iint_cache *iint,
struct file *file, void *buf, loff_t size,
@@ -235,7 +236,8 @@ const char *ima_d_path(const struct path *path, char **pathbuf, char *filename);
/* IMA policy related functions */
int ima_match_policy(struct inode *inode, const struct cred *cred, u32 secid,
enum ima_hooks func, int mask, int flags, int *pcr,
- struct ima_template_desc **template_desc);
+ struct ima_template_desc **template_desc,
+ char **keyrings);
void ima_init_policy(void);
void ima_update_policy(void);
void ima_update_policy_flag(void);
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 610759fe63b8..fa2cd71ddf1a 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -169,12 +169,13 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
* @func: caller identifier
* @pcr: pointer filled in if matched measure policy sets pcr=
* @template_desc: pointer filled in if matched measure policy sets template=
+ * @keyrings: pointer filled in if matched measure policy sets keyrings=
*
* The policy is defined in terms of keypairs:
* subj=, obj=, type=, func=, mask=, fsmagic=
* subj,obj, and type: are LSM specific.
* func: FILE_CHECK | BPRM_CHECK | CREDS_CHECK | MMAP_CHECK | MODULE_CHECK
- * | KEXEC_CMDLINE
+ * | KEXEC_CMDLINE | KEYRING_CHECK
* mask: contains the permission mask
* fsmagic: hex value
*
@@ -183,14 +184,15 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
*/
int ima_get_action(struct inode *inode, const struct cred *cred, u32 secid,
int mask, enum ima_hooks func, int *pcr,
- struct ima_template_desc **template_desc)
+ struct ima_template_desc **template_desc,
+ char **keyrings)
{
int flags = IMA_MEASURE | IMA_AUDIT | IMA_APPRAISE | IMA_HASH;
flags &= ima_policy_flag;
return ima_match_policy(inode, cred, secid, func, mask, flags, pcr,
- template_desc);
+ template_desc, keyrings);
}
/*
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 300c8d2943c5..47ad4f56c0a8 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -55,7 +55,7 @@ int ima_must_appraise(struct inode *inode, int mask, enum ima_hooks func)
security_task_getsecid(current, &secid);
return ima_match_policy(inode, current_cred(), secid, func, mask,
- IMA_APPRAISE | IMA_HASH, NULL, NULL);
+ IMA_APPRAISE | IMA_HASH, NULL, NULL, NULL);
}
static int ima_fix_xattr(struct dentry *dentry,
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index a0e233afe876..b6d17f37ba61 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -215,7 +215,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
* Included is the appraise submask.
*/
action = ima_get_action(inode, cred, secid, mask, func, &pcr,
- &template_desc);
+ &template_desc, NULL);
violation_check = ((func == FILE_CHECK || func == MMAP_CHECK) &&
(ima_policy_flag & IMA_MEASURE));
if (!action && !violation_check)
@@ -647,6 +647,7 @@ void process_buffer_measurement(const void *buf, int size,
.buf = buf,
.buf_len = size};
struct ima_template_desc *template = NULL;
+ char *keyrings = NULL;
struct {
struct ima_digest_data hdr;
char digest[IMA_MAX_DIGEST_SIZE];
@@ -665,7 +666,7 @@ void process_buffer_measurement(const void *buf, int size,
if (func) {
security_task_getsecid(current, &secid);
action = ima_get_action(NULL, current_cred(), secid, 0, func,
- &pcr, &template);
+ &pcr, &template, &keyrings);
if (!(action & IMA_MEASURE))
return;
}
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 74a727dc6030..53379a19de43 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -481,6 +481,7 @@ static int get_subaction(struct ima_rule_entry *rule, enum ima_hooks func)
* @mask: requested action (MAY_READ | MAY_WRITE | MAY_APPEND | MAY_EXEC)
* @pcr: set the pcr to extend
* @template_desc: the template that should be used for this rule
+ * @keyrings: set the keyrings for this rule, if specified
*
* Measure decision based on func/mask/fsmagic and LSM(subj/obj/type)
* conditions.
@@ -491,7 +492,8 @@ static int get_subaction(struct ima_rule_entry *rule, enum ima_hooks func)
*/
int ima_match_policy(struct inode *inode, const struct cred *cred, u32 secid,
enum ima_hooks func, int mask, int flags, int *pcr,
- struct ima_template_desc **template_desc)
+ struct ima_template_desc **template_desc,
+ char **keyrings)
{
struct ima_rule_entry *entry;
int action = 0, actmask = flags | (flags << 1);
@@ -527,6 +529,9 @@ int ima_match_policy(struct inode *inode, const struct cred *cred, u32 secid,
if ((pcr) && (entry->flags & IMA_PCR))
*pcr = entry->pcr;
+ if ((keyrings) && (entry->flags & IMA_KEYRINGS))
+ *keyrings = entry->keyrings;
+
if (template_desc && entry->template)
*template_desc = entry->template;
--
2.17.1
^ permalink raw reply related
* [PATCH v4 06/10] IMA: Measure key if the IMA policy allows measurement for the keyring to which the key is linked to
From: Lakshmi Ramasubramanian @ 2019-11-06 19:01 UTC (permalink / raw)
To: zohar, dhowells, matthewgarrett, sashal, jamorris,
linux-integrity, linux-security-module, keyrings, linux-kernel
In-Reply-To: <20191106190116.2578-1-nramas@linux.microsoft.com>
process_buffer_measurement() needs to check if the keyring to which
the given key is linked to is listed in the keyrings option in
the IMA policy.
This patch adds a new parameter "keyring" to process_buffer_measurement().
If process_buffer_measurement() is called with func KEYRING_CHECK and
the name of the keyring to which the key is linked to, then the given
key is measured if:
1, IMA policy did not specify "keyrings=" option.
2, Or, the given keyring name is listed in the "keyrings=" option.
Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
security/integrity/ima/ima.h | 2 +-
security/integrity/ima/ima_appraise.c | 2 +-
security/integrity/ima/ima_main.c | 26 ++++++++++++++++++++++++--
3 files changed, 26 insertions(+), 4 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 387829afb9a2..f15199f7ff2a 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -221,7 +221,7 @@ void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file,
struct ima_template_desc *template_desc);
void process_buffer_measurement(const void *buf, int size,
const char *eventname, enum ima_hooks func,
- int pcr);
+ int pcr, const char *keyring);
void ima_audit_measurement(struct integrity_iint_cache *iint,
const unsigned char *filename);
int ima_alloc_init_template(struct ima_event_data *event_data,
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 47ad4f56c0a8..a9649b04b9f1 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -330,7 +330,7 @@ int ima_check_blacklist(struct integrity_iint_cache *iint,
if ((rc == -EPERM) && (iint->flags & IMA_MEASURE))
process_buffer_measurement(digest, digestsize,
"blacklisted-hash", NONE,
- pcr);
+ pcr, NULL);
}
return rc;
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index b6d17f37ba61..56540357c854 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -632,12 +632,22 @@ int ima_load_data(enum kernel_load_data_id id)
* @eventname: event name to be used for the buffer entry.
* @func: IMA hook
* @pcr: pcr to extend the measurement
+ * @keyring: keyring for the measurement
+ *
+ * The following scenarios are possible with respect to
+ * the parameter "keyring":
+ * 1, keyring is NULL. In this case buffer is measured.
+ * 2, keyring is not NULL, but ima_get_action returned
+ * a NULL keyrings. In this case also the buffer is measured.
+ * 3, keyring is not NULL and ima_get_action returned
+ * a non-NULL keyrings. In this case measure the buffer
+ * only if the given keyring is present in the keyrings.
*
* Based on policy, the buffer is measured into the ima log.
*/
void process_buffer_measurement(const void *buf, int size,
const char *eventname, enum ima_hooks func,
- int pcr)
+ int pcr, const char *keyring)
{
int ret = 0;
struct ima_template_entry *entry = NULL;
@@ -656,6 +666,13 @@ void process_buffer_measurement(const void *buf, int size,
int action = 0;
u32 secid;
+ /*
+ * If IMA is not yet initialized or IMA policy is empty
+ * then there is no need to measure.
+ */
+ if (!ima_policy_flag)
+ return;
+
/*
* Both LSM hooks and auxilary based buffer measurements are
* based on policy. To avoid code duplication, differentiate
@@ -671,6 +688,11 @@ void process_buffer_measurement(const void *buf, int size,
return;
}
+ if ((keyring != NULL) && (keyrings != NULL)
+ && (strstr(keyrings, keyring) == NULL)) {
+ return;
+ }
+
if (!pcr)
pcr = CONFIG_IMA_MEASURE_PCR_IDX;
@@ -719,7 +741,7 @@ void ima_kexec_cmdline(const void *buf, int size)
{
if (buf && size != 0)
process_buffer_measurement(buf, size, "kexec-cmdline",
- KEXEC_CMDLINE, 0);
+ KEXEC_CMDLINE, 0, NULL);
}
/**
--
2.17.1
^ permalink raw reply related
* [PATCH v4 03/10] IMA: Added keyrings= option in IMA policy to only measure keys added to the specified keyrings.
From: Lakshmi Ramasubramanian @ 2019-11-06 19:01 UTC (permalink / raw)
To: zohar, dhowells, matthewgarrett, sashal, jamorris,
linux-integrity, linux-security-module, keyrings, linux-kernel
In-Reply-To: <20191106190116.2578-1-nramas@linux.microsoft.com>
IMA policy needs to support measuring only those keys linked to
a specific set of keyrings.
This patch defines a new IMA policy option namely "keyrings=" that
can be used to specify a set of keyrings. If this option is specified
in the policy for func=KEYRING_CHECK then only the keys linked to
the keyrings given in "keyrings=" option are measured.
If "keyrings=" option is not specified for func=KEYRING_CHECK then
all keys are measured.
Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
Documentation/ABI/testing/ima_policy | 10 +++++++++-
security/integrity/ima/ima_policy.c | 2 ++
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index 341df49b5ad1..be2874fa3928 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -25,7 +25,7 @@ Description:
lsm: [[subj_user=] [subj_role=] [subj_type=]
[obj_user=] [obj_role=] [obj_type=]]
option: [[appraise_type=]] [template=] [permit_directio]
- [appraise_flag=]
+ [appraise_flag=] [keyrings=]
base: func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
[FIRMWARE_CHECK]
[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
@@ -43,6 +43,9 @@ Description:
appraise_flag:= [check_blacklist]
Currently, blacklist check is only for files signed with appended
signature.
+ keyrings:= list of keyrings
+ (eg, .builtin_trusted_keys|.ima). Only valid
+ when action is "measure" and func is KEYRING_CHECK.
template:= name of a defined IMA template type
(eg, ima-ng). Only valid when action is "measure".
pcr:= decimal value
@@ -119,3 +122,8 @@ Description:
all keys:
measure func=KEYRING_CHECK
+
+ Example of measure rule using KEYRING_CHECK to only measure
+ keys added to .builtin_trusted_keys or .ima keyring:
+
+ measure func=KEYRING_CHECK keyrings=.builtin_trusted_keys|.ima
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 4344b7354fc5..4d68ad8ed91c 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -34,6 +34,7 @@
#define IMA_EUID 0x0080
#define IMA_PCR 0x0100
#define IMA_FSNAME 0x0200
+#define IMA_KEYRINGS 0x0400
#define UNKNOWN 0
#define MEASURE 0x0001 /* same as IMA_MEASURE */
@@ -79,6 +80,7 @@ struct ima_rule_entry {
int type; /* audit type */
} lsm[MAX_LSM_RULES];
char *fsname;
+ char *keyrings; /* Measure keys added to these keyrings */
struct ima_template_desc *template;
};
--
2.17.1
^ permalink raw reply related
* [PATCH v4 01/10] IMA: Defined an IMA hook to measure keys on key create or update
From: Lakshmi Ramasubramanian @ 2019-11-06 19:01 UTC (permalink / raw)
To: zohar, dhowells, matthewgarrett, sashal, jamorris,
linux-integrity, linux-security-module, keyrings, linux-kernel
In-Reply-To: <20191106190116.2578-1-nramas@linux.microsoft.com>
Asymmetric keys used for verifying file signatures or certificates
are currently not included in the IMA measurement list.
This patch defines a new IMA hook namely ima_post_key_create_or_update()
to measure asymmetric keys.
Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
security/integrity/ima/ima_main.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index d7e987baf127..a0e233afe876 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -721,6 +721,22 @@ void ima_kexec_cmdline(const void *buf, int size)
KEXEC_CMDLINE, 0);
}
+/**
+ * ima_post_key_create_or_update - measure asymmetric keys
+ * @keyring: keyring to which the key is linked to
+ * @key: created or updated key
+ * @flags: key flags
+ * @create: flag indicating whether the key was created or updated
+ *
+ * Keys can only be measured, not appraised.
+ */
+void ima_post_key_create_or_update(struct key *keyring, struct key *key,
+ unsigned long flags, bool create)
+{
+ if ((keyring != NULL) && (key != NULL))
+ return;
+}
+
static int __init init_ima(void)
{
int error;
--
2.17.1
^ permalink raw reply related
* [PATCH v4 02/10] IMA: Added KEYRING_CHECK func in IMA policy to measure keys
From: Lakshmi Ramasubramanian @ 2019-11-06 19:01 UTC (permalink / raw)
To: zohar, dhowells, matthewgarrett, sashal, jamorris,
linux-integrity, linux-security-module, keyrings, linux-kernel
In-Reply-To: <20191106190116.2578-1-nramas@linux.microsoft.com>
IMA policy needs to support a func to enable measurement of
asymmetric keys.
This patch defines a new IMA policy func namely KEYRING_CHECK to
measure asymmetric keys.
Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
Documentation/ABI/testing/ima_policy | 6 ++++++
security/integrity/ima/ima.h | 1 +
security/integrity/ima/ima_policy.c | 2 +-
3 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index 29aaedf33246..341df49b5ad1 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -30,6 +30,7 @@ Description:
[FIRMWARE_CHECK]
[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
[KEXEC_CMDLINE]
+ [KEYRING_CHECK]
mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
[[^]MAY_EXEC]
fsmagic:= hex value
@@ -113,3 +114,8 @@ Description:
Example of appraise rule allowing modsig appended signatures:
appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig
+
+ Example of measure rule using KEYRING_CHECK to measure
+ all keys:
+
+ measure func=KEYRING_CHECK
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index df4ca482fb53..7f23405b2718 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -193,6 +193,7 @@ static inline unsigned long ima_hash_key(u8 *digest)
hook(KEXEC_INITRAMFS_CHECK) \
hook(POLICY_CHECK) \
hook(KEXEC_CMDLINE) \
+ hook(KEYRING_CHECK) \
hook(MAX_CHECK)
#define __ima_hook_enumify(ENUM) ENUM,
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index f19a895ad7cd..4344b7354fc5 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -373,7 +373,7 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
{
int i;
- if (func == KEXEC_CMDLINE) {
+ if ((func == KEXEC_CMDLINE) || (func == KEYRING_CHECK)) {
if ((rule->flags & IMA_FUNC) && (rule->func == func))
return true;
return false;
--
2.17.1
^ permalink raw reply related
* Re: [PATCH bpf-next v13 4/7] landlock: Add ptrace LSM hooks
From: KP Singh @ 2019-11-06 21:45 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Alexei Starovoitov, linux-kernel, Alexei Starovoitov,
Andy Lutomirski, Casey Schaufler, Daniel Borkmann, David Drysdale,
Florent Revest, James Morris, Jann Horn, John Johansen,
Jonathan Corbet, Kees Cook, Michael Kerrisk,
Mickaël Salaün, Paul Moore, Sargun Dhillon,
Serge E . Hallyn, Shuah Khan, Stephen Smalley, Tejun Heo,
Tetsuo Handa, Tycho Andersen, Will Drewry, bpf, kernel-hardening,
linux-api, linux-security-module, netdev
In-Reply-To: <813cedde-8ed7-2d3b-883d-909efa978d41@digikod.net>
On 06-Nov 17:55, Mickaël Salaün wrote:
>
> On 06/11/2019 11:06, KP Singh wrote:
> > On 05-Nov 11:34, Alexei Starovoitov wrote:
> >> On Tue, Nov 05, 2019 at 07:01:41PM +0100, Mickaël Salaün wrote:
> >>> On 05/11/2019 18:18, Alexei Starovoitov wrote:
>
> [...]
>
> >>>> I think the only way bpf-based LSM can land is both landlock and KRSI
> >>>> developers work together on a design that solves all use cases.
> >>>
> >>> As I said in a previous cover letter [1], that would be great. I think
> >>> that the current Landlock bases (almost everything from this series
> >>> except the seccomp interface) should meet both needs, but I would like
> >>> to have the point of view of the KRSI developers.
> >>>
> >>> [1] https://lore.kernel.org/lkml/20191029171505.6650-1-mic@digikod.net/
> >>>
> >>>> BPF is capable
> >>>> to be a superset of all existing LSMs whereas landlock and KRSI propsals today
> >>>> are custom solutions to specific security concerns. BPF subsystem was extended
> >>>> with custom things in the past. In networking we have lwt, skb, tc, xdp, sk
> >>>> program types with a lot of overlapping functionality. We couldn't figure out
> >>>> how to generalize them into single 'networking' program. Now we can and we
> >>>> should. Accepting two partially overlapping bpf-based LSMs would be repeating
> >>>> the same mistake again.
> >>>
> >>> I'll let the LSM maintainers comment on whether BPF could be a superset
> >>> of all LSM, but given the complexity of an access-control system, I have
> >>> some doubts though. Anyway, we need to start somewhere and then iterate.
> >>> This patch series is a first step.
> >>
> >> I would like KRSI folks to speak up. So far I don't see any sharing happening
> >> between landlock and KRSI. You're claiming this set is a first step. They're
> >> claiming the same about their patches. I'd like to set a patchset that was
> >> jointly developed.
> >
> > We are willing to collaborate with the Landlock developers and come up
> > with a common approach that would work for Landlock and KRSI. I want
> > to mention that this collaboration and the current Landlock approach
> > of using an eBPF based LSM for unprivileged sandboxing only makes sense
> > if unprivileged usage of eBPF is going to be ever allowed.
>
> The ability to *potentially* do unprivileged sandboxing is definitely
> not tied nor a blocker to the unprivileged usage of eBPF. As explained
> in the documentation [1] (cf. Guiding principles / Unprivileged use),
> Landlock is designed to be as safe as possible (from a security point of
> view). The impact is more complex and important than just using
> unprivileged eBPF, which may not be required. Unprivileged use of eBPF
> would be nice, but I think the current direction is to extend the Linux
> capabilities with one or multiple dedicated to eBPF [2] (e.g. CAP_BPF +
> something else), which may be even better (and a huge difference with
> CAP_SYS_ADMIN, a.k.a. privileged mode or root). Landlock is designed to
> deal with unprivileged (i.e. non-root) use cases, but of course, if the
> Landlock architecture may enable to do unprivileged stuff, it definitely
> can do privileged stuff too. However, having an architecture designed
> with safe unprivileged use in mind can't be achieve afterwards.
>
> [1] https://lore.kernel.org/lkml/20191104172146.30797-8-mic@digikod.net/
> [2] https://lore.kernel.org/bpf/20190827205213.456318-1-ast@kernel.org/
>
>
> >
> > Purely from a technical standpoint, both the current designs for
> > Landlock and KRSI target separate use cases and it would not be
> > possible to build "one on top of the other". We've tried to identify
> > the lowest denominator ("eBPF+LSM") requirements for both Landlock
> > (unprivileged sandboxing / Discretionary Access Control) and KRSI
> > (flexibility and unification of privileged MAC and Audit) and
> > prototyped an implementation based on the newly added / upcoming
> > features in BPF.
>
> This is not as binary as that. Sandboxing can be seen as DAC but also as
> MAC, depending on the subject which apply the security policy and the
> subjects which are enforced by this policy. If the sandboxing is applied
> system-wide, it is what we usually call MAC. DAC, in the Linux world,
> enables any user to restrict access to their files to other users.
>
> With Landlock it is not the same because a process can restrict itself
> but also enforce these restrictions on all its future children (which
> may be malicious, whatever their UID/GID). The threat and the definition
> of the attacker are not the same in both cases. With the Linux DAC the
> potentially malicious subjects are the other users, whereas with
> Landlock the potentially malicious subjects are (for now) the current
> process and all its children. Another way to explain it, and how
> Landlock is designed, is that a specific enforcement (i.e. a set of BPF
> programs) is tied to a domain, in which a set of subject are. From this
> perspective, this approach (subjects/tasks in a domain) is orthogonal to
> the DAC system (subjects/users). This design may apply to a system-wide
> MAC system by putting all the system tasks in one domain, and managing
> restrictions (by subject) with other means (e.g. task's UID,
> command-line strings, environment variables). In short, Landlock (in
> this patch series) is closer to a (potentially scoped) MAC system. But
> thanks to eBPF, Landlock is firstly a programmatic access-control, which
> means that the one who write the programs and tie them to a set of
> tasks, can implement their own access-control system (e.g. RBAC,
> time-based…), or something else (e.g. an audit system).
>
> The audit part can simply be achieve with dedicated helpers and programs
> that always allow accesses.
>
> Landlock evolved over multiple iterations and is now designed to be very
> flexible. The current way to enforce a security policy is to go through
> the seccomp syscall (which makes sense for multiple reasons explained
> and accepted before). But Landlock is designed to enable similar
> enforcements (or audit) with other ways to define a domain (e.g. cgroups
> [3], or system-wide securityfs as done in KRSI). Indeed, the only part
> tied to this scoped enforcement is in the domain_syscall.c file. A new
> file domain_fs.c could be added to implement a securityfs for a
> system-wide enforcement (and have other features as KRSI does).
>
Given the current way landlock exposes LSM hooks, I don't think it's
possible to build system-wide detections. But let’s try to come to a
consensus on the semantics of the how the LSM hooks are exposed to
BPF. At the moment I think we should:
* Bring the core interface exposed to eBPF closer to the LSM surface in
a way that supports both use cases. One way Landlock can still provide
a more abstract interface is by providing some BPF helper libraries
that build on top of the core framework.
* Use a single BPF program type; this is necessary for a key requirement
of KRSI, i.e. runtime instrumentation. The upcoming prototype should
illustrate how this works for KRSI - note that it’s possible to vary
the context types exposed by different hooks.
It would be nice to get the BPF maintainers’ opinion on these points.
> [3] https://lore.kernel.org/lkml/20160914072415.26021-17-mic@digikod.net/
>
> One possible important difference between Landlock and KRSI right now is
> the BPF program management. Both manage a list of programs per hook.
> However KRSI needs to be able to replace a program in these lists. This
> is not implemented in this Landlock patch series, first because it is
> not the main use-case and it is safer to have an append-only way to add
> restrictions (cf. seccomp-bpf model), and second because it is simpler
> to deal with immutable lists. However, it is worth considering extending
> the Landlock domain management with the ability to update the program
> lists. One challenge may be to identify which program should be replaced
> (which KRSI does with the program name). I think it would be wiser to
> implement this in a second step though, maybe not for the syscall
> interface (thanks to a new seccomp operation), but first with the
> securityfs one.
>
>
> >
> > We've been successfully able to prototype the use cases for KRSI
> > (privileged MAC and Audit) using this "eBPF+LSM" and shared our
> > approach at the Linux Security Summit [1]:
> >
> > * Use the new in-kernel BTF (CO-RE eBPF programs) [2] and the ability
> > of the BPF verifier to use the BTF information for access validation
> > to provide a more generic way to attach to the various LSM hooks.
> > This potentially saves a lot of redundant work:
> >
> > - Creation of new program types.
> > - Multiple types of contexts (or a single context with Unions).
> > - Changes to the verifier and creation of new BPF argument types
> > (eg. PTR_TO_TASK)
>
> As I understood from the LSS talk, KRSI's approach is to use the same
> hooks as LSM (cf. the securityfs). As said Alexei [4] "It must not make
> LSM hooks into stable ABI". Moveover, the LSM hooks may change
> according to internal kernel evolution, and their semantic may not make
I think you misunderstand Alexei here. I will let him elaborate.
> sense from a user space point of view. This is one reason for which
> Lanlock abstract those hooks into something that is simpler and designed
> to fit well with eBPF (program contexts and their attached types, as
> explained in the documentation).
>
> [4]
> https://lore.kernel.org/lkml/20191105215453.szhdkrvuekwfz6le@ast-mbp.dhcp.thefacebook.com/
>
> How does KRSI plan to deal with one LSM hook being split in two hooks in
> a next version of the kernel (cf. [5])?
How often has that happened in the past? And even if it does happen,
it can still be handled as a part of the base framework we are trying
to implement.
>
> [5] https://lore.kernel.org/lkml/20190910115527.5235-6-kpsingh@chromium.org/
>
>
> Another reason to have multiple different attach types/contexts (cf.
> landlock_domain->programs[]) is to limit useless BPF program
> interpretation (in addition to the non-system-wide scoped of programs).
> It also enables to handle and verify strict context use (which is also
> explain in the Guiding principles). It would be a huge wast of time to
> run every BPF programs for all LSM hooks. KRSI does the same but instead
> of relying on the program type it rely on the list tied to the
> securityfs file.
>
> BTF is great, but as far as I know, it's goal is to easily deal with the
> moving kernel ABI (e.g. task_struct layout, config/constant variables),
> and it is definitely useful to programs using bpf_probe_read() and
> similar accessors. However, I don't see how KRSI would avoid BPF types
> thanks to BTF.
>
This should become clearer once we post our updated patch-set. Do note
that I am currently traveling and will be away for the next couple of
weeks.
> There is only one program type for Landlock (i.e.
> BPF_PROG_TYPE_LANDLOCK_HOOK), and I don't see why adding new program
> *attach* types (e.g. BPF_LANDLOCK_PTRACE) may be an issue. The kernel
> will still need to be modified to implement new hooks and the new BPF
> helpers anyway, BTF will not change that, except maybe if the internal
> LSM API is exposed in a way or another to BPF (thanks to BTF), which
> does not seem acceptable. Am I missing something?
>
>
> The current KRSI approach is to allow a common set of helpers to be
> called by all programs (because there is no way to differentiate them
> with their type).
> How KRSI would deal with kernel objects other than the current task
> (e.g. a ptrace hook with a tracer and a tracee, a file open/read) with
> the struct krsi_ctx unions [6]?
>
> [6] https://lore.kernel.org/lkml/20190910115527.5235-7-kpsingh@chromium.org/
>
The best part of BTF is that it can provide a common way to pass
different contexts to the various attachments points and the verifier
can use the BTF information to validate accesses which essentially
allows us to change the helpers from:
is_running_executable(magical_krsi_ctx)
to
is_running_executable(inode)
which can work on any inode (ARG_PTR_TO_BTF_ID = btf_id(struct inode))
This makes the helpers much more useful and generic. All this is
better explained in our upcoming patch-set.
>
> How does KRSI plan to deal with security blobs?
The new prototype uses security blobs but does not expose them to
user-space. These blobs are then used in various helpers like
“is_running_executable” which uses blobs on the inode and the
task_struct. This should become clearer when the next patchset is
posted.
I don’t think it’s currently possible to allow the blobs to be set
using eBPF programs with the main reason being that the blob will only
be set after the program is loaded. The answer to
“is_running_executable” becomes dependent on whether the file was
executed before the blob setting eBPF program was loaded.
Blob management with eBPF is not possible unless we can load eBPF
programs that can set blobs at boot-time.
In short, the next KRSI version will not give eBPF
programs access to arbitrarily write security blobs.
>
>
> >
> > * These new BPF features also alleviate the original concerns that we
> > raised when initially proposing KRSI and designing for precise BPF
> > helpers. We have some patches coming up which incorporate these new
> > changes and will be sharing something on the mailing list after some
> > cleanup.
> >
> > We can use the common "eBPF+LSM" for both privileged MAC and Audit and
> > unprivileged sandboxing i.e. Discretionary Access Control.
> > Here's what it could look like:
> >
> > * Common infrastructure allows attachment to all hooks which works well
> > for privileged MAC and Audit. This could be extended to provide
> > another attachment type for unprivileged DAC, which can restrict the
> > hooks that can be attached to, and also the information that is
> > exposed to the eBPF programs which is something that Landlock could
> > build.
>
> I agree that the "privileged-only" hooks should be a superset of the
> "security-safe-and-potentially-unprivileged" hooks. :)
> However, as said previously, I'm convinced it is a requirement to have
> abstract hooks (and associated program attach types) as defined by Landlock.
I would like to hear the BPF maintainers’ perspective on this. I am
not sure they agree with you here.
- KP Singh
>
> I'm not sure what you mean by "the information that is exposed to the
> eBPF program". Is it the current Landlock implementation of specific
> contexts and attach types?
>
> >
> > * This attachment could use the proposed landlock domains and attach to
> > the task_struct providing the discretionary access control semantics.
>
> Not task_struct but creds, yes. This is a characteristic of sandboxing,
> which may not be useful for the KRSI use case. It makes sense for KRSI
> to attach program sets (or Landlock domains) to the whole system, then
> using the creds does not make sense here. This difference is small and a
> previous version of Landlock already validated this use case with
> cgroups [3] (which is postponed to simplify the patch series).
>
> [3] https://lore.kernel.org/lkml/20160914072415.26021-17-mic@digikod.net/
>
>
> >
> > [1] https://static.sched.com/hosted_files/lsseu2019/a2/Kernel%20Runtime%20Security%20Instrumentation.pdf
> > [2] http://vger.kernel.org/bpfconf2019_talks/bpf-core.pdf
> >
> > - KP Singh
>
> I think it should be OK to first land something close to this Landlock
> patch series and then we could extend the domain management features and
> add the securityfs support that KRSI needs. The main concern seems to be
> about hook definitions.
>
> Another approach would be to land Landlock and KRSI as distinct LSM
> while trying as much as possible to mutualize code/helpers.
^ permalink raw reply
* Re: [PATCH v4 08/10] IMA: Defined functions to queue and dequeue keys for measurement
From: Mimi Zohar @ 2019-11-06 22:44 UTC (permalink / raw)
To: Lakshmi Ramasubramanian, dhowells, matthewgarrett, sashal,
jamorris, linux-integrity, linux-security-module, keyrings,
linux-kernel
In-Reply-To: <20191106190116.2578-9-nramas@linux.microsoft.com>
On Wed, 2019-11-06 at 11:01 -0800, Lakshmi Ramasubramanian wrote:
> +int ima_queue_or_process_key_for_measurement(struct key *keyring,
> + struct key *key)
> +{
> + int rc = 0;
> + struct ima_measure_key_entry *entry = NULL;
> + const struct public_key *pk;
> +
> + if (key->type != &key_type_asymmetric)
> + return 0;
> +
> + mutex_lock(&ima_measure_keys_mutex);
Unless the key is being queued, there's no reason to take the lock.
> +
> + if (ima_initialized) {
ima_initialized is being set in ima_init(), before a custom policy is
loaded. I would think that is too early. ima_update_policy() is
called after loading a custom policy. Please see how to detect when a
custom policy is loaded.
> + /*
> + * keyring->description points to the name of the keyring
> + * (such as ".builtin_trusted_keys", ".ima", etc.) to
> + * which the given key is linked to.
> + *
> + * The name of the keyring is passed in the "eventname"
> + * parameter to process_buffer_measurement() and is set
> + * in the "eventname" field in ima_event_data for
> + * the key measurement IMA event.
> + *
> + * The name of the keyring is also passed in the "keyring"
> + * parameter to process_buffer_measurement() to check
> + * if the IMA policy is configured to measure a key linked
> + * to the given keyring.
> + */
> + pk = key->payload.data[asym_crypto];
> + process_buffer_measurement(pk->key, pk->keylen,
> + keyring->description,
> + KEYRING_CHECK, 0,
> + keyring->description);
Measuring the key should be done in ima_post_key_create_or_update()
unless, it is being deferred. Please update the function name to
reflect this.
Mimi
> + } else {
> + entry = ima_alloc_measure_key_entry(keyring, key);
> + if (entry != NULL) {
> + INIT_LIST_HEAD(&entry->list);
> + list_add_tail(&entry->list, &ima_measure_keys);
> + } else
> + rc = -ENOMEM;
> + }
> +
> + mutex_unlock(&ima_measure_keys_mutex);
> +
> + return rc;
> +}
^ permalink raw reply
* Re: [PATCH v4 01/10] IMA: Defined an IMA hook to measure keys on key create or update
From: Mimi Zohar @ 2019-11-06 22:43 UTC (permalink / raw)
To: Lakshmi Ramasubramanian, dhowells, matthewgarrett, sashal,
jamorris, linux-integrity, linux-security-module, keyrings,
linux-kernel
In-Reply-To: <20191106190116.2578-2-nramas@linux.microsoft.com>
On Wed, 2019-11-06 at 11:01 -0800, Lakshmi Ramasubramanian wrote:
> Asymmetric keys used for verifying file signatures or certificates
> are currently not included in the IMA measurement list.
>
> This patch defines a new IMA hook namely ima_post_key_create_or_update()
> to measure asymmetric keys.
>
> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> ---
> security/integrity/ima/ima_main.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index d7e987baf127..a0e233afe876 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -721,6 +721,22 @@ void ima_kexec_cmdline(const void *buf, int size)
> KEXEC_CMDLINE, 0);
> }
>
> +/**
> + * ima_post_key_create_or_update - measure asymmetric keys
> + * @keyring: keyring to which the key is linked to
> + * @key: created or updated key
> + * @flags: key flags
> + * @create: flag indicating whether the key was created or updated
> + *
> + * Keys can only be measured, not appraised.
> + */
> +void ima_post_key_create_or_update(struct key *keyring, struct key *key,
> + unsigned long flags, bool create)
> +{
> + if ((keyring != NULL) && (key != NULL))
> + return;
I would move the patch that defines the "keyring=" policy option prior
to this one. Include the call to process_buffer_measurement() in this
patch. A subsequent patch would add support to defer measuring the
key, by calling a function named something like
ima_queue_key_measurement().
Mimi
> +}
> +
> static int __init init_ima(void)
> {
> int error;
^ permalink raw reply
* Re: [PATCH v4 08/10] IMA: Defined functions to queue and dequeue keys for measurement
From: Lakshmi Ramasubramanian @ 2019-11-06 23:52 UTC (permalink / raw)
To: Mimi Zohar, dhowells, matthewgarrett, sashal, jamorris,
linux-integrity, linux-security-module, keyrings, linux-kernel
In-Reply-To: <1573080281.5028.314.camel@linux.ibm.com>
On 11/6/2019 2:44 PM, Mimi Zohar wrote:
>> +int ima_queue_or_process_key_for_measurement(struct key *keyring,
>> + struct key *key)
>> +{
>> + int rc = 0;
>> + struct ima_measure_key_entry *entry = NULL;
>> + const struct public_key *pk;
>> +
>> + if (key->type != &key_type_asymmetric)
>> + return 0;
>> +
>> + mutex_lock(&ima_measure_keys_mutex);
>
> Unless the key is being queued, there's no reason to take the lock.
Reason the lock is taken even in the case the key is not queued is to
avoid the following race condition:
=> ima_init() sets ima_initialized flag and calls the dequeue function
=> If IMA hook checks ima_initialized flag outside the lock and sees
the flag is not set, it will call the queue function.
=> If the above two steps race, the key could get added to the queue
after ima_init() has processed the queued keys.
That's the reason I named the function called by the IMA hook to
ima_queue_or_process_key_for_measurement().
But I can make the following change:
=> IMA hook checks the flag.
=> If it is set, process key immediately
=> If the flag is not set, call ima_queue_or_process_key_for_measurement()
ima_queue_or_process_key_for_measurement() will do the following:
=> With the lock held check ima_initialized flag
=> If true release the lock and call process_buffer_measurement()
=> If false, queue the key and then release the lock
Would that be acceptable?
> Measuring the key should be done in ima_post_key_create_or_update()
> unless, it is being deferred. Please update the function name to
> reflect this.
Just wanted to confirm:
Rename ima_post_key_create_or_update() to a more appropriate name?
Another reason for doing all key related operations in
ima_queue_or_process_key_for_measurement() is to isolate key related
code in a separate C file and build it if and only if the CONFIG
dependencies are met.
With respect to loading custom policy, I will take a look at how to
handle that case. Thanks for pointing that out.
> Mimi
thanks,
-lakshmi
^ permalink raw reply
* Re: [PATCH v4 01/10] IMA: Defined an IMA hook to measure keys on key create or update
From: Lakshmi Ramasubramanian @ 2019-11-07 0:21 UTC (permalink / raw)
To: Mimi Zohar, dhowells, matthewgarrett, sashal, jamorris,
linux-integrity, linux-security-module, keyrings, linux-kernel
In-Reply-To: <1573080189.5028.313.camel@linux.ibm.com>
On 11/6/2019 2:43 PM, Mimi Zohar wrote:
>> +void ima_post_key_create_or_update(struct key *keyring, struct key *key,
>> + unsigned long flags, bool create)
>> +{
>> + if ((keyring != NULL) && (key != NULL))
>> + return;
>
> I would move the patch that defines the "keyring=" policy option prior
> to this one. Include the call to process_buffer_measurement() in this
> patch. A subsequent patch would add support to defer measuring the
> key, by calling a function named something like
> ima_queue_key_measurement().
>
> Mimi
As I'd stated in the other response, I wanted to isolate all key related
code in a separate C file and build it if and only if all CONFIG
dependencies are met.
I can do the following:
=> Define the IMA hook in ima_asymmetric_keys.c instead of ima_main.c
=> In include/linux/ima.h declare the IMA hook if
CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS is enabled.
Else, NOP it.
#ifdef CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS
extern void ima_post_key_create_or_update(struct key *keyring,
struct key *key,
unsigned long flags,
bool create);
#else
static inline void ima_post_key_create_or_update(struct key *keyring,
struct key *key,
unsigned long flags,
bool create) {}
#endif
Would that be acceptable?
thanks,
-lakshmi
^ permalink raw reply
* Re: [PATCH v4 08/10] IMA: Defined functions to queue and dequeue keys for measurement
From: Lakshmi Ramasubramanian @ 2019-11-07 2:20 UTC (permalink / raw)
To: Mimi Zohar, dhowells, matthewgarrett, sashal, jamorris,
linux-integrity, linux-security-module, keyrings, linux-kernel
In-Reply-To: <1573080281.5028.314.camel@linux.ibm.com>
On 11/6/19 2:44 PM, Mimi Zohar wrote:
Hi Mimi,
>> +
>> + if (ima_initialized) {
>
> ima_initialized is being set in ima_init(), before a custom policy is
> loaded. I would think that is too early. ima_update_policy() is
> called after loading a custom policy. Please see how to detect when a
> custom policy is loaded.
ima_init_policy() is called before ima_initialized flag is set.
As far as I understand ima_init_policy() loads custom policies as well.
So custom policies (such as arch specific policies, secure boot
policies, etc.) are loaded before the queued keys are processed.
But if CONFIG_IMA_WRITE_POLICY is enabled, the policy can be updated
anytime. This scenario is not handled in my implementation.
Please correct me if my understanding is wrong.
thanks,
-lakshmi
^ permalink raw reply
* Re: [PATCH v4 01/10] IMA: Defined an IMA hook to measure keys on key create or update
From: Mimi Zohar @ 2019-11-07 3:40 UTC (permalink / raw)
To: Lakshmi Ramasubramanian, dhowells, matthewgarrett, sashal,
jamorris, linux-integrity, linux-security-module, keyrings,
linux-kernel
In-Reply-To: <c838a233-28fb-cad2-4694-18366c2643a4@linux.microsoft.com>
On Wed, 2019-11-06 at 16:21 -0800, Lakshmi Ramasubramanian wrote:
> On 11/6/2019 2:43 PM, Mimi Zohar wrote:
>
> >> +void ima_post_key_create_or_update(struct key *keyring, struct key *key,
> >> + unsigned long flags, bool create)
> >> +{
> >> + if ((keyring != NULL) && (key != NULL))
> >> + return;
> >
> > I would move the patch that defines the "keyring=" policy option prior
> > to this one. Include the call to process_buffer_measurement() in this
> > patch. A subsequent patch would add support to defer measuring the
> > key, by calling a function named something like
> > ima_queue_key_measurement().
> >
> > Mimi
>
> As I'd stated in the other response, I wanted to isolate all key related
> code in a separate C file and build it if and only if all CONFIG
> dependencies are met.
The basic measuring of keys shouldn't be any different than any other
policy rule, other than it is a key and not a file. This is the
reason that I keep saying start out with the basics and then add
support to defer measuring keys on the trusted keyrings.
Only the queueing code needed for measuring keys on the trusted
keyrings would be in a separate file.
Mimi
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox