Re: [PATCH v4 3/3] Allows reading back the current IMA policy;

[Petko Manolov]

On 15-10-16 22:31:31, Petko Manolov wrote:
> When in development it is useful to read back the IMA policy.  This patch
> provides the functionality.  However, this is a potential security hole so
> it should not be used in production-grade kernels.
> 
> Signed-off-by: Petko Manolov 
> ---
>  security/integrity/ima/Kconfig  |  13 +++
>  security/integrity/ima/ima.h|  13 ++-
>  security/integrity/ima/ima_fs.c |  29 +-
>  security/integrity/ima/ima_policy.c | 190 
> 
>  4 files changed, 239 insertions(+), 6 deletions(-)
> 
> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> index 235b3c2..040778f 100644
> --- a/security/integrity/ima/Kconfig
> +++ b/security/integrity/ima/Kconfig
> @@ -121,6 +121,19 @@ config IMA_WRITE_POLICY
>  
> If unsure, say N.
>  
> +config IMA_READ_POLICY
> + bool "Enable reading back the current IMA policy"
> + depends on IMA
> + default n
> + help
> +When developing and debugging an IMA enabled system it is often
> +useful to be able to read the IMA policy.  This patch allows
> +for doing so.  However, being able to read the IMA policy is
> +considered insecure and is strongly discouraged for
> +production-grade kernels.
> +
> +If unsure, say N.
> +
>  config IMA_APPRAISE
>   bool "Appraise integrity measurements"
>   depends on IMA
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index e2a60c3..ebc3554 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -166,6 +166,10 @@ void ima_update_policy(void);
>  void ima_update_policy_flag(void);
>  ssize_t ima_parse_add_rule(char *);
>  void ima_delete_rules(void);
> +void *ima_policy_start(struct seq_file *m, loff_t *pos);
> +void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos);
> +void ima_policy_stop(struct seq_file *m, void *v);
> +int ima_policy_show(struct seq_file *m, void *v);
>  
>  /* Appraise integrity measurements */
>  #define IMA_APPRAISE_ENFORCE 0x01
> @@ -263,4 +267,11 @@ static inline int ima_init_keyring(const unsigned int id)
>   return 0;
>  }
>  #endif /* CONFIG_IMA_TRUSTED_KEYRING */
> -#endif
> +
> +#ifdef   CONFIG_IMA_READ_POLICY
> +#define  POLICY_FILE_FLAGS   (S_IWUSR | S_IRUSR)
> +#else
> +#define  POLICY_FILE_FLAGS   S_IWUSR
> +#endif /* CONFIG_IMA_WRITE_POLICY */
> +
> +#endif /* __LINUX_IMA_H */
> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> index a3cf5c0..85bd129 100644
> --- a/security/integrity/ima/ima_fs.c
> +++ b/security/integrity/ima/ima_fs.c
> @@ -311,14 +311,29 @@ enum ima_fs_flags {
>  
>  static unsigned long ima_fs_flags;
>  
> +#ifdef   CONFIG_IMA_READ_POLICY
> +static const struct seq_operations ima_policy_seqops = {
> + .start = ima_policy_start,
> + .next = ima_policy_next,
> + .stop = ima_policy_stop,
> + .show = ima_policy_show,
> +};
> +#endif
> +
>  /*
>   * ima_open_policy: sequentialize access to the policy file
>   */
>  static int ima_open_policy(struct inode *inode, struct file *filp)
>  {
> - /* No point in being allowed to open it if you aren't going to write */
> - if (!(filp->f_flags & O_WRONLY))
> + if (!(filp->f_flags & O_WRONLY)) {
> +#ifndef  CONFIG_IMA_READ_POLICY
>   return -EACCES;
> +#else
> + if (!capable(CAP_SYS_ADMIN))
> + return -EPERM;
> + return seq_open(filp, _policy_seqops);
> +#endif
> + }
>   if (test_and_set_bit(IMA_FS_BUSY, _fs_flags))
>   return -EBUSY;
>   return 0;
> @@ -334,6 +349,10 @@ static int ima_open_policy(struct inode *inode, struct 
> file *filp)
>  static int ima_release_policy(struct inode *inode, struct file *file)
>  {
>   const char *cause = valid_policy ? "completed" : "failed";
> + int policy_write = (file->f_flags & O_WRONLY);
> +
> + if (!policy_write)
> + return 0;

The above check is bogus so these four lines should go away too.  Silently 
returning if 'policy' is readable is now what we mean.  I guess we should check 
the operation, not the file flags.

Petko


>   pr_info("IMA: policy update %s\n", cause);
>   integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL, NULL,
> @@ -342,9 +361,9 @@ static int ima_release_policy(struct inode *inode, struct 
> file *file)
>   if (!valid_policy) {
>   ima_delete_rules();
>   valid_policy = 1;
> - clear_bit(IMA_FS_BUSY, _fs_flags);
>   return 0;
>   }
> +
>   ima_update_policy();
>  #ifndef  CONFIG_IMA_WRITE_POLICY
>   securityfs_remove(ima_policy);
> @@ -358,6 +377,7 @@ static int ima_release_policy(struct inode *inode, struct 
> file *file)
>  static const struct file_operations ima_measure_policy_ops = {
>   .open = 

Re: [PATCH v4 3/3] Allows reading back the current IMA policy;

[Mimi Zohar]

On Tue, 2015-10-20 at 10:26 +0300, Petko Manolov wrote:
> On 15-10-19 14:21:42, Mimi Zohar wrote:
> > On Fri, 2015-10-16 at 22:31 +0300, Petko Manolov wrote:
> > > When in development it is useful to read back the IMA policy.  This patch
> > > provides the functionality.  However, this is a potential security hole so
> > > it should not be used in production-grade kernels.
> >  
> > Like the other IMA securityfs files, only root would be able to read it.
> > Once we start allowing additional rules to be appended to the policy,
> > being able to view the resulting policy is important.  Is there a reason
> > for limiting this option to development?
> 
> I have not considered allowing non-root users to read the policy - i was 
> merely 
> cleaning up the Zbigniew's patch.  I guess it might be useful to be able to 
> read 
> the policy when in development mode.

I guess I wasn't clear.  I don't have a problem with the patch itself,
just with the patch description.  What is this "security hole" that the
option should ONLY be configured for development?Only privileged
users can view the policy.  I don't see the problem with configuring it
in general.  Please remove the comment.

Since responding, I've enabled this feature.  Very nice!

Mimi

--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 3/3] Allows reading back the current IMA policy;

[Petko Manolov]

On 15-10-19 14:21:42, Mimi Zohar wrote:
> On Fri, 2015-10-16 at 22:31 +0300, Petko Manolov wrote:
> > When in development it is useful to read back the IMA policy.  This patch
> > provides the functionality.  However, this is a potential security hole so
> > it should not be used in production-grade kernels.
>  
> Like the other IMA securityfs files, only root would be able to read it.
> Once we start allowing additional rules to be appended to the policy,
> being able to view the resulting policy is important.  Is there a reason
> for limiting this option to development?

I have not considered allowing non-root users to read the policy - i was merely 
cleaning up the Zbigniew's patch.  I guess it might be useful to be able to 
read 
the policy when in development mode.


Petko


> > Signed-off-by: Petko Manolov 
> > ---
> >  security/integrity/ima/Kconfig  |  13 +++
> >  security/integrity/ima/ima.h|  13 ++-
> >  security/integrity/ima/ima_fs.c |  29 +-
> >  security/integrity/ima/ima_policy.c | 190 
> > 
> >  4 files changed, 239 insertions(+), 6 deletions(-)
> > 
> > diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> > index 235b3c2..040778f 100644
> > --- a/security/integrity/ima/Kconfig
> > +++ b/security/integrity/ima/Kconfig
> > @@ -121,6 +121,19 @@ config IMA_WRITE_POLICY
> > 
> >   If unsure, say N.
> > 
> > +config IMA_READ_POLICY
> > +   bool "Enable reading back the current IMA policy"
> > +   depends on IMA
> > +   default n
> > +   help
> > +  When developing and debugging an IMA enabled system it is often
> > +  useful to be able to read the IMA policy.  This patch allows
> > +  for doing so.  However, being able to read the IMA policy is
> > +  considered insecure and is strongly discouraged for
> > +  production-grade kernels.
> > +
> > +  If unsure, say N.
> > +
> >  config IMA_APPRAISE
> > bool "Appraise integrity measurements"
> > depends on IMA
> > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> > index e2a60c3..ebc3554 100644
> > --- a/security/integrity/ima/ima.h
> > +++ b/security/integrity/ima/ima.h
> > @@ -166,6 +166,10 @@ void ima_update_policy(void);
> >  void ima_update_policy_flag(void);
> >  ssize_t ima_parse_add_rule(char *);
> >  void ima_delete_rules(void);
> > +void *ima_policy_start(struct seq_file *m, loff_t *pos);
> > +void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos);
> > +void ima_policy_stop(struct seq_file *m, void *v);
> > +int ima_policy_show(struct seq_file *m, void *v);
> > 
> >  /* Appraise integrity measurements */
> >  #define IMA_APPRAISE_ENFORCE   0x01
> > @@ -263,4 +267,11 @@ static inline int ima_init_keyring(const unsigned int 
> > id)
> > return 0;
> >  }
> >  #endif /* CONFIG_IMA_TRUSTED_KEYRING */
> > -#endif
> > +
> > +#ifdef CONFIG_IMA_READ_POLICY
> > +#definePOLICY_FILE_FLAGS   (S_IWUSR | S_IRUSR)
> > +#else
> > +#definePOLICY_FILE_FLAGS   S_IWUSR
> > +#endif /* CONFIG_IMA_WRITE_POLICY */
> > +
> > +#endif /* __LINUX_IMA_H */
> > diff --git a/security/integrity/ima/ima_fs.c 
> > b/security/integrity/ima/ima_fs.c
> > index a3cf5c0..85bd129 100644
> > --- a/security/integrity/ima/ima_fs.c
> > +++ b/security/integrity/ima/ima_fs.c
> > @@ -311,14 +311,29 @@ enum ima_fs_flags {
> > 
> >  static unsigned long ima_fs_flags;
> > 
> > +#ifdef CONFIG_IMA_READ_POLICY
> > +static const struct seq_operations ima_policy_seqops = {
> > +   .start = ima_policy_start,
> > +   .next = ima_policy_next,
> > +   .stop = ima_policy_stop,
> > +   .show = ima_policy_show,
> > +};
> > +#endif
> > +
> >  /*
> >   * ima_open_policy: sequentialize access to the policy file
> >   */
> >  static int ima_open_policy(struct inode *inode, struct file *filp)
> >  {
> > -   /* No point in being allowed to open it if you aren't going to write */
> > -   if (!(filp->f_flags & O_WRONLY))
> > +   if (!(filp->f_flags & O_WRONLY)) {
> > +#ifndefCONFIG_IMA_READ_POLICY
> > return -EACCES;
> > +#else
> > +   if (!capable(CAP_SYS_ADMIN))
> > +   return -EPERM;
> > +   return seq_open(filp, _policy_seqops);
> > +#endif
> > +   }
> > if (test_and_set_bit(IMA_FS_BUSY, _fs_flags))
> > return -EBUSY;
> > return 0;
> > @@ -334,6 +349,10 @@ static int ima_open_policy(struct inode *inode, struct 
> > file *filp)
> >  static int ima_release_policy(struct inode *inode, struct file *file)
> >  {
> > const char *cause = valid_policy ? "completed" : "failed";
> > +   int policy_write = (file->f_flags & O_WRONLY);
> > +
> > +   if (!policy_write)
> > +   return 0;
> > 
> > pr_info("IMA: policy update %s\n", cause);
> > integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL, NULL,
> > @@ -342,9 +361,9 @@ static int ima_release_policy(struct inode *inode, 
> > struct 

Re: [PATCH v4 3/3] Allows reading back the current IMA policy;

[Mimi Zohar]

On Tue, 2015-10-20 at 15:10 +0300, Petko Manolov wrote:
> On 15-10-20 08:00:29, Mimi Zohar wrote:
> > On Tue, 2015-10-20 at 10:26 +0300, Petko Manolov wrote:
> > > On 15-10-19 14:21:42, Mimi Zohar wrote:
> > > > On Fri, 2015-10-16 at 22:31 +0300, Petko Manolov wrote:
> > > > > When in development it is useful to read back the IMA policy.  This 
> > > > > patch
> > > > > provides the functionality.  However, this is a potential security 
> > > > > hole so
> > > > > it should not be used in production-grade kernels.
> > > >  
> > > > Like the other IMA securityfs files, only root would be able to read it.
> > > > Once we start allowing additional rules to be appended to the policy,
> > > > being able to view the resulting policy is important.  Is there a reason
> > > > for limiting this option to development?
> > > 
> > > I have not considered allowing non-root users to read the policy - i was 
> > > merely 
> > > cleaning up the Zbigniew's patch.  I guess it might be useful to be able 
> > > to read 
> > > the policy when in development mode.
> > 
> > I guess I wasn't clear.  I don't have a problem with the patch itself, just 
> > with the patch description.  What is this "security hole" that the option 
> > should ONLY be configured for development?  Only privileged users can view 
> > the 
> > policy.  I don't see the problem with configuring it in general.  Please 
> > remove the comment.
> 
> By "security hole" i mean being able to read it at all.  Root or non-root.  
> Knowing what the IMA policy is may give the attacker an idea how to 
> circumvent 
> it.  I used stronger words in order to attract the user's attention and 
> consider 
> carefully what the implications are when enabling this option.
> 
> However, i do not insist on keeping this comment.  I will remove it or 
> re-word 
> it if you think it is nonsensical in it's present form.
> 
> BTW, i still think it is a good idea that only the root user have access to 
> the 
> IMA policy.  Unless i hear otherwise i am planning to keep the current 
> functionality.

Exactly!  Because only privileged users (eg. root) have access to
securityfs files, I don't see the security concern.

> > Since responding, I've enabled this feature.  Very nice!
> 
> Have you tried it?

Yes, being able to see the existing policy is nice.

BTW, I haven't compared this patch with the original one yet.  Unless
there were so many changes so that it isn't the same patch anymore, the
patch author should be Zbigniew Jasiński .  Any
changes you made would be listed in the change log.

Mimi

--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 3/3] Allows reading back the current IMA policy;

[Petko Manolov]

On 15-10-20 09:03:19, Mimi Zohar wrote:
> On Tue, 2015-10-20 at 15:10 +0300, Petko Manolov wrote:
> > 
> > By "security hole" i mean being able to read it at all.  Root or non-root.  
> > Knowing what the IMA policy is may give the attacker an idea how to 
> > circumvent it.  I used stronger words in order to attract the user's 
> > attention and consider carefully what the implications are when enabling 
> > this option.
> > 
> > However, i do not insist on keeping this comment.  I will remove it or 
> > re-word it if you think it is nonsensical in it's present form.
> > 
> > BTW, i still think it is a good idea that only the root user have access to 
> > the IMA policy.  Unless i hear otherwise i am planning to keep the current 
> > functionality.
> 
> Exactly!  Because only privileged users (eg. root) have access to securityfs 
> files, I don't see the security concern.

OK, so i'll remove the scary warning. :)

> > > Since responding, I've enabled this feature.  Very nice!
> > 
> > Have you tried it?
> 
> Yes, being able to see the existing policy is nice.
> 
> BTW, I haven't compared this patch with the original one yet.  Unless there 
> were so many changes so that it isn't the same patch anymore, the patch 
> author 
> should be Zbigniew Jasiński .  Any changes you made 
> would be listed in the change log.

The patch is modified significantly, but not to the point to be a new one.  The 
changes mostly address your comments about using raw strings instead of those 
that already exist.  I feel that there's more to be done there, but will wait 
for you to review it first.

Unfortunately i never heard from Zbigniew even though he's copied in this 
thread 
since the beginning.  I would very much like to hear his comments.


Petko
--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 3/3] Allows reading back the current IMA policy;

[Mimi Zohar]

On Fri, 2015-10-16 at 22:31 +0300, Petko Manolov wrote:
> When in development it is useful to read back the IMA policy.  This patch
> provides the functionality.  However, this is a potential security hole so
> it should not be used in production-grade kernels.
 
Like the other IMA securityfs files, only root would be able to read it.
Once we start allowing additional rules to be appended to the policy,
being able to view the resulting policy is important.  Is there a reason
for limiting this option to development?

(Still reviewing the code ...)

Mimi

> Signed-off-by: Petko Manolov 
> ---
>  security/integrity/ima/Kconfig  |  13 +++
>  security/integrity/ima/ima.h|  13 ++-
>  security/integrity/ima/ima_fs.c |  29 +-
>  security/integrity/ima/ima_policy.c | 190 
> 
>  4 files changed, 239 insertions(+), 6 deletions(-)
> 
> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> index 235b3c2..040778f 100644
> --- a/security/integrity/ima/Kconfig
> +++ b/security/integrity/ima/Kconfig
> @@ -121,6 +121,19 @@ config IMA_WRITE_POLICY
> 
> If unsure, say N.
> 
> +config IMA_READ_POLICY
> + bool "Enable reading back the current IMA policy"
> + depends on IMA
> + default n
> + help
> +When developing and debugging an IMA enabled system it is often
> +useful to be able to read the IMA policy.  This patch allows
> +for doing so.  However, being able to read the IMA policy is
> +considered insecure and is strongly discouraged for
> +production-grade kernels.
> +
> +If unsure, say N.
> +
>  config IMA_APPRAISE
>   bool "Appraise integrity measurements"
>   depends on IMA
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index e2a60c3..ebc3554 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -166,6 +166,10 @@ void ima_update_policy(void);
>  void ima_update_policy_flag(void);
>  ssize_t ima_parse_add_rule(char *);
>  void ima_delete_rules(void);
> +void *ima_policy_start(struct seq_file *m, loff_t *pos);
> +void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos);
> +void ima_policy_stop(struct seq_file *m, void *v);
> +int ima_policy_show(struct seq_file *m, void *v);
> 
>  /* Appraise integrity measurements */
>  #define IMA_APPRAISE_ENFORCE 0x01
> @@ -263,4 +267,11 @@ static inline int ima_init_keyring(const unsigned int id)
>   return 0;
>  }
>  #endif /* CONFIG_IMA_TRUSTED_KEYRING */
> -#endif
> +
> +#ifdef   CONFIG_IMA_READ_POLICY
> +#define  POLICY_FILE_FLAGS   (S_IWUSR | S_IRUSR)
> +#else
> +#define  POLICY_FILE_FLAGS   S_IWUSR
> +#endif /* CONFIG_IMA_WRITE_POLICY */
> +
> +#endif /* __LINUX_IMA_H */
> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> index a3cf5c0..85bd129 100644
> --- a/security/integrity/ima/ima_fs.c
> +++ b/security/integrity/ima/ima_fs.c
> @@ -311,14 +311,29 @@ enum ima_fs_flags {
> 
>  static unsigned long ima_fs_flags;
> 
> +#ifdef   CONFIG_IMA_READ_POLICY
> +static const struct seq_operations ima_policy_seqops = {
> + .start = ima_policy_start,
> + .next = ima_policy_next,
> + .stop = ima_policy_stop,
> + .show = ima_policy_show,
> +};
> +#endif
> +
>  /*
>   * ima_open_policy: sequentialize access to the policy file
>   */
>  static int ima_open_policy(struct inode *inode, struct file *filp)
>  {
> - /* No point in being allowed to open it if you aren't going to write */
> - if (!(filp->f_flags & O_WRONLY))
> + if (!(filp->f_flags & O_WRONLY)) {
> +#ifndef  CONFIG_IMA_READ_POLICY
>   return -EACCES;
> +#else
> + if (!capable(CAP_SYS_ADMIN))
> + return -EPERM;
> + return seq_open(filp, _policy_seqops);
> +#endif
> + }
>   if (test_and_set_bit(IMA_FS_BUSY, _fs_flags))
>   return -EBUSY;
>   return 0;
> @@ -334,6 +349,10 @@ static int ima_open_policy(struct inode *inode, struct 
> file *filp)
>  static int ima_release_policy(struct inode *inode, struct file *file)
>  {
>   const char *cause = valid_policy ? "completed" : "failed";
> + int policy_write = (file->f_flags & O_WRONLY);
> +
> + if (!policy_write)
> + return 0;
> 
>   pr_info("IMA: policy update %s\n", cause);
>   integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL, NULL,
> @@ -342,9 +361,9 @@ static int ima_release_policy(struct inode *inode, struct 
> file *file)
>   if (!valid_policy) {
>   ima_delete_rules();
>   valid_policy = 1;
> - clear_bit(IMA_FS_BUSY, _fs_flags);
>   return 0;
>   }
> +
>   ima_update_policy();
>  #ifndef  CONFIG_IMA_WRITE_POLICY
>   securityfs_remove(ima_policy);
> @@ -358,6 +377,7 @@ static int ima_release_policy(struct inode *inode, struct 
> file *file)
>