Message-ID: <84e6acad739a415aa3e2457b5c37979f@huawei.com>
Date:   Fri, 8 May 2020 10:20:53 +0000
From:   Roberto Sassu <roberto.sassu@...wei.com>
To:     Mimi Zohar <zohar@...ux.ibm.com>,
        "david.safford@...il.com" <david.safford@...il.com>,
        "viro@...iv.linux.org.uk" <viro@...iv.linux.org.uk>,
        "jmorris@...ei.org" <jmorris@...ei.org>,
        "John Johansen" <john.johansen@...onical.com>
CC:     "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
        "linux-integrity@...r.kernel.org" <linux-integrity@...r.kernel.org>,
        "linux-security-module@...r.kernel.org" 
        <linux-security-module@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Silviu Vlasceanu <Silviu.Vlasceanu@...wei.com>
Subject: RE: [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure

> From: Mimi Zohar [mailto:zohar@...ux.ibm.com]
> On Thu, 2020-05-07 at 16:47 +0000, Roberto Sassu wrote:
> > > > > On Wed, 2020-05-06 at 15:44 -0400, Mimi Zohar wrote:
> > > > > > Since copying the EVM HMAC or original signature isn't applicable, I
> > > > > > would prefer exploring an EVM portable and immutable signature
> only
> > > > > > solution.
> > > > >
> > > > > To prevent copying the EVM xattr, we added "security.evm" to
> > > > > /etc/xattr.conf.  To support copying just the EVM portable and
> > > > > immutable signatures will require a different solution.
> > > >
> > > > This patch set removes the need for ignoring security.evm. It can be
> > > always
> > > > copied, even if it is an HMAC. EVM will update it only when verification
> in
> > > > the pre hook is successful. Combined with the ability of protecting a
> > > subset
> > > > of files without introducing an EVM policy, these advantages seem to
> > > > outweigh the effort necessary to make the switch.
> > >
> > > As the EVM file HMAC and original signature contain inode specific
> > > information (eg. i_version, i_generation), these xattrs cannot ever be
> > > copied.  The proposed change is in order to support just the new EVM
> > > signatures.
> >
> > Right, I didn't consider it.
> >
> > Would it make sense instead to introduce an alias like
> security.evm_immutable
> > so that this xattr can be copied?
> 
> Being portable, not the attribute of being immutable, allows copying
> the EVM xattr.  Your original problem - the order in which the xattrs
> are copied - might still be an issue.  We need to look at "cp" closer
> to understand what it is doing.  For example, are the xattrs written
> while the target file is tagged as a new file?

Yes:

openat(AT_FDCWD, "/bin/cat", O_RDONLY|O_NOFOLLOW) = 3
openat(AT_FDCWD, "cat", O_WRONLY|O_CREAT|O_EXCL, 0700) = 4
read(3, "\177ELF\2\1\1\0\0\0\0\0\0\0\0\0\3\0>\0\1\0\0\0\3601\0\0\0\0\0\0"..., 131072) = 85520
write(4, "\177ELF\2\1\1\0\0\0\0\0\0\0\0\0\3\0>\0\1\0\0\0\3601\0\0\0\0\0\0"..., 85520) = 85520
read(3, "", 131072)                     = 0
flistxattr(3, NULL, 0)                  = 43
flistxattr(3, "security.selinux\0security.evm\0se"..., 43) = 43
fgetxattr(3, "security.evm", NULL, 0)   = 521
fgetxattr(3, "security.evm", "\5\2\4\256\316\302\206\2\09\226L52=\233^n\376:\363+\231\272\213\t\247\312\324\263\222N"..., 521) = 521
fsetxattr(4, "security.evm", "\5\2\4\256\316\302\206\2\09\226L52=\233^n\376:\363+\231\272\213\t\247\312\324\263\222N"..., 521, 0) = 0
fgetxattr(3, "security.ima", NULL, 0)   = 34
fgetxattr(3, "security.ima", "\4\4\323\327\215\202I1~\325\0V\354}\4\3328$\210\363ja'\364\351\26\27\222\331\177\23\341"..., 34) = 34
fsetxattr(4, "security.ima", "\4\4\323\327\215\202I1~\325\0V\354}\4\3328$\210\363ja'\364\351\26\27\222\331\177\23\341"..., 34, 0) = 0
fgetxattr(3, "system.posix_acl_access", 0x7ffdf6929310, 132) = -1 ENODATA (No data available)
fstat(3, {st_mode=S_IFREG|0755, st_size=85520, ...}) = 0
fsetxattr(4, "system.posix_acl_access", "\2\0\0\0\1\0\7\0\377\377\377\377\4\0\5\0\377\377\377\377 \0\5\0\377\377\377\377", 28, 0) = 0
close(4)                                = 0
close(3)                                = 0

This will fail. After security.evm is added to the target, the status when
security.ima is added is INTEGRITY_FAIL.

openat(AT_FDCWD, "test-archive.tar", O_RDONLY) = 3
mknodat(AT_FDCWD, "cat", 0755)          = 0
setxattr("cat", "security.selinux", "system_u:object_r:bin_t:s0", 27, 0) = 0
setxattr("cat", "security.evm", "\5\2\4\256\316\302\206\2\09\226L52=\233^n\376:\363+\231\272\213\t\247\312\324\263\222N"..., 521, 0) = 0
setxattr("cat", "security.ima", "\4\4\323\327\215\202I1~\325\0V\354}\4\3328$\210\363ja'\364\351\26\27\222\331\177\23\341"..., 34, 0) = 0
openat(AT_FDCWD, "cat", O_WRONLY|O_CREAT|O_NOCTTY|O_NONBLOCK|O_CLOEXEC, 0700) = 4
write(4, "\177ELF\2\1\1\0\0\0\0\0\0\0\0\0\3\0>\0\1\0\0\0\3601\0\0\0\0\0\0"..., 8192) = 8192
read(3, "\363\17\36\372H\203\354\10H\213\5\301\217\0\0H\205\300t\2\377\320H\203\304\10\303\0\0\0\0\0"..., 10240) = 10240
write(4, "\363\17\36\372H\203\354\10H\213\5\301\217\0\0H\205\300t\2\377\320H\203\304\10\303\0\0\0\0\0"..., 10240) = 10240
read(3, "\300E\204\311t\16M9\346v\5C\306\4'\\I\203\304\1H\203\303\1H9\313\17\203M\1\0"..., 10240) = 10240
write(4, "\300E\204\311t\16M9\346v\5C\306\4'\\I\203\304\1H\203\303\1H9\313\17\203M\1\0"..., 10240) = 10240
read(3, "\1\0\2\0write error\0cat\0[\0test invoc"..., 10240) = 10240
write(4, "\1\0\2\0write error\0cat\0[\0test invoc"..., 10240) = 10240
read(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 10240) = 10240
write(4, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 10240) = 10240
read(3, "\0\0\0\0\0\1\0\0GA*\6\22\0\0\0\21\0\f\0\21\0\0\0\0\0\0\0\0\1\0\0"..., 10240) = 10240
write(4, "\0\0\0\0\0\1\0\0GA*\6\22\0\0\0\21\0\f\0\21\0\0\0\0\0\0\0\0\1\0\0"..., 10240) = 10240
read(3, "\0\0\0\0\27\0\0\0\0\0\0\0\0\1\0\0GA$\5gcc 9.0.1 20"..., 10240) = 10240
write(4, "\0\0\0\0\27\0\0\0\0\0\0\0\0\1\0\0GA$\5gcc 9.0.1 20"..., 10240) = 10240
read(3, "\0\0\0\0\0\1\0\0GA+stack_clash\0\0\23\0\0\0\0\0\0\0"..., 10240) = 10240
write(4, "\0\0\0\0\0\1\0\0GA+stack_clash\0\0\23\0\0\0\0\0\0\0"..., 10240) = 10240
read(3, "\0\0\0\0\0\1\0\0GA+GLIBCXX_ASSERTIONS\0\0\0"..., 10240) = 10240
write(4, "\0\0\0\0\0\1\0\0GA+GLIBCXX_ASSERTIONS\0\0\0"..., 5648) = 5648
fchown(4, 0, 0)                         = 0
fchmod(4, 0755)                         = 0
close(4)                                = 0
close(3)                                = 0

Also this will fail, but for a different reason. After security.selinux is
written, the status when security.evm is added is INTEGRITY_NOLABEL
(unless the HMAC key is loaded, otherwise it will fail when tar sets
security.ima).

Even with my patches to ignore verification errors, there is still another
problem. The signature becomes valid when all xattrs are set, but tar
tries anyway to adjust the owner even if it is already correct. Since
metadata are immutable, fchown() fails (this status is not ignored).

After we solve the problem of copying xattrs, I will send a patch to
determine whether setxattr()/setattr() change metadata. If not, the
operation is allowed even if metadata are immutable.

> There have been similar problems in the past.  For example, tar calls
> mknodat to create the file, but doesn't write the file data.  The
> solution there was to tag the file as a new file.
> 
> We need to understand the problem better, before deciding how to
> resolve it.
> 
> >
> > > At least IMA file hashes should always be used in conjunction with
> > > EVM.  EVM xattrs should always require a security.ima xattr to bind
> >
> > I proposed to enforce this restriction some time ago:
> >
> > https://patchwork.kernel.org/patch/10979351/
> >
> > Is it ok to enforce it globally?
> 
> Doing this would then be dependent on upstreaming the initramfs xattr
> patches first, wouldn't it?  :)

Well, this patch set and the metadata change detection are a dependency
of the initramfs xattr patch set. Without them, we cannot create a ram disk
with files with portable signatures, if enforcement is enabled.

The new appraisal modes solve a general security issue that would occur
also when appraisal is done in the root filesystem (it is possible to gain
access to all files for which the IMA policy does not require a signature
by removing the EVM keys and adding to those files a valid security.ima
with the file digest).

> > > the file metadata to the file data.  The IMA and EVM policies really
> > > need to be in sync.
> >
> > It would be nice, but at the moment EVM considers also files that are
> > not selected by the IMA policy. An example of why this is a problem is
> > the audit service that fails to start when it tries to adjust the permissions
> > of the log files. Those files don't have security.evm because they are
> > not appraised by IMA, but EVM denies the operation.
> 
> No, this is a timing issue as to whether or not the builtin policy or
> a custom policy has been loaded.  A custom policy could exclude the
> log files based on LSM labels, but they are included in the builtin
> policy.

Yes, I was referring to a custom policy. In this case, EVM will not adapt
to the custom policy but still verifies all files. If access control is done
exclusively by IMA at the time evm_verifyxattr() is called, we wouldn't
need to add security.evm to all files.

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

Powered by blists - more mailing lists