* Re: [RFC PATCH v9 03/13] mm: Add support for eXclusive Page Frame Ownership (XPFO)
From: Khalid Aziz @ 2019-04-17 16:49 UTC (permalink / raw)
To: Ingo Molnar
Cc: juergh, tycho, jsteckli, keescook, konrad.wilk, Juerg Haefliger,
deepa.srinivasan, chris.hyser, tyhicks, dwmw, andrew.cooper3, jcm,
boris.ostrovsky, iommu, x86, linux-arm-kernel, linux-doc,
linux-kernel, linux-mm, linux-security-module, Khalid Aziz,
Linus Torvalds, Andrew Morton, Thomas Gleixner, Andy Lutomirski,
Peter Zijlstra, Dave Hansen, Borislav Petkov, H. Peter Anvin,
Arjan van de Ven, Greg Kroah-Hartman
In-Reply-To: <20190417161042.GA43453@gmail.com>
On 4/17/19 10:15 AM, Ingo Molnar wrote:
>
> [ Sorry, had to trim the Cc: list from hell. Tried to keep all the
> mailing lists and all x86 developers. ]
>
> * Khalid Aziz <khalid.aziz@oracle.com> wrote:
>
>> From: Juerg Haefliger <juerg.haefliger@canonical.com>
>>
>> This patch adds basic support infrastructure for XPFO which protects
>> against 'ret2dir' kernel attacks. The basic idea is to enforce
>> exclusive ownership of page frames by either the kernel or userspace,
>> unless explicitly requested by the kernel. Whenever a page destined for
>> userspace is allocated, it is unmapped from physmap (the kernel's page
>> table). When such a page is reclaimed from userspace, it is mapped back
>> to physmap. Individual architectures can enable full XPFO support using
>> this infrastructure by supplying architecture specific pieces.
>
> I have a higher level, meta question:
>
> Is there any updated analysis outlining why this XPFO overhead would be
> required on x86-64 kernels running on SMAP/SMEP CPUs which should be all
> recent Intel and AMD CPUs, and with kernel that mark all direct kernel
> mappings as non-executable - which should be all reasonably modern
> kernels later than v4.0 or so?
>
> I.e. the original motivation of the XPFO patches was to prevent execution
> of direct kernel mappings. Is this motivation still present if those
> mappings are non-executable?
>
> (Sorry if this has been asked and answered in previous discussions.)
Hi Ingo,
That is a good question. Because of the cost of XPFO, we have to be very
sure we need this protection. The paper from Vasileios, Michalis and
Angelos - <http://www.cs.columbia.edu/~vpk/papers/ret2dir.sec14.pdf>,
does go into how ret2dir attacks can bypass SMAP/SMEP in sections 6.1
and 6.2.
Thanks,
Khalid
^ permalink raw reply
* Re: [PATCH] mm: security: introduce CONFIG_INIT_HEAP_ALL
From: Alexander Potapenko @ 2019-04-17 17:04 UTC (permalink / raw)
To: Christopher Lameter
Cc: Andrew Morton, linux-security-module,
Linux Memory Management List, Nick Desaulniers, Kostya Serebryany,
Dmitriy Vyukov, Kees Cook, Sandeep Patil, Laura Abbott,
Kernel Hardening
In-Reply-To: <CAG_fn=XW=-=SiAjToBNGDBdr1iZFA-9Ri_a4tF40448yPTbU4w@mail.gmail.com>
On Wed, Apr 17, 2019 at 1:03 PM Alexander Potapenko <glider@google.com> wrote:
>
> On Tue, Apr 16, 2019 at 6:30 PM Christopher Lameter <cl@linux.com> wrote:
> >
> > On Tue, 16 Apr 2019, Alexander Potapenko wrote:
> >
> > > > Hmmm... But we already have debugging options that poison objects and
> > > > pages?
> > > Laura Abbott mentioned in one of the previous threads
> > > (https://marc.info/?l=kernel-hardening&m=155474181528491&w=2) that:
> > >
> > > """
> > > I've looked at doing something similar in the past (failing to find
> > > the thread this morning...) and while this will work, it has pretty
> > > serious performance issues. It's not actually the poisoning which
> > > is expensive but that turning on debugging removes the cpu slab
> > > which has significant performance penalties.
> >
> > Ok you could rework that logic to be able to keep the per cpu slabs?
> I'll look into that. There's a lot going on with checking those
> poisoned bytes, although we don't need that for hardening.
>
> What do you think about the proposed approach to page initialization?
> We could separate that part from slab poisoning.
>
> > Also if you do the zeroing then you need to do it in the hotpath. And this
> > patch introduces new instructions to that hotpath for checking and
> > executing the zeroing.
> Right now the patch doesn't slow down the default case when
> CONFIG_INIT_HEAP_ALL=n, as GFP_INIT_ALWAYS_ON is 0.
> In the case heap initialization is enabled we could probably omit the
> gfp_flags check, as it'll be always zero in the case there's a
> constructor or RCU flag is set.
> So we'll have two branches instead of one in the case CONFIG_INIT_HEAP_ALL=y.
>
Ok, I think we could do the same without extra branches.
Right now I'm working on a patch that uses static branches in the
function that checks GFP flags:
static inline bool want_init_memory(gfp_t flags)
{
if (static_branch_unlikely(&init_allocations))
return true;
return flags & __GFP_ZERO;
}
and does the following in slab_alloc_node():
if (unlikely(want_init_memory(gfpflags)) && object)
s->poison_fn(s, object);
, where s->poison_fn is either memset(object, 0, s->object_size) for
normal SLAB caches or a no-op for SLAB caches that have ctors
(I _think_ I don't have to special-case SLAB_TYPESAFE_BY_RCU).
With init_allocations disabled this doesn't affect the kernel
performance (hackbench shows negative slowdown within the standard
deviation). Most certainly the indirect call is performed not too
often.
With init_allocations enabled this yields ~7% slowdown on hackbench. I
believe most of that is caused by double initialization, which we can
eliminate by passing an extra GFP flag to the page allocator.
>
> --
> Alexander Potapenko
> Software Engineer
>
> Google Germany GmbH
> Erika-Mann-Straße, 33
> 80636 München
>
> Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
> Registergericht und -nummer: Hamburg, HRB 86891
> Sitz der Gesellschaft: Hamburg
--
Alexander Potapenko
Software Engineer
Google Germany GmbH
Erika-Mann-Straße, 33
80636 München
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
^ permalink raw reply
* Re: [RFC PATCH v9 03/13] mm: Add support for eXclusive Page Frame Ownership (XPFO)
From: Ingo Molnar @ 2019-04-17 17:09 UTC (permalink / raw)
To: Khalid Aziz
Cc: juergh, tycho, jsteckli, keescook, konrad.wilk, Juerg Haefliger,
deepa.srinivasan, chris.hyser, tyhicks, dwmw, andrew.cooper3, jcm,
boris.ostrovsky, iommu, x86, linux-arm-kernel, linux-doc,
linux-kernel, linux-mm, linux-security-module, Khalid Aziz,
Linus Torvalds, Andrew Morton, Thomas Gleixner, Andy Lutomirski,
Peter Zijlstra, Dave Hansen, Borislav Petkov, H. Peter Anvin,
Arjan van de Ven, Greg Kroah-Hartman
In-Reply-To: <e16c1d73-d361-d9c7-5b8e-c495318c2509@oracle.com>
* Khalid Aziz <khalid.aziz@oracle.com> wrote:
> > I.e. the original motivation of the XPFO patches was to prevent execution
> > of direct kernel mappings. Is this motivation still present if those
> > mappings are non-executable?
> >
> > (Sorry if this has been asked and answered in previous discussions.)
>
> Hi Ingo,
>
> That is a good question. Because of the cost of XPFO, we have to be very
> sure we need this protection. The paper from Vasileios, Michalis and
> Angelos - <http://www.cs.columbia.edu/~vpk/papers/ret2dir.sec14.pdf>,
> does go into how ret2dir attacks can bypass SMAP/SMEP in sections 6.1
> and 6.2.
So it would be nice if you could generally summarize external arguments
when defending a patchset, instead of me having to dig through a PDF
which not only causes me to spend time that you probably already spent
reading that PDF, but I might also interpret it incorrectly. ;-)
The PDF you cited says this:
"Unfortunately, as shown in Table 1, the W^X prop-erty is not enforced
in many platforms, including x86-64. In our example, the content of
user address 0xBEEF000 is also accessible through kernel address
0xFFFF87FF9F080000 as plain, executable code."
Is this actually true of modern x86-64 kernels? We've locked down W^X
protections in general.
I.e. this conclusion:
"Therefore, by simply overwriting kfptr with 0xFFFF87FF9F080000 and
triggering the kernel to dereference it, an attacker can directly
execute shell code with kernel privileges."
... appears to be predicated on imperfect W^X protections on the x86-64
kernel.
Do such holes exist on the latest x86-64 kernel? If yes, is there a
reason to believe that these W^X holes cannot be fixed, or that any fix
would be more expensive than XPFO?
Thanks,
Ingo
^ permalink raw reply
* Re: [RFC PATCH v9 03/13] mm: Add support for eXclusive Page Frame Ownership (XPFO)
From: Nadav Amit @ 2019-04-17 17:19 UTC (permalink / raw)
To: Ingo Molnar
Cc: Khalid Aziz, juergh, Tycho Andersen, jsteckli, keescook,
Konrad Rzeszutek Wilk, Juerg Haefliger, deepa.srinivasan,
chris.hyser, tyhicks, David Woodhouse, Andrew Cooper, jcm,
Boris Ostrovsky, iommu, X86 ML, linux-arm-kernel,
open list:DOCUMENTATION, Linux List Kernel Mailing, Linux-MM,
LSM List, Khalid Aziz, Linus Torvalds, Andrew Morton,
Thomas Gleixner, Andy Lutomirski, Peter Zijlstra, Dave Hansen,
Borislav Petkov, H. Peter Anvin, Arjan van de Ven,
Greg Kroah-Hartman
In-Reply-To: <20190417170918.GA68678@gmail.com>
> On Apr 17, 2019, at 10:09 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
>
> * Khalid Aziz <khalid.aziz@oracle.com> wrote:
>
>>> I.e. the original motivation of the XPFO patches was to prevent execution
>>> of direct kernel mappings. Is this motivation still present if those
>>> mappings are non-executable?
>>>
>>> (Sorry if this has been asked and answered in previous discussions.)
>>
>> Hi Ingo,
>>
>> That is a good question. Because of the cost of XPFO, we have to be very
>> sure we need this protection. The paper from Vasileios, Michalis and
>> Angelos - <http://www.cs.columbia.edu/~vpk/papers/ret2dir.sec14.pdf>,
>> does go into how ret2dir attacks can bypass SMAP/SMEP in sections 6.1
>> and 6.2.
>
> So it would be nice if you could generally summarize external arguments
> when defending a patchset, instead of me having to dig through a PDF
> which not only causes me to spend time that you probably already spent
> reading that PDF, but I might also interpret it incorrectly. ;-)
>
> The PDF you cited says this:
>
> "Unfortunately, as shown in Table 1, the W^X prop-erty is not enforced
> in many platforms, including x86-64. In our example, the content of
> user address 0xBEEF000 is also accessible through kernel address
> 0xFFFF87FF9F080000 as plain, executable code."
>
> Is this actually true of modern x86-64 kernels? We've locked down W^X
> protections in general.
As I was curious, I looked at the paper. Here is a quote from it:
"In x86-64, however, the permissions of physmap are not in sane state.
Kernels up to v3.8.13 violate the W^X property by mapping the entire region
as “readable, writeable, and executable” (RWX)—only very recent kernels
(≥v3.9) use the more conservative RW mapping.”
^ permalink raw reply
* Re: [RFC PATCH v9 03/13] mm: Add support for eXclusive Page Frame Ownership (XPFO)
From: Ingo Molnar @ 2019-04-17 17:26 UTC (permalink / raw)
To: Nadav Amit
Cc: Khalid Aziz, juergh, Tycho Andersen, jsteckli, keescook,
Konrad Rzeszutek Wilk, Juerg Haefliger, deepa.srinivasan,
chris.hyser, tyhicks, David Woodhouse, Andrew Cooper, jcm,
Boris Ostrovsky, iommu, X86 ML, linux-arm-kernel,
open list:DOCUMENTATION, Linux List Kernel Mailing, Linux-MM,
LSM List, Khalid Aziz, Linus Torvalds, Andrew Morton,
Thomas Gleixner, Andy Lutomirski, Peter Zijlstra, Dave Hansen,
Borislav Petkov, H. Peter Anvin, Arjan van de Ven,
Greg Kroah-Hartman
In-Reply-To: <56A175F6-E5DA-4BBD-B244-53B786F27B7F@gmail.com>
* Nadav Amit <nadav.amit@gmail.com> wrote:
> > On Apr 17, 2019, at 10:09 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> >
> > * Khalid Aziz <khalid.aziz@oracle.com> wrote:
> >
> >>> I.e. the original motivation of the XPFO patches was to prevent execution
> >>> of direct kernel mappings. Is this motivation still present if those
> >>> mappings are non-executable?
> >>>
> >>> (Sorry if this has been asked and answered in previous discussions.)
> >>
> >> Hi Ingo,
> >>
> >> That is a good question. Because of the cost of XPFO, we have to be very
> >> sure we need this protection. The paper from Vasileios, Michalis and
> >> Angelos - <http://www.cs.columbia.edu/~vpk/papers/ret2dir.sec14.pdf>,
> >> does go into how ret2dir attacks can bypass SMAP/SMEP in sections 6.1
> >> and 6.2.
> >
> > So it would be nice if you could generally summarize external arguments
> > when defending a patchset, instead of me having to dig through a PDF
> > which not only causes me to spend time that you probably already spent
> > reading that PDF, but I might also interpret it incorrectly. ;-)
> >
> > The PDF you cited says this:
> >
> > "Unfortunately, as shown in Table 1, the W^X prop-erty is not enforced
> > in many platforms, including x86-64. In our example, the content of
> > user address 0xBEEF000 is also accessible through kernel address
> > 0xFFFF87FF9F080000 as plain, executable code."
> >
> > Is this actually true of modern x86-64 kernels? We've locked down W^X
> > protections in general.
>
> As I was curious, I looked at the paper. Here is a quote from it:
>
> "In x86-64, however, the permissions of physmap are not in sane state.
> Kernels up to v3.8.13 violate the W^X property by mapping the entire region
> as “readable, writeable, and executable” (RWX)—only very recent kernels
> (≥v3.9) use the more conservative RW mapping.”
But v3.8.13 is a 5+ years old kernel, it doesn't count as a "modern"
kernel in any sense of the word. For any proposed patchset with
significant complexity and non-trivial costs the benchmark version
threshold is the "current upstream kernel".
So does that quote address my followup questions:
> Is this actually true of modern x86-64 kernels? We've locked down W^X
> protections in general.
>
> I.e. this conclusion:
>
> "Therefore, by simply overwriting kfptr with 0xFFFF87FF9F080000 and
> triggering the kernel to dereference it, an attacker can directly
> execute shell code with kernel privileges."
>
> ... appears to be predicated on imperfect W^X protections on the x86-64
> kernel.
>
> Do such holes exist on the latest x86-64 kernel? If yes, is there a
> reason to believe that these W^X holes cannot be fixed, or that any fix
> would be more expensive than XPFO?
?
What you are proposing here is a XPFO patch-set against recent kernels
with significant runtime overhead, so my questions about the W^X holes
are warranted.
Thanks,
Ingo
^ permalink raw reply
* Re: [RFC PATCH v9 03/13] mm: Add support for eXclusive Page Frame Ownership (XPFO)
From: Khalid Aziz @ 2019-04-17 17:33 UTC (permalink / raw)
To: Ingo Molnar
Cc: juergh, tycho, jsteckli, keescook, konrad.wilk, Juerg Haefliger,
deepa.srinivasan, chris.hyser, tyhicks, dwmw, andrew.cooper3, jcm,
boris.ostrovsky, iommu, x86, linux-arm-kernel, linux-doc,
linux-kernel, linux-mm, linux-security-module, Khalid Aziz,
Linus Torvalds, Andrew Morton, Thomas Gleixner, Andy Lutomirski,
Peter Zijlstra, Dave Hansen, Borislav Petkov, H. Peter Anvin,
Arjan van de Ven, Greg Kroah-Hartman
In-Reply-To: <20190417170918.GA68678@gmail.com>
On 4/17/19 11:09 AM, Ingo Molnar wrote:
>
> * Khalid Aziz <khalid.aziz@oracle.com> wrote:
>
>>> I.e. the original motivation of the XPFO patches was to prevent execution
>>> of direct kernel mappings. Is this motivation still present if those
>>> mappings are non-executable?
>>>
>>> (Sorry if this has been asked and answered in previous discussions.)
>>
>> Hi Ingo,
>>
>> That is a good question. Because of the cost of XPFO, we have to be very
>> sure we need this protection. The paper from Vasileios, Michalis and
>> Angelos - <http://www.cs.columbia.edu/~vpk/papers/ret2dir.sec14.pdf>,
>> does go into how ret2dir attacks can bypass SMAP/SMEP in sections 6.1
>> and 6.2.
>
> So it would be nice if you could generally summarize external arguments
> when defending a patchset, instead of me having to dig through a PDF
> which not only causes me to spend time that you probably already spent
> reading that PDF, but I might also interpret it incorrectly. ;-)
Sorry, you are right. Even though that paper explains it well, a summary
is always useful.
>
> The PDF you cited says this:
>
> "Unfortunately, as shown in Table 1, the W^X prop-erty is not enforced
> in many platforms, including x86-64. In our example, the content of
> user address 0xBEEF000 is also accessible through kernel address
> 0xFFFF87FF9F080000 as plain, executable code."
>
> Is this actually true of modern x86-64 kernels? We've locked down W^X
> protections in general.
>
> I.e. this conclusion:
>
> "Therefore, by simply overwriting kfptr with 0xFFFF87FF9F080000 and
> triggering the kernel to dereference it, an attacker can directly
> execute shell code with kernel privileges."
>
> ... appears to be predicated on imperfect W^X protections on the x86-64
> kernel.
>
> Do such holes exist on the latest x86-64 kernel? If yes, is there a
> reason to believe that these W^X holes cannot be fixed, or that any fix
> would be more expensive than XPFO?
Even if physmap is not executable, return-oriented programming (ROP) can
still be used to launch an attack. Instead of placing executable code at
user address 0xBEEF000, attacker can place an ROP payload there. kfptr
is then overwritten to point to a stack-pivoting gadget. Using the
physmap address aliasing, the ROP payload becomes kernel-mode stack. The
execution can then be hijacked upon execution of ret instruction. This
is a gist of the subsection titled "Non-executable physmap" under
section 6.2 and it looked convincing enough to me. If you have a
different take on this, I am very interested in your point of view.
Thanks,
Khalid
^ permalink raw reply
* Re: [RFC PATCH v9 03/13] mm: Add support for eXclusive Page Frame Ownership (XPFO)
From: Nadav Amit @ 2019-04-17 17:44 UTC (permalink / raw)
To: Ingo Molnar
Cc: Khalid Aziz, juergh, Tycho Andersen, jsteckli, keescook,
Konrad Rzeszutek Wilk, Juerg Haefliger, deepa.srinivasan,
chris.hyser, tyhicks, David Woodhouse, Andrew Cooper, jcm,
Boris Ostrovsky, iommu, X86 ML, linux-arm-kernel,
open list:DOCUMENTATION, Linux List Kernel Mailing, Linux-MM,
LSM List, Khalid Aziz, Linus Torvalds, Andrew Morton,
Thomas Gleixner, Andy Lutomirski, Peter Zijlstra, Dave Hansen,
Borislav Petkov, H. Peter Anvin, Arjan van de Ven,
Greg Kroah-Hartman
In-Reply-To: <20190417172632.GA95485@gmail.com>
> On Apr 17, 2019, at 10:26 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
>
> * Nadav Amit <nadav.amit@gmail.com> wrote:
>
>>> On Apr 17, 2019, at 10:09 AM, Ingo Molnar <mingo@kernel.org> wrote:
>>>
>>>
>>> * Khalid Aziz <khalid.aziz@oracle.com> wrote:
>>>
>>>>> I.e. the original motivation of the XPFO patches was to prevent execution
>>>>> of direct kernel mappings. Is this motivation still present if those
>>>>> mappings are non-executable?
>>>>>
>>>>> (Sorry if this has been asked and answered in previous discussions.)
>>>>
>>>> Hi Ingo,
>>>>
>>>> That is a good question. Because of the cost of XPFO, we have to be very
>>>> sure we need this protection. The paper from Vasileios, Michalis and
>>>> Angelos - <http://www.cs.columbia.edu/~vpk/papers/ret2dir.sec14.pdf>,
>>>> does go into how ret2dir attacks can bypass SMAP/SMEP in sections 6.1
>>>> and 6.2.
>>>
>>> So it would be nice if you could generally summarize external arguments
>>> when defending a patchset, instead of me having to dig through a PDF
>>> which not only causes me to spend time that you probably already spent
>>> reading that PDF, but I might also interpret it incorrectly. ;-)
>>>
>>> The PDF you cited says this:
>>>
>>> "Unfortunately, as shown in Table 1, the W^X prop-erty is not enforced
>>> in many platforms, including x86-64. In our example, the content of
>>> user address 0xBEEF000 is also accessible through kernel address
>>> 0xFFFF87FF9F080000 as plain, executable code."
>>>
>>> Is this actually true of modern x86-64 kernels? We've locked down W^X
>>> protections in general.
>>
>> As I was curious, I looked at the paper. Here is a quote from it:
>>
>> "In x86-64, however, the permissions of physmap are not in sane state.
>> Kernels up to v3.8.13 violate the W^X property by mapping the entire region
>> as “readable, writeable, and executable” (RWX)—only very recent kernels
>> (≥v3.9) use the more conservative RW mapping.”
>
> But v3.8.13 is a 5+ years old kernel, it doesn't count as a "modern"
> kernel in any sense of the word. For any proposed patchset with
> significant complexity and non-trivial costs the benchmark version
> threshold is the "current upstream kernel".
>
> So does that quote address my followup questions:
>
>> Is this actually true of modern x86-64 kernels? We've locked down W^X
>> protections in general.
>>
>> I.e. this conclusion:
>>
>> "Therefore, by simply overwriting kfptr with 0xFFFF87FF9F080000 and
>> triggering the kernel to dereference it, an attacker can directly
>> execute shell code with kernel privileges."
>>
>> ... appears to be predicated on imperfect W^X protections on the x86-64
>> kernel.
>>
>> Do such holes exist on the latest x86-64 kernel? If yes, is there a
>> reason to believe that these W^X holes cannot be fixed, or that any fix
>> would be more expensive than XPFO?
>
> ?
>
> What you are proposing here is a XPFO patch-set against recent kernels
> with significant runtime overhead, so my questions about the W^X holes
> are warranted.
>
Just to clarify - I am an innocent bystander and have no part in this work.
I was just looking (again) at the paper, as I was curious due to the recent
patches that I sent that improve W^X protection.
^ permalink raw reply
* Re: [RFC PATCH v9 03/13] mm: Add support for eXclusive Page Frame Ownership (XPFO)
From: Andy Lutomirski @ 2019-04-17 19:49 UTC (permalink / raw)
To: Khalid Aziz
Cc: Ingo Molnar, Juerg Haefliger, Tycho Andersen, jsteckli, Kees Cook,
Konrad Rzeszutek Wilk, Juerg Haefliger, deepa.srinivasan,
chris hyser, Tyler Hicks, Woodhouse, David, Andrew Cooper,
Jon Masters, Boris Ostrovsky, iommu, X86 ML, linux-arm-kernel,
open list:DOCUMENTATION, LKML, Linux-MM, LSM List, Khalid Aziz,
Linus Torvalds, Andrew Morton, Thomas Gleixner, Andy Lutomirski,
Peter Zijlstra, Dave Hansen, Borislav Petkov, H. Peter Anvin,
Arjan van de Ven, Greg Kroah-Hartman
In-Reply-To: <8d314750-251c-7e6a-7002-5df2462ada6b@oracle.com>
On Wed, Apr 17, 2019 at 10:33 AM Khalid Aziz <khalid.aziz@oracle.com> wrote:
>
> On 4/17/19 11:09 AM, Ingo Molnar wrote:
> >
> > * Khalid Aziz <khalid.aziz@oracle.com> wrote:
> >
> >>> I.e. the original motivation of the XPFO patches was to prevent execution
> >>> of direct kernel mappings. Is this motivation still present if those
> >>> mappings are non-executable?
> >>>
> >>> (Sorry if this has been asked and answered in previous discussions.)
> >>
> >> Hi Ingo,
> >>
> >> That is a good question. Because of the cost of XPFO, we have to be very
> >> sure we need this protection. The paper from Vasileios, Michalis and
> >> Angelos - <http://www.cs.columbia.edu/~vpk/papers/ret2dir.sec14.pdf>,
> >> does go into how ret2dir attacks can bypass SMAP/SMEP in sections 6.1
> >> and 6.2.
> >
> > So it would be nice if you could generally summarize external arguments
> > when defending a patchset, instead of me having to dig through a PDF
> > which not only causes me to spend time that you probably already spent
> > reading that PDF, but I might also interpret it incorrectly. ;-)
>
> Sorry, you are right. Even though that paper explains it well, a summary
> is always useful.
>
> >
> > The PDF you cited says this:
> >
> > "Unfortunately, as shown in Table 1, the W^X prop-erty is not enforced
> > in many platforms, including x86-64. In our example, the content of
> > user address 0xBEEF000 is also accessible through kernel address
> > 0xFFFF87FF9F080000 as plain, executable code."
> >
> > Is this actually true of modern x86-64 kernels? We've locked down W^X
> > protections in general.
> >
> > I.e. this conclusion:
> >
> > "Therefore, by simply overwriting kfptr with 0xFFFF87FF9F080000 and
> > triggering the kernel to dereference it, an attacker can directly
> > execute shell code with kernel privileges."
> >
> > ... appears to be predicated on imperfect W^X protections on the x86-64
> > kernel.
> >
> > Do such holes exist on the latest x86-64 kernel? If yes, is there a
> > reason to believe that these W^X holes cannot be fixed, or that any fix
> > would be more expensive than XPFO?
>
> Even if physmap is not executable, return-oriented programming (ROP) can
> still be used to launch an attack. Instead of placing executable code at
> user address 0xBEEF000, attacker can place an ROP payload there. kfptr
> is then overwritten to point to a stack-pivoting gadget. Using the
> physmap address aliasing, the ROP payload becomes kernel-mode stack. The
> execution can then be hijacked upon execution of ret instruction. This
> is a gist of the subsection titled "Non-executable physmap" under
> section 6.2 and it looked convincing enough to me. If you have a
> different take on this, I am very interested in your point of view.
My issue with all this is that XPFO is really very expensive. I think
that, if we're going to seriously consider upstreaming expensive
exploit mitigations like this, we should consider others first, in
particular CFI techniques. grsecurity's RAP would be a great start.
I also proposed using a gcc plugin (or upstream gcc feature) to add
some instrumentation to any code that pops RSP to verify that the
resulting (unsigned) change in RSP is between 0 and THREAD_SIZE bytes.
This will make ROP quite a bit harder.
^ permalink raw reply
* Re: [RFC PATCH v9 03/13] mm: Add support for eXclusive Page Frame Ownership (XPFO)
From: Tycho Andersen @ 2019-04-17 19:52 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Khalid Aziz, Ingo Molnar, Juerg Haefliger, jsteckli, Kees Cook,
Konrad Rzeszutek Wilk, Juerg Haefliger, deepa.srinivasan,
chris hyser, Tyler Hicks, Woodhouse, David, Andrew Cooper,
Jon Masters, Boris Ostrovsky, iommu, X86 ML, linux-arm-kernel,
open list:DOCUMENTATION, LKML, Linux-MM, LSM List, Khalid Aziz,
Linus Torvalds, Andrew Morton, Thomas Gleixner, Peter Zijlstra,
Dave Hansen, Borislav Petkov, H. Peter Anvin, Arjan van de Ven,
Greg Kroah-Hartman
In-Reply-To: <CALCETrXFzWFMrV-zDa4QFjB=4WnC9RZmorBko65dLGhymDpeQw@mail.gmail.com>
On Wed, Apr 17, 2019 at 12:49:04PM -0700, Andy Lutomirski wrote:
> I also proposed using a gcc plugin (or upstream gcc feature) to add
> some instrumentation to any code that pops RSP to verify that the
> resulting (unsigned) change in RSP is between 0 and THREAD_SIZE bytes.
> This will make ROP quite a bit harder.
I've been playing around with this for a bit, and hope to have
something to post Soon :)
Tycho
^ permalink raw reply
* Re: [RFC PATCH v9 03/13] mm: Add support for eXclusive Page Frame Ownership (XPFO)
From: Khalid Aziz @ 2019-04-17 20:12 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Ingo Molnar, Juerg Haefliger, Tycho Andersen, jsteckli, Kees Cook,
Konrad Rzeszutek Wilk, Juerg Haefliger, deepa.srinivasan,
chris hyser, Tyler Hicks, Woodhouse, David, Andrew Cooper,
Jon Masters, Boris Ostrovsky, iommu, X86 ML, linux-arm-kernel,
open list:DOCUMENTATION, LKML, Linux-MM, LSM List, Khalid Aziz,
Linus Torvalds, Andrew Morton, Thomas Gleixner, Peter Zijlstra,
Dave Hansen, Borislav Petkov, H. Peter Anvin, Arjan van de Ven,
Greg Kroah-Hartman
In-Reply-To: <CALCETrXFzWFMrV-zDa4QFjB=4WnC9RZmorBko65dLGhymDpeQw@mail.gmail.com>
On 4/17/19 1:49 PM, Andy Lutomirski wrote:
> On Wed, Apr 17, 2019 at 10:33 AM Khalid Aziz <khalid.aziz@oracle.com> wrote:
>>
>> On 4/17/19 11:09 AM, Ingo Molnar wrote:
>>>
>>> * Khalid Aziz <khalid.aziz@oracle.com> wrote:
>>>
>>>>> I.e. the original motivation of the XPFO patches was to prevent execution
>>>>> of direct kernel mappings. Is this motivation still present if those
>>>>> mappings are non-executable?
>>>>>
>>>>> (Sorry if this has been asked and answered in previous discussions.)
>>>>
>>>> Hi Ingo,
>>>>
>>>> That is a good question. Because of the cost of XPFO, we have to be very
>>>> sure we need this protection. The paper from Vasileios, Michalis and
>>>> Angelos - <http://www.cs.columbia.edu/~vpk/papers/ret2dir.sec14.pdf>,
>>>> does go into how ret2dir attacks can bypass SMAP/SMEP in sections 6.1
>>>> and 6.2.
>>>
>>> So it would be nice if you could generally summarize external arguments
>>> when defending a patchset, instead of me having to dig through a PDF
>>> which not only causes me to spend time that you probably already spent
>>> reading that PDF, but I might also interpret it incorrectly. ;-)
>>
>> Sorry, you are right. Even though that paper explains it well, a summary
>> is always useful.
>>
>>>
>>> The PDF you cited says this:
>>>
>>> "Unfortunately, as shown in Table 1, the W^X prop-erty is not enforced
>>> in many platforms, including x86-64. In our example, the content of
>>> user address 0xBEEF000 is also accessible through kernel address
>>> 0xFFFF87FF9F080000 as plain, executable code."
>>>
>>> Is this actually true of modern x86-64 kernels? We've locked down W^X
>>> protections in general.
>>>
>>> I.e. this conclusion:
>>>
>>> "Therefore, by simply overwriting kfptr with 0xFFFF87FF9F080000 and
>>> triggering the kernel to dereference it, an attacker can directly
>>> execute shell code with kernel privileges."
>>>
>>> ... appears to be predicated on imperfect W^X protections on the x86-64
>>> kernel.
>>>
>>> Do such holes exist on the latest x86-64 kernel? If yes, is there a
>>> reason to believe that these W^X holes cannot be fixed, or that any fix
>>> would be more expensive than XPFO?
>>
>> Even if physmap is not executable, return-oriented programming (ROP) can
>> still be used to launch an attack. Instead of placing executable code at
>> user address 0xBEEF000, attacker can place an ROP payload there. kfptr
>> is then overwritten to point to a stack-pivoting gadget. Using the
>> physmap address aliasing, the ROP payload becomes kernel-mode stack. The
>> execution can then be hijacked upon execution of ret instruction. This
>> is a gist of the subsection titled "Non-executable physmap" under
>> section 6.2 and it looked convincing enough to me. If you have a
>> different take on this, I am very interested in your point of view.
>
> My issue with all this is that XPFO is really very expensive. I think
> that, if we're going to seriously consider upstreaming expensive
> exploit mitigations like this, we should consider others first, in
> particular CFI techniques. grsecurity's RAP would be a great start.
> I also proposed using a gcc plugin (or upstream gcc feature) to add
> some instrumentation to any code that pops RSP to verify that the
> resulting (unsigned) change in RSP is between 0 and THREAD_SIZE bytes.
> This will make ROP quite a bit harder.
>
Yes, XPFO is expensive. I have been able to reduce the overhead of XPFO
from 2537% to 28% (on large servers) but 28% is still quite significant.
Alternative mitigation techniques with lower impact would easily be more
acceptable as long as they provide same level of protection. If we have
to go with XPFO, we will continue to look for more performance
improvement to bring that number down further from 28%. Hopefully what
Tycho is working on will yield better results. I am continuing to look
for improvements to XPFO in parallel.
Thanks,
Khalid
^ permalink raw reply
* Re: [RFC PATCH v9 03/13] mm: Add support for eXclusive Page Frame Ownership (XPFO)
From: Thomas Gleixner @ 2019-04-17 21:19 UTC (permalink / raw)
To: Nadav Amit
Cc: Ingo Molnar, Khalid Aziz, juergh, Tycho Andersen, jsteckli,
keescook, Konrad Rzeszutek Wilk, Juerg Haefliger,
deepa.srinivasan, chris.hyser, tyhicks, David Woodhouse,
Andrew Cooper, jcm, Boris Ostrovsky, iommu, X86 ML,
linux-arm-kernel, open list:DOCUMENTATION,
Linux List Kernel Mailing, Linux-MM, LSM List, Khalid Aziz,
Linus Torvalds, Andrew Morton, Andy Lutomirski, Peter Zijlstra,
Dave Hansen, Borislav Petkov, H. Peter Anvin, Arjan van de Ven,
Greg Kroah-Hartman
In-Reply-To: <063753CC-5D83-4789-B594-019048DE22D9@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2067 bytes --]
On Wed, 17 Apr 2019, Nadav Amit wrote:
> > On Apr 17, 2019, at 10:26 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >> As I was curious, I looked at the paper. Here is a quote from it:
> >>
> >> "In x86-64, however, the permissions of physmap are not in sane state.
> >> Kernels up to v3.8.13 violate the W^X property by mapping the entire region
> >> as “readable, writeable, and executable” (RWX)—only very recent kernels
> >> (≥v3.9) use the more conservative RW mapping.”
> >
> > But v3.8.13 is a 5+ years old kernel, it doesn't count as a "modern"
> > kernel in any sense of the word. For any proposed patchset with
> > significant complexity and non-trivial costs the benchmark version
> > threshold is the "current upstream kernel".
> >
> > So does that quote address my followup questions:
> >
> >> Is this actually true of modern x86-64 kernels? We've locked down W^X
> >> protections in general.
> >>
> >> I.e. this conclusion:
> >>
> >> "Therefore, by simply overwriting kfptr with 0xFFFF87FF9F080000 and
> >> triggering the kernel to dereference it, an attacker can directly
> >> execute shell code with kernel privileges."
> >>
> >> ... appears to be predicated on imperfect W^X protections on the x86-64
> >> kernel.
> >>
> >> Do such holes exist on the latest x86-64 kernel? If yes, is there a
> >> reason to believe that these W^X holes cannot be fixed, or that any fix
> >> would be more expensive than XPFO?
> >
> > ?
> >
> > What you are proposing here is a XPFO patch-set against recent kernels
> > with significant runtime overhead, so my questions about the W^X holes
> > are warranted.
> >
>
> Just to clarify - I am an innocent bystander and have no part in this work.
> I was just looking (again) at the paper, as I was curious due to the recent
> patches that I sent that improve W^X protection.
It's not necessarily a W+X issue. The user space text is mapped in the
kernel as well and even if it is mapped RX then this can happen. So any
kernel mappings of user space text need to be mapped NX!
Thanks,
tglx
^ permalink raw reply
* Re: kernel BUG at kernel/cred.c:434!
From: Paul Moore @ 2019-04-17 23:39 UTC (permalink / raw)
To: Oleg Nesterov, Casey Schaufler, john.johansen
Cc: chengjian (D), Kees Cook, NeilBrown, Anna Schumaker,
linux-kernel@vger.kernel.org, Al Viro, Xiexiuqi (Xie XiuQi),
Li Bin, Jason Yan, Peter Zijlstra, Ingo Molnar,
Linux Security Module list, SELinux, Yang Yingliang
In-Reply-To: <20190417162723.GK32622@redhat.com>
On Wed, Apr 17, 2019 at 12:27 PM Oleg Nesterov <oleg@redhat.com> wrote:
> On 04/17, Paul Moore wrote:
> >
> > On Wed, Apr 17, 2019 at 10:57 AM Oleg Nesterov <oleg@redhat.com> wrote:
> > > On 04/17, Paul Moore wrote:
> > > >
> > > > I'm tempted to simply return an error in selinux_setprocattr() if
> > > > the task's credentials are not the same as its real_cred;
> > >
> > > What about other modules? I have no idea what smack_setprocattr() is,
> > > but it too does prepare_creds/commit creds.
> > >
> > > it seems that the simplest workaround should simply add the additional
> > > cred == real_cred into proc_pid_attr_write().
> >
> > Yes, that is simple, but I worry about what other LSMs might want to
> > do. While I believe failing if the effective creds are not the same
> > as the real_creds is okay for SELinux (possibly Smack too), I worry
> > about what other LSMs may want to do. After all,
> > proc_pid_attr_write() doesn't change the the creds itself, that is
> > something the specific LSMs do.
>
> Yes, but if proc_pid_attr_write() is called with cred != real_cred then
> something is already wrong?
True, or at least I would think so.
Looking at the current tree there are three LSMs which implement
setprocattr hooks: SELinux, Smack, and AppArmor. I know Casey has
already mentioned that he wasn't able to trigger the problem in Smack,
but looking at smack_setprocattr() I see the similar commit_creds()
usage so I would expect the same problem in Smack; what say you Casey?
Looking at apparmor_setprocattr(), it appears that it too could end
up calling commit_creds() via aa_set_current_hat().
Since it looks like all three LSMs which implement the setprocattr
hook are vulnerable I'm open to the idea that proc_pid_attr_write() is
a better choice for the cred != read_cred check, but I would want to
make sure John and Casey are okay with that.
John?
Casey?
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [RFC PATCH v9 03/13] mm: Add support for eXclusive Page Frame Ownership (XPFO)
From: Thomas Gleixner @ 2019-04-17 23:42 UTC (permalink / raw)
To: Linus Torvalds
Cc: Nadav Amit, Ingo Molnar, Khalid Aziz, juergh, Tycho Andersen,
jsteckli, keescook, Konrad Rzeszutek Wilk, Juerg Haefliger,
deepa.srinivasan, chris.hyser, tyhicks, David Woodhouse,
Andrew Cooper, jcm, Boris Ostrovsky, iommu, X86 ML,
linux-arm-kernel, open list:DOCUMENTATION,
Linux List Kernel Mailing, Linux-MM, LSM List, Khalid Aziz,
Andrew Morton, Andy Lutomirski, Peter Zijlstra, Dave Hansen,
Borislav Petkov, H. Peter Anvin, Arjan van de Ven,
Greg Kroah-Hartman
In-Reply-To: <CAHk-=wgBMg9P-nYQR2pS0XwVdikPCBqLsMFqR9nk=wSmAd4_5g@mail.gmail.com>
On Wed, 17 Apr 2019, Linus Torvalds wrote:
> On Wed, Apr 17, 2019, 14:20 Thomas Gleixner <tglx@linutronix.de> wrote:
>
> >
> > It's not necessarily a W+X issue. The user space text is mapped in the
> > kernel as well and even if it is mapped RX then this can happen. So any
> > kernel mappings of user space text need to be mapped NX!
>
> With SMEP, user space pages are always NX.
We talk past each other. The user space page in the ring3 valid virtual
address space (non negative) is of course protected by SMEP.
The attack utilizes the kernel linear mapping of the physical
memory. I.e. user space address 0x43210 has a kernel equivalent at
0xfxxxxxxxxxx. So if the attack manages to trick the kernel to that valid
kernel address and that is mapped X --> game over. SMEP does not help
there.
From the top of my head I'd say this is a non issue as those kernel address
space mappings _should_ be NX, but we got bitten by _should_ in the past:)
Thanks,
tglx
^ permalink raw reply
* Re: [RFC PATCH v9 03/13] mm: Add support for eXclusive Page Frame Ownership (XPFO)
From: Linus Torvalds @ 2019-04-17 23:52 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Nadav Amit, Ingo Molnar, Khalid Aziz, juergh, Tycho Andersen,
jsteckli, Kees Cook, Konrad Rzeszutek Wilk, Juerg Haefliger,
deepa.srinivasan, chris.hyser, Tyler Hicks, David Woodhouse,
Andrew Cooper, Jon Masters, Boris Ostrovsky, iommu, X86 ML,
linux-alpha@vger.kernel.org, open list:DOCUMENTATION,
Linux List Kernel Mailing, Linux-MM, LSM List, Khalid Aziz,
Andrew Morton, Andy Lutomirski, Peter Zijlstra, Dave Hansen,
Borislav Petkov, H. Peter Anvin, Arjan van de Ven,
Greg Kroah-Hartman
In-Reply-To: <alpine.DEB.2.21.1904180129000.3174@nanos.tec.linutronix.de>
On Wed, Apr 17, 2019 at 4:42 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Wed, 17 Apr 2019, Linus Torvalds wrote:
>
> > With SMEP, user space pages are always NX.
>
> We talk past each other. The user space page in the ring3 valid virtual
> address space (non negative) is of course protected by SMEP.
>
> The attack utilizes the kernel linear mapping of the physical
> memory. I.e. user space address 0x43210 has a kernel equivalent at
> 0xfxxxxxxxxxx. So if the attack manages to trick the kernel to that valid
> kernel address and that is mapped X --> game over. SMEP does not help
> there.
Oh, agreed.
But that would simply be a kernel bug. We should only map kernel pages
executable when we have kernel code in them, and we should certainly
not allow those pages to be mapped writably in user space.
That kind of "executable in kernel, writable in user" would be a
horrendous and major bug.
So i think it's a non-issue.
> From the top of my head I'd say this is a non issue as those kernel address
> space mappings _should_ be NX, but we got bitten by _should_ in the past:)
I do agree that bugs can happen, obviously, and we might have missed something.
But in the context of XPFO, I would argue (*very* strongly) that the
likelihood of the above kind of bug is absolutely *miniscule* compared
to the likelihood that we'd have something wrong in the software
implementation of XPFO.
So if the argument is "we might have bugs in software", then I think
that's an argument _against_ XPFO rather than for it.
Linus
^ permalink raw reply
* Re: kernel BUG at kernel/cred.c:434!
From: John Johansen @ 2019-04-18 0:17 UTC (permalink / raw)
To: Paul Moore, Oleg Nesterov, Casey Schaufler
Cc: chengjian (D), Kees Cook, NeilBrown, Anna Schumaker,
linux-kernel@vger.kernel.org, Al Viro, Xiexiuqi (Xie XiuQi),
Li Bin, Jason Yan, Peter Zijlstra, Ingo Molnar,
Linux Security Module list, SELinux, Yang Yingliang
In-Reply-To: <CAHC9VhT_nk1jmrTj3W=F+1gP1vU3iZsZ=UGOCwdiBLbUsOc9=w@mail.gmail.com>
On 4/17/19 4:39 PM, Paul Moore wrote:
> On Wed, Apr 17, 2019 at 12:27 PM Oleg Nesterov <oleg@redhat.com> wrote:
>> On 04/17, Paul Moore wrote:
>>>
>>> On Wed, Apr 17, 2019 at 10:57 AM Oleg Nesterov <oleg@redhat.com> wrote:
>>>> On 04/17, Paul Moore wrote:
>>>>>
>>>>> I'm tempted to simply return an error in selinux_setprocattr() if
>>>>> the task's credentials are not the same as its real_cred;
>>>>
>>>> What about other modules? I have no idea what smack_setprocattr() is,
>>>> but it too does prepare_creds/commit creds.
>>>>
>>>> it seems that the simplest workaround should simply add the additional
>>>> cred == real_cred into proc_pid_attr_write().
>>>
>>> Yes, that is simple, but I worry about what other LSMs might want to
>>> do. While I believe failing if the effective creds are not the same
>>> as the real_creds is okay for SELinux (possibly Smack too), I worry
>>> about what other LSMs may want to do. After all,
>>> proc_pid_attr_write() doesn't change the the creds itself, that is
>>> something the specific LSMs do.
>>
>> Yes, but if proc_pid_attr_write() is called with cred != real_cred then
>> something is already wrong?
>
> True, or at least I would think so.
>
> Looking at the current tree there are three LSMs which implement
> setprocattr hooks: SELinux, Smack, and AppArmor. I know Casey has
> already mentioned that he wasn't able to trigger the problem in Smack,
> but looking at smack_setprocattr() I see the similar commit_creds()
> usage so I would expect the same problem in Smack; what say you Casey?
> Looking at apparmor_setprocattr(), it appears that it too could end
> up calling commit_creds() via aa_set_current_hat().
>
> Since it looks like all three LSMs which implement the setprocattr
> hook are vulnerable I'm open to the idea that proc_pid_attr_write() is
> a better choice for the cred != read_cred check, but I would want to
> make sure John and Casey are okay with that.
>
> John?
heh yeah,
seems I messed up our check, we actually have
if (current_cred() != current_real_cred())
return -EBUSY;
on the change_profile path and missed it on the change_hat
one.
It makes sense to lift the check up earlier
^ permalink raw reply
* Re: kernel BUG at kernel/cred.c:434!
From: Casey Schaufler @ 2019-04-18 0:24 UTC (permalink / raw)
To: Paul Moore, Oleg Nesterov, john.johansen
Cc: chengjian (D), Kees Cook, NeilBrown, Anna Schumaker,
linux-kernel@vger.kernel.org, Al Viro, Xiexiuqi (Xie XiuQi),
Li Bin, Jason Yan, Peter Zijlstra, Ingo Molnar,
Linux Security Module list, SELinux, Yang Yingliang
In-Reply-To: <CAHC9VhT_nk1jmrTj3W=F+1gP1vU3iZsZ=UGOCwdiBLbUsOc9=w@mail.gmail.com>
On 4/17/2019 4:39 PM, Paul Moore wrote:
> On Wed, Apr 17, 2019 at 12:27 PM Oleg Nesterov <oleg@redhat.com> wrote:
>> On 04/17, Paul Moore wrote:
>>> On Wed, Apr 17, 2019 at 10:57 AM Oleg Nesterov <oleg@redhat.com> wrote:
>>>> On 04/17, Paul Moore wrote:
>>>>> I'm tempted to simply return an error in selinux_setprocattr() if
>>>>> the task's credentials are not the same as its real_cred;
>>>> What about other modules? I have no idea what smack_setprocattr() is,
>>>> but it too does prepare_creds/commit creds.
>>>>
>>>> it seems that the simplest workaround should simply add the additional
>>>> cred == real_cred into proc_pid_attr_write().
>>> Yes, that is simple, but I worry about what other LSMs might want to
>>> do. While I believe failing if the effective creds are not the same
>>> as the real_creds is okay for SELinux (possibly Smack too), I worry
>>> about what other LSMs may want to do. After all,
>>> proc_pid_attr_write() doesn't change the the creds itself, that is
>>> something the specific LSMs do.
>> Yes, but if proc_pid_attr_write() is called with cred != real_cred then
>> something is already wrong?
> True, or at least I would think so.
>
> Looking at the current tree there are three LSMs which implement
> setprocattr hooks: SELinux, Smack, and AppArmor. I know Casey has
> already mentioned that he wasn't able to trigger the problem in Smack,
> but looking at smack_setprocattr() I see the similar commit_creds()
> usage so I would expect the same problem in Smack; what say you Casey?
I say that my test program runs without ill effect. I call acct()
with "/proc/self/attr/current", which succeeds and enables accounting
just like it is supposed to. I then have the program open
"/proc/self/attr/current" and read it, all of which goes swimmingly.
When Smack frees a cred it usually does not free any memory of its
own, so it is conceivable that I'm just getting lucky. Or, I may not
have sufficient debug enabled.
> Looking at apparmor_setprocattr(), it appears that it too could end
> up calling commit_creds() via aa_set_current_hat().
>
> Since it looks like all three LSMs which implement the setprocattr
> hook are vulnerable I'm open to the idea that proc_pid_attr_write() is
> a better choice for the cred != read_cred check, but I would want to
> make sure John and Casey are okay with that.
>
> John?
>
> Casey?
I'm fine with the change going into proc_pid_attr_write().
^ permalink raw reply
* Re: kernel BUG at kernel/cred.c:434!
From: Yang Yingliang @ 2019-04-18 2:49 UTC (permalink / raw)
To: Casey Schaufler, Paul Moore, Oleg Nesterov, john.johansen
Cc: chengjian (D), Kees Cook, NeilBrown, Anna Schumaker,
linux-kernel@vger.kernel.org, Al Viro, Xiexiuqi (Xie XiuQi),
Li Bin, Jason Yan, Peter Zijlstra, Ingo Molnar,
Linux Security Module list, SELinux, Cheng Jian
In-Reply-To: <0ca3f4cf-5c64-2fc0-1885-9dbcca2f4b47@schaufler-ca.com>
Hi, Casey
On 2019/4/18 8:24, Casey Schaufler wrote:
> On 4/17/2019 4:39 PM, Paul Moore wrote:
>> On Wed, Apr 17, 2019 at 12:27 PM Oleg Nesterov <oleg@redhat.com> wrote:
>>> On 04/17, Paul Moore wrote:
>>>> On Wed, Apr 17, 2019 at 10:57 AM Oleg Nesterov <oleg@redhat.com>
>>>> wrote:
>>>>> On 04/17, Paul Moore wrote:
>>>>>> I'm tempted to simply return an error in selinux_setprocattr() if
>>>>>> the task's credentials are not the same as its real_cred;
>>>>> What about other modules? I have no idea what smack_setprocattr() is,
>>>>> but it too does prepare_creds/commit creds.
>>>>>
>>>>> it seems that the simplest workaround should simply add the
>>>>> additional
>>>>> cred == real_cred into proc_pid_attr_write().
>>>> Yes, that is simple, but I worry about what other LSMs might want to
>>>> do. While I believe failing if the effective creds are not the same
>>>> as the real_creds is okay for SELinux (possibly Smack too), I worry
>>>> about what other LSMs may want to do. After all,
>>>> proc_pid_attr_write() doesn't change the the creds itself, that is
>>>> something the specific LSMs do.
>>> Yes, but if proc_pid_attr_write() is called with cred != real_cred then
>>> something is already wrong?
>> True, or at least I would think so.
>>
>> Looking at the current tree there are three LSMs which implement
>> setprocattr hooks: SELinux, Smack, and AppArmor. I know Casey has
>> already mentioned that he wasn't able to trigger the problem in Smack,
>> but looking at smack_setprocattr() I see the similar commit_creds()
>> usage so I would expect the same problem in Smack; what say you Casey?
>
> I say that my test program runs without ill effect. I call acct()
> with "/proc/self/attr/current", which succeeds and enables accounting
> just like it is supposed to. I then have the program open
> "/proc/self/attr/current" and read it, all of which goes swimmingly.
> When Smack frees a cred it usually does not free any memory of its
> own, so it is conceivable that I'm just getting lucky. Or, I may not
> have sufficient debug enabled.
>
>> Looking at apparmor_setprocattr(), it appears that it too could end
>> up calling commit_creds() via aa_set_current_hat().
>>
>> Since it looks like all three LSMs which implement the setprocattr
>> hook are vulnerable I'm open to the idea that proc_pid_attr_write() is
>> a better choice for the cred != read_cred check, but I would want to
>> make sure John and Casey are okay with that.
>>
>> John?
>>
>> Casey?
>
> I'm fine with the change going into proc_pid_attr_write().
The cred != real_cred checking is not enough.
Consider this situation, when doing override, cred, real_cred and
new_cred are all same:
after override_creds() cred == real_cred == new1_cred
after prepare_creds() new2_cred
after commit_creds() becasue the check is false, so cred ==
real_cred == new2_cred
after revert_creds() cred == new1_cred, real_cred == new2_cred
It will cause cred != real_cred finally.
Regards,
Yang
^ permalink raw reply
* [PATCH v10 00/12] Appended signatures support for IMA appraisal
From: Thiago Jung Bauermann @ 2019-04-18 3:51 UTC (permalink / raw)
To: linux-integrity
Cc: linux-security-module, keyrings, linux-crypto, linuxppc-dev,
linux-doc, linux-kernel, Mimi Zohar, Dmitry Kasatkin,
James Morris, Serge E. Hallyn, David Howells, David Woodhouse,
Jessica Yu, Herbert Xu, David S. Miller, Jonathan Corbet,
AKASHI, Takahiro, Thiago Jung Bauermann
Hello,
There are two big changes in this version:
1. The modsig contents aren't stored anymore in the struct xattr_value which
is passed around in IMA for the xattr sig or digest. Instead, a new struct
modsig argument is passed alongside xattr_value in relevant IMA functions.
This change was suggested by Mimi Zohar, and allowed cleaner handling of the
modsig in the code (especially in process_measurement()).
2. There are separate template fields for the modsig and its digest. This
was also suggested by Mimi Zhoar. Because of this change, we don't need to
know anymore at measurement time whether the modsig or the xattr sig will be
used for appraisal (in the case where a file has both types of signature
available). This avoids needing to peek at the xattr sig and at the modsig
to see if their signatures use known keys, which we had to do before in
order to know at measurement time which sig would be used for appraisal, so
that we would store the correct signature in the measurement list.
The changelog below has the details. The patches apply on today's
linux-integrity/next-integrity.
Original cover letter:
On the OpenPOWER platform, secure boot and trusted boot are being
implemented using IMA for taking measurements and verifying signatures.
Since the kernel image on Power servers is an ELF binary, kernels are
signed using the scripts/sign-file tool and thus use the same signature
format as signed kernel modules.
This patch series adds support in IMA for verifying those signatures.
It adds flexibility to OpenPOWER secure boot, because it allows it to boot
kernels with the signature appended to them as well as kernels where the
signature is stored in the IMA extended attribute.
Changes since v9:
- Patch "MODSIGN: Export module signature definitions"
- Moved mod_check_sig() to a new file so that CONFIG_IMA_APPRAISE_MODSIG
doesn't have to depend on CONFIG_MODULES.
- Changed scripts/Makefile to build sign-file if CONFIG_MODULE_SIG_FORMAT
is set.
- Removed Mimi's Reviewed-by because of the changes in this version.
- Patch "PKCS#7: Refactor verify_pkcs7_signature()"
- Don't add function pkcs7_get_message_sig() anymore, since it's not
needed in the current version.
- Patch "PKCS#7: Introduce pkcs7_get_digest()"
- Changed 'len' argument from 'u8 *' to 'u32 *'.
- Added 'hash_algo' argument to obtain the algo used for the digest.
- Don't check whether 'buf', 'len' and 'hash_algo' output arguments are NULL,
since the function's only caller always sets them.
- Removed Mimi's Reviewed-by because of the changes in this version.
- Patch "integrity: Introduce asymmetric_sig_has_known_key()"
- Dropped.
- Patch "integrity: Introduce integrity_keyring_from_id"
- Squashed into "ima: Implement support for module-style appended signatures"
- Changed integrity_keyring_from_id() to a static function (suggested by Mimi
Zohar).
- Patch "ima: Introduce is_signed()"
- Dropped.
- Patch "ima: Export func_tokens"
- Squashed into "ima: Implement support for module-style appended signatures"
- Patch "ima: Use designated initializers for struct ima_event_data"
- New patch.
- Patch "ima: Factor xattr_verify() out of ima_appraise_measurement()"
- New patch.
- Patch "ima: Implement support for module-style appended signatures"
- Renamed 'struct modsig_hdr' to 'struct modsig'.
- Added integrity_modsig_verify() to integrity/digsig.c so that it's not
necessary to export integrity_keyring_from_id() (Suggested by Mimi Zohar).
- Don't add functions ima_xattr_sig_known_key() and
modsig_has_known_key() since they're not necessary anymore.
- Added modsig argument to ima_appraise_measurement().
- Verify modsig in a separate function called by ima_appraise_measurement().
- Renamed ima_read_collect_modsig() to ima_read_modsig(), with a separate
collect function added in patch "ima: Collect modsig" (suggested by Mimi
Zohar).
- In ima_read_modsig(), moved code saving of raw PKCS7 data to 'struct
modsig' to patch "ima: Collect modsig".
- In ima_read_modsig(), moved all parts related to the modsig hash to
patch "ima: Collect modsig".
- In ima_read_modsig(), don't check if the buf pointer is NULL since it's
never supposed to happen.
- Renamed ima_free_xattr_data() to ima_free_modsig().
- No need to check for modsig in ima_read_xattr() and
ima_inode_set_xattr() anymore.
- In ima_modsig_verify(), don't check if the modsig pointer is NULL since
it's not supposed to happen.
- Don't define IMA_MODSIG element in enum evm_ima_xattr_type.
- Patch "ima: Collect modsig"
- New patch.
- Patch "ima: Define ima-modsig template"
- Patch renamed from "ima: Add new "d-sig" template field"
- Renamed 'd-sig' template field to 'd-modsig'.
- Added 'modsig' template field.
- Added 'ima-modsig' defined template descriptor.
- Renamed ima_modsig_serialize_data() to ima_modsig_serialize().
- Renamed ima_get_modsig_hash() to ima_get_modsig_digest(). Also the
function is a lot simpler now since what it used to do is now done in
ima_collect_modsig() and pkcs7_get_digest().
- Added check for failed modsig collection in ima_eventdigest_modsig_init().
- Added modsig argument to ima_store_measurement().
- Added 'modsig' field to struct ima_event_data.
- Removed check for modsig == NULL in ima_get_modsig_digest() and in
ima_modsig_serialize_data() since their callers already performs that
check.
- Moved check_current_template_modsig() to this patch, previously was in
"ima: Store the measurement again when appraising a modsig".
- Patch "ima: Store the measurement again when appraising a modsig"
- Renamed ima_template_has_sig() to ima_template_has_modsig().
- Added a change to ima_collect_measurement(), making it to call
ima_collect_modsig() even if IMA_COLLECT is set in iint->flags.
- Removed IMA_READ_MEASURE flag.
- Renamed template_has_sig global variable to template_has_modsig.
- Renamed find_sig_in_template() to find_modsig_in_template().
Changes since v8:
- Patch "MODSIGN: Export module signature definitions"
- Renamed validate_module_sig() to mod_check_sig(). (Suggested by
Mimi Zohar).
- Patch "integrity: Introduce struct evm_xattr"
- Added comment mentioning that the evm_xattr usage is limited to HMAC
before the structure definition. (Suggested by Mimi Zohar)
- Patch "ima: Add modsig appraise_type option for module-style appended
signatures"
- Added MODULE_CHECK to whitelist of hooks allowed to use modsig, and
removed FIRMWARE_CHECK. (Suggested by Mimi Zohar and James Morris)
- Patch "ima: Implement support for module-style appended signatures"
- Moved call to ima_modsig_verify() from ima_appraise_measurement() to
integrity_digsig_verify(). (Suggested by Mimi Zohar)
- Renamed ima_read_modsig() to ima_read_collect_modsig() and made it force
PKCS7 code to calculate the file hash. (Suggested by Mimi Zohar)
- Build sign-file tool if IMA_APPRAISE_MODSIG is enabled.
- Check whether the signing key is in the platform keyring as a fallback
for the KEXEC_KERNEL hook. (Suggested by Mimi Zohar)
- Patch "ima: Store the measurement again when appraising a modsig"
- In process_measurement(), when a new measurement needs to be stored
re-add IMA_MEASURE flag when the modsig is read rather than changing the
if condition when calling ima_store_measurement(). (Suggested by Mimi
Zohar)
- Check whether ima_template has "sig" and "d-sig" fields at
initialization rather than at the first time the check is needed.
(suggested by Mimi Zohar)
Changes since v7:
- Patch "MODSIGN: Export module signature definitions"
- Added module name parameter to validate_module_sig() so that it can be
shown in error messages.
- Patch "integrity: Introduce struct evm_xattr"
- Dropped use of struct evm_xattr in evm_update_evmxattr() and
evm_verify_hmac(). It's not needed there anymore because of changes
to support portable EVM signatures.
Thiago Jung Bauermann (12):
MODSIGN: Export module signature definitions
PKCS#7: Refactor verify_pkcs7_signature()
PKCS#7: Introduce pkcs7_get_digest()
integrity: Introduce struct evm_xattr
integrity: Select CONFIG_KEYS instead of depending on it
ima: Use designated initializers for struct ima_event_data
ima: Add modsig appraise_type option for module-style appended
signatures
ima: Factor xattr_verify() out of ima_appraise_measurement()
ima: Implement support for module-style appended signatures
ima: Collect modsig
ima: Define ima-modsig template
ima: Store the measurement again when appraising a modsig
Documentation/ABI/testing/ima_policy | 6 +-
Documentation/security/IMA-templates.rst | 7 +-
certs/system_keyring.c | 61 +++++--
crypto/asymmetric_keys/pkcs7_verify.c | 33 ++++
include/crypto/pkcs7.h | 4 +
include/linux/module.h | 3 -
include/linux/module_signature.h | 44 +++++
include/linux/verification.h | 10 ++
init/Kconfig | 6 +-
kernel/Makefile | 1 +
kernel/module.c | 1 +
kernel/module_signature.c | 45 +++++
kernel/module_signing.c | 56 +-----
scripts/Makefile | 2 +-
security/integrity/Kconfig | 2 +-
security/integrity/digsig.c | 39 ++++-
security/integrity/evm/evm_main.c | 8 +-
security/integrity/ima/Kconfig | 13 ++
security/integrity/ima/Makefile | 1 +
security/integrity/ima/ima.h | 61 ++++++-
security/integrity/ima/ima_api.c | 33 +++-
security/integrity/ima/ima_appraise.c | 198 ++++++++++++++--------
security/integrity/ima/ima_init.c | 4 +-
security/integrity/ima/ima_main.c | 23 ++-
security/integrity/ima/ima_modsig.c | 169 ++++++++++++++++++
security/integrity/ima/ima_policy.c | 64 ++++++-
security/integrity/ima/ima_template.c | 31 +++-
security/integrity/ima/ima_template_lib.c | 60 ++++++-
security/integrity/ima/ima_template_lib.h | 4 +
security/integrity/integrity.h | 26 +++
30 files changed, 836 insertions(+), 179 deletions(-)
create mode 100644 include/linux/module_signature.h
create mode 100644 kernel/module_signature.c
create mode 100644 security/integrity/ima/ima_modsig.c
^ permalink raw reply
* [PATCH v10 01/12] MODSIGN: Export module signature definitions
From: Thiago Jung Bauermann @ 2019-04-18 3:51 UTC (permalink / raw)
To: linux-integrity
Cc: linux-security-module, keyrings, linux-crypto, linuxppc-dev,
linux-doc, linux-kernel, Mimi Zohar, Dmitry Kasatkin,
James Morris, Serge E. Hallyn, David Howells, David Woodhouse,
Jessica Yu, Herbert Xu, David S. Miller, Jonathan Corbet,
AKASHI, Takahiro, Thiago Jung Bauermann
In-Reply-To: <20190418035120.2354-1-bauerman@linux.ibm.com>
IMA will use the module_signature format for append signatures, so export
the relevant definitions and factor out the code which verifies that the
appended signature trailer is valid.
Also, create a CONFIG_MODULE_SIG_FORMAT option so that IMA can select it
and be able to use mod_check_sig() without having to depend on either
CONFIG_MODULE_SIG or CONFIG_MODULES.
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
Cc: Jessica Yu <jeyu@kernel.org>
---
include/linux/module.h | 3 --
include/linux/module_signature.h | 44 +++++++++++++++++++++++++
init/Kconfig | 6 +++-
kernel/Makefile | 1 +
kernel/module.c | 1 +
kernel/module_signature.c | 45 +++++++++++++++++++++++++
kernel/module_signing.c | 56 +++++---------------------------
scripts/Makefile | 2 +-
8 files changed, 105 insertions(+), 53 deletions(-)
diff --git a/include/linux/module.h b/include/linux/module.h
index 73ee2b10e816..cd919ec972d6 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -25,9 +25,6 @@
#include <linux/percpu.h>
#include <asm/module.h>
-/* In stripped ARM and x86-64 modules, ~ is surprisingly rare. */
-#define MODULE_SIG_STRING "~Module signature appended~\n"
-
/* Not Yet Implemented */
#define MODULE_SUPPORTED_DEVICE(name)
diff --git a/include/linux/module_signature.h b/include/linux/module_signature.h
new file mode 100644
index 000000000000..523617fc5b6a
--- /dev/null
+++ b/include/linux/module_signature.h
@@ -0,0 +1,44 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Module signature handling.
+ *
+ * Copyright (C) 2012 Red Hat, Inc. All Rights Reserved.
+ * Written by David Howells (dhowells@redhat.com)
+ */
+
+#ifndef _LINUX_MODULE_SIGNATURE_H
+#define _LINUX_MODULE_SIGNATURE_H
+
+/* In stripped ARM and x86-64 modules, ~ is surprisingly rare. */
+#define MODULE_SIG_STRING "~Module signature appended~\n"
+
+enum pkey_id_type {
+ PKEY_ID_PGP, /* OpenPGP generated key ID */
+ PKEY_ID_X509, /* X.509 arbitrary subjectKeyIdentifier */
+ PKEY_ID_PKCS7, /* Signature in PKCS#7 message */
+};
+
+/*
+ * Module signature information block.
+ *
+ * The constituents of the signature section are, in order:
+ *
+ * - Signer's name
+ * - Key identifier
+ * - Signature data
+ * - Information block
+ */
+struct module_signature {
+ u8 algo; /* Public-key crypto algorithm [0] */
+ u8 hash; /* Digest algorithm [0] */
+ u8 id_type; /* Key identifier type [PKEY_ID_PKCS7] */
+ u8 signer_len; /* Length of signer's name [0] */
+ u8 key_id_len; /* Length of key identifier [0] */
+ u8 __pad[3];
+ __be32 sig_len; /* Length of signature data */
+};
+
+int mod_check_sig(const struct module_signature *ms, size_t file_len,
+ const char *name);
+
+#endif /* _LINUX_MODULE_SIGNATURE_H */
diff --git a/init/Kconfig b/init/Kconfig
index 4592bf7997c0..a71019553ee1 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1906,7 +1906,7 @@ config MODULE_SRCVERSION_ALL
config MODULE_SIG
bool "Module signature verification"
depends on MODULES
- select SYSTEM_DATA_VERIFICATION
+ select MODULE_SIG_FORMAT
help
Check modules for valid signatures upon load: the signature
is simply appended to the module. For more information see
@@ -2036,6 +2036,10 @@ config TRIM_UNUSED_KSYMS
endif # MODULES
+config MODULE_SIG_FORMAT
+ def_bool n
+ select SYSTEM_DATA_VERIFICATION
+
config MODULES_TREE_LOOKUP
def_bool y
depends on PERF_EVENTS || TRACING
diff --git a/kernel/Makefile b/kernel/Makefile
index 6c57e78817da..d2f2488f80ab 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -57,6 +57,7 @@ endif
obj-$(CONFIG_UID16) += uid16.o
obj-$(CONFIG_MODULES) += module.o
obj-$(CONFIG_MODULE_SIG) += module_signing.o
+obj-$(CONFIG_MODULE_SIG_FORMAT) += module_signature.o
obj-$(CONFIG_KALLSYMS) += kallsyms.o
obj-$(CONFIG_BSD_PROCESS_ACCT) += acct.o
obj-$(CONFIG_CRASH_CORE) += crash_core.o
diff --git a/kernel/module.c b/kernel/module.c
index 985caa467aef..326ddeb364dd 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -19,6 +19,7 @@
#include <linux/export.h>
#include <linux/extable.h>
#include <linux/moduleloader.h>
+#include <linux/module_signature.h>
#include <linux/trace_events.h>
#include <linux/init.h>
#include <linux/kallsyms.h>
diff --git a/kernel/module_signature.c b/kernel/module_signature.c
new file mode 100644
index 000000000000..6d5e59f27f55
--- /dev/null
+++ b/kernel/module_signature.c
@@ -0,0 +1,45 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Module signature checker
+ *
+ * Copyright (C) 2012 Red Hat, Inc. All Rights Reserved.
+ * Written by David Howells (dhowells@redhat.com)
+ */
+
+#include <linux/errno.h>
+#include <linux/printk.h>
+#include <linux/module_signature.h>
+#include <asm/byteorder.h>
+
+/**
+ * mod_check_sig - check that the given signature is sane
+ *
+ * @ms: Signature to check.
+ * @file_len: Size of the file to which @ms is appended.
+ */
+int mod_check_sig(const struct module_signature *ms, size_t file_len,
+ const char *name)
+{
+ if (be32_to_cpu(ms->sig_len) >= file_len - sizeof(*ms))
+ return -EBADMSG;
+
+ if (ms->id_type != PKEY_ID_PKCS7) {
+ pr_err("%s: Module is not signed with expected PKCS#7 message\n",
+ name);
+ return -ENOPKG;
+ }
+
+ if (ms->algo != 0 ||
+ ms->hash != 0 ||
+ ms->signer_len != 0 ||
+ ms->key_id_len != 0 ||
+ ms->__pad[0] != 0 ||
+ ms->__pad[1] != 0 ||
+ ms->__pad[2] != 0) {
+ pr_err("%s: PKCS#7 signature info has unexpected non-zero params\n",
+ name);
+ return -EBADMSG;
+ }
+
+ return 0;
+}
diff --git a/kernel/module_signing.c b/kernel/module_signing.c
index 6b9a926fd86b..cdd04a6b8074 100644
--- a/kernel/module_signing.c
+++ b/kernel/module_signing.c
@@ -11,37 +11,13 @@
#include <linux/kernel.h>
#include <linux/errno.h>
+#include <linux/module.h>
+#include <linux/module_signature.h>
#include <linux/string.h>
#include <linux/verification.h>
#include <crypto/public_key.h>
#include "module-internal.h"
-enum pkey_id_type {
- PKEY_ID_PGP, /* OpenPGP generated key ID */
- PKEY_ID_X509, /* X.509 arbitrary subjectKeyIdentifier */
- PKEY_ID_PKCS7, /* Signature in PKCS#7 message */
-};
-
-/*
- * Module signature information block.
- *
- * The constituents of the signature section are, in order:
- *
- * - Signer's name
- * - Key identifier
- * - Signature data
- * - Information block
- */
-struct module_signature {
- u8 algo; /* Public-key crypto algorithm [0] */
- u8 hash; /* Digest algorithm [0] */
- u8 id_type; /* Key identifier type [PKEY_ID_PKCS7] */
- u8 signer_len; /* Length of signer's name [0] */
- u8 key_id_len; /* Length of key identifier [0] */
- u8 __pad[3];
- __be32 sig_len; /* Length of signature data */
-};
-
/*
* Verify the signature on a module.
*/
@@ -49,6 +25,7 @@ int mod_verify_sig(const void *mod, struct load_info *info)
{
struct module_signature ms;
size_t sig_len, modlen = info->len;
+ int ret;
pr_devel("==>%s(,%zu)\n", __func__, modlen);
@@ -56,32 +33,15 @@ int mod_verify_sig(const void *mod, struct load_info *info)
return -EBADMSG;
memcpy(&ms, mod + (modlen - sizeof(ms)), sizeof(ms));
- modlen -= sizeof(ms);
+
+ ret = mod_check_sig(&ms, modlen, info->name);
+ if (ret)
+ return ret;
sig_len = be32_to_cpu(ms.sig_len);
- if (sig_len >= modlen)
- return -EBADMSG;
- modlen -= sig_len;
+ modlen -= sig_len + sizeof(ms);
info->len = modlen;
- if (ms.id_type != PKEY_ID_PKCS7) {
- pr_err("%s: Module is not signed with expected PKCS#7 message\n",
- info->name);
- return -ENOPKG;
- }
-
- if (ms.algo != 0 ||
- ms.hash != 0 ||
- ms.signer_len != 0 ||
- ms.key_id_len != 0 ||
- ms.__pad[0] != 0 ||
- ms.__pad[1] != 0 ||
- ms.__pad[2] != 0) {
- pr_err("%s: PKCS#7 signature info has unexpected non-zero params\n",
- info->name);
- return -EBADMSG;
- }
-
return verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len,
VERIFY_USE_SECONDARY_KEYRING,
VERIFYING_MODULE_SIGNATURE,
diff --git a/scripts/Makefile b/scripts/Makefile
index 9d442ee050bd..52098b080ab7 100644
--- a/scripts/Makefile
+++ b/scripts/Makefile
@@ -17,7 +17,7 @@ hostprogs-$(CONFIG_VT) += conmakehash
hostprogs-$(BUILD_C_RECORDMCOUNT) += recordmcount
hostprogs-$(CONFIG_BUILDTIME_EXTABLE_SORT) += sortextable
hostprogs-$(CONFIG_ASN1) += asn1_compiler
-hostprogs-$(CONFIG_MODULE_SIG) += sign-file
+hostprogs-$(CONFIG_MODULE_SIG_FORMAT) += sign-file
hostprogs-$(CONFIG_SYSTEM_TRUSTED_KEYRING) += extract-cert
hostprogs-$(CONFIG_SYSTEM_EXTRA_CERTIFICATE) += insert-sys-cert
^ permalink raw reply related
* [PATCH v10 02/12] PKCS#7: Refactor verify_pkcs7_signature()
From: Thiago Jung Bauermann @ 2019-04-18 3:51 UTC (permalink / raw)
To: linux-integrity
Cc: linux-security-module, keyrings, linux-crypto, linuxppc-dev,
linux-doc, linux-kernel, Mimi Zohar, Dmitry Kasatkin,
James Morris, Serge E. Hallyn, David Howells, David Woodhouse,
Jessica Yu, Herbert Xu, David S. Miller, Jonathan Corbet,
AKASHI, Takahiro, Thiago Jung Bauermann
In-Reply-To: <20190418035120.2354-1-bauerman@linux.ibm.com>
IMA will need to verify a PKCS#7 signature which has already been parsed.
For this reason, factor out the code which does that from
verify_pkcs7_signature() into a new function which takes a struct
pkcs7_message instead of a data buffer.
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
Cc: David Howells <dhowells@redhat.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "David S. Miller" <davem@davemloft.net>
---
certs/system_keyring.c | 61 ++++++++++++++++++++++++++----------
include/linux/verification.h | 10 ++++++
2 files changed, 55 insertions(+), 16 deletions(-)
diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index c05c29ae4d5d..4ba82e52e4b4 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -194,33 +194,27 @@ late_initcall(load_system_certificate_list);
#ifdef CONFIG_SYSTEM_DATA_VERIFICATION
/**
- * verify_pkcs7_signature - Verify a PKCS#7-based signature on system data.
+ * verify_pkcs7_message_sig - Verify a PKCS#7-based signature on system data.
* @data: The data to be verified (NULL if expecting internal data).
* @len: Size of @data.
- * @raw_pkcs7: The PKCS#7 message that is the signature.
- * @pkcs7_len: The size of @raw_pkcs7.
+ * @pkcs7: The PKCS#7 message that is the signature.
* @trusted_keys: Trusted keys to use (NULL for builtin trusted keys only,
* (void *)1UL for all trusted keys).
* @usage: The use to which the key is being put.
* @view_content: Callback to gain access to content.
* @ctx: Context for callback.
*/
-int verify_pkcs7_signature(const void *data, size_t len,
- const void *raw_pkcs7, size_t pkcs7_len,
- struct key *trusted_keys,
- enum key_being_used_for usage,
- int (*view_content)(void *ctx,
- const void *data, size_t len,
- size_t asn1hdrlen),
- void *ctx)
+int verify_pkcs7_message_sig(const void *data, size_t len,
+ struct pkcs7_message *pkcs7,
+ struct key *trusted_keys,
+ enum key_being_used_for usage,
+ int (*view_content)(void *ctx,
+ const void *data, size_t len,
+ size_t asn1hdrlen),
+ void *ctx)
{
- struct pkcs7_message *pkcs7;
int ret;
- pkcs7 = pkcs7_parse_message(raw_pkcs7, pkcs7_len);
- if (IS_ERR(pkcs7))
- return PTR_ERR(pkcs7);
-
/* The data should be detached - so we need to supply it. */
if (data && pkcs7_supply_detached_data(pkcs7, data, len) < 0) {
pr_err("PKCS#7 signature with non-detached data\n");
@@ -273,6 +267,41 @@ int verify_pkcs7_signature(const void *data, size_t len,
}
error:
+ pr_devel("<==%s() = %d\n", __func__, ret);
+ return ret;
+}
+
+/**
+ * verify_pkcs7_signature - Verify a PKCS#7-based signature on system data.
+ * @data: The data to be verified (NULL if expecting internal data).
+ * @len: Size of @data.
+ * @raw_pkcs7: The PKCS#7 message that is the signature.
+ * @pkcs7_len: The size of @raw_pkcs7.
+ * @trusted_keys: Trusted keys to use (NULL for builtin trusted keys only,
+ * (void *)1UL for all trusted keys).
+ * @usage: The use to which the key is being put.
+ * @view_content: Callback to gain access to content.
+ * @ctx: Context for callback.
+ */
+int verify_pkcs7_signature(const void *data, size_t len,
+ const void *raw_pkcs7, size_t pkcs7_len,
+ struct key *trusted_keys,
+ enum key_being_used_for usage,
+ int (*view_content)(void *ctx,
+ const void *data, size_t len,
+ size_t asn1hdrlen),
+ void *ctx)
+{
+ struct pkcs7_message *pkcs7;
+ int ret;
+
+ pkcs7 = pkcs7_parse_message(raw_pkcs7, pkcs7_len);
+ if (IS_ERR(pkcs7))
+ return PTR_ERR(pkcs7);
+
+ ret = verify_pkcs7_message_sig(data, len, pkcs7, trusted_keys, usage,
+ view_content, ctx);
+
pkcs7_free_message(pkcs7);
pr_devel("<==%s() = %d\n", __func__, ret);
return ret;
diff --git a/include/linux/verification.h b/include/linux/verification.h
index 018fb5f13d44..5e1d41f2b336 100644
--- a/include/linux/verification.h
+++ b/include/linux/verification.h
@@ -36,6 +36,7 @@ extern const char *const key_being_used_for[NR__KEY_BEING_USED_FOR];
#ifdef CONFIG_SYSTEM_DATA_VERIFICATION
struct key;
+struct pkcs7_message;
extern int verify_pkcs7_signature(const void *data, size_t len,
const void *raw_pkcs7, size_t pkcs7_len,
@@ -45,6 +46,15 @@ extern int verify_pkcs7_signature(const void *data, size_t len,
const void *data, size_t len,
size_t asn1hdrlen),
void *ctx);
+extern int verify_pkcs7_message_sig(const void *data, size_t len,
+ struct pkcs7_message *pkcs7,
+ struct key *trusted_keys,
+ enum key_being_used_for usage,
+ int (*view_content)(void *ctx,
+ const void *data,
+ size_t len,
+ size_t asn1hdrlen),
+ void *ctx);
#ifdef CONFIG_SIGNED_PE_FILE_VERIFICATION
extern int verify_pefile_signature(const void *pebuf, unsigned pelen,
^ permalink raw reply related
* [PATCH v10 05/12] integrity: Select CONFIG_KEYS instead of depending on it
From: Thiago Jung Bauermann @ 2019-04-18 3:51 UTC (permalink / raw)
To: linux-integrity
Cc: linux-security-module, keyrings, linux-crypto, linuxppc-dev,
linux-doc, linux-kernel, Mimi Zohar, Dmitry Kasatkin,
James Morris, Serge E. Hallyn, David Howells, David Woodhouse,
Jessica Yu, Herbert Xu, David S. Miller, Jonathan Corbet,
AKASHI, Takahiro, Thiago Jung Bauermann
In-Reply-To: <20190418035120.2354-1-bauerman@linux.ibm.com>
This avoids a dependency cycle in soon-to-be-introduced
CONFIG_IMA_APPRAISE_MODSIG: it will select CONFIG_MODULE_SIG_FORMAT
which in turn selects CONFIG_KEYS. Kconfig then complains that
CONFIG_INTEGRITY_SIGNATURE depends on CONFIG_KEYS.
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
security/integrity/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig
index 2ea4ec9991d5..bf76f842fcb8 100644
--- a/security/integrity/Kconfig
+++ b/security/integrity/Kconfig
@@ -17,8 +17,8 @@ if INTEGRITY
config INTEGRITY_SIGNATURE
bool "Digital signature verification using multiple keyrings"
- depends on KEYS
default n
+ select KEYS
select SIGNATURE
help
This option enables digital signature verification support
^ permalink raw reply related
* [PATCH v10 04/12] integrity: Introduce struct evm_xattr
From: Thiago Jung Bauermann @ 2019-04-18 3:51 UTC (permalink / raw)
To: linux-integrity
Cc: linux-security-module, keyrings, linux-crypto, linuxppc-dev,
linux-doc, linux-kernel, Mimi Zohar, Dmitry Kasatkin,
James Morris, Serge E. Hallyn, David Howells, David Woodhouse,
Jessica Yu, Herbert Xu, David S. Miller, Jonathan Corbet,
AKASHI, Takahiro, Thiago Jung Bauermann
In-Reply-To: <20190418035120.2354-1-bauerman@linux.ibm.com>
Even though struct evm_ima_xattr_data includes a fixed-size array to hold a
SHA1 digest, most of the code ignores the array and uses the struct to mean
"type indicator followed by data of unspecified size" and tracks the real
size of what the struct represents in a separate length variable.
The only exception to that is the EVM code, which correctly uses the
definition of struct evm_ima_xattr_data.
So make this explicit in the code by removing the length specification from
the array in struct evm_ima_xattr_data. Also, change the name of the
element from digest to data since in most places the array doesn't hold a
digest.
A separate struct evm_xattr is introduced, with the original definition of
evm_ima_xattr_data to be used in the places that actually expect that
definition, specifically the EVM HMAC code.
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
---
security/integrity/evm/evm_main.c | 8 ++++----
security/integrity/ima/ima_appraise.c | 7 ++++---
security/integrity/integrity.h | 6 ++++++
3 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index b6d9f14bc234..588f22f1b5bd 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -169,7 +169,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
/* check value type */
switch (xattr_data->type) {
case EVM_XATTR_HMAC:
- if (xattr_len != sizeof(struct evm_ima_xattr_data)) {
+ if (xattr_len != sizeof(struct evm_xattr)) {
evm_status = INTEGRITY_FAIL;
goto out;
}
@@ -179,7 +179,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
xattr_value_len, &digest);
if (rc)
break;
- rc = crypto_memneq(xattr_data->digest, digest.digest,
+ rc = crypto_memneq(xattr_data->data, digest.digest,
SHA1_DIGEST_SIZE);
if (rc)
rc = -EINVAL;
@@ -523,7 +523,7 @@ int evm_inode_init_security(struct inode *inode,
const struct xattr *lsm_xattr,
struct xattr *evm_xattr)
{
- struct evm_ima_xattr_data *xattr_data;
+ struct evm_xattr *xattr_data;
int rc;
if (!evm_key_loaded() || !evm_protected_xattr(lsm_xattr->name))
@@ -533,7 +533,7 @@ int evm_inode_init_security(struct inode *inode,
if (!xattr_data)
return -ENOMEM;
- xattr_data->type = EVM_XATTR_HMAC;
+ xattr_data->data.type = EVM_XATTR_HMAC;
rc = evm_init_hmac(inode, lsm_xattr, xattr_data->digest);
if (rc < 0)
goto out;
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 5fb7127bbe68..ea8fa29f07d3 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -168,7 +168,8 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
return sig->hash_algo;
break;
case IMA_XATTR_DIGEST_NG:
- ret = xattr_value->digest[0];
+ /* first byte contains algorithm id */
+ ret = xattr_value->data[0];
if (ret < HASH_ALGO__LAST)
return ret;
break;
@@ -176,7 +177,7 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
/* this is for backward compatibility */
if (xattr_len == 21) {
unsigned int zero = 0;
- if (!memcmp(&xattr_value->digest[16], &zero, 4))
+ if (!memcmp(&xattr_value->data[16], &zero, 4))
return HASH_ALGO_MD5;
else
return HASH_ALGO_SHA1;
@@ -275,7 +276,7 @@ int ima_appraise_measurement(enum ima_hooks func,
/* xattr length may be longer. md5 hash in previous
version occupied 20 bytes in xattr, instead of 16
*/
- rc = memcmp(&xattr_value->digest[hash_start],
+ rc = memcmp(&xattr_value->data[hash_start],
iint->ima_hash->digest,
iint->ima_hash->length);
else
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 7de59f44cba3..88a29f72a74f 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -79,6 +79,12 @@ enum evm_ima_xattr_type {
struct evm_ima_xattr_data {
u8 type;
+ u8 data[];
+} __packed;
+
+/* Only used in the EVM HMAC code. */
+struct evm_xattr {
+ struct evm_ima_xattr_data data;
u8 digest[SHA1_DIGEST_SIZE];
} __packed;
^ permalink raw reply related
* [PATCH v10 03/12] PKCS#7: Introduce pkcs7_get_digest()
From: Thiago Jung Bauermann @ 2019-04-18 3:51 UTC (permalink / raw)
To: linux-integrity
Cc: linux-security-module, keyrings, linux-crypto, linuxppc-dev,
linux-doc, linux-kernel, Mimi Zohar, Dmitry Kasatkin,
James Morris, Serge E. Hallyn, David Howells, David Woodhouse,
Jessica Yu, Herbert Xu, David S. Miller, Jonathan Corbet,
AKASHI, Takahiro, Thiago Jung Bauermann
In-Reply-To: <20190418035120.2354-1-bauerman@linux.ibm.com>
IMA will need to access the digest of the PKCS7 message (as calculated by
the kernel) before the signature is verified, so introduce
pkcs7_get_digest() for that purpose.
Also, modify pkcs7_digest() to detect when the digest was already
calculated so that it doesn't have to do redundant work. Verifying that
sinfo->sig->digest isn't NULL is sufficient because both places which
allocate sinfo->sig (pkcs7_parse_message() and pkcs7_note_signed_info())
use kzalloc() so sig->digest is always initialized to zero.
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "David S. Miller" <davem@davemloft.net>
---
crypto/asymmetric_keys/pkcs7_verify.c | 33 +++++++++++++++++++++++++++
include/crypto/pkcs7.h | 4 ++++
2 files changed, 37 insertions(+)
diff --git a/crypto/asymmetric_keys/pkcs7_verify.c b/crypto/asymmetric_keys/pkcs7_verify.c
index 97c77f66b20d..5c3de46b0b73 100644
--- a/crypto/asymmetric_keys/pkcs7_verify.c
+++ b/crypto/asymmetric_keys/pkcs7_verify.c
@@ -16,6 +16,7 @@
#include <linux/err.h>
#include <linux/asn1.h>
#include <crypto/hash.h>
+#include <crypto/hash_info.h>
#include <crypto/public_key.h>
#include "pkcs7_parser.h"
@@ -33,6 +34,10 @@ static int pkcs7_digest(struct pkcs7_message *pkcs7,
kenter(",%u,%s", sinfo->index, sinfo->sig->hash_algo);
+ /* The digest was calculated already. */
+ if (sig->digest)
+ return 0;
+
if (!sinfo->sig->hash_algo)
return -ENOPKG;
@@ -122,6 +127,34 @@ static int pkcs7_digest(struct pkcs7_message *pkcs7,
return ret;
}
+int pkcs7_get_digest(struct pkcs7_message *pkcs7, const u8 **buf, u32 *len,
+ enum hash_algo *hash_algo)
+{
+ struct pkcs7_signed_info *sinfo = pkcs7->signed_infos;
+ int i, ret;
+
+ /*
+ * This function doesn't support messages with more than one signature.
+ */
+ if (sinfo == NULL || sinfo->next != NULL)
+ return -EBADMSG;
+
+ ret = pkcs7_digest(pkcs7, sinfo);
+ if (ret)
+ return ret;
+
+ *buf = sinfo->sig->digest;
+ *len = sinfo->sig->digest_size;
+
+ for (i = 0; i < HASH_ALGO__LAST; i++)
+ if (!strcmp(hash_algo_name[i], sinfo->sig->hash_algo)) {
+ *hash_algo = i;
+ break;
+ }
+
+ return 0;
+}
+
/*
* Find the key (X.509 certificate) to use to verify a PKCS#7 message. PKCS#7
* uses the issuer's name and the issuing certificate serial number for
diff --git a/include/crypto/pkcs7.h b/include/crypto/pkcs7.h
index 583f199400a3..3bfe6829eaae 100644
--- a/include/crypto/pkcs7.h
+++ b/include/crypto/pkcs7.h
@@ -13,6 +13,7 @@
#define _CRYPTO_PKCS7_H
#include <linux/verification.h>
+#include <linux/hash_info.h>
#include <crypto/public_key.h>
struct key;
@@ -44,4 +45,7 @@ extern int pkcs7_verify(struct pkcs7_message *pkcs7,
extern int pkcs7_supply_detached_data(struct pkcs7_message *pkcs7,
const void *data, size_t datalen);
+extern int pkcs7_get_digest(struct pkcs7_message *pkcs7, const u8 **buf,
+ u32 *len, enum hash_algo *hash_algo);
+
#endif /* _CRYPTO_PKCS7_H */
^ permalink raw reply related
* [PATCH v10 06/12] ima: Use designated initializers for struct ima_event_data
From: Thiago Jung Bauermann @ 2019-04-18 3:51 UTC (permalink / raw)
To: linux-integrity
Cc: linux-security-module, keyrings, linux-crypto, linuxppc-dev,
linux-doc, linux-kernel, Mimi Zohar, Dmitry Kasatkin,
James Morris, Serge E. Hallyn, David Howells, David Woodhouse,
Jessica Yu, Herbert Xu, David S. Miller, Jonathan Corbet,
AKASHI, Takahiro, Thiago Jung Bauermann
In-Reply-To: <20190418035120.2354-1-bauerman@linux.ibm.com>
Designated initializers allow specifying only the members of the struct
that need initialization. Non-mentioned members are initialized to zero.
This makes the code a bit clearer (particularly in ima_add_boot_aggregate()
and also allows adding a new member to the struct without having to update
all struct initializations.
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
---
security/integrity/ima/ima_api.c | 11 +++++++----
security/integrity/ima/ima_init.c | 4 ++--
2 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index c7505fb122d4..0639d0631f2c 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -133,8 +133,9 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
{
struct ima_template_entry *entry;
struct inode *inode = file_inode(file);
- struct ima_event_data event_data = {iint, file, filename, NULL, 0,
- cause};
+ struct ima_event_data event_data = { .iint = iint, .file = file,
+ .filename = filename,
+ .violation = cause };
int violation = 1;
int result;
@@ -284,8 +285,10 @@ void ima_store_measurement(struct integrity_iint_cache *iint,
int result = -ENOMEM;
struct inode *inode = file_inode(file);
struct ima_template_entry *entry;
- struct ima_event_data event_data = {iint, file, filename, xattr_value,
- xattr_len, NULL};
+ struct ima_event_data event_data = { .iint = iint, .file = file,
+ .filename = filename,
+ .xattr_value = xattr_value,
+ .xattr_len = xattr_len };
int violation = 0;
if (iint->measured_pcrs & (0x1 << pcr))
diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index 6c9295449751..ef6c3a26296e 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -49,8 +49,8 @@ static int __init ima_add_boot_aggregate(void)
const char *audit_cause = "ENOMEM";
struct ima_template_entry *entry;
struct integrity_iint_cache tmp_iint, *iint = &tmp_iint;
- struct ima_event_data event_data = {iint, NULL, boot_aggregate_name,
- NULL, 0, NULL};
+ struct ima_event_data event_data = { .iint = iint,
+ .filename = boot_aggregate_name };
int result = -ENOMEM;
int violation = 0;
struct {
^ permalink raw reply related
* [PATCH v10 07/12] ima: Add modsig appraise_type option for module-style appended signatures
From: Thiago Jung Bauermann @ 2019-04-18 3:51 UTC (permalink / raw)
To: linux-integrity
Cc: linux-security-module, keyrings, linux-crypto, linuxppc-dev,
linux-doc, linux-kernel, Mimi Zohar, Dmitry Kasatkin,
James Morris, Serge E. Hallyn, David Howells, David Woodhouse,
Jessica Yu, Herbert Xu, David S. Miller, Jonathan Corbet,
AKASHI, Takahiro, Thiago Jung Bauermann
In-Reply-To: <20190418035120.2354-1-bauerman@linux.ibm.com>
Introduce the modsig keyword to the IMA policy syntax to specify that
a given hook should expect the file to have the IMA signature appended
to it. Here is how it can be used in a rule:
appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig
With this rule, IMA will accept either a signature stored in the extended
attribute or an appended signature.
For now, the rule above will behave exactly the same as if
appraise_type=imasig was specified. The actual modsig implementation
will be introduced separately.
Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
---
Documentation/ABI/testing/ima_policy | 6 +++++-
security/integrity/ima/Kconfig | 10 +++++++++
security/integrity/ima/Makefile | 1 +
security/integrity/ima/ima.h | 9 ++++++++
security/integrity/ima/ima_modsig.c | 31 ++++++++++++++++++++++++++++
security/integrity/ima/ima_policy.c | 12 +++++++++--
security/integrity/integrity.h | 1 +
7 files changed, 67 insertions(+), 3 deletions(-)
diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index 74c6702de74e..9d1dfd0a8891 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -37,7 +37,7 @@ Description:
euid:= decimal value
fowner:= decimal value
lsm: are LSM specific
- option: appraise_type:= [imasig]
+ option: appraise_type:= [imasig] [imasig|modsig]
pcr:= decimal value
default policy:
@@ -103,3 +103,7 @@ Description:
measure func=KEXEC_KERNEL_CHECK pcr=4
measure func=KEXEC_INITRAMFS_CHECK pcr=5
+
+ Example of appraise rule allowing modsig appended signatures:
+
+ appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index a18f8c6d13b5..bba19f9ea184 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -231,6 +231,16 @@ config IMA_APPRAISE_BOOTPARAM
This option enables the different "ima_appraise=" modes
(eg. fix, log) from the boot command line.
+config IMA_APPRAISE_MODSIG
+ bool "Support module-style signatures for appraisal"
+ depends on IMA_APPRAISE
+ default n
+ help
+ Adds support for signatures appended to files. The format of the
+ appended signature is the same used for signed kernel modules.
+ The modsig keyword can be used in the IMA policy to allow a hook
+ to accept such signatures.
+
config IMA_TRUSTED_KEYRING
bool "Require all keys on the .ima keyring be signed (deprecated)"
depends on IMA_APPRAISE && SYSTEM_TRUSTED_KEYRING
diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
index d921dc4f9eb0..31d57cdf2421 100644
--- a/security/integrity/ima/Makefile
+++ b/security/integrity/ima/Makefile
@@ -9,5 +9,6 @@ obj-$(CONFIG_IMA) += ima.o
ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \
ima_policy.o ima_template.o ima_template_lib.o
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
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index d213e835c498..0c3e5a59270f 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -293,6 +293,15 @@ static inline int ima_read_xattr(struct dentry *dentry,
#endif /* CONFIG_IMA_APPRAISE */
+#ifdef CONFIG_IMA_APPRAISE_MODSIG
+bool ima_hook_supports_modsig(enum ima_hooks func);
+#else
+static inline bool ima_hook_supports_modsig(enum ima_hooks func)
+{
+ return false;
+}
+#endif /* CONFIG_IMA_APPRAISE_MODSIG */
+
/* LSM based policy rules require audit */
#ifdef CONFIG_IMA_LSM_RULES
diff --git a/security/integrity/ima/ima_modsig.c b/security/integrity/ima/ima_modsig.c
new file mode 100644
index 000000000000..87503bfe8c8b
--- /dev/null
+++ b/security/integrity/ima/ima_modsig.c
@@ -0,0 +1,31 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * IMA support for appraising module-style appended signatures.
+ *
+ * Copyright (C) 2019 IBM Corporation
+ *
+ * Author:
+ * Thiago Jung Bauermann <bauerman@linux.ibm.com>
+ */
+
+#include "ima.h"
+
+/**
+ * ima_hook_supports_modsig - can the policy allow modsig for this hook?
+ *
+ * modsig is only supported by hooks using ima_post_read_file(), because only
+ * they preload the contents of the file in a buffer. FILE_CHECK does that in
+ * some cases, but not when reached from vfs_open(). POLICY_CHECK can support
+ * it, but it's not useful in practice because it's a text file so deny.
+ */
+bool ima_hook_supports_modsig(enum ima_hooks func)
+{
+ switch (func) {
+ case KEXEC_KERNEL_CHECK:
+ case KEXEC_INITRAMFS_CHECK:
+ case MODULE_CHECK:
+ return true;
+ default:
+ return false;
+ }
+}
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index e0cc323f948f..fca7a3f23321 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -1038,6 +1038,10 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
ima_log_string(ab, "appraise_type", args[0].from);
if ((strcmp(args[0].from, "imasig")) == 0)
entry->flags |= IMA_DIGSIG_REQUIRED;
+ else if (ima_hook_supports_modsig(entry->func) &&
+ strcmp(args[0].from, "imasig|modsig") == 0)
+ entry->flags |= IMA_DIGSIG_REQUIRED
+ | IMA_MODSIG_ALLOWED;
else
result = -EINVAL;
break;
@@ -1330,8 +1334,12 @@ int ima_policy_show(struct seq_file *m, void *v)
}
}
}
- if (entry->flags & IMA_DIGSIG_REQUIRED)
- seq_puts(m, "appraise_type=imasig ");
+ if (entry->flags & IMA_DIGSIG_REQUIRED) {
+ if (entry->flags & IMA_MODSIG_ALLOWED)
+ seq_puts(m, "appraise_type=imasig|modsig ");
+ else
+ seq_puts(m, "appraise_type=imasig ");
+ }
if (entry->flags & IMA_PERMIT_DIRECTIO)
seq_puts(m, "permit_directio ");
rcu_read_unlock();
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 88a29f72a74f..0e7330a36a9d 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -36,6 +36,7 @@
#define IMA_NEW_FILE 0x04000000
#define EVM_IMMUTABLE_DIGSIG 0x08000000
#define IMA_FAIL_UNVERIFIABLE_SIGS 0x10000000
+#define IMA_MODSIG_ALLOWED 0x20000000
#define IMA_DO_MASK (IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \
IMA_HASH | IMA_APPRAISE_SUBMASK)
^ permalink raw reply related
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