mbox series

[v2,0/3] LTP reproducer on broken IMA on overlayfs

Message ID 20190405165225.27216-1-pvorel@suse.cz (mailing list archive)
Headers show
Series LTP reproducer on broken IMA on overlayfs | expand

Message

Petr Vorel April 5, 2019, 4:52 p.m. UTC
Hi,

this is a second version of patch demonstrating a bug on overlayfs when
combining IMA with EVM. There is ongoing work made by Ignaz Forster and
Fabian Vogt [1] [2], IMA only behavior was already fixed [3].

Main patch is the last one (previous are just a cleanup and not changed).

Kind regards,
Petr

[1] https://www.spinics.net/lists/linux-integrity/msg05926.html
[2] https://www.spinics.net/lists/linux-integrity/msg03593.html
[3] https://patchwork.kernel.org/patch/10776231/

Petr Vorel (3):
  ima: Call test's cleanup inside ima_setup.sh cleanup
  shell: Add $TST_DEVICE as default parameter to tst_umount
  ima: Add overlay test

 doc/test-writing-guidelines.txt               |  4 +-
 runtest/ima                                   |  1 +
 testcases/commands/df/df01.sh                 |  7 +-
 testcases/commands/mkfs/mkfs01.sh             |  2 +-
 .../integrity/ima/tests/evm_overlay.sh        | 86 +++++++++++++++++++
 .../security/integrity/ima/tests/ima_setup.sh | 12 ++-
 .../integrity/ima/tests/ima_violations.sh     |  2 -
 testcases/lib/tst_test.sh                     |  2 +-
 8 files changed, 100 insertions(+), 16 deletions(-)
 create mode 100755 testcases/kernel/security/integrity/ima/tests/evm_overlay.sh

Comments

Petr Vorel May 14, 2019, 12:12 p.m. UTC | #1
Hi Mimi, Ignaz,

Mimi, could you please have a second look on this [4] patchset? We've had a
discussion about second patch [5], I can drop it if you don't like it, but
that's not a main concern about this test. More important is whether the
testcase looks valid for you. It's about overlayfs broken in IMA+EVM,
which is currently broken on mainline.
There is different reproducer (C code) for a slightly different scenario,
but I'm not going to port it before this got merged.

Ignaz, could you please test this patchset? Could you, please, share your setup?
ima_policy=appraise_tcb kernel parameter and loading IMA and EVM keys over
dracut-ima scripts? (IMA appraisal and EVM using digital signatures? I guess
using hashes for IMA appraisal would work as well).

Kind regards,
Petr

> this is a second version of patch demonstrating a bug on overlayfs when
> combining IMA with EVM. There is ongoing work made by Ignaz Forster and
> Fabian Vogt [1] [2], IMA only behavior was already fixed [3].

> Main patch is the last one (previous are just a cleanup and not changed).

> [1] https://www.spinics.net/lists/linux-integrity/msg05926.html
> [2] https://www.spinics.net/lists/linux-integrity/msg03593.html
> [3] https://patchwork.kernel.org/patch/10776231/

[4] https://patchwork.ozlabs.org/project/ltp/list/?series=101213&state=*
[5] https://patchwork.ozlabs.org/patch/1078553/
Ignaz Forster May 14, 2019, 7:19 p.m. UTC | #2
Hi Petr,

Am 14.05.19 um 14:12 Uhr schrieb Petr Vorel:
> Could you, please, share your setup?

The system was installed with IMA and EVM enabled during installation, 
using the following kernel parameters:
"ima_policy=appraise_tcb ima_appraise=fix evm=fix"

The EVM key was generated in the live system before starting the actual 
installation and copied into the installed system later.

See the attached installation notes for an openSUSE system (which should 
also be usable on other distributions).

> ima_policy=appraise_tcb kernel parameter and loading IMA and EVM keys over
> dracut-ima scripts?

Exactly.

> (IMA appraisal and EVM using digital signatures? I guess
> using hashes for IMA appraisal would work as well).

I focused on hashes, as those are more relevant for the overlayfs use 
case I was thinking of.

Ignaz
Manual IMA / EVM installation:
* Use a net install image (some of the necessary packages are not available in DVD image)
* Boot install system with "ima_policy=appraise_tcb ima_appraise=fix evm=fix" (for IMA measurement, IMA appraisal and EVM protection)
* Proceed with installation until summary screen, but do not start the installation yet
* Remove "evm=fix" from kernel boot parameters
* Change kernel boot parameter "ima_appraise=fix" to "ima_appraise=appraise_tcb"
* Select package "dracut-ima" (required for early boot EVM support) for installation
* Change to a console window
* mkdir /etc/keys
* /bin/keyctl add user kmk-user "`dd if=/dev/urandom bs=1 count=32 2>/dev/null`" @u
* /bin/keyctl pipe `/bin/keyctl search @u user kmk-user` > /etc/keys/kmk-user.blob
* /bin/keyctl add encrypted evm-key "new user:kmk-user 64" @u
* /bin/keyctl pipe `/bin/keyctl search @u encrypted evm-key` >/etc/keys/evm.blob
* cat <<END >/etc/sysconfig/masterkey
MASTERKEYTYPE="user"
MASTERKEY="/etc/keys/kmk-user.blob"
END
* cat <<END >/etc/sysconfig/evm
EVMKEY="/etc/keys/evm.blob"
END
* mount -t securityfs security /sys/kernel/security
* echo 1 >/sys/kernel/security/evm
* Go back to the installation summary screen and start the installation
* During the installation execute the following commands from the console:
* cp -r /etc/keys /mnt/etc/
* cp /etc/sysconfig/{evm,masterkey} /mnt/etc/sysconfig/
Mimi Zohar May 15, 2019, 3:01 a.m. UTC | #3
On Tue, 2019-05-14 at 14:12 +0200, Petr Vorel wrote:
> Hi Mimi, Ignaz,
> 
> Mimi, could you please have a second look on this [4] patchset? We've had a
> discussion about second patch [5], I can drop it if you don't like it, but
> that's not a main concern about this test. More important is whether the
> testcase looks valid for you. It's about overlayfs broken in IMA+EVM,
> which is currently broken on mainline.

The first two patches are fine.  From the test, I'm seeing the
following results:

evm_overlay 1 TINFO: overwrite file in overlay
tst_rod: Failed to open '(null)' for writing: Operation not permitted
evm_overlay 1 TFAIL: echo overlay > mntpoint/merged/foo1.txt failed unexpectedly
evm_overlay 2 TINFO: append file in overlay: mntpoint/lower/foo2.txt
evm_overlay 2 TPASS: echo overlay >> mntpoint/merged/foo2.txt passed as expected
evm_overlay 3 TINFO: create a new file in overlay
evm_overlay 3 TPASS: echo overlay > mntpoint/merged/foo3.txt passed as expected
evm_overlay 4 TINFO: read all created files
evm_overlay 4 TFAIL: cat mntpoint/merged/foo1.txt > /dev/null 2> /dev/null failed unexpectedly
evm_overlay 4 TFAIL: cat mntpoint/merged/foo2.txt > /dev/null 2> /dev/null failed unexpectedly
evm_overlay 4 TFAIL: cat mntpoint/merged/foo3.txt > /dev/null 2> /dev/null failed unexpectedly
evm_overlay 5 TINFO: SELinux enabled in enforcing mode, this may affect test results
evm_overlay 5 TINFO: You can try to disable it with TST_DISABLE_SELINUX=1 (requires super/root)
evm_overlay 5 TINFO: loaded SELinux profiles: none

With "evm: instead of using the overlayfs i_ino, use the real i_ino"
patch, I'm only seeing the first failure.

Mimi


> There is different reproducer (C code) for a slightly different scenario,
> but I'm not going to port it before this got merged.
> 
> Ignaz, could you please test this patchset? Could you, please, share your setup?
> ima_policy=appraise_tcb kernel parameter and loading IMA and EVM keys over
> dracut-ima scripts? (IMA appraisal and EVM using digital signatures? I guess
> using hashes for IMA appraisal would work as well).
> 
> Kind regards,
> Petr
> 
> > this is a second version of patch demonstrating a bug on overlayfs when
> > combining IMA with EVM. There is ongoing work made by Ignaz Forster and
> > Fabian Vogt [1] [2], IMA only behavior was already fixed [3].
> 
> > Main patch is the last one (previous are just a cleanup and not changed).
> 
> > [1] https://www.spinics.net/lists/linux-integrity/msg05926.html
> > [2] https://www.spinics.net/lists/linux-integrity/msg03593.html
> > [3] https://patchwork.kernel.org/patch/10776231/
> 
> [4] https://patchwork.ozlabs.org/project/ltp/list/?series=101213&state=*
> [5] https://patchwork.ozlabs.org/patch/1078553/
>
Petr Vorel May 15, 2019, 11:34 a.m. UTC | #4
Hi Ignaz,

Thanks a lot for a complete howto!

Kind regards,
Petr
Petr Vorel May 15, 2019, 12:08 p.m. UTC | #5
Hi Mimi,

> The first two patches are fine.  From the test, I'm seeing the
> following results:
Thanks a lot for reviewing and testing.

> evm_overlay 1 TINFO: overwrite file in overlay
> tst_rod: Failed to open '(null)' for writing: Operation not permitted
> evm_overlay 1 TFAIL: echo overlay > mntpoint/merged/foo1.txt failed unexpectedly
I've fixed '(null)' [1], with that one applied it should be 'mntpoint/merged/foo1.txt'
But what is strange to me is that it continues to execute second line. return 1 [2]
should cause ROD() to quit with TBROK [3].
Maybe that ROD in test1() should be replaced EXPECT_PASS.

> evm_overlay 2 TINFO: append file in overlay: mntpoint/lower/foo2.txt
> evm_overlay 2 TPASS: echo overlay >> mntpoint/merged/foo2.txt passed as expected
> evm_overlay 3 TINFO: create a new file in overlay
> evm_overlay 3 TPASS: echo overlay > mntpoint/merged/foo3.txt passed as expected
> evm_overlay 4 TINFO: read all created files
> evm_overlay 4 TFAIL: cat mntpoint/merged/foo1.txt > /dev/null 2> /dev/null failed unexpectedly
> evm_overlay 4 TFAIL: cat mntpoint/merged/foo2.txt > /dev/null 2> /dev/null failed unexpectedly
> evm_overlay 4 TFAIL: cat mntpoint/merged/foo3.txt > /dev/null 2> /dev/null failed unexpectedly
> evm_overlay 5 TINFO: SELinux enabled in enforcing mode, this may affect test results
> evm_overlay 5 TINFO: You can try to disable it with TST_DISABLE_SELINUX=1 (requires super/root)
> evm_overlay 5 TINFO: loaded SELinux profiles: none

> With "evm: instead of using the overlayfs i_ino, use the real i_ino"
> patch, I'm only seeing the first failure.

> Mimi

Kind regards,
Petr

[1] https://github.com/linux-test-project/ltp/commit/8a35daf6bb175391fd43cd28d9ca2d0d5b06157c
[2] https://github.com/linux-test-project/ltp/blob/master/testcases/lib/tst_rod.c#L117
[3] https://github.com/linux-test-project/ltp/blob/master/testcases/lib/tst_test.sh#L150
Mimi Zohar May 16, 2019, 10:10 p.m. UTC | #6
Hi Petr,

On Wed, 2019-05-15 at 14:08 +0200, Petr Vorel wrote:
> > evm_overlay 1 TINFO: overwrite file in overlay
> > tst_rod: Failed to open '(null)' for writing: Operation not permitted
> > evm_overlay 1 TFAIL: echo overlay > mntpoint/merged/foo1.txt failed unexpectedly

> I've fixed '(null)' [1], with that one applied it should be 'mntpoint/merged/foo1.txt'

Thanks!

> But what is strange to me is that it continues to execute second line. return 1 [2]
> should cause ROD() to quit with TBROK [3].
> Maybe that ROD in test1() should be replaced EXPECT_PASS.

With just the first patch of Ignaz's path set [1] and the TPM 2.0 test
[2], there aren't any errors.  Without [1], it's now failing with the
correct name.  I'm now seeing:

evm_overlay 1 TINFO: $TMPDIR is on tmpfs => run on loop device
evm_overlay 1 TINFO: Formatting /dev/loop0 with ext3 extra opts=''
evm_overlay 1 TINFO: overwrite file in overlay
tst_rod: Failed to open 'mntpoint/merged/foo1.txt' for writing: Permission denied
evm_overlay 1 TFAIL: echo overlay > mntpoint/merged/foo1.txt failed unexpectedly

Mimi

[1] https://www.spinics.net/lists/linux-integrity/msg05926.html
[2] https://lore.kernel.org/linux-integrity/1558041162.3971.2.camel@linux.ibm.com/T/#u
Petr Vorel May 17, 2019, 7:50 a.m. UTC | #7
Hi Mimi,

> > But what is strange to me is that it continues to execute second line. return 1 [2]
> > should cause ROD() to quit with TBROK [3].
> > Maybe that ROD in test1() should be replaced EXPECT_PASS.

> With just the first patch of Ignaz's path set [1] and the TPM 2.0 test
> [2], there aren't any errors.  Without [1], it's now failing with the
> correct name.  I'm now seeing:
I guess, that justifies [1] to be merged into kernel.

> evm_overlay 1 TINFO: $TMPDIR is on tmpfs => run on loop device
> evm_overlay 1 TINFO: Formatting /dev/loop0 with ext3 extra opts=''
> evm_overlay 1 TINFO: overwrite file in overlay
> tst_rod: Failed to open 'mntpoint/merged/foo1.txt' for writing: Permission denied
> evm_overlay 1 TFAIL: echo overlay > mntpoint/merged/foo1.txt failed unexpectedly
That still does not explain, why test doesn't exit before this last line.
I'll have a closer look into it. But as I wrote, I'll make these changes:

diff --git testcases/kernel/security/integrity/ima/tests/evm_overlay.sh testcases/kernel/security/integrity/ima/tests/evm_overlay.sh
index 08ec1ea37..1d05b9e1c 100755
--- testcases/kernel/security/integrity/ima/tests/evm_overlay.sh
+++ testcases/kernel/security/integrity/ima/tests/evm_overlay.sh
@@ -40,7 +40,7 @@ test1()
 	local file="foo1.txt"
 
 	tst_res TINFO "overwrite file in overlay"
-	ROD echo lower \> $lower/$file
+	EXPECT_PASS echo lower \> $lower/$file
 	EXPECT_PASS echo overlay \> $merged/$file
 }
 
@@ -49,7 +49,7 @@ test2()
 	local file="foo2.txt"
 
 	tst_res TINFO "append file in overlay"
-	ROD echo lower \> $lower/$file
+	EXPECT_PASS echo lower \> $lower/$file
 	EXPECT_PASS echo overlay \>\> $merged/$file
 }
 
---
If it's ok for you and it's a valid test do you give an ack?

Kind regards,
Petr

> Mimi

> [1] https://www.spinics.net/lists/linux-integrity/msg05926.html
> [2] https://lore.kernel.org/linux-integrity/1558041162.3971.2.camel@linux.ibm.com/T/#u
Mimi Zohar May 17, 2019, 11 a.m. UTC | #8
On Fri, 2019-05-17 at 09:50 +0200, Petr Vorel wrote:
> 
> If it's ok for you and it's a valid test do you give an ack?

Of course!  Thanks!

Mimi
Petr Vorel May 17, 2019, 3:41 p.m. UTC | #9
Hi Mimi,

> On Fri, 2019-05-17 at 09:50 +0200, Petr Vorel wrote:

> > If it's ok for you and it's a valid test do you give an ack?

> Of course!  Thanks!
Thanks! I'll add also Ignaz's description (create README.md in IMA folder),
thus probably send a v3 to ML first, but not expecting to get much review :).

> Mimi

Kind regards,
Petr