* Re: [PATCH V8 2/3] Define a new ima template field buf
From: prakhar srivastava @ 2019-06-14 17:52 UTC (permalink / raw)
To: Mimi Zohar
Cc: linux-integrity, linux-security-module, linux-kernel,
Roberto Sassu, thiago Jung Bauermann
In-Reply-To: <1560521651.4072.7.camel@linux.ibm.com>
On Fri, Jun 14, 2019 at 7:14 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> > > > diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
> > > > index 993d0f1915ff..c8591406c0e2 100644
> > > > --- a/security/integrity/ima/ima_init.c
> > > > +++ b/security/integrity/ima/ima_init.c
> > > > @@ -50,7 +50,7 @@ static int __init ima_add_boot_aggregate(void)
> > > > 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};
> > > > + NULL, 0, NULL, NULL, 0};
> > >
> > > here, don't belong in this patch. It belongs in "IMA: support for per
> > > policy rule template formats", in case it should ever be backported.
> > > Please post this as a separate patch, that will be squashed with
> > > "IMA: support for per policy rule template formats".
> >
> > My mistake. I should have picked up Thaigo's "ima: Use designated
> > initializers for struct ima_event_data". Please drop these changes
> > instead.
>
> Sorry for the confusion. I just pushed out Thiago's patch.
>
Just to clarify:
- no split up of patch is needed.
- only formatting needs to cleaned up.
Apologies for the formatting issues, my editor switches back to
tab as 4 chars.
Thanks,
Prakhar Srivastava
> thanks,
>
> Mimi
>
^ permalink raw reply
* Re: [PATCH V8 1/3] Define a new IMA hook to measure the boot command line arguments
From: prakhar srivastava @ 2019-06-14 17:48 UTC (permalink / raw)
To: Mimi Zohar
Cc: linux-integrity, linux-security-module, linux-kernel,
Roberto Sassu, vgoyal
In-Reply-To: <1560453720.4805.46.camel@linux.ibm.com>
On Thu, Jun 13, 2019 at 12:22 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> Hi Prakhar,
>
> Patches titles in the subject line need to be prefixed with the
> subsystem, in this case "ima: ".
>
> On Wed, 2019-06-12 at 15:15 -0700, Prakhar Srivastava wrote:
> > This patch adds support in ima to measure kexec cmdline args
> > during soft reboot(kexec_file_load).
>
> Based on the patch title, the word "ima" is redundant. Patch
> descriptions are suppose to be written in the third person. "This
> patch adds" is unnecessary. Please review section 3 "Describe your
> changes" in Documentation/process/submitting-patches.rst.
>
> >
> > - A new ima hook ima_kexec_cmdline is defined to be called by the
> > kexec code.
> > - A new function process_buffer_measurement is defined to measure
> > the buffer hash into the ima log.
> > - A new func policy KEXEC_CMDLINE is defined to control the
> > measurement.[Suggested by Mimi]
> >
> > Signed-off-by: Prakhar Srivastava <prsriva02@gmail.com>
>
>
> > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> > index fd9b01881d17..98e351e13557 100644
> > --- a/security/integrity/ima/ima_policy.c
> > +++ b/security/integrity/ima/ima_policy.c
> > @@ -292,6 +292,13 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
> > {
> > int i;
> >
> > + /* only incase of KEXEC_CMDLINE, inode is NULL */
> > + if (func == KEXEC_CMDLINE) {
> > + if ((rule->flags & IMA_FUNC) &&
> > + (rule->func == func) && (!inode))
>
> Thank you for fixing the other formatting issues. Here's another one.
> Is checking !inode needed?
Since i am adding a new type(buffer) for measurement, and only
one (file or buffer) can be passed in, this is guarding against passing
the func as KEXEC_CMDLINE for a file.
I will remove it, since the check will still return true/false, if the
rule doesn't
exist.
and fix other formatting issues.
Thanks,
- Prakhar Srivastava
> Mimi
>
> > + return true;
> > + return false;
> > + }
> > if ((rule->flags & IMA_FUNC) &&
> > (rule->func != func && func != POST_SETATTR))
> > return false;
> >
>
^ permalink raw reply
* Re: [RFC PATCH v1 2/3] LSM/x86/sgx: Implement SGX specific hooks in SELinux
From: Sean Christopherson @ 2019-06-14 17:45 UTC (permalink / raw)
To: Xing, Cedric
Cc: Stephen Smalley, linux-security-module@vger.kernel.org,
selinux@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-sgx@vger.kernel.org, jarkko.sakkinen@linux.intel.com,
luto@kernel.org, jmorris@namei.org, serge@hallyn.com,
paul@paul-moore.com, eparis@parisplace.org, jethro@fortanix.com,
Hansen, Dave, tglx@linutronix.de, torvalds@linux-foundation.org,
akpm@linux-foundation.org, nhorman@redhat.com,
pmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, andriy.shevchenko@linux.intel.com, Svahn, Kai,
bp@alien8.de, josh@joshtriplett.org, Huang, Kai,
rientjes@google.com, Roberts, William C, Tricca, Philip B
In-Reply-To: <960B34DE67B9E140824F1DCDEC400C0F65504665@ORSMSX116.amr.corp.intel.com>
On Fri, Jun 14, 2019 at 10:16:55AM -0700, Xing, Cedric wrote:
> > From: Christopherson, Sean J
> > Sent: Thursday, June 13, 2019 5:46 PM
> >
> > On Thu, Jun 13, 2019 at 01:02:17PM -0400, Stephen Smalley wrote:
> > > On 6/11/19 6:02 PM, Sean Christopherson wrote:
> > > >My RFC series[1] implements #1. My understanding is that Andy
> > > >(Lutomirski) prefers #2. Cedric's RFC series implements #3.
> > > >
> > > >Perhaps the easiest way to make forward progress is to rule out the
> > > >options we absolutely *don't* want by focusing on the potentially
> > > >blocking issue with each option:
> > > >
> > > > #1 - SGX UAPI funkiness
> > > >
> > > > #2 - Auditing complexity, potential enclave lock contention
> > > >
> > > > #3 - Pushing SGX details into LSMs and complexity of kernel
> > > > implementation
> > > >
> > > >
> > > >[1]
> > > >https://lkml.kernel.org/r/20190606021145.12604-1-sean.j.christopherso
> > > >n@intel.com
> > >
> > > Given the complexity tradeoff, what is the clear motivating example
> > > for why
> > > #1 isn't the obvious choice? That the enclave loader has no way of
> > > knowing a priori whether the enclave will require W->X or WX? But
> > > aren't we better off requiring enclaves to be explicitly marked as
> > > needing such so that we can make a more informed decision about
> > > whether to load them in the first place?
> >
> > Andy and/or Cedric, can you please weigh in with a concrete (and
> > practical) use case that will break if we go with #1? The auditing
> > issues for #2/#3 are complex to say the least...
>
> How does enclave loader provide per-page ALLOW_* flags?
Unchanged from my RFC, i.e. specified at SGX_IOC_ENCLAVE_ADD_PAGE(S).
> And a related question is why they are necessary for enclaves but
> unnecessary for regular executables or shared objects.
Because at mmap()/mprotect() time we don't have the source file of the
enclave page to check SELinux's FILE__EXECUTE or AppArmor's AA_EXEC_MMAP.
> What's the story for SGX2 if mmap()'ing non-existing pages is not allowed?
Userspace will need to invoke an ioctl() to tell SGX "this range can be
EAUG'd".
>
> What's the story for auditing?
It happens naturally when security_enclave_load() is called. Am I
missing something?
> After everything above has been taken care of properly, will #1 still be
> simpler than #2/#3?
The state tracking of #2/#3 doesn't scare me, it's purely the auditing.
Holding an audit message for an indeterminate amount of time is a
nightmare.
Here's a thought. What if we simply require FILE__EXECUTE or AA_EXEC_MAP
to load any enclave page from a file? Alternatively, we could add an SGX
specific file policity, e.g. FILE__ENCLAVELOAD and AA_MAY_LOAD_ENCLAVE.
As in my other email, SELinux's W^X restrictions can be tied to the process,
i.e. they can be checked at mmap()/mprotect() without throwing a wrench in
auditing.
^ permalink raw reply
* Re: [PATCH V8 0/3] Add support for measuring the boot command line during kexec_file_load
From: prakhar srivastava @ 2019-06-14 17:39 UTC (permalink / raw)
To: Mimi Zohar
Cc: linux-integrity, linux-security-module, linux-kernel,
Roberto Sassu, vgoyal
In-Reply-To: <1560458898.4805.76.camel@linux.ibm.com>
On Thu, Jun 13, 2019 at 1:48 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Wed, 2019-06-12 at 15:15 -0700, Prakhar Srivastava wrote:
>
> > The kexec cmdline hash is stored in the "d-ng" field of the template data.
> > and can be verified using
> > sudo cat /sys/kernel/security/integrity/ima/ascii_runtime_measurements |
> > grep kexec-cmdline | cut -d' ' -f 6 | xxd -r -p | sha256sum
>
> This information should also be included in one of the patches.
>
Noted.
I will add this to the 2/3 patch, since that the one that adds the template.
- Thanks,
Prakhar Srivastava
> Mimi
>
^ permalink raw reply
* RE: [RFC PATCH v1 2/3] LSM/x86/sgx: Implement SGX specific hooks in SELinux
From: Xing, Cedric @ 2019-06-14 17:16 UTC (permalink / raw)
To: Christopherson, Sean J, Stephen Smalley
Cc: linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-sgx@vger.kernel.org,
jarkko.sakkinen@linux.intel.com, luto@kernel.org,
jmorris@namei.org, serge@hallyn.com, paul@paul-moore.com,
eparis@parisplace.org, jethro@fortanix.com, Hansen, Dave,
tglx@linutronix.de, torvalds@linux-foundation.org,
akpm@linux-foundation.org, nhorman@redhat.com,
pmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, andriy.shevchenko@linux.intel.com, Svahn, Kai,
bp@alien8.de, josh@joshtriplett.org, Huang, Kai,
rientjes@google.com, Roberts, William C, Tricca, Philip B
In-Reply-To: <20190614004600.GF18385@linux.intel.com>
> From: Christopherson, Sean J
> Sent: Thursday, June 13, 2019 5:46 PM
>
> On Thu, Jun 13, 2019 at 01:02:17PM -0400, Stephen Smalley wrote:
> > On 6/11/19 6:02 PM, Sean Christopherson wrote:
> > >On Tue, Jun 11, 2019 at 09:40:25AM -0400, Stephen Smalley wrote:
> > >>I haven't looked at this code closely, but it feels like a lot of
> > >>SGX-specific logic embedded into SELinux that will have to be
> > >>repeated or reused for every security module. Does SGX not track
> this state itself?
> > >
> > >SGX does track equivalent state.
> > >
> > >There are three proposals on the table (I think):
> > >
> > > 1. Require userspace to explicitly specificy (maximal) enclave
> page
> > > permissions at build time. The enclave page permissions are
> provided
> > > to, and checked by, LSMs at enclave build time.
> > >
> > > Pros: Low-complexity kernel implementation, straightforward
> auditing
> > > Cons: Sullies the SGX UAPI to some extent, may increase
> complexity of
> > > SGX2 enclave loaders.
> > >
> > > 2. Pre-check LSM permissions and dynamically track mappings to
> enclave
> > > pages, e.g. add an SGX mprotect() hook to restrict W->X and WX
> > > based on the pre-checked permissions.
> > >
> > > Pros: Does not impact SGX UAPI, medium kernel complexity
> > > Cons: Auditing is complex/weird, requires taking enclave-
> specific
> > > lock during mprotect() to query/update tracking.
> > >
> > > 3. Implement LSM hooks in SGX to allow LSMs to track enclave
> regions
> > > from cradle to grave, but otherwise defer everything to LSMs.
> > >
> > > Pros: Does not impact SGX UAPI, maximum flexibility, precise
> auditing
> > > Cons: Most complex and "heaviest" kernel implementation of the
> three,
> > > pushes more SGX details into LSMs.
> > >
> > >My RFC series[1] implements #1. My understanding is that Andy
> > >(Lutomirski) prefers #2. Cedric's RFC series implements #3.
> > >
> > >Perhaps the easiest way to make forward progress is to rule out the
> > >options we absolutely *don't* want by focusing on the potentially
> > >blocking issue with each option:
> > >
> > > #1 - SGX UAPI funkiness
> > >
> > > #2 - Auditing complexity, potential enclave lock contention
> > >
> > > #3 - Pushing SGX details into LSMs and complexity of kernel
> > > implementation
> > >
> > >
> > >[1]
> > >https://lkml.kernel.org/r/20190606021145.12604-1-sean.j.christopherso
> > >n@intel.com
> >
> > Given the complexity tradeoff, what is the clear motivating example
> > for why
> > #1 isn't the obvious choice? That the enclave loader has no way of
> > knowing a priori whether the enclave will require W->X or WX? But
> > aren't we better off requiring enclaves to be explicitly marked as
> > needing such so that we can make a more informed decision about
> > whether to load them in the first place?
>
> Andy and/or Cedric, can you please weigh in with a concrete (and
> practical) use case that will break if we go with #1? The auditing
> issues for #2/#3 are complex to say the least...
How does enclave loader provide per-page ALLOW_* flags? And a related question is why they are necessary for enclaves but unnecessary for regular executables or shared objects.
What's the story for SGX2 if mmap()'ing non-existing pages is not allowed?
What's the story for auditing?
After everything above has been taken care of properly, will #1 still be simpler than #2/#3?
^ permalink raw reply
* Re: [RFC PATCH v1 2/3] LSM/x86/sgx: Implement SGX specific hooks in SELinux
From: Sean Christopherson @ 2019-06-14 15:38 UTC (permalink / raw)
To: Stephen Smalley
Cc: Cedric Xing, linux-security-module, selinux, linux-kernel,
linux-sgx, jarkko.sakkinen, luto, jmorris, serge, paul, eparis,
jethro, dave.hansen, tglx, torvalds, akpm, nhorman, pmccallum,
serge.ayoun, shay.katz-zamir, haitao.huang, andriy.shevchenko,
kai.svahn, bp, josh, kai.huang, rientjes, william.c.roberts,
philip.b.tricca
In-Reply-To: <20190614004600.GF18385@linux.intel.com>
On Thu, Jun 13, 2019 at 05:46:00PM -0700, Sean Christopherson wrote:
> On Thu, Jun 13, 2019 at 01:02:17PM -0400, Stephen Smalley wrote:
> > On 6/11/19 6:02 PM, Sean Christopherson wrote:
> > >On Tue, Jun 11, 2019 at 09:40:25AM -0400, Stephen Smalley wrote:
> > >>I haven't looked at this code closely, but it feels like a lot of
> > >>SGX-specific logic embedded into SELinux that will have to be repeated or
> > >>reused for every security module. Does SGX not track this state itself?
> > >
> > >SGX does track equivalent state.
> > >
> > >There are three proposals on the table (I think):
> > >
> > > 1. Require userspace to explicitly specificy (maximal) enclave page
> > > permissions at build time. The enclave page permissions are provided
> > > to, and checked by, LSMs at enclave build time.
> > >
> > > Pros: Low-complexity kernel implementation, straightforward auditing
> > > Cons: Sullies the SGX UAPI to some extent, may increase complexity of
> > > SGX2 enclave loaders.
> > >
> > > 2. Pre-check LSM permissions and dynamically track mappings to enclave
> > > pages, e.g. add an SGX mprotect() hook to restrict W->X and WX
> > > based on the pre-checked permissions.
> > >
> > > Pros: Does not impact SGX UAPI, medium kernel complexity
> > > Cons: Auditing is complex/weird, requires taking enclave-specific
> > > lock during mprotect() to query/update tracking.
> > >
> > > 3. Implement LSM hooks in SGX to allow LSMs to track enclave regions
> > > from cradle to grave, but otherwise defer everything to LSMs.
> > >
> > > Pros: Does not impact SGX UAPI, maximum flexibility, precise auditing
> > > Cons: Most complex and "heaviest" kernel implementation of the three,
> > > pushes more SGX details into LSMs.
> > >
> > >My RFC series[1] implements #1. My understanding is that Andy (Lutomirski)
> > >prefers #2. Cedric's RFC series implements #3.
> > >
> > >Perhaps the easiest way to make forward progress is to rule out the
> > >options we absolutely *don't* want by focusing on the potentially blocking
> > >issue with each option:
> > >
> > > #1 - SGX UAPI funkiness
> > >
> > > #2 - Auditing complexity, potential enclave lock contention
> > >
> > > #3 - Pushing SGX details into LSMs and complexity of kernel implementation
> > >
> > >
> > >[1] https://lkml.kernel.org/r/20190606021145.12604-1-sean.j.christopherson@intel.com
> >
> > Given the complexity tradeoff, what is the clear motivating example for why
> > #1 isn't the obvious choice? That the enclave loader has no way of knowing a
> > priori whether the enclave will require W->X or WX? But aren't we better
> > off requiring enclaves to be explicitly marked as needing such so that we
> > can make a more informed decision about whether to load them in the first
> > place?
>
> Andy and/or Cedric, can you please weigh in with a concrete (and practical)
> use case that will break if we go with #1? The auditing issues for #2/#3
> are complex to say the least...
Follow-up question, is #1 any more palatable if SELinux adds SGX specific
permissions and ties them to the process (instead of the vma or sigstruct)?
Something like this for SELinux, where the absolute worst case scenario is
that SGX2 enclave loaders need SGXEXECMEM. Graphene would need SGXEXECUNMR
and probably SGXEXECANON.
static inline int sgx_has_perm(u32 sid, u32 requested)
{
return avc_has_perm(&selinux_state, sid, sid,
SECCLASS_PROCESS2, requested, NULL);
}
static int selinux_enclave_load(struct vm_area_struct *vma, unsigned long prot,
bool measured)
{
const struct cred *cred = current_cred();
u32 sid = cred_sid(cred);
int ret;
/* SGX is supported only in 64-bit kernels. */
WARN_ON_ONCE(!default_noexec);
/* Only executable enclave pages are restricted in any way. */
if (!(prot & PROT_EXEC))
return 0;
/*
* Private mappings to enclave pages are impossible, ergo we don't
* differentiate between W->X and WX, either case requires EXECMEM.
*/
if (prot & PROT_WRITE) {
ret = sgx_has_perm(sid, PROCESS2__SGXEXECMEM);
if (ret)
goto out;
}
if (!measured) {
ret = sgx_has_perm(sid, PROCESS2__SGXEXECUNMR);
if (ret)
goto out;
}
if (!vma->vm_file || !IS_PRIVATE(file_inode(vma->vm_file)) ||
vma->anon_vma) {
/*
* Loading enclave code from an anonymous mapping or from a
* modified private file mapping.
*/
ret = sgx_has_perm(sid, PROCESS2__SGXEXECANON);
if (ret)
goto out;
} else {
/* Loading from a shared or unmodified private file mapping. */
ret = sgx_has_perm(sid, PROCESS2__SGXEXECFILE);
if (ret)
goto out;
/* The source file must be executable in this case. */
ret = file_has_perm(cred, vma->vm_file, FILE__EXECUTE);
if (ret)
goto out;
}
out:
return ret;
}
Given that AppArmor generally only cares about accessing files, its
enclave_load() implementation would be something like:
static int apparmor_enclave_load(struct vm_area_struct *vma, unsigned long prot,
bool measured)
{
if (!(prot & PROT_EXEC))
return 0;
return common_file_perm(OP_ENCL_LOAD, vma->vm_file, AA_EXEC_MMAP);
}
^ permalink raw reply
* Re: [RFC 6/7] doc: keys: Document usage of TEE based Trusted Keys
From: Jarkko Sakkinen @ 2019-06-14 15:36 UTC (permalink / raw)
To: Sumit Garg
Cc: keyrings, linux-integrity, linux-security-module, Jens Wiklander,
corbet, dhowells, jejb, zohar, jmorris, serge, Ard Biesheuvel,
Daniel Thompson, linux-doc, Linux Kernel Mailing List, tee-dev
In-Reply-To: <CAFA6WYP7qi_NBRUDBhcEAEzJY-iFvJdXqtCtgQxqAvPSXDjEng@mail.gmail.com>
On Fri, Jun 14, 2019 at 11:07:23AM +0530, Sumit Garg wrote:
> On Thu, 13 Jun 2019 at 21:04, Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > On Thu, Jun 13, 2019 at 04:00:32PM +0530, Sumit Garg wrote:
> > > Provide documentation for usage of TEE based Trusted Keys via existing
> > > user-space "keyctl" utility. Also, document various use-cases.
> > >
> > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> >
> > Sorry missed this patch. Anyway, I don't think we want multiple trusted
> > keys subsystems. You have to fix the existing one if you care to get
> > these changes in. There is no really other way around this.
> >
>
> I understand your point.
>
> When I initially looked at trusted key implementation, it seemed to be
> tightly coupled to use TPM device. So I implemented a parallel
> implementation to get initial feedback (functionality-wise) on this
> new approach.
Yeah, I completely get this. My feedback this is: we can definitely
consider TEE based trusted keys, and I know that trusted.ko is a mess,
but still that is the only right long-term path. Think about the
positive side: if you as a side-effect can make it cleaner and more
versatile, your patch set will improve the quality of the kernel as a
whole i.e. you benefit larger audience than just TEE user base :-)
> I will work on abstraction of trusted key apis to use either approach.
> But is it fine with you if I send if I send a separate RFC patch for
> abstraction and later once reviewed I will incorporate that patch in
> this patch-set.
>
> It will be really helpful if you could help to test that abstraction
> patch with a real TPM device as I doesn't posses one to test.
I can, yes.
/Jarkko
^ permalink raw reply
* Re: [RFC PATCH 2/9] x86/sgx: Do not naturally align MAP_FIXED address
From: Jarkko Sakkinen @ 2019-06-14 15:18 UTC (permalink / raw)
To: Sean Christopherson
Cc: Xing, Cedric, Andy Lutomirski, Andy Lutomirski, Stephen Smalley,
James Morris, Serge E . Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jethro Beekman, Hansen, Dave,
Thomas Gleixner, Linus Torvalds, LKML, X86 ML,
linux-sgx@vger.kernel.org, Andrew Morton, nhorman@redhat.com,
npmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, Andy Shevchenko, Svahn, Kai, Borislav Petkov,
Josh Triplett, Huang, Kai, David Rientjes, Roberts, William C,
Tricca, Philip B
In-Reply-To: <20190613171451.GD5850@linux.intel.com>
On Thu, Jun 13, 2019 at 10:14:51AM -0700, Sean Christopherson wrote:
> > I don't get this. The swapper takes a read lock on mm->mmap_sem, which locks
> > the vma, which in turn reference counts vma->vm_file. Why is the internal
> > refcount still needed?
>
> mmap_sem is only held when reclaim is touching PTEs, e.g. to test/clear
> its accessed bit and to zap the PTE. The liveliness of the enclave needs
> to be guaranteed for the entire duration of reclaim, e.g. we can't have
> the enclave disappearing when we go to do EWB. It's also worth nothing
> that a single reclaim may operate on more than one mmap_sem, as enclaves
> can be shared across processes (mm_structs).
Anyway, the takeaway I got from this is that encl->refcount does not
need to be updated for VMAs (sent a patch to linux-sgx that I plan
merge).
/Jarkko
^ permalink raw reply
* Re: [PATCH V8 2/3] Define a new ima template field buf
From: Mimi Zohar @ 2019-06-14 14:14 UTC (permalink / raw)
To: Prakhar Srivastava, linux-integrity, linux-security-module,
linux-kernel
Cc: roberto.sassu, thiago Jung Bauermann
In-Reply-To: <1560509860.4171.13.camel@linux.ibm.com>
> > > diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
> > > index 993d0f1915ff..c8591406c0e2 100644
> > > --- a/security/integrity/ima/ima_init.c
> > > +++ b/security/integrity/ima/ima_init.c
> > > @@ -50,7 +50,7 @@ static int __init ima_add_boot_aggregate(void)
> > > 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};
> > > + NULL, 0, NULL, NULL, 0};
> >
> > here, don't belong in this patch. It belongs in "IMA: support for per
> > policy rule template formats", in case it should ever be backported.
> > Please post this as a separate patch, that will be squashed with
> > "IMA: support for per policy rule template formats".
>
> My mistake. I should have picked up Thaigo's "ima: Use designated
> initializers for struct ima_event_data". Please drop these changes
> instead.
Sorry for the confusion. I just pushed out Thiago's patch.
thanks,
Mimi
^ permalink raw reply
* Re: [PATCH V8 2/3] Define a new ima template field buf
From: Mimi Zohar @ 2019-06-14 10:57 UTC (permalink / raw)
To: Prakhar Srivastava, linux-integrity, linux-security-module,
linux-kernel
Cc: roberto.sassu, vgoyal
In-Reply-To: <1560455980.4805.57.camel@linux.ibm.com>
Hi Prakhar,
> > diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> > index ea7d8cbf712f..83ca99d65e4b 100644
> > --- a/security/integrity/ima/ima_api.c
> > +++ b/security/integrity/ima/ima_api.c
> > @@ -140,7 +140,7 @@ 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};
> > + cause, NULL, 0};
>
> This change here and
>
> > int violation = 1;
> > int result;
> >
> > @@ -296,7 +296,7 @@ void ima_store_measurement(struct integrity_iint_cache *iint,
> > 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};
> > + xattr_len, NULL, NULL, 0};
>
> here and
>
> > 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 993d0f1915ff..c8591406c0e2 100644
> > --- a/security/integrity/ima/ima_init.c
> > +++ b/security/integrity/ima/ima_init.c
> > @@ -50,7 +50,7 @@ static int __init ima_add_boot_aggregate(void)
> > 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};
> > + NULL, 0, NULL, NULL, 0};
>
> here, don't belong in this patch. It belongs in "IMA: support for per
> policy rule template formats", in case it should ever be backported.
> Please post this as a separate patch, that will be squashed with
> "IMA: support for per policy rule template formats".
Might mistake. I should have picked up Thaigo's "ima: Use designated
initializers for struct ima_event_data". Please drop these changes
instead.
thanks,
Mimi
>
> > int result = -ENOMEM;
> > int violation = 0;
> > struct {
>
^ permalink raw reply
* Re: [RFC 0/7] Introduce TEE based Trusted Keys support
From: Sumit Garg @ 2019-06-14 8:17 UTC (permalink / raw)
To: Mimi Zohar
Cc: Casey Schaufler, keyrings, linux-integrity, linux-security-module,
Jens Wiklander, corbet, dhowells, jejb, Jarkko Sakkinen, jmorris,
serge, Ard Biesheuvel, Daniel Thompson, linux-doc,
Linux Kernel Mailing List, tee-dev
In-Reply-To: <1560470593.4805.109.camel@linux.ibm.com>
Thanks Mimi for your comments.
On Fri, 14 Jun 2019 at 05:33, Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Thu, 2019-06-13 at 09:40 -0700, Casey Schaufler wrote:
> > On 6/13/2019 3:30 AM, Sumit Garg wrote:
> > > Add support for TEE based trusted keys where TEE provides the functionality
> > > to seal and unseal trusted keys using hardware unique key. Also, this is
> > > an alternative in case platform doesn't possess a TPM device.
> > >
> > > This series also adds some TEE features like:
> >
> > Please expand the acronym TEE on first use. That will
> > help people who don't work with it on a daily basis
> > understand what you're going on about.
>
> Thanks, Casey.
>
> "[6/7] doc: keys: Document usage of TEE based Trusted Keys" refers to
> the kernel tee documentation, but that documentation is limited to
> userspace interaction with the tee.
>
Thanks for pointing this out. I will update documentation to include
TEE bus approach and communication apis for kernel clients.
BTW, the interface is similar as with user-space. Only difference is
instead of IOCTL's from user-space, there are wrapper apis to
communicate with TEE.
Also, in case someone is interested to learn about TEE technology,
this webinar [1] could be one of starting points.
> A trusted key is a random number generated and sealed(encrypted) by
> the TPM, so that only the TPM may unseal it. The sealing key never
> leaves the TPM. The sealed, trusted key may be exported to userspace.
Understood.
> In the tee case, can the "sealing" key ever leave the tee?
No, the "sealing" key never leaves TEE. Its basically a Hardware
Unique Key (HUK) tied to a particular SoC.
> Can the
> sealed, trusted key, exported to userspace, be unsealed by the tee?
You mean using user-space interface to TEE? If yes, then answer is
"no" as user-space can't communicate with this TEE service as its
accessible to kernel clients only (see patch [2]).
In case you meant loading exported trusted key blob via "keyctl", then
"yes" this driver can unseal the trusted key. Have a look at examples
I have listed in documentation patch [3]. Also, this approach works
well across power cycles too.
> Are the tee security protections similar to those of the TPM? How do
> they compare?
>
Let me try to compare both environments. Regarding TEE, I will refer
to OP-TEE [4] as one of its implementation.
TPM:
1. External hardware.
2. Sealing key resides inside TPM.
3. Communicates via SPI, I2C etc.
OP-TEE:
1. On chip, trusted execution environment enforced via ARM TrustZone.
2. Sealing key is unique to a particular SoC provided by secure fuses,
secure crypto engine etc.
3. Communicates via Secure Monitor Calls (SMCs [5]).
[1] https://globalplatform.org/resource-publication/webinar-an-introduction-to-tee-technology/
[2] [RFC 3/7] tee: add private login method for kernel clients
[3] [RFC 6/7] doc: keys: Document usage of TEE based Trusted Keys
[4] https://optee.readthedocs.io/general/about.html
[5] http://infocenter.arm.com/help/topic/com.arm.doc.den0028b/ARM_DEN0028B_SMC_Calling_Convention.pdf
-Sumit
> Mimi
>
> >
> > >
> > > Patch #1, #2 enables support for registered kernel shared memory with TEE.
> > >
> > > Patch #3 enables support for private kernel login method required for
> > > cases like trusted keys where we don't wan't user-space to directly access
> > > TEE service to retrieve trusted key contents.
> > >
> > > Rest of the patches from #4 to #7 adds support for TEE based trusted keys.
> > >
> > > This patch-set has been tested with OP-TEE based pseudo TA which can be
> > > found here [1].
> > >
> > > Looking forward to your valuable feedback/suggestions.
>
^ permalink raw reply
* Re: [RFC 2/7] tee: enable support to register kernel memory
From: Jens Wiklander @ 2019-06-14 8:16 UTC (permalink / raw)
To: Sumit Garg
Cc: keyrings, linux-integrity, linux-security-module, corbet,
dhowells, jejb, jarkko.sakkinen, zohar, jmorris, serge,
ard.biesheuvel, daniel.thompson, linux-doc, linux-kernel, tee-dev
In-Reply-To: <1560421833-27414-3-git-send-email-sumit.garg@linaro.org>
On Thu, Jun 13, 2019 at 04:00:28PM +0530, Sumit Garg wrote:
> Enable support to register kernel memory reference with TEE. This change
> will allow TEE bus drivers to register memory references.
>
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
Thanks,
Jens
^ permalink raw reply
* Re: [RFC 1/7] tee: optee: allow kernel pages to register as shm
From: Jens Wiklander @ 2019-06-14 8:15 UTC (permalink / raw)
To: Sumit Garg
Cc: keyrings, linux-integrity, linux-security-module, corbet,
dhowells, jejb, jarkko.sakkinen, zohar, jmorris, serge,
ard.biesheuvel, daniel.thompson, linux-doc, linux-kernel, tee-dev
In-Reply-To: <1560421833-27414-2-git-send-email-sumit.garg@linaro.org>
On Thu, Jun 13, 2019 at 04:00:27PM +0530, Sumit Garg wrote:
> Kernel pages are marked as normal type memory only so allow kernel pages
> to be registered as shared memory with OP-TEE.
>
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
Thanks,
Jens
^ permalink raw reply
* Re: [RFC 0/7] Introduce TEE based Trusted Keys support
From: Sumit Garg @ 2019-06-14 5:58 UTC (permalink / raw)
To: Casey Schaufler
Cc: keyrings, linux-integrity, linux-security-module, Jens Wiklander,
corbet, dhowells, jejb, Jarkko Sakkinen, zohar, jmorris, serge,
Ard Biesheuvel, Daniel Thompson, linux-doc,
Linux Kernel Mailing List, tee-dev
In-Reply-To: <d803283e-5e69-5deb-fe94-3f2e45fb95af@schaufler-ca.com>
On Thu, 13 Jun 2019 at 22:10, Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> On 6/13/2019 3:30 AM, Sumit Garg wrote:
> > Add support for TEE based trusted keys where TEE provides the functionality
> > to seal and unseal trusted keys using hardware unique key. Also, this is
> > an alternative in case platform doesn't possess a TPM device.
> >
> > This series also adds some TEE features like:
>
> Please expand the acronym TEE on first use. That will
> help people who don't work with it on a daily basis
> understand what you're going on about.
>
Sure will take care of this. BTW, its Trusted Execution Environment (TEE).
-Sumit
> >
> > Patch #1, #2 enables support for registered kernel shared memory with TEE.
> >
> > Patch #3 enables support for private kernel login method required for
> > cases like trusted keys where we don't wan't user-space to directly access
> > TEE service to retrieve trusted key contents.
> >
> > Rest of the patches from #4 to #7 adds support for TEE based trusted keys.
> >
> > This patch-set has been tested with OP-TEE based pseudo TA which can be
> > found here [1].
> >
> > Looking forward to your valuable feedback/suggestions.
> >
> > [1] https://github.com/OP-TEE/optee_os/pull/3082
> >
> > Sumit Garg (7):
> > tee: optee: allow kernel pages to register as shm
> > tee: enable support to register kernel memory
> > tee: add private login method for kernel clients
> > KEYS: trusted: Introduce TEE based Trusted Keys
> > KEYS: encrypted: Allow TEE based trusted master keys
> > doc: keys: Document usage of TEE based Trusted Keys
> > MAINTAINERS: Add entry for TEE based Trusted Keys
> >
> > Documentation/security/keys/tee-trusted.rst | 93 +++++
> > MAINTAINERS | 9 +
> > drivers/tee/optee/call.c | 7 +
> > drivers/tee/tee_core.c | 6 +
> > drivers/tee/tee_shm.c | 16 +-
> > include/keys/tee_trusted.h | 84 ++++
> > include/keys/trusted-type.h | 1 +
> > include/linux/tee_drv.h | 1 +
> > include/uapi/linux/tee.h | 2 +
> > security/keys/Kconfig | 3 +
> > security/keys/Makefile | 3 +
> > security/keys/encrypted-keys/masterkey_trusted.c | 10 +-
> > security/keys/tee_trusted.c | 506 +++++++++++++++++++++++
> > 13 files changed, 737 insertions(+), 4 deletions(-)
> > create mode 100644 Documentation/security/keys/tee-trusted.rst
> > create mode 100644 include/keys/tee_trusted.h
> > create mode 100644 security/keys/tee_trusted.c
> >
^ permalink raw reply
* Re: [RFC 4/7] KEYS: trusted: Introduce TEE based Trusted Keys
From: Sumit Garg @ 2019-06-14 5:43 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: keyrings, linux-integrity, linux-security-module, Jens Wiklander,
corbet, dhowells, jejb, zohar, jmorris, serge, Ard Biesheuvel,
Daniel Thompson, linux-doc, Linux Kernel Mailing List, tee-dev
In-Reply-To: <20190613153202.GF18488@linux.intel.com>
On Thu, 13 Jun 2019 at 21:02, Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Thu, Jun 13, 2019 at 04:00:30PM +0530, Sumit Garg wrote:
> > Add support for TEE based trusted keys where TEE provides the functionality
> > to seal and unseal trusted keys using hardware unique key.
> >
> > Refer to Documentation/tee.txt for detailed information about TEE.
> >
> > Approach taken in this patch acts as an alternative to a TPM device in case
> > platform doesn't possess one.
> >
> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
>
> How does this interact with the trusted module? Why there is no update
> to security/keys/trusted-encrypted.txt?
>
You already found documentation patch [1].
> Somehow the existing trusted module needs to be re-architected to work
> with either. Otherwise, this will turn out to be a mess.
>
See my reply on this patch [1].
[1] [RFC 6/7] doc: keys: Document usage of TEE based Trusted Keys
-Sumit
> /Jarkko
^ permalink raw reply
* Re: [RFC 6/7] doc: keys: Document usage of TEE based Trusted Keys
From: Sumit Garg @ 2019-06-14 5:37 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: keyrings, linux-integrity, linux-security-module, Jens Wiklander,
corbet, dhowells, jejb, zohar, jmorris, serge, Ard Biesheuvel,
Daniel Thompson, linux-doc, Linux Kernel Mailing List, tee-dev
In-Reply-To: <20190613153414.GG18488@linux.intel.com>
On Thu, 13 Jun 2019 at 21:04, Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Thu, Jun 13, 2019 at 04:00:32PM +0530, Sumit Garg wrote:
> > Provide documentation for usage of TEE based Trusted Keys via existing
> > user-space "keyctl" utility. Also, document various use-cases.
> >
> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
>
> Sorry missed this patch. Anyway, I don't think we want multiple trusted
> keys subsystems. You have to fix the existing one if you care to get
> these changes in. There is no really other way around this.
>
I understand your point.
When I initially looked at trusted key implementation, it seemed to be
tightly coupled to use TPM device. So I implemented a parallel
implementation to get initial feedback (functionality-wise) on this
new approach.
I will work on abstraction of trusted key apis to use either approach.
But is it fine with you if I send if I send a separate RFC patch for
abstraction and later once reviewed I will incorporate that patch in
this patch-set.
It will be really helpful if you could help to test that abstraction
patch with a real TPM device as I doesn't posses one to test.
-Sumit
> /Jarkko
^ permalink raw reply
* Re: [RFC 2/7] tee: enable support to register kernel memory
From: Sumit Garg @ 2019-06-14 5:13 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: keyrings, linux-integrity, linux-security-module, Jens Wiklander,
corbet, dhowells, jejb, zohar, jmorris, serge, Ard Biesheuvel,
Daniel Thompson, linux-doc, Linux Kernel Mailing List, tee-dev
In-Reply-To: <20190613152003.GE18488@linux.intel.com>
On Thu, 13 Jun 2019 at 20:50, Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Thu, Jun 13, 2019 at 04:00:28PM +0530, Sumit Garg wrote:
> > Enable support to register kernel memory reference with TEE. This change
> > will allow TEE bus drivers to register memory references.
> >
> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
>
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>
Thanks.
-Sumit
> /Jarkko
^ permalink raw reply
* Re: [RFC 1/7] tee: optee: allow kernel pages to register as shm
From: Sumit Garg @ 2019-06-14 5:12 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: keyrings, linux-integrity, linux-security-module, Jens Wiklander,
corbet, dhowells, jejb, zohar, jmorris, serge, Ard Biesheuvel,
Daniel Thompson, linux-doc, Linux Kernel Mailing List, tee-dev
In-Reply-To: <20190613151746.GD18488@linux.intel.com>
On Thu, 13 Jun 2019 at 20:47, Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Thu, Jun 13, 2019 at 06:17:14PM +0300, Jarkko Sakkinen wrote:
> > On Thu, Jun 13, 2019 at 06:12:57PM +0300, Jarkko Sakkinen wrote:
> > > On Thu, Jun 13, 2019 at 04:00:27PM +0530, Sumit Garg wrote:
> > > > Kernel pages are marked as normal type memory only so allow kernel pages
> > > > to be registered as shared memory with OP-TEE.
> > > >
> > > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > >
> > > Just out of pure interest why this was not allowed before?
> >
> > Please spare me and ignore that one :-) Obviouslly because it
> > was not used.
> >
> > Acked-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>
> Actually,
>
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>
Thanks.
-Sumit
> /Jarkko
^ permalink raw reply
* Re: [RFC PATCH 0/1] security: add SECURE_KEEP_FSUID to preserve fsuid/fsgid across execve
From: James Morris @ 2019-06-14 5:09 UTC (permalink / raw)
To: Igor Lubashev; +Cc: Serge Hallyn, linux-security-module, linux-kernel
In-Reply-To: <1560473087-27754-1-git-send-email-ilubashe@akamai.com>
On Thu, 13 Jun 2019, Igor Lubashev wrote:
> I've posted this in March but received no response. Reposting.
>
> This patch introduces SECURE_KEEP_FSUID to allow fsuid/fsgid to be
> preserved across execve. It is currently impossible to execve a
> program such that effective and filesystem uid differ.
>
> The need for this functionality arose from a desire to allow certain
> non-privileged users to run perf. To do this, we install perf without
> set-uid-root and have a set-uid-root wrapper decide who is allowed to
> run perf (and with what arguments).
>
> The wrapper must execve perf with real and effective root uid, because
> perf and KASLR require this. However, that presently resets fsuid to
> root, giving the user ability to read and overwrite any file owned by
> root (perf report -i, perf record -o). Also, perf record will create
> perf.data that cannot be deleted by the user.
>
> We cannot reset /proc/sys/kernel/perf_event_paranoid to a permissive
> level, since we must be selective which users have the permissions.
>
> Of course, we could fix our problem by a patch to perf to allow
> passing a username on the command line and having perf execute
> setfsuid before opening files. However, perf is not the only program
> that uses kernel features that require root uid/euid, so a general
> solution that does not involve updating all such programs seems
> warranted.
This seems like a very specific corner case, depending on fsuid!=0 for an
euid=0 process, along with a whitelist policy for perf arguments. It would
be a great way to escalate to root via a bug in an executed app or via a
wrapper misconfiguration.
It also adds complexity to kernel credential handling -- it's yet another
thing to consider when trying to reason about this.
Have you considered the example security configuration in
Documentation/admin-guide/perf-security.rst ?
What are some other examples of programs that could utilize this scheme?
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [RFC PATCH 0/1] security: add SECURE_KEEP_FSUID to preserve fsuid/fsgid across execve
From: James Morris @ 2019-06-14 3:38 UTC (permalink / raw)
To: Igor Lubashev
Cc: Serge Hallyn, linux-security-module, linux-kernel, David Howells,
Al Viro
In-Reply-To: <1560473087-27754-1-git-send-email-ilubashe@akamai.com>
[Adding David and Al]
On Thu, 13 Jun 2019, Igor Lubashev wrote:
> I've posted this in March but received no response. Reposting.
>
> This patch introduces SECURE_KEEP_FSUID to allow fsuid/fsgid to be
> preserved across execve. It is currently impossible to execve a
> program such that effective and filesystem uid differ.
>
> The need for this functionality arose from a desire to allow certain
> non-privileged users to run perf. To do this, we install perf without
> set-uid-root and have a set-uid-root wrapper decide who is allowed to
> run perf (and with what arguments).
>
> The wrapper must execve perf with real and effective root uid, because
> perf and KASLR require this. However, that presently resets fsuid to
> root, giving the user ability to read and overwrite any file owned by
> root (perf report -i, perf record -o). Also, perf record will create
> perf.data that cannot be deleted by the user.
>
> We cannot reset /proc/sys/kernel/perf_event_paranoid to a permissive
> level, since we must be selective which users have the permissions.
>
> Of course, we could fix our problem by a patch to perf to allow
> passing a username on the command line and having perf execute
> setfsuid before opening files. However, perf is not the only program
> that uses kernel features that require root uid/euid, so a general
> solution that does not involve updating all such programs seems
> warranted.
>
> I will update man pages, if this patch is deemed a good idea.
>
> Igor Lubashev (1):
> security: add SECURE_KEEP_FSUID to preserve fsuid/fsgid across execve
>
> include/uapi/linux/securebits.h | 10 +++++++++-
> security/commoncap.c | 9 +++++++--
> 2 files changed, 16 insertions(+), 3 deletions(-)
>
>
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* [RFC PATCH 0/1] security: add SECURE_KEEP_FSUID to preserve fsuid/fsgid across execve
From: Igor Lubashev @ 2019-06-14 0:44 UTC (permalink / raw)
To: Serge Hallyn, James Morris, linux-security-module
Cc: linux-kernel, Igor Lubashev
I've posted this in March but received no response. Reposting.
This patch introduces SECURE_KEEP_FSUID to allow fsuid/fsgid to be
preserved across execve. It is currently impossible to execve a
program such that effective and filesystem uid differ.
The need for this functionality arose from a desire to allow certain
non-privileged users to run perf. To do this, we install perf without
set-uid-root and have a set-uid-root wrapper decide who is allowed to
run perf (and with what arguments).
The wrapper must execve perf with real and effective root uid, because
perf and KASLR require this. However, that presently resets fsuid to
root, giving the user ability to read and overwrite any file owned by
root (perf report -i, perf record -o). Also, perf record will create
perf.data that cannot be deleted by the user.
We cannot reset /proc/sys/kernel/perf_event_paranoid to a permissive
level, since we must be selective which users have the permissions.
Of course, we could fix our problem by a patch to perf to allow
passing a username on the command line and having perf execute
setfsuid before opening files. However, perf is not the only program
that uses kernel features that require root uid/euid, so a general
solution that does not involve updating all such programs seems
warranted.
I will update man pages, if this patch is deemed a good idea.
Igor Lubashev (1):
security: add SECURE_KEEP_FSUID to preserve fsuid/fsgid across execve
include/uapi/linux/securebits.h | 10 +++++++++-
security/commoncap.c | 9 +++++++--
2 files changed, 16 insertions(+), 3 deletions(-)
--
2.7.4
^ permalink raw reply
* [RFC PATCH 1/1] security: add SECURE_KEEP_FSUID to preserve fsuid/fsgid across execve
From: Igor Lubashev @ 2019-06-14 0:44 UTC (permalink / raw)
To: Serge Hallyn, James Morris, linux-security-module
Cc: linux-kernel, Igor Lubashev
In-Reply-To: <1560473087-27754-1-git-send-email-ilubashe@akamai.com>
Many kernel interfaces require real and/or effective root uid instead
of relying solely of capabilities. An executable that uses such
interfaces has to be set-uid-root or be executed by a thread with
effective root uid. Presently, fsuid and saved uid will reset to the
effective uid during execve. As a result, it is not possible to
execute a binary such that it has effective root uid but retains
fsuid/fsgid of the actual user. Retaining fsuid/fsgid of the actual
user could be required if the executable needs to access the
filesystem. It may also be desired from the security perspective of
delegating the minimal set of privileges.
Setting SECURE_KEEP_FSUID bit ensures that the current fsuid/fsgiud is
retained by execve.
Signed-off-by: Igor Lubashev <ilubashe@akamai.com>
---
include/uapi/linux/securebits.h | 10 +++++++++-
security/commoncap.c | 9 +++++++--
2 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/include/uapi/linux/securebits.h b/include/uapi/linux/securebits.h
index d6d98877ff1a..6c20b2287d6f 100644
--- a/include/uapi/linux/securebits.h
+++ b/include/uapi/linux/securebits.h
@@ -52,10 +52,18 @@
#define SECBIT_NO_CAP_AMBIENT_RAISE_LOCKED \
(issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE_LOCKED))
+/* When set, execve does not reset fsuid/fsgid to euid/egid */
+#define SECURE_KEEP_FSUID 8
+#define SECURE_KEEP_FSUID_LOCKED 9 /* make bit-8 immutable */
+
+#define SECBIT_KEEP_FSUID (issecure_mask(SECURE_KEEP_FSUID))
+#define SECBIT_KEEP_FSUID_LOCKED (issecure_mask(SECURE_KEEP_FSUID_LOCKED))
+
#define SECURE_ALL_BITS (issecure_mask(SECURE_NOROOT) | \
issecure_mask(SECURE_NO_SETUID_FIXUP) | \
issecure_mask(SECURE_KEEP_CAPS) | \
- issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE))
+ issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE) | \
+ issecure_mask(SECURE_KEEP_FSUID))
#define SECURE_ALL_LOCKS (SECURE_ALL_BITS << 1)
#endif /* _UAPI_LINUX_SECUREBITS_H */
diff --git a/security/commoncap.c b/security/commoncap.c
index c0b9664ee49e..e4de823a1d4e 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -847,8 +847,13 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
old->cap_permitted);
}
- new->suid = new->fsuid = new->euid;
- new->sgid = new->fsgid = new->egid;
+ new->suid = new->euid;
+ new->sgid = new->egid;
+
+ if (!issecure(SECURE_KEEP_FSUID)) {
+ new->fsuid = new->euid;
+ new->fsgid = new->egid;
+ }
/* File caps or setid cancels ambient. */
if (has_fcap || is_setid)
--
2.7.4
^ permalink raw reply related
* Re: [RFC PATCH v1 2/3] LSM/x86/sgx: Implement SGX specific hooks in SELinux
From: Sean Christopherson @ 2019-06-14 0:46 UTC (permalink / raw)
To: Stephen Smalley
Cc: Cedric Xing, linux-security-module, selinux, linux-kernel,
linux-sgx, jarkko.sakkinen, luto, jmorris, serge, paul, eparis,
jethro, dave.hansen, tglx, torvalds, akpm, nhorman, pmccallum,
serge.ayoun, shay.katz-zamir, haitao.huang, andriy.shevchenko,
kai.svahn, bp, josh, kai.huang, rientjes, william.c.roberts,
philip.b.tricca
In-Reply-To: <8d99d8fb-a921-286a-8cf0-cd522e09b37c@tycho.nsa.gov>
On Thu, Jun 13, 2019 at 01:02:17PM -0400, Stephen Smalley wrote:
> On 6/11/19 6:02 PM, Sean Christopherson wrote:
> >On Tue, Jun 11, 2019 at 09:40:25AM -0400, Stephen Smalley wrote:
> >>I haven't looked at this code closely, but it feels like a lot of
> >>SGX-specific logic embedded into SELinux that will have to be repeated or
> >>reused for every security module. Does SGX not track this state itself?
> >
> >SGX does track equivalent state.
> >
> >There are three proposals on the table (I think):
> >
> > 1. Require userspace to explicitly specificy (maximal) enclave page
> > permissions at build time. The enclave page permissions are provided
> > to, and checked by, LSMs at enclave build time.
> >
> > Pros: Low-complexity kernel implementation, straightforward auditing
> > Cons: Sullies the SGX UAPI to some extent, may increase complexity of
> > SGX2 enclave loaders.
> >
> > 2. Pre-check LSM permissions and dynamically track mappings to enclave
> > pages, e.g. add an SGX mprotect() hook to restrict W->X and WX
> > based on the pre-checked permissions.
> >
> > Pros: Does not impact SGX UAPI, medium kernel complexity
> > Cons: Auditing is complex/weird, requires taking enclave-specific
> > lock during mprotect() to query/update tracking.
> >
> > 3. Implement LSM hooks in SGX to allow LSMs to track enclave regions
> > from cradle to grave, but otherwise defer everything to LSMs.
> >
> > Pros: Does not impact SGX UAPI, maximum flexibility, precise auditing
> > Cons: Most complex and "heaviest" kernel implementation of the three,
> > pushes more SGX details into LSMs.
> >
> >My RFC series[1] implements #1. My understanding is that Andy (Lutomirski)
> >prefers #2. Cedric's RFC series implements #3.
> >
> >Perhaps the easiest way to make forward progress is to rule out the
> >options we absolutely *don't* want by focusing on the potentially blocking
> >issue with each option:
> >
> > #1 - SGX UAPI funkiness
> >
> > #2 - Auditing complexity, potential enclave lock contention
> >
> > #3 - Pushing SGX details into LSMs and complexity of kernel implementation
> >
> >
> >[1] https://lkml.kernel.org/r/20190606021145.12604-1-sean.j.christopherson@intel.com
>
> Given the complexity tradeoff, what is the clear motivating example for why
> #1 isn't the obvious choice? That the enclave loader has no way of knowing a
> priori whether the enclave will require W->X or WX? But aren't we better
> off requiring enclaves to be explicitly marked as needing such so that we
> can make a more informed decision about whether to load them in the first
> place?
Andy and/or Cedric, can you please weigh in with a concrete (and practical)
use case that will break if we go with #1? The auditing issues for #2/#3
are complex to say the least...
^ permalink raw reply
* Re: [RFC PATCH v1 2/3] LSM/x86/sgx: Implement SGX specific hooks in SELinux
From: Sean Christopherson @ 2019-06-14 0:37 UTC (permalink / raw)
To: Stephen Smalley
Cc: Xing, Cedric, linux-security-module@vger.kernel.org,
selinux@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-sgx@vger.kernel.org, jarkko.sakkinen@linux.intel.com,
luto@kernel.org, jmorris@namei.org, serge@hallyn.com,
paul@paul-moore.com, eparis@parisplace.org, jethro@fortanix.com,
Hansen, Dave, tglx@linutronix.de, torvalds@linux-foundation.org,
akpm@linux-foundation.org, nhorman@redhat.com,
pmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, andriy.shevchenko@linux.intel.com, Svahn, Kai,
bp@alien8.de, josh@joshtriplett.org, Huang, Kai,
rientjes@google.com, Roberts, William C, Tricca, Philip B
In-Reply-To: <b7f3decf-b1d2-01b1-6294-ccf9ba2d5df4@tycho.nsa.gov>
On Thu, Jun 13, 2019 at 02:00:29PM -0400, Stephen Smalley wrote:
> On 6/11/19 6:55 PM, Xing, Cedric wrote:
> >*_noaudit() is exactly what I wanted. But I couldn't find
> >selinux_file_mprotect_noaudit()/file_has_perm_noaudit(), and I'm reluctant
> >to duplicate code. Any suggestions?
>
> I would have no objection to adding _noaudit() variants of these, either
> duplicating code (if sufficiently small/simple) or creating a common helper
> with a bool audit flag that gets used for both. But the larger issue would
> be to resolve how to ultimately ensure that a denial is audited later if the
> denied permission is actually requested and blocked via sgxsec_mprotect().
I too would like to see a solution to the auditing issue. I wrongly
assumed Cedric's approach (option #3) didn't suffer the same auditing
problem as Andy's dynamic tracking proposal (option #2). After reading
through the code more carefully (trying to steal ideas to finish off an
implementation of #2), I've come to realize options #2 (Andy) and #3
(Cedric) are basically identical concepts, the only difference being who
tracks state.
We can use the f_security blob sizes to identify which LSM denied
something, but I haven't the faintest idea how to track the auditing
information in a sane fashion. We'd basically have to do a deep copy on
struct common_audit_data, or pre-generate and store the audit message.
For SELinux, a deep copy is somewhat feasible because selinux_audit_data
distills everything down to basic types. AppArmor on the other hand has
'struct aa_label *label', which at a glance all but requires pre-generating
the audit message, and since AppArmor logs denials from every profile, it's
possible the "sgx audit blob" will consume a non-trivial amount of data.
Even if we figure out a way to store the audit messages without exploding
memory consumption or making things horrendously complex, we still have a
problem of reporting state info. Any number of things could be removed or
modified by the time the audit is actually triggered, e.g. files removed,
AppArmor profiles modified, etc... Any such change means we're logging
garbage.
^ permalink raw reply
* RE: [RFC PATCH v1 2/3] LSM/x86/sgx: Implement SGX specific hooks in SELinux
From: Xing, Cedric @ 2019-06-14 0:31 UTC (permalink / raw)
To: Christopherson, Sean J
Cc: Stephen Smalley, linux-security-module@vger.kernel.org,
selinux@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-sgx@vger.kernel.org, jarkko.sakkinen@linux.intel.com,
luto@kernel.org, jmorris@namei.org, serge@hallyn.com,
paul@paul-moore.com, eparis@parisplace.org, jethro@fortanix.com,
Hansen, Dave, tglx@linutronix.de, torvalds@linux-foundation.org,
akpm@linux-foundation.org, nhorman@redhat.com,
pmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, andriy.shevchenko@linux.intel.com, Svahn, Kai,
bp@alien8.de, josh@joshtriplett.org, Huang, Kai,
rientjes@google.com, Roberts, William C, Tricca, Philip B
In-Reply-To: <20190613231755.GD18385@linux.intel.com>
> From: Christopherson, Sean J
> Sent: Thursday, June 13, 2019 4:18 PM
>
> On Thu, Jun 13, 2019 at 04:03:24PM -0700, Xing, Cedric wrote:
> > > From: Stephen Smalley [mailto:sds@tycho.nsa.gov]
> > > Sent: Thursday, June 13, 2019 10:02 AM
> > >
> > > > My RFC series[1] implements #1. My understanding is that Andy
> > > > (Lutomirski) prefers #2. Cedric's RFC series implements #3.
> > > >
> > > > Perhaps the easiest way to make forward progress is to rule out
> the
> > > > options we absolutely *don't* want by focusing on the potentially
> > > > blocking issue with each option:
> > > >
> > > > #1 - SGX UAPI funkiness
> > > >
> > > > #2 - Auditing complexity, potential enclave lock contention
> > > >
> > > > #3 - Pushing SGX details into LSMs and complexity of kernel
> > > > implementation
> > > >
> > > >
> > > > [1]
> > > > https://lkml.kernel.org/r/20190606021145.12604-1-
> sean.j.christopherson
> > > > @intel.com
> > >
> > > Given the complexity tradeoff, what is the clear motivating example
> for
> > > why #1 isn't the obvious choice? That the enclave loader has no way
> of
> > > knowing a priori whether the enclave will require W->X or WX? But
> > > aren't we better off requiring enclaves to be explicitly marked as
> > > needing such so that we can make a more informed decision about
> whether
> > > to load them in the first place?
> >
> > Are you asking this question at a) page granularity, b) file
> granularity or
> > c) enclave (potentially comprised of multiple executable files)
> granularity?
> >
> > #b is what we have on regular executable files and shared objects (i.e.
> > FILE__EXECMOD). We all know how to do that.
> >
> > #c is kind of new but could be done via some proxy file (e.g.
> sigstruct file)
> > hence reduced to #b.
> >
> > #a is problematic. It'd require compilers/linkers to generate such
> > information, and proper executable image file format to carry that
> > information, to be eventually picked up the loader. SELinux doesn't
> have
> > PAGE__EXECMOD I guess is because it is generally considered
> impractical.
> >
> > Option #1 however requires #a because the driver doesn't track which
> page was
> > loaded from which file, otherwise it can no longer be qualified
> "simple". Or
> > we could just implement #c, which will make all options simpler. But I
> guess
> > #b is still preferred, to be aligned with what SELinux is enforcing
> today on
> > regular memory pages.o
>
> Option #1 doesn't require (a). The checks will happen for every page,
> but in the RFCs I sent, the policies are still attached to files and
> processes, i.e. (b).
I was talking at the UAPI level - i.e. your ioctl requires ALLOW_* at page granularity, hence #a.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox