diff mbox series

[RFC,v1,4/7] integrity: Fix inode numbers in audit records

Message ID 20241010152649.849254-4-mic@digikod.net (mailing list archive)
State New
Headers show
Series [RFC,v1,1/7] fs: Add inode_get_ino() and implement get_ino() for NFS | expand

Commit Message

Mickaël Salaün Oct. 10, 2024, 3:26 p.m. UTC
Use the new inode_get_ino() helper to log the user space's view of
inode's numbers instead of the private kernel values.

Cc: Mimi Zohar <zohar@linux.ibm.com>
Cc: Roberto Sassu <roberto.sassu@huawei.com>
Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
Cc: Eric Snowberg <eric.snowberg@oracle.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
 security/integrity/integrity_audit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Paul Moore Oct. 11, 2024, 1:20 a.m. UTC | #1
On Oct 10, 2024 =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= <mic@digikod.net> wrote:
> 
> Use the new inode_get_ino() helper to log the user space's view of
> inode's numbers instead of the private kernel values.
> 
> Cc: Mimi Zohar <zohar@linux.ibm.com>
> Cc: Roberto Sassu <roberto.sassu@huawei.com>
> Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
> Cc: Eric Snowberg <eric.snowberg@oracle.com>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> ---
>  security/integrity/integrity_audit.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Should we also need to update the inode value used in hmac_add_misc()?

diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index 7c06ffd633d2..68ae454e187f 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -155,7 +155,7 @@ static void hmac_add_misc(struct shash_desc *desc, struct inode *inode,
         * signatures
         */
        if (type != EVM_XATTR_PORTABLE_DIGSIG) {
-               hmac_misc.ino = inode->i_ino;
+               hmac_misc.ino = inode_get_ino(inode->i_ino);
                hmac_misc.generation = inode->i_generation;
        }
        /* The hmac uid and gid must be encoded in the initial user

--
paul-moore.com
Mickaël Salaün Oct. 11, 2024, 10:15 a.m. UTC | #2
On Thu, Oct 10, 2024 at 09:20:52PM -0400, Paul Moore wrote:
> On Oct 10, 2024 =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= <mic@digikod.net> wrote:
> > 
> > Use the new inode_get_ino() helper to log the user space's view of
> > inode's numbers instead of the private kernel values.
> > 
> > Cc: Mimi Zohar <zohar@linux.ibm.com>
> > Cc: Roberto Sassu <roberto.sassu@huawei.com>
> > Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
> > Cc: Eric Snowberg <eric.snowberg@oracle.com>
> > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > ---
> >  security/integrity/integrity_audit.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Should we also need to update the inode value used in hmac_add_misc()?

I'm not sure what the impact will be wrt backward compatibility. Mimi,
Roberto?

> 
> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> index 7c06ffd633d2..68ae454e187f 100644
> --- a/security/integrity/evm/evm_crypto.c
> +++ b/security/integrity/evm/evm_crypto.c
> @@ -155,7 +155,7 @@ static void hmac_add_misc(struct shash_desc *desc, struct inode *inode,
>          * signatures
>          */
>         if (type != EVM_XATTR_PORTABLE_DIGSIG) {
> -               hmac_misc.ino = inode->i_ino;
> +               hmac_misc.ino = inode_get_ino(inode->i_ino);
>                 hmac_misc.generation = inode->i_generation;
>         }
>         /* The hmac uid and gid must be encoded in the initial user
> 
> --
> paul-moore.com
Roberto Sassu Oct. 11, 2024, 11:34 a.m. UTC | #3
On Fri, 2024-10-11 at 12:15 +0200, Mickaël Salaün wrote:
> On Thu, Oct 10, 2024 at 09:20:52PM -0400, Paul Moore wrote:
> > On Oct 10, 2024 =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= <mic@digikod.net> wrote:
> > > 
> > > Use the new inode_get_ino() helper to log the user space's view of
> > > inode's numbers instead of the private kernel values.
> > > 
> > > Cc: Mimi Zohar <zohar@linux.ibm.com>
> > > Cc: Roberto Sassu <roberto.sassu@huawei.com>
> > > Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
> > > Cc: Eric Snowberg <eric.snowberg@oracle.com>
> > > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > > ---
> > >  security/integrity/integrity_audit.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > Should we also need to update the inode value used in hmac_add_misc()?
> 
> I'm not sure what the impact will be wrt backward compatibility. Mimi,
> Roberto?

Changing the inode number the HMAC was calculated with has the
potential effect of making the file inaccessible.

In order to use the new inode number, we need to define a new EVM xattr
type, and update the previous xattr version with the new one. We could
deprecate the old xattr version after a while (to be discussed with
Mimi).

Roberto

> > 
> > diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> > index 7c06ffd633d2..68ae454e187f 100644
> > --- a/security/integrity/evm/evm_crypto.c
> > +++ b/security/integrity/evm/evm_crypto.c
> > @@ -155,7 +155,7 @@ static void hmac_add_misc(struct shash_desc *desc, struct inode *inode,
> >          * signatures
> >          */
> >         if (type != EVM_XATTR_PORTABLE_DIGSIG) {
> > -               hmac_misc.ino = inode->i_ino;
> > +               hmac_misc.ino = inode_get_ino(inode->i_ino);
> >                 hmac_misc.generation = inode->i_generation;
> >         }
> >         /* The hmac uid and gid must be encoded in the initial user
> > 
> > --
> > paul-moore.com
Mickaël Salaün Oct. 11, 2024, 12:38 p.m. UTC | #4
On Fri, Oct 11, 2024 at 01:34:39PM +0200, Roberto Sassu wrote:
> On Fri, 2024-10-11 at 12:15 +0200, Mickaël Salaün wrote:
> > On Thu, Oct 10, 2024 at 09:20:52PM -0400, Paul Moore wrote:
> > > On Oct 10, 2024 =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= <mic@digikod.net> wrote:
> > > > 
> > > > Use the new inode_get_ino() helper to log the user space's view of
> > > > inode's numbers instead of the private kernel values.
> > > > 
> > > > Cc: Mimi Zohar <zohar@linux.ibm.com>
> > > > Cc: Roberto Sassu <roberto.sassu@huawei.com>
> > > > Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
> > > > Cc: Eric Snowberg <eric.snowberg@oracle.com>
> > > > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > > > ---
> > > >  security/integrity/integrity_audit.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > Should we also need to update the inode value used in hmac_add_misc()?
> > 
> > I'm not sure what the impact will be wrt backward compatibility. Mimi,
> > Roberto?
> 
> Changing the inode number the HMAC was calculated with has the
> potential effect of making the file inaccessible.
> 
> In order to use the new inode number, we need to define a new EVM xattr
> type, and update the previous xattr version with the new one. We could
> deprecate the old xattr version after a while (to be discussed with
> Mimi).

That was my though.  I don't we should patch hmac_add_misc() because it
is already in the IMA/EVM ABI and not directly reflected to user space.
The issue might be that user space cannot recreate this hmac because
this private inode number is not known to user space, but I don't know
if there is such user space implementation of IMA/EVM.

> 
> Roberto
> 
> > > 
> > > diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> > > index 7c06ffd633d2..68ae454e187f 100644
> > > --- a/security/integrity/evm/evm_crypto.c
> > > +++ b/security/integrity/evm/evm_crypto.c
> > > @@ -155,7 +155,7 @@ static void hmac_add_misc(struct shash_desc *desc, struct inode *inode,
> > >          * signatures
> > >          */
> > >         if (type != EVM_XATTR_PORTABLE_DIGSIG) {
> > > -               hmac_misc.ino = inode->i_ino;
> > > +               hmac_misc.ino = inode_get_ino(inode->i_ino);
> > >                 hmac_misc.generation = inode->i_generation;
> > >         }
> > >         /* The hmac uid and gid must be encoded in the initial user
> > > 
> > > --
> > > paul-moore.com
> 
>
Roberto Sassu Oct. 11, 2024, 12:45 p.m. UTC | #5
On Fri, 2024-10-11 at 14:38 +0200, Mickaël Salaün wrote:
> On Fri, Oct 11, 2024 at 01:34:39PM +0200, Roberto Sassu wrote:
> > On Fri, 2024-10-11 at 12:15 +0200, Mickaël Salaün wrote:
> > > On Thu, Oct 10, 2024 at 09:20:52PM -0400, Paul Moore wrote:
> > > > On Oct 10, 2024 =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= <mic@digikod.net> wrote:
> > > > > 
> > > > > Use the new inode_get_ino() helper to log the user space's view of
> > > > > inode's numbers instead of the private kernel values.
> > > > > 
> > > > > Cc: Mimi Zohar <zohar@linux.ibm.com>
> > > > > Cc: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
> > > > > Cc: Eric Snowberg <eric.snowberg@oracle.com>
> > > > > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > > > > ---
> > > > >  security/integrity/integrity_audit.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > Should we also need to update the inode value used in hmac_add_misc()?
> > > 
> > > I'm not sure what the impact will be wrt backward compatibility. Mimi,
> > > Roberto?
> > 
> > Changing the inode number the HMAC was calculated with has the
> > potential effect of making the file inaccessible.
> > 
> > In order to use the new inode number, we need to define a new EVM xattr
> > type, and update the previous xattr version with the new one. We could
> > deprecate the old xattr version after a while (to be discussed with
> > Mimi).
> 
> That was my though.  I don't we should patch hmac_add_misc() because it
> is already in the IMA/EVM ABI and not directly reflected to user space.
> The issue might be that user space cannot recreate this hmac because
> this private inode number is not known to user space, but I don't know
> if there is such user space implementation of IMA/EVM.

EVM will recalculate the HMAC of the file metadata based on the new
inode number, and will conclude that metadata was corrupted (same as if
someone modified a protected xattr during an offline attack).

Roberto

> > 
> > Roberto
> > 
> > > > 
> > > > diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> > > > index 7c06ffd633d2..68ae454e187f 100644
> > > > --- a/security/integrity/evm/evm_crypto.c
> > > > +++ b/security/integrity/evm/evm_crypto.c
> > > > @@ -155,7 +155,7 @@ static void hmac_add_misc(struct shash_desc *desc, struct inode *inode,
> > > >          * signatures
> > > >          */
> > > >         if (type != EVM_XATTR_PORTABLE_DIGSIG) {
> > > > -               hmac_misc.ino = inode->i_ino;
> > > > +               hmac_misc.ino = inode_get_ino(inode->i_ino);
> > > >                 hmac_misc.generation = inode->i_generation;
> > > >         }
> > > >         /* The hmac uid and gid must be encoded in the initial user
> > > > 
> > > > --
> > > > paul-moore.com
> > 
> >
diff mbox series

Patch

diff --git a/security/integrity/integrity_audit.c b/security/integrity/integrity_audit.c
index 0ec5e4c22cb2..e344d5bcf99c 100644
--- a/security/integrity/integrity_audit.c
+++ b/security/integrity/integrity_audit.c
@@ -62,7 +62,7 @@  void integrity_audit_message(int audit_msgno, struct inode *inode,
 	if (inode) {
 		audit_log_format(ab, " dev=");
 		audit_log_untrustedstring(ab, inode->i_sb->s_id);
-		audit_log_format(ab, " ino=%lu", inode->i_ino);
+		audit_log_format(ab, " ino=%llu", inode_get_ino(inode));
 	}
 	audit_log_format(ab, " res=%d errno=%d", !result, errno);
 	audit_log_end(ab);