Re: [GIT PULL] Security subsystem updates for 4.14

[Mimi Zohar]

On Sat, 2017-09-09 at 05:57 +1000, James Morris wrote:
> On Fri, 8 Sep 2017, Linus Torvalds wrote:
> 
> > Now the whole security pull will be ignored because of this thing. I
> > refuse to pull garbage where I notice major fundamental problems in
> > code that has obviously never ever been tested.
> 
> If I revert the change from my my tree, will you pull it then?

Sigh, none of the patches, other than those included in Paul's pull
request, are in 4.14-rc1.  I feel really horrible!

Linus, there would never have been a good time for something like this
to happen, but this past week, in case you didn't realize, most of us
were at the Linux Security Summit.  The timing couldn't have been
worse.

Mimi

Re: [GIT PULL] Security subsystem updates for 4.14

[Mimi Zohar]

On Sat, 2017-09-09 at 05:57 +1000, James Morris wrote:
> On Fri, 8 Sep 2017, Linus Torvalds wrote:
> 
> > Now the whole security pull will be ignored because of this thing. I
> > refuse to pull garbage where I notice major fundamental problems in
> > code that has obviously never ever been tested.
> 
> If I revert the change from my my tree, will you pull it then?

Sigh, none of the patches, other than those included in Paul's pull
request, are in 4.14-rc1.  I feel really horrible!

Linus, there would never have been a good time for something like this
to happen, but this past week, in case you didn't realize, most of us
were at the Linux Security Summit.  The timing couldn't have been
worse.

Mimi

Re: [GIT PULL] Security subsystem updates for 4.14

[Kees Cook]

On Thu, Sep 14, 2017 at 2:13 PM, James Morris  wrote:
> On Thu, 14 Sep 2017, Kees Cook wrote:
>
>> How separable are the patches, normally?
>
> They're usually mostly separate, but may depend on some core LSM change,
> or similar.
>
> Casey has mentioned off-list that it is useful to have a coherent up to
> date security branch to work against when making core LSM changes.

Yeah, for sure. This "merge all topics" tree is what I have for KSPP
(and it's what I hand to -next).

-Kees

-- 
Kees Cook
Pixel Security

Re: [GIT PULL] Security subsystem updates for 4.14

[Kees Cook]

On Thu, Sep 14, 2017 at 2:13 PM, James Morris  wrote:
> On Thu, 14 Sep 2017, Kees Cook wrote:
>
>> How separable are the patches, normally?
>
> They're usually mostly separate, but may depend on some core LSM change,
> or similar.
>
> Casey has mentioned off-list that it is useful to have a coherent up to
> date security branch to work against when making core LSM changes.

Yeah, for sure. This "merge all topics" tree is what I have for KSPP
(and it's what I hand to -next).

-Kees

-- 
Kees Cook
Pixel Security

Re: [GIT PULL] Security subsystem updates for 4.14

[James Morris]

On Thu, 14 Sep 2017, Kees Cook wrote:

> How separable are the patches, normally?

They're usually mostly separate, but may depend on some core LSM change, 
or similar.

Casey has mentioned off-list that it is useful to have a coherent up to 
date security branch to work against when making core LSM changes.


-- 
James Morris

Re: [GIT PULL] Security subsystem updates for 4.14

[James Morris]

On Thu, 14 Sep 2017, Kees Cook wrote:

> How separable are the patches, normally?

They're usually mostly separate, but may depend on some core LSM change, 
or similar.

Casey has mentioned off-list that it is useful to have a coherent up to 
date security branch to work against when making core LSM changes.


-- 
James Morris

Re: [GIT PULL] Security subsystem updates for 4.14

[Kees Cook]

On Sat, Sep 9, 2017 at 9:32 PM, James Morris  wrote:
> On Fri, 8 Sep 2017, Paul Moore wrote:
>
>> > This is also why I tend to prefer getting multiple branches for
>> > independent things.
>
> [...]
>
>>
>> Is it time to start sending pull request for each LSM and thing under
>> security/ directly?  I'm not sure I have a strong preference either
>> way, I just don't want to see the SELinux changes ignored during the
>> merge window.
>
> They won't be ignored, we just need to get this issue resolved now and
> figure out how to implement multiple branches in the security tree.
>
> Looking at other git repos, the x86 folk have multiple branches.

Yeah, the x86 approach is what inspired my tree layout.

> One option for me would be to publish the trees I pull from as branches
> along side mine, with 'next' being a merge of all of directly applied
> patchsets and those ready for Linus to pull as one.
>
> So, branches in
> git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security
>
> might be:
>
>   next-selinux (Paul's next branch)
>   next-apparmor   (JJ's next branch)
>   next-integrity  (Mimi's)
>   next-tpm(Jarkko's)
>   [etc.]
>
>   next (merge all of the above to here)
>
> That way, we have a coherent 'next' branch for people to develop against
> and to push to Linus, but he can pull individual branches feeding into it
> if something is broken in one of them.
>
> Does that sound useful?

This is what I do with the KSPP tree (since it has a few unrelated
things in it), but you run the risk of getting too fine-grain and
creating dependencies between trees (e.g. adding a new hook that two
LSMs implement means either they depend on each other or both depend
on some third "core" tree).

How separable are the patches, normally?

-Kees

-- 
Kees Cook
Pixel Security

Re: [GIT PULL] Security subsystem updates for 4.14

[Kees Cook]

On Sat, Sep 9, 2017 at 9:32 PM, James Morris  wrote:
> On Fri, 8 Sep 2017, Paul Moore wrote:
>
>> > This is also why I tend to prefer getting multiple branches for
>> > independent things.
>
> [...]
>
>>
>> Is it time to start sending pull request for each LSM and thing under
>> security/ directly?  I'm not sure I have a strong preference either
>> way, I just don't want to see the SELinux changes ignored during the
>> merge window.
>
> They won't be ignored, we just need to get this issue resolved now and
> figure out how to implement multiple branches in the security tree.
>
> Looking at other git repos, the x86 folk have multiple branches.

Yeah, the x86 approach is what inspired my tree layout.

> One option for me would be to publish the trees I pull from as branches
> along side mine, with 'next' being a merge of all of directly applied
> patchsets and those ready for Linus to pull as one.
>
> So, branches in
> git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security
>
> might be:
>
>   next-selinux (Paul's next branch)
>   next-apparmor   (JJ's next branch)
>   next-integrity  (Mimi's)
>   next-tpm(Jarkko's)
>   [etc.]
>
>   next (merge all of the above to here)
>
> That way, we have a coherent 'next' branch for people to develop against
> and to push to Linus, but he can pull individual branches feeding into it
> if something is broken in one of them.
>
> Does that sound useful?

This is what I do with the KSPP tree (since it has a few unrelated
things in it), but you run the risk of getting too fine-grain and
creating dependencies between trees (e.g. adding a new hook that two
LSMs implement means either they depend on each other or both depend
on some third "core" tree).

How separable are the patches, normally?

-Kees

-- 
Kees Cook
Pixel Security

Re: [GIT PULL] Security subsystem updates for 4.14

[Paul Moore]

On Sun, Sep 10, 2017 at 12:32 AM, James Morris  wrote:
> On Fri, 8 Sep 2017, Paul Moore wrote:
>
>> > This is also why I tend to prefer getting multiple branches for
>> > independent things.
>
> [...]
>
>>
>> Is it time to start sending pull request for each LSM and thing under
>> security/ directly?  I'm not sure I have a strong preference either
>> way, I just don't want to see the SELinux changes ignored during the
>> merge window.
>
> They won't be ignored, we just need to get this issue resolved now and
> figure out how to implement multiple branches in the security tree.

Once again, I don't really care too much either way.  My only selfish
motivation is to make it as frictionless as possible to get the
SELinux tree merged into Linus' tree.

> Looking at other git repos, the x86 folk have multiple branches.

I don't really understand what advantage one repo with multiple
branches has over multiple repos, e.g. Linus' just pulling from the
individual LSM trees directly.  I suppose one could make an argument
about linux-next, but I know they prefer to pull from the individual
repos directly (they pull selinux/next directly).  Is it to help
reduce the load on Linus?

>From my perspective, the linux-security tree only introduces another
opportunity for things to go wrong during the merge window (as
evidenced by this latest snafu).  Help me understand why a single tree
with multiple branches is beneficial to multiple trees?

Also, to be clear, I'm not picking on IMA or Mimi; this could have
easily been SELinux screwing things up for IMA (or Smack, or AppArmor,
etc.).

> One option for me would be to publish the trees I pull from as branches
> along side mine, with 'next' being a merge of all of directly applied
> patchsets and those ready for Linus to pull as one.
>
> So, branches in
> git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security
>
> might be:
>
>   next-selinux (Paul's next branch)
>   next-apparmor-next   (JJ's next branch)
>   next-integrity-next  (Mimi's)
>   next-tpm-next(Jarkko's)
>   [etc.]
>
>   next (merge all of the above to here)
>
> That way, we have a coherent 'next' branch for people to develop against
> and to push to Linus, but he can pull individual branches feeding into it
> if something is broken in one of them.
>
> Does that sound useful?

-- 
paul moore
www.paul-moore.com

Re: [GIT PULL] Security subsystem updates for 4.14

[Paul Moore]

On Sun, Sep 10, 2017 at 12:32 AM, James Morris  wrote:
> On Fri, 8 Sep 2017, Paul Moore wrote:
>
>> > This is also why I tend to prefer getting multiple branches for
>> > independent things.
>
> [...]
>
>>
>> Is it time to start sending pull request for each LSM and thing under
>> security/ directly?  I'm not sure I have a strong preference either
>> way, I just don't want to see the SELinux changes ignored during the
>> merge window.
>
> They won't be ignored, we just need to get this issue resolved now and
> figure out how to implement multiple branches in the security tree.

Once again, I don't really care too much either way.  My only selfish
motivation is to make it as frictionless as possible to get the
SELinux tree merged into Linus' tree.

> Looking at other git repos, the x86 folk have multiple branches.

I don't really understand what advantage one repo with multiple
branches has over multiple repos, e.g. Linus' just pulling from the
individual LSM trees directly.  I suppose one could make an argument
about linux-next, but I know they prefer to pull from the individual
repos directly (they pull selinux/next directly).  Is it to help
reduce the load on Linus?

>From my perspective, the linux-security tree only introduces another
opportunity for things to go wrong during the merge window (as
evidenced by this latest snafu).  Help me understand why a single tree
with multiple branches is beneficial to multiple trees?

Also, to be clear, I'm not picking on IMA or Mimi; this could have
easily been SELinux screwing things up for IMA (or Smack, or AppArmor,
etc.).

> One option for me would be to publish the trees I pull from as branches
> along side mine, with 'next' being a merge of all of directly applied
> patchsets and those ready for Linus to pull as one.
>
> So, branches in
> git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security
>
> might be:
>
>   next-selinux (Paul's next branch)
>   next-apparmor-next   (JJ's next branch)
>   next-integrity-next  (Mimi's)
>   next-tpm-next(Jarkko's)
>   [etc.]
>
>   next (merge all of the above to here)
>
> That way, we have a coherent 'next' branch for people to develop against
> and to push to Linus, but he can pull individual branches feeding into it
> if something is broken in one of them.
>
> Does that sound useful?

-- 
paul moore
www.paul-moore.com

Re: [GIT PULL] Security subsystem updates for 4.14

[Mimi Zohar]

On Sun, 2017-09-10 at 23:38 -0700, Christoph Hellwig wrote:
> On Sun, Sep 10, 2017 at 10:02:42AM -0400, Mimi Zohar wrote:
> > We need to differentiate between policies and x509 certificates.  In
> > the policy case, they need to be signed and appraised, while in the
> > x509 certificate case, the certificate itself is signed so the file
> > doesn't need to be signed or verified.
> 
> How about you take this sketch over - I don't know much about the
> integrity code, and it seems like you actually wrote
> kernel_read_file_from_path as well - so you're at least 3 times as
> qualified as I am in this area..

Sure, it's been on my to do list.

Mimi

Re: [GIT PULL] Security subsystem updates for 4.14

[Mimi Zohar]

On Sun, 2017-09-10 at 23:38 -0700, Christoph Hellwig wrote:
> On Sun, Sep 10, 2017 at 10:02:42AM -0400, Mimi Zohar wrote:
> > We need to differentiate between policies and x509 certificates.  In
> > the policy case, they need to be signed and appraised, while in the
> > x509 certificate case, the certificate itself is signed so the file
> > doesn't need to be signed or verified.
> 
> How about you take this sketch over - I don't know much about the
> integrity code, and it seems like you actually wrote
> kernel_read_file_from_path as well - so you're at least 3 times as
> qualified as I am in this area..

Sure, it's been on my to do list.

Mimi

Re: [GIT PULL] Security subsystem updates for 4.14

[Christoph Hellwig]

On Sun, Sep 10, 2017 at 10:02:42AM -0400, Mimi Zohar wrote:
> We need to differentiate between policies and x509 certificates.  In
> the policy case, they need to be signed and appraised, while in the
> x509 certificate case, the certificate itself is signed so the file
> doesn't need to be signed or verified.

How about you take this sketch over - I don't know much about the
integrity code, and it seems like you actually wrote
kernel_read_file_from_path as well - so you're at least 3 times as
qualified as I am in this area..

Re: [GIT PULL] Security subsystem updates for 4.14

[Christoph Hellwig]

On Sun, Sep 10, 2017 at 10:02:42AM -0400, Mimi Zohar wrote:
> We need to differentiate between policies and x509 certificates.  In
> the policy case, they need to be signed and appraised, while in the
> x509 certificate case, the certificate itself is signed so the file
> doesn't need to be signed or verified.

How about you take this sketch over - I don't know much about the
integrity code, and it seems like you actually wrote
kernel_read_file_from_path as well - so you're at least 3 times as
qualified as I am in this area..

Re: [GIT PULL] Security subsystem updates for 4.14

[Mimi Zohar]

On Sun, 2017-09-10 at 01:10 -0700, Christoph Hellwig wrote:
> On Fri, Sep 08, 2017 at 10:25:53AM -0700, Linus Torvalds wrote:
> > I don't think anybody actually tests linux-next kernels in any big
> > way, and the automated tests that do get run probably don't run with
> > any integrity checking enabled.
> 
> Well, for the atual IMA deadlock issue I asked Mimi to produce automated
> tests and we get started on it.  I was pretty pissed about the
> assumptions IMA made on the fs, whch weren't documented or automatically
> tested - coming from the XFS background where we want all our features
> to run through automated tests that was just not how I'd expect thing
> to work.
> 
> But now as part of that I messed up the other caller of it, so I
> shouldn't complain too loud..
> 
> That being said - I really hink the certificate loading should not
> even go thorught this whole call path, but use our common helper to
> load a file into a buffer.  Something like the patch below, I'm just
> not sure if the last policy argument is what people want or if we'd need
> to add a new policy type for certificates.

We need to differentiate between policies and x509 certificates.  In
the policy case, they need to be signed and appraised, while in the
x509 certificate case, the certificate itself is signed so the file
doesn't need to be signed or verified.

> 
> ---
> From 3092ca95dfeff0dbd4e03ae62a51762facb15592 Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig 
> Date: Sun, 10 Sep 2017 09:49:45 +0200
> Subject: security/digisig: read file using kernel_read_file_from_path
> 
> This avoid using the new integrity_read file operation which requires
> i_rwsem already to be held, and avoids a lot of code duplication and
> call the proper LSM hooks.
> 
> [also constifies the path argument to kernel_read_file_from_path,
> as the callers needs it. Should probably be split into a separate patch]
> 
> Signed-off-by: Christoph Hellwig 
> ---
>  fs/exec.c  |  2 +-
>  include/linux/fs.h |  2 +-
>  security/integrity/digsig.c|  8 ---
>  security/integrity/iint.c  | 49 
> --
>  security/integrity/integrity.h |  2 --
>  5 files changed, 7 insertions(+), 56 deletions(-)
> 
> diff --git a/fs/exec.c b/fs/exec.c
> index 01a9fb9d8ac3..957a8ce294af 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -974,7 +974,7 @@ int kernel_read_file(struct file *file, void **buf, 
> loff_t *size,
>  }
>  EXPORT_SYMBOL_GPL(kernel_read_file);
> 
> -int kernel_read_file_from_path(char *path, void **buf, loff_t *size,
> +int kernel_read_file_from_path(const char *path, void **buf, loff_t *size,
>  loff_t max_size, enum kernel_read_file_id id)
>  {
>   struct file *file;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 2d0e6748e46e..58855daba1eb 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2791,7 +2791,7 @@ static inline const char *kernel_read_file_id_str(enum 
> kernel_read_file_id id)
>  extern int kernel_read(struct file *, loff_t, char *, unsigned long);
>  extern int kernel_read_file(struct file *, void **, loff_t *, loff_t,
>   enum kernel_read_file_id);
> -extern int kernel_read_file_from_path(char *, void **, loff_t *, loff_t,
> +extern int kernel_read_file_from_path(const char *, void **, loff_t *, 
> loff_t,
> enum kernel_read_file_id);
>  extern int kernel_read_file_from_fd(int, void **, loff_t *, loff_t,
>   enum kernel_read_file_id);
> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> index 06554c448dce..8112cd3c 100644
> --- a/security/integrity/digsig.c
> +++ b/security/integrity/digsig.c
> @@ -112,13 +112,15 @@ int __init integrity_init_keyring(const unsigned int id)
>  int __init integrity_load_x509(const unsigned int id, const char *path)
>  {
>   key_ref_t key;
> - char *data;
> + void *data = NULL;
> + loff_t size;
>   int rc;
> 
>   if (!keyring[id])
>   return -EINVAL;
> 
> - rc = integrity_read_file(path, );
> + rc = kernel_read_file_from_path(path, data, ,
> + 0, READING_POLICY);

READING_X509_CERT?

>   if (rc < 0)
>   return rc;
> 
> @@ -139,6 +141,6 @@ int __init integrity_load_x509(const unsigned int id, 
> const char *path)
> key_ref_to_ptr(key)->description, path);
>   key_ref_put(key);
>   }
> - kfree(data);
> + vfree(data);
>   return 0;
>  }
> diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> index 6fc888ca468e..c84e05866052 100644
> --- a/security/integrity/iint.c
> +++ b/security/integrity/iint.c
> @@ -200,55 +200,6 @@ int integrity_kernel_read(struct file *file, loff_t 
> offset,
>  }
> 
>  /*
> - * integrity_read_file - read entire file content into the buffer
> - *
> - * This is function 

Re: [GIT PULL] Security subsystem updates for 4.14

[Mimi Zohar]

On Sun, 2017-09-10 at 01:10 -0700, Christoph Hellwig wrote:
> On Fri, Sep 08, 2017 at 10:25:53AM -0700, Linus Torvalds wrote:
> > I don't think anybody actually tests linux-next kernels in any big
> > way, and the automated tests that do get run probably don't run with
> > any integrity checking enabled.
> 
> Well, for the atual IMA deadlock issue I asked Mimi to produce automated
> tests and we get started on it.  I was pretty pissed about the
> assumptions IMA made on the fs, whch weren't documented or automatically
> tested - coming from the XFS background where we want all our features
> to run through automated tests that was just not how I'd expect thing
> to work.
> 
> But now as part of that I messed up the other caller of it, so I
> shouldn't complain too loud..
> 
> That being said - I really hink the certificate loading should not
> even go thorught this whole call path, but use our common helper to
> load a file into a buffer.  Something like the patch below, I'm just
> not sure if the last policy argument is what people want or if we'd need
> to add a new policy type for certificates.

We need to differentiate between policies and x509 certificates.  In
the policy case, they need to be signed and appraised, while in the
x509 certificate case, the certificate itself is signed so the file
doesn't need to be signed or verified.

> 
> ---
> From 3092ca95dfeff0dbd4e03ae62a51762facb15592 Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig 
> Date: Sun, 10 Sep 2017 09:49:45 +0200
> Subject: security/digisig: read file using kernel_read_file_from_path
> 
> This avoid using the new integrity_read file operation which requires
> i_rwsem already to be held, and avoids a lot of code duplication and
> call the proper LSM hooks.
> 
> [also constifies the path argument to kernel_read_file_from_path,
> as the callers needs it. Should probably be split into a separate patch]
> 
> Signed-off-by: Christoph Hellwig 
> ---
>  fs/exec.c  |  2 +-
>  include/linux/fs.h |  2 +-
>  security/integrity/digsig.c|  8 ---
>  security/integrity/iint.c  | 49 
> --
>  security/integrity/integrity.h |  2 --
>  5 files changed, 7 insertions(+), 56 deletions(-)
> 
> diff --git a/fs/exec.c b/fs/exec.c
> index 01a9fb9d8ac3..957a8ce294af 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -974,7 +974,7 @@ int kernel_read_file(struct file *file, void **buf, 
> loff_t *size,
>  }
>  EXPORT_SYMBOL_GPL(kernel_read_file);
> 
> -int kernel_read_file_from_path(char *path, void **buf, loff_t *size,
> +int kernel_read_file_from_path(const char *path, void **buf, loff_t *size,
>  loff_t max_size, enum kernel_read_file_id id)
>  {
>   struct file *file;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 2d0e6748e46e..58855daba1eb 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2791,7 +2791,7 @@ static inline const char *kernel_read_file_id_str(enum 
> kernel_read_file_id id)
>  extern int kernel_read(struct file *, loff_t, char *, unsigned long);
>  extern int kernel_read_file(struct file *, void **, loff_t *, loff_t,
>   enum kernel_read_file_id);
> -extern int kernel_read_file_from_path(char *, void **, loff_t *, loff_t,
> +extern int kernel_read_file_from_path(const char *, void **, loff_t *, 
> loff_t,
> enum kernel_read_file_id);
>  extern int kernel_read_file_from_fd(int, void **, loff_t *, loff_t,
>   enum kernel_read_file_id);
> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> index 06554c448dce..8112cd3c 100644
> --- a/security/integrity/digsig.c
> +++ b/security/integrity/digsig.c
> @@ -112,13 +112,15 @@ int __init integrity_init_keyring(const unsigned int id)
>  int __init integrity_load_x509(const unsigned int id, const char *path)
>  {
>   key_ref_t key;
> - char *data;
> + void *data = NULL;
> + loff_t size;
>   int rc;
> 
>   if (!keyring[id])
>   return -EINVAL;
> 
> - rc = integrity_read_file(path, );
> + rc = kernel_read_file_from_path(path, data, ,
> + 0, READING_POLICY);

READING_X509_CERT?

>   if (rc < 0)
>   return rc;
> 
> @@ -139,6 +141,6 @@ int __init integrity_load_x509(const unsigned int id, 
> const char *path)
> key_ref_to_ptr(key)->description, path);
>   key_ref_put(key);
>   }
> - kfree(data);
> + vfree(data);
>   return 0;
>  }
> diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> index 6fc888ca468e..c84e05866052 100644
> --- a/security/integrity/iint.c
> +++ b/security/integrity/iint.c
> @@ -200,55 +200,6 @@ int integrity_kernel_read(struct file *file, loff_t 
> offset,
>  }
> 
>  /*
> - * integrity_read_file - read entire file content into the buffer
> - *
> - * This is function opens a file, allocates 

Re: [GIT PULL] Security subsystem updates for 4.14

[Theodore Ts'o]

On Sun, Sep 10, 2017 at 03:13:23AM -0400, Mimi Zohar wrote:
> 
> From a file integrity perspective this would be interesting, but that
> only addresses IMA-appraisal, not IMA-integrity or IMA-audit.  We
> would still need to calculate the file hash to be included in the
> measurement list and used for auditing.
> 
> Have you done any work on protecting the directory information itself
> (eg. file names) using Merkle trees?

I have not, because the problem that I was trying to address was
primarily concerned with immutable files.  I did do some brainstorming
about adding the filename into the data integrity header to prevent
someone who had direct access to the flash exchanging the inode
numbers for "rm" and "ls", such that if you had a policy which
required that all ELF executables be signed, that a trickster couldn't
cause the user to run "rm" when they meant to run, say, "ls", just for
the lulz.  :-)

But that would break various symlink or hardlinks that are
legitimately present (e.g., /sbin/mkfs.ext[234] being a link to
/sbin/mke2fs), and if the adversary can carry out a chip-off attack
against your root file system, (a) it's not clear how much this would
help, and (b) this is really what dm-verity is for.

The main security problem I was looking at is one where the system
image is already protected using dm-verity, but you might have (for
example) some APK files which are downloaded once and stored in some
shared-user directory hierarchy (so file-based encryption can't even
provide pretend integrity protection), integrity checked at download
time, and never checked again.  Adding some kind of per-file Merkle
tree might be useful in that particular use case.

Cheers,

- Ted

Re: [GIT PULL] Security subsystem updates for 4.14

[Theodore Ts'o]

On Sun, Sep 10, 2017 at 03:13:23AM -0400, Mimi Zohar wrote:
> 
> From a file integrity perspective this would be interesting, but that
> only addresses IMA-appraisal, not IMA-integrity or IMA-audit.  We
> would still need to calculate the file hash to be included in the
> measurement list and used for auditing.
> 
> Have you done any work on protecting the directory information itself
> (eg. file names) using Merkle trees?

I have not, because the problem that I was trying to address was
primarily concerned with immutable files.  I did do some brainstorming
about adding the filename into the data integrity header to prevent
someone who had direct access to the flash exchanging the inode
numbers for "rm" and "ls", such that if you had a policy which
required that all ELF executables be signed, that a trickster couldn't
cause the user to run "rm" when they meant to run, say, "ls", just for
the lulz.  :-)

But that would break various symlink or hardlinks that are
legitimately present (e.g., /sbin/mkfs.ext[234] being a link to
/sbin/mke2fs), and if the adversary can carry out a chip-off attack
against your root file system, (a) it's not clear how much this would
help, and (b) this is really what dm-verity is for.

The main security problem I was looking at is one where the system
image is already protected using dm-verity, but you might have (for
example) some APK files which are downloaded once and stored in some
shared-user directory hierarchy (so file-based encryption can't even
provide pretend integrity protection), integrity checked at download
time, and never checked again.  Adding some kind of per-file Merkle
tree might be useful in that particular use case.

Cheers,

- Ted

Re: [GIT PULL] Security subsystem updates for 4.14

[Christoph Hellwig]

On Fri, Sep 08, 2017 at 10:25:53AM -0700, Linus Torvalds wrote:
> I don't think anybody actually tests linux-next kernels in any big
> way, and the automated tests that do get run probably don't run with
> any integrity checking enabled.

Well, for the atual IMA deadlock issue I asked Mimi to produce automated
tests and we get started on it.  I was pretty pissed about the
assumptions IMA made on the fs, whch weren't documented or automatically
tested - coming from the XFS background where we want all our features
to run through automated tests that was just not how I'd expect thing
to work.

But now as part of that I messed up the other caller of it, so I
shouldn't complain too loud..

That being said - I really hink the certificate loading should not
even go thorught this whole call path, but use our common helper to
load a file into a buffer.  Something like the patch below, I'm just
not sure if the last policy argument is what people want or if we'd need
to add a new policy type for certificates.

---
>From 3092ca95dfeff0dbd4e03ae62a51762facb15592 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig 
Date: Sun, 10 Sep 2017 09:49:45 +0200
Subject: security/digisig: read file using kernel_read_file_from_path

This avoid using the new integrity_read file operation which requires
i_rwsem already to be held, and avoids a lot of code duplication and
call the proper LSM hooks.

[also constifies the path argument to kernel_read_file_from_path,
as the callers needs it. Should probably be split into a separate patch]

Signed-off-by: Christoph Hellwig 
---
 fs/exec.c  |  2 +-
 include/linux/fs.h |  2 +-
 security/integrity/digsig.c|  8 ---
 security/integrity/iint.c  | 49 --
 security/integrity/integrity.h |  2 --
 5 files changed, 7 insertions(+), 56 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 01a9fb9d8ac3..957a8ce294af 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -974,7 +974,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t 
*size,
 }
 EXPORT_SYMBOL_GPL(kernel_read_file);
 
-int kernel_read_file_from_path(char *path, void **buf, loff_t *size,
+int kernel_read_file_from_path(const char *path, void **buf, loff_t *size,
   loff_t max_size, enum kernel_read_file_id id)
 {
struct file *file;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 2d0e6748e46e..58855daba1eb 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2791,7 +2791,7 @@ static inline const char *kernel_read_file_id_str(enum 
kernel_read_file_id id)
 extern int kernel_read(struct file *, loff_t, char *, unsigned long);
 extern int kernel_read_file(struct file *, void **, loff_t *, loff_t,
enum kernel_read_file_id);
-extern int kernel_read_file_from_path(char *, void **, loff_t *, loff_t,
+extern int kernel_read_file_from_path(const char *, void **, loff_t *, loff_t,
  enum kernel_read_file_id);
 extern int kernel_read_file_from_fd(int, void **, loff_t *, loff_t,
enum kernel_read_file_id);
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 06554c448dce..8112cd3c 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -112,13 +112,15 @@ int __init integrity_init_keyring(const unsigned int id)
 int __init integrity_load_x509(const unsigned int id, const char *path)
 {
key_ref_t key;
-   char *data;
+   void *data = NULL;
+   loff_t size;
int rc;
 
if (!keyring[id])
return -EINVAL;
 
-   rc = integrity_read_file(path, );
+   rc = kernel_read_file_from_path(path, data, ,
+   0, READING_POLICY);
if (rc < 0)
return rc;
 
@@ -139,6 +141,6 @@ int __init integrity_load_x509(const unsigned int id, const 
char *path)
  key_ref_to_ptr(key)->description, path);
key_ref_put(key);
}
-   kfree(data);
+   vfree(data);
return 0;
 }
diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index 6fc888ca468e..c84e05866052 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -200,55 +200,6 @@ int integrity_kernel_read(struct file *file, loff_t offset,
 }
 
 /*
- * integrity_read_file - read entire file content into the buffer
- *
- * This is function opens a file, allocates the buffer of required
- * size, read entire file content to the buffer and closes the file
- *
- * It is used only by init code.
- *
- */
-int __init integrity_read_file(const char *path, char **data)
-{
-   struct file *file;
-   loff_t size;
-   char *buf;
-   int rc = -EINVAL;
-
-   if (!path || !*path)
-   return -EINVAL;
-
-   file = filp_open(path, O_RDONLY, 0);
-   if (IS_ERR(file)) {
-   rc = PTR_ERR(file);
- 

Re: [GIT PULL] Security subsystem updates for 4.14

[Christoph Hellwig]

On Fri, Sep 08, 2017 at 10:25:53AM -0700, Linus Torvalds wrote:
> I don't think anybody actually tests linux-next kernels in any big
> way, and the automated tests that do get run probably don't run with
> any integrity checking enabled.

Well, for the atual IMA deadlock issue I asked Mimi to produce automated
tests and we get started on it.  I was pretty pissed about the
assumptions IMA made on the fs, whch weren't documented or automatically
tested - coming from the XFS background where we want all our features
to run through automated tests that was just not how I'd expect thing
to work.

But now as part of that I messed up the other caller of it, so I
shouldn't complain too loud..

That being said - I really hink the certificate loading should not
even go thorught this whole call path, but use our common helper to
load a file into a buffer.  Something like the patch below, I'm just
not sure if the last policy argument is what people want or if we'd need
to add a new policy type for certificates.

---
>From 3092ca95dfeff0dbd4e03ae62a51762facb15592 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig 
Date: Sun, 10 Sep 2017 09:49:45 +0200
Subject: security/digisig: read file using kernel_read_file_from_path

This avoid using the new integrity_read file operation which requires
i_rwsem already to be held, and avoids a lot of code duplication and
call the proper LSM hooks.

[also constifies the path argument to kernel_read_file_from_path,
as the callers needs it. Should probably be split into a separate patch]

Signed-off-by: Christoph Hellwig 
---
 fs/exec.c  |  2 +-
 include/linux/fs.h |  2 +-
 security/integrity/digsig.c|  8 ---
 security/integrity/iint.c  | 49 --
 security/integrity/integrity.h |  2 --
 5 files changed, 7 insertions(+), 56 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 01a9fb9d8ac3..957a8ce294af 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -974,7 +974,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t 
*size,
 }
 EXPORT_SYMBOL_GPL(kernel_read_file);
 
-int kernel_read_file_from_path(char *path, void **buf, loff_t *size,
+int kernel_read_file_from_path(const char *path, void **buf, loff_t *size,
   loff_t max_size, enum kernel_read_file_id id)
 {
struct file *file;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 2d0e6748e46e..58855daba1eb 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2791,7 +2791,7 @@ static inline const char *kernel_read_file_id_str(enum 
kernel_read_file_id id)
 extern int kernel_read(struct file *, loff_t, char *, unsigned long);
 extern int kernel_read_file(struct file *, void **, loff_t *, loff_t,
enum kernel_read_file_id);
-extern int kernel_read_file_from_path(char *, void **, loff_t *, loff_t,
+extern int kernel_read_file_from_path(const char *, void **, loff_t *, loff_t,
  enum kernel_read_file_id);
 extern int kernel_read_file_from_fd(int, void **, loff_t *, loff_t,
enum kernel_read_file_id);
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 06554c448dce..8112cd3c 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -112,13 +112,15 @@ int __init integrity_init_keyring(const unsigned int id)
 int __init integrity_load_x509(const unsigned int id, const char *path)
 {
key_ref_t key;
-   char *data;
+   void *data = NULL;
+   loff_t size;
int rc;
 
if (!keyring[id])
return -EINVAL;
 
-   rc = integrity_read_file(path, );
+   rc = kernel_read_file_from_path(path, data, ,
+   0, READING_POLICY);
if (rc < 0)
return rc;
 
@@ -139,6 +141,6 @@ int __init integrity_load_x509(const unsigned int id, const 
char *path)
  key_ref_to_ptr(key)->description, path);
key_ref_put(key);
}
-   kfree(data);
+   vfree(data);
return 0;
 }
diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index 6fc888ca468e..c84e05866052 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -200,55 +200,6 @@ int integrity_kernel_read(struct file *file, loff_t offset,
 }
 
 /*
- * integrity_read_file - read entire file content into the buffer
- *
- * This is function opens a file, allocates the buffer of required
- * size, read entire file content to the buffer and closes the file
- *
- * It is used only by init code.
- *
- */
-int __init integrity_read_file(const char *path, char **data)
-{
-   struct file *file;
-   loff_t size;
-   char *buf;
-   int rc = -EINVAL;
-
-   if (!path || !*path)
-   return -EINVAL;
-
-   file = filp_open(path, O_RDONLY, 0);
-   if (IS_ERR(file)) {
-   rc = PTR_ERR(file);
-   pr_err("Unable to open 

Re: [GIT PULL] Security subsystem updates for 4.14

[Mimi Zohar]

On Fri, 2017-09-08 at 18:38 -0400, Theodore Ts'o wrote:
> On Fri, Sep 08, 2017 at 02:48:51PM +1000, James Morris wrote:
> > 
> > Mimi and Christoph worked together on this over several iterations -- I'll 
> > let them respond.
> 
> Mimi --- we should chat next week in LA.  I've been working on a
> design internally at work which proposes a generic VFS-layer library
> (ala how fscrypt in fs/crypto works) which provides data integrity
> using per-file Merkle trees.
> 
> The goals of this design:
> 
>* Simplicity; for ease in security review and upstream review and
>  acceptance
>* Useful for multiple use cases.  It is *not* Android/APK specific,
>  and indeed can be used for other things
>   * A better way of providing Linux IMA/EVM support for immutable
> files by moving the verification from time-of-open to
> time-of-readpage.  (This significantly reduces the performance
>   impact, since we don't need to lock down the file while the kernel
>   needs to run SHA1 on potentially gigabytes worth of file data.)
>   * Most use cases for file-level checksums are for files that
> don’t change over time (e.g., for Video, Audio, Backup files,
> etc.)  This allows us to provide a cheap and efficient way to
> provide checksum protect against storage-level corruption
> fairly easily.  So by supporting both SHA and CRC-32, we can
>   make this feature useful for more than just the security heads.  :-)
>* Like the encryption/fscrypt feature, most of the code to this
>  feature can be in a VFS-level library, with minimal hooks needed
>  to those file systems (ext4, f2fs) that wish to provide this
>  functionality.

>From a file integrity perspective this would be interesting, but that
only addresses IMA-appraisal, not IMA-integrity or IMA-audit.  We
would still need to calculate the file hash to be included in the
measurement list and used for auditing.

Have you done any work on protecting the directory information itself
(eg. file names) using Merkle trees?

Mimi

Re: [GIT PULL] Security subsystem updates for 4.14

[Mimi Zohar]

On Fri, 2017-09-08 at 18:38 -0400, Theodore Ts'o wrote:
> On Fri, Sep 08, 2017 at 02:48:51PM +1000, James Morris wrote:
> > 
> > Mimi and Christoph worked together on this over several iterations -- I'll 
> > let them respond.
> 
> Mimi --- we should chat next week in LA.  I've been working on a
> design internally at work which proposes a generic VFS-layer library
> (ala how fscrypt in fs/crypto works) which provides data integrity
> using per-file Merkle trees.
> 
> The goals of this design:
> 
>* Simplicity; for ease in security review and upstream review and
>  acceptance
>* Useful for multiple use cases.  It is *not* Android/APK specific,
>  and indeed can be used for other things
>   * A better way of providing Linux IMA/EVM support for immutable
> files by moving the verification from time-of-open to
> time-of-readpage.  (This significantly reduces the performance
>   impact, since we don't need to lock down the file while the kernel
>   needs to run SHA1 on potentially gigabytes worth of file data.)
>   * Most use cases for file-level checksums are for files that
> don’t change over time (e.g., for Video, Audio, Backup files,
> etc.)  This allows us to provide a cheap and efficient way to
> provide checksum protect against storage-level corruption
> fairly easily.  So by supporting both SHA and CRC-32, we can
>   make this feature useful for more than just the security heads.  :-)
>* Like the encryption/fscrypt feature, most of the code to this
>  feature can be in a VFS-level library, with minimal hooks needed
>  to those file systems (ext4, f2fs) that wish to provide this
>  functionality.

>From a file integrity perspective this would be interesting, but that
only addresses IMA-appraisal, not IMA-integrity or IMA-audit.  We
would still need to calculate the file hash to be included in the
measurement list and used for auditing.

Have you done any work on protecting the directory information itself
(eg. file names) using Merkle trees?

Mimi

Re: [GIT PULL] Security subsystem updates for 4.14

[Mimi Zohar]

On Thu, 2017-09-07 at 11:19 -0700, Linus Torvalds wrote:
> On Mon, Sep 4, 2017 at 3:29 AM, James Morris  wrote:
> >
> > IMA:
> >   - A new integrity_read file operation method, avoids races when
> > calculating file hashes
> 
> Honestly, this seems really odd.
> 
> It documents that it needs to be called with i_rwsem held exclusively,
> and even has a lockdep assert to that effect (well, not really: the
> code claims "exclusive", but the lockdep assert does not), but I'm not
> actually seeing anybody doing it.
> 
> Quite the reverse, I just see integrity_read_file() doing filp_open()
> on the pathname and passing it to integrity_kernel_read() with no
> locking.
> 
> It really looks like just pure garbage to me. I pulled, and I'm not
> unpulling the whole thing. I don't think it's been tested, and I don't
> think it can be right.
> 
> Tell me why I'm wrong, or tell me why that garbage made it in in the
> first place?

I'm really sorry for the long delay in responding.  I've been on
vacation the last week, mostly without cell phone and very limited
wifi access. 

True, there is a side case where integrity_read_file() is being called
without first taking the i_rwsem.  This side case permits signed x509
certificates to be loaded onto the trusted IMA/EVM keyrings, without
verifying the file signature stored as security.ima/security.evm
xattrs.  Basically, the xattr signatures can not be verified until the
keys are loaded.  The main use case is embedded systems which do not
have an initramfs, but have a specially crafted init script.  It
requires enabling CONFIG_IMA_LOAD_X509 or CONFIG_EVM_LOAD_X509.  The
new VFS integrity_read() file operation method would not be called.

The main use case for the new VFS integrity_read() file operation
method is to calculate the file hash, as Christoph described.

Mimi

Re: [GIT PULL] Security subsystem updates for 4.14

[Mimi Zohar]

On Thu, 2017-09-07 at 11:19 -0700, Linus Torvalds wrote:
> On Mon, Sep 4, 2017 at 3:29 AM, James Morris  wrote:
> >
> > IMA:
> >   - A new integrity_read file operation method, avoids races when
> > calculating file hashes
> 
> Honestly, this seems really odd.
> 
> It documents that it needs to be called with i_rwsem held exclusively,
> and even has a lockdep assert to that effect (well, not really: the
> code claims "exclusive", but the lockdep assert does not), but I'm not
> actually seeing anybody doing it.
> 
> Quite the reverse, I just see integrity_read_file() doing filp_open()
> on the pathname and passing it to integrity_kernel_read() with no
> locking.
> 
> It really looks like just pure garbage to me. I pulled, and I'm not
> unpulling the whole thing. I don't think it's been tested, and I don't
> think it can be right.
> 
> Tell me why I'm wrong, or tell me why that garbage made it in in the
> first place?

I'm really sorry for the long delay in responding.  I've been on
vacation the last week, mostly without cell phone and very limited
wifi access. 

True, there is a side case where integrity_read_file() is being called
without first taking the i_rwsem.  This side case permits signed x509
certificates to be loaded onto the trusted IMA/EVM keyrings, without
verifying the file signature stored as security.ima/security.evm
xattrs.  Basically, the xattr signatures can not be verified until the
keys are loaded.  The main use case is embedded systems which do not
have an initramfs, but have a specially crafted init script.  It
requires enabling CONFIG_IMA_LOAD_X509 or CONFIG_EVM_LOAD_X509.  The
new VFS integrity_read() file operation method would not be called.

The main use case for the new VFS integrity_read() file operation
method is to calculate the file hash, as Christoph described.

Mimi

Re: [GIT PULL] Security subsystem updates for 4.14

[James Morris]

On Sun, 10 Sep 2017, James Morris wrote:

>   next-apparmor-next   (JJ's next branch)
>   next-integrity-next  (Mimi's)
>   next-tpm-next(Jarkko's)

without '-next' on the end... (editing while jetlagged).


-- 
James Morris

Re: [GIT PULL] Security subsystem updates for 4.14

[James Morris]

On Sun, 10 Sep 2017, James Morris wrote:

>   next-apparmor-next   (JJ's next branch)
>   next-integrity-next  (Mimi's)
>   next-tpm-next(Jarkko's)

without '-next' on the end... (editing while jetlagged).


-- 
James Morris

Re: [GIT PULL] Security subsystem updates for 4.14

[James Morris]

On Fri, 8 Sep 2017, Paul Moore wrote:

> > This is also why I tend to prefer getting multiple branches for
> > independent things.

[...]

> 
> Is it time to start sending pull request for each LSM and thing under
> security/ directly?  I'm not sure I have a strong preference either
> way, I just don't want to see the SELinux changes ignored during the
> merge window.

They won't be ignored, we just need to get this issue resolved now and 
figure out how to implement multiple branches in the security tree.

Looking at other git repos, the x86 folk have multiple branches.

One option for me would be to publish the trees I pull from as branches 
along side mine, with 'next' being a merge of all of directly applied 
patchsets and those ready for Linus to pull as one.

So, branches in 
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security

might be:

  next-selinux (Paul's next branch)
  next-apparmor-next   (JJ's next branch)
  next-integrity-next  (Mimi's)
  next-tpm-next(Jarkko's)
  [etc.]

  next (merge all of the above to here)

That way, we have a coherent 'next' branch for people to develop against 
and to push to Linus, but he can pull individual branches feeding into it 
if something is broken in one of them.

Does that sound useful?


-- 
James Morris

Re: [GIT PULL] Security subsystem updates for 4.14

[James Morris]

On Fri, 8 Sep 2017, Paul Moore wrote:

> > This is also why I tend to prefer getting multiple branches for
> > independent things.

[...]

> 
> Is it time to start sending pull request for each LSM and thing under
> security/ directly?  I'm not sure I have a strong preference either
> way, I just don't want to see the SELinux changes ignored during the
> merge window.

They won't be ignored, we just need to get this issue resolved now and 
figure out how to implement multiple branches in the security tree.

Looking at other git repos, the x86 folk have multiple branches.

One option for me would be to publish the trees I pull from as branches 
along side mine, with 'next' being a merge of all of directly applied 
patchsets and those ready for Linus to pull as one.

So, branches in 
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security

might be:

  next-selinux (Paul's next branch)
  next-apparmor-next   (JJ's next branch)
  next-integrity-next  (Mimi's)
  next-tpm-next(Jarkko's)
  [etc.]

  next (merge all of the above to here)

That way, we have a coherent 'next' branch for people to develop against 
and to push to Linus, but he can pull individual branches feeding into it 
if something is broken in one of them.

Does that sound useful?


-- 
James Morris

Re: [GIT PULL] Security subsystem updates for 4.14

[James Morris]

On Fri, 8 Sep 2017, Theodore Ts'o wrote:

> On Fri, Sep 08, 2017 at 02:48:51PM +1000, James Morris wrote:
> > 
> > Mimi and Christoph worked together on this over several iterations -- I'll 
> > let them respond.
> 
> Mimi --- we should chat next week in LA.  I've been working on a
> design internally at work which proposes a generic VFS-layer library
> (ala how fscrypt in fs/crypto works) which provides data integrity
> using per-file Merkle trees.

It's possible we could add a BoF at LSS on the Thursday (from 5:45pm), or 
even Friday afternoon:

http://events.linuxfoundation.org/events/linux-security-summit/program/schedule

Let me know if you want to schedule something.


-- 
James Morris

Re: [GIT PULL] Security subsystem updates for 4.14

[James Morris]

On Fri, 8 Sep 2017, Theodore Ts'o wrote:

> On Fri, Sep 08, 2017 at 02:48:51PM +1000, James Morris wrote:
> > 
> > Mimi and Christoph worked together on this over several iterations -- I'll 
> > let them respond.
> 
> Mimi --- we should chat next week in LA.  I've been working on a
> design internally at work which proposes a generic VFS-layer library
> (ala how fscrypt in fs/crypto works) which provides data integrity
> using per-file Merkle trees.

It's possible we could add a BoF at LSS on the Thursday (from 5:45pm), or 
even Friday afternoon:

http://events.linuxfoundation.org/events/linux-security-summit/program/schedule

Let me know if you want to schedule something.


-- 
James Morris

Re: [GIT PULL] Security subsystem updates for 4.14

[Theodore Ts'o]

On Fri, Sep 08, 2017 at 02:48:51PM +1000, James Morris wrote:
> 
> Mimi and Christoph worked together on this over several iterations -- I'll 
> let them respond.

Mimi --- we should chat next week in LA.  I've been working on a
design internally at work which proposes a generic VFS-layer library
(ala how fscrypt in fs/crypto works) which provides data integrity
using per-file Merkle trees.

The goals of this design:

   * Simplicity; for ease in security review and upstream review and
 acceptance
   * Useful for multiple use cases.  It is *not* Android/APK specific,
 and indeed can be used for other things
  * A better way of providing Linux IMA/EVM support for immutable
files by moving the verification from time-of-open to
time-of-readpage.  (This significantly reduces the performance
impact, since we don't need to lock down the file while the kernel
needs to run SHA1 on potentially gigabytes worth of file data.)
  * Most use cases for file-level checksums are for files that
don’t change over time (e.g., for Video, Audio, Backup files,
etc.)  This allows us to provide a cheap and efficient way to
provide checksum protect against storage-level corruption
fairly easily.  So by supporting both SHA and CRC-32, we can
make this feature useful for more than just the security heads.  :-)
   * Like the encryption/fscrypt feature, most of the code to this
 feature can be in a VFS-level library, with minimal hooks needed
 to those file systems (ext4, f2fs) that wish to provide this
 functionality.

- Ted

Re: [GIT PULL] Security subsystem updates for 4.14

[Theodore Ts'o]

On Fri, Sep 08, 2017 at 02:48:51PM +1000, James Morris wrote:
> 
> Mimi and Christoph worked together on this over several iterations -- I'll 
> let them respond.

Mimi --- we should chat next week in LA.  I've been working on a
design internally at work which proposes a generic VFS-layer library
(ala how fscrypt in fs/crypto works) which provides data integrity
using per-file Merkle trees.

The goals of this design:

   * Simplicity; for ease in security review and upstream review and
 acceptance
   * Useful for multiple use cases.  It is *not* Android/APK specific,
 and indeed can be used for other things
  * A better way of providing Linux IMA/EVM support for immutable
files by moving the verification from time-of-open to
time-of-readpage.  (This significantly reduces the performance
impact, since we don't need to lock down the file while the kernel
needs to run SHA1 on potentially gigabytes worth of file data.)
  * Most use cases for file-level checksums are for files that
don’t change over time (e.g., for Video, Audio, Backup files,
etc.)  This allows us to provide a cheap and efficient way to
provide checksum protect against storage-level corruption
fairly easily.  So by supporting both SHA and CRC-32, we can
make this feature useful for more than just the security heads.  :-)
   * Like the encryption/fscrypt feature, most of the code to this
 feature can be in a VFS-level library, with minimal hooks needed
 to those file systems (ext4, f2fs) that wish to provide this
 functionality.

- Ted

Re: [GIT PULL] Security subsystem updates for 4.14

[James Morris]

On Fri, 8 Sep 2017, Linus Torvalds wrote:

> Now the whole security pull will be ignored because of this thing. I
> refuse to pull garbage where I notice major fundamental problems in
> code that has obviously never ever been tested.

If I revert the change from my my tree, will you pull it then?

-- 
James Morris

Re: [GIT PULL] Security subsystem updates for 4.14

[James Morris]

On Fri, 8 Sep 2017, Linus Torvalds wrote:

> Now the whole security pull will be ignored because of this thing. I
> refuse to pull garbage where I notice major fundamental problems in
> code that has obviously never ever been tested.

If I revert the change from my my tree, will you pull it then?

-- 
James Morris

Re: [GIT PULL] Security subsystem updates for 4.14

[Paul Moore]

On Fri, Sep 8, 2017 at 1:25 PM, Linus Torvalds
 wrote:
> On Fri, Sep 8, 2017 at 12:09 AM, Christoph Hellwig  wrote:
>>
>> But yes, for the init-time integrity_read_file this is incorrect.
>> It never tripped up, and I explicitly added the lockdep annotations
>> so that anything would show up, and it's been half a year since
>> I sent that first RFC patch..
>
> I don't think anybody actually tests linux-next kernels in any big
> way, and the automated tests that do get run probably don't run with
> any integrity checking enabled.
>
> Which is why I actually look at the code when merging unexpected stuff.
>
> This is also why I tend to prefer getting multiple branches for
> independent things.
>
> Now the whole security pull will be ignored because of this thing. I
> refuse to pull garbage where I notice major fundamental problems in
> code that has obviously never ever been tested.

Is it time to start sending pull request for each LSM and thing under
security/ directly?  I'm not sure I have a strong preference either
way, I just don't want to see the SELinux changes ignored during the
merge window.

-- 
paul moore
www.paul-moore.com

Re: [GIT PULL] Security subsystem updates for 4.14

[Paul Moore]

On Fri, Sep 8, 2017 at 1:25 PM, Linus Torvalds
 wrote:
> On Fri, Sep 8, 2017 at 12:09 AM, Christoph Hellwig  wrote:
>>
>> But yes, for the init-time integrity_read_file this is incorrect.
>> It never tripped up, and I explicitly added the lockdep annotations
>> so that anything would show up, and it's been half a year since
>> I sent that first RFC patch..
>
> I don't think anybody actually tests linux-next kernels in any big
> way, and the automated tests that do get run probably don't run with
> any integrity checking enabled.
>
> Which is why I actually look at the code when merging unexpected stuff.
>
> This is also why I tend to prefer getting multiple branches for
> independent things.
>
> Now the whole security pull will be ignored because of this thing. I
> refuse to pull garbage where I notice major fundamental problems in
> code that has obviously never ever been tested.

Is it time to start sending pull request for each LSM and thing under
security/ directly?  I'm not sure I have a strong preference either
way, I just don't want to see the SELinux changes ignored during the
merge window.

-- 
paul moore
www.paul-moore.com

Re: [GIT PULL] Security subsystem updates for 4.14

[Linus Torvalds]

On Fri, Sep 8, 2017 at 12:09 AM, Christoph Hellwig  wrote:
>
> But yes, for the init-time integrity_read_file this is incorrect.
> It never tripped up, and I explicitly added the lockdep annotations
> so that anything would show up, and it's been half a year since
> I sent that first RFC patch..

I don't think anybody actually tests linux-next kernels in any big
way, and the automated tests that do get run probably don't run with
any integrity checking enabled.

Which is why I actually look at the code when merging unexpected stuff.

This is also why I tend to prefer getting multiple branches for
independent things.

Now the whole security pull will be ignored because of this thing. I
refuse to pull garbage where I notice major fundamental problems in
code that has obviously never ever been tested.

Side note: one of the reasons why I _looked_ at this code was because
the exclusive lock requirement was entirely unexplained in the first
place.

Linus

Re: [GIT PULL] Security subsystem updates for 4.14

[Linus Torvalds]

On Fri, Sep 8, 2017 at 12:09 AM, Christoph Hellwig  wrote:
>
> But yes, for the init-time integrity_read_file this is incorrect.
> It never tripped up, and I explicitly added the lockdep annotations
> so that anything would show up, and it's been half a year since
> I sent that first RFC patch..

I don't think anybody actually tests linux-next kernels in any big
way, and the automated tests that do get run probably don't run with
any integrity checking enabled.

Which is why I actually look at the code when merging unexpected stuff.

This is also why I tend to prefer getting multiple branches for
independent things.

Now the whole security pull will be ignored because of this thing. I
refuse to pull garbage where I notice major fundamental problems in
code that has obviously never ever been tested.

Side note: one of the reasons why I _looked_ at this code was because
the exclusive lock requirement was entirely unexplained in the first
place.

Linus

Re: [GIT PULL] Security subsystem updates for 4.14

[Christoph Hellwig]

The reason why I send out the original version of this patch
is because IMA used to call ->read under i_rwsem, and that deadlocked
on XFS and NFS, or ext3/4 with DAX.  The call path for that is

process_measurement (takes i_rwsem)
  -> ima_collect_measurement
-> ima_calc_file_hash
   -> ima_calc_file_ahash / ima_calc_file_shash
 -> ima_calc_file_hash_atfm / ima_calc_file_hash_tfm
   -> integrity_kernel_read

ima_check_last_writer (takes i_rwsem)
  -> ima_update_xattr
-> ima_collect_measurement
   -> (as above)

But yes, for the init-time integrity_read_file this is incorrect.
It never tripped up, and I explicitly added the lockdep annotations
so that anything would show up, and it's been half a year since
I sent that first RFC patch..

Re: [GIT PULL] Security subsystem updates for 4.14

[Christoph Hellwig]

The reason why I send out the original version of this patch
is because IMA used to call ->read under i_rwsem, and that deadlocked
on XFS and NFS, or ext3/4 with DAX.  The call path for that is

process_measurement (takes i_rwsem)
  -> ima_collect_measurement
-> ima_calc_file_hash
   -> ima_calc_file_ahash / ima_calc_file_shash
 -> ima_calc_file_hash_atfm / ima_calc_file_hash_tfm
   -> integrity_kernel_read

ima_check_last_writer (takes i_rwsem)
  -> ima_update_xattr
-> ima_collect_measurement
   -> (as above)

But yes, for the init-time integrity_read_file this is incorrect.
It never tripped up, and I explicitly added the lockdep annotations
so that anything would show up, and it's been half a year since
I sent that first RFC patch..

Re: [GIT PULL] Security subsystem updates for 4.14

[James Morris]

On Thu, 7 Sep 2017, Linus Torvalds wrote:

> On Mon, Sep 4, 2017 at 3:29 AM, James Morris  wrote:
> >
> > IMA:
> >   - A new integrity_read file operation method, avoids races when
> > calculating file hashes
> 
> Honestly, this seems really odd.
> 
> It documents that it needs to be called with i_rwsem held exclusively,
> and even has a lockdep assert to that effect (well, not really: the
> code claims "exclusive", but the lockdep assert does not), but I'm not
> actually seeing anybody doing it.
> 
> Quite the reverse, I just see integrity_read_file() doing filp_open()
> on the pathname and passing it to integrity_kernel_read() with no
> locking.
> 
> It really looks like just pure garbage to me. I pulled, and I'm not
> unpulling the whole thing. I don't think it's been tested, and I don't
> think it can be right.
> 
> Tell me why I'm wrong, or tell me why that garbage made it in in the
> first place?

Mimi and Christoph worked together on this over several iterations -- I'll 
let them respond.


-- 
James Morris

Re: [GIT PULL] Security subsystem updates for 4.14

[James Morris]

On Thu, 7 Sep 2017, Linus Torvalds wrote:

> On Mon, Sep 4, 2017 at 3:29 AM, James Morris  wrote:
> >
> > IMA:
> >   - A new integrity_read file operation method, avoids races when
> > calculating file hashes
> 
> Honestly, this seems really odd.
> 
> It documents that it needs to be called with i_rwsem held exclusively,
> and even has a lockdep assert to that effect (well, not really: the
> code claims "exclusive", but the lockdep assert does not), but I'm not
> actually seeing anybody doing it.
> 
> Quite the reverse, I just see integrity_read_file() doing filp_open()
> on the pathname and passing it to integrity_kernel_read() with no
> locking.
> 
> It really looks like just pure garbage to me. I pulled, and I'm not
> unpulling the whole thing. I don't think it's been tested, and I don't
> think it can be right.
> 
> Tell me why I'm wrong, or tell me why that garbage made it in in the
> first place?

Mimi and Christoph worked together on this over several iterations -- I'll 
let them respond.


-- 
James Morris

Re: [GIT PULL] Security subsystem updates for 4.14

[Linus Torvalds]

On Mon, Sep 4, 2017 at 3:29 AM, James Morris  wrote:
>
> IMA:
>   - A new integrity_read file operation method, avoids races when
> calculating file hashes

Honestly, this seems really odd.

It documents that it needs to be called with i_rwsem held exclusively,
and even has a lockdep assert to that effect (well, not really: the
code claims "exclusive", but the lockdep assert does not), but I'm not
actually seeing anybody doing it.

Quite the reverse, I just see integrity_read_file() doing filp_open()
on the pathname and passing it to integrity_kernel_read() with no
locking.

It really looks like just pure garbage to me. I pulled, and I'm not
unpulling the whole thing. I don't think it's been tested, and I don't
think it can be right.

Tell me why I'm wrong, or tell me why that garbage made it in in the
first place?

 Linus

Re: [GIT PULL] Security subsystem updates for 4.14

[Linus Torvalds]

On Mon, Sep 4, 2017 at 3:29 AM, James Morris  wrote:
>
> IMA:
>   - A new integrity_read file operation method, avoids races when
> calculating file hashes

Honestly, this seems really odd.

It documents that it needs to be called with i_rwsem held exclusively,
and even has a lockdep assert to that effect (well, not really: the
code claims "exclusive", but the lockdep assert does not), but I'm not
actually seeing anybody doing it.

Quite the reverse, I just see integrity_read_file() doing filp_open()
on the pathname and passing it to integrity_kernel_read() with no
locking.

It really looks like just pure garbage to me. I pulled, and I'm not
unpulling the whole thing. I don't think it's been tested, and I don't
think it can be right.

Tell me why I'm wrong, or tell me why that garbage made it in in the
first place?

 Linus