| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <81c57884-8a57-ae1f-1d85-929c3cb5bd58@linux.microsoft.com>
Date: Thu, 4 Nov 2021 09:52:35 -0700
From: Deven Bowers <deven.desai@...ux.microsoft.com>
To: Roberto Sassu <roberto.sassu@...wei.com>,
"corbet@....net" <corbet@....net>,
"axboe@...nel.dk" <axboe@...nel.dk>,
"agk@...hat.com" <agk@...hat.com>,
"snitzer@...hat.com" <snitzer@...hat.com>,
"ebiggers@...nel.org" <ebiggers@...nel.org>,
"tytso@....edu" <tytso@....edu>,
"paul@...l-moore.com" <paul@...l-moore.com>,
"eparis@...hat.com" <eparis@...hat.com>,
"jmorris@...ei.org" <jmorris@...ei.org>,
"serge@...lyn.com" <serge@...lyn.com>
Cc: "jannh@...gle.com" <jannh@...gle.com>,
"dm-devel@...hat.com" <dm-devel@...hat.com>,
"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
"linux-fscrypt@...r.kernel.org" <linux-fscrypt@...r.kernel.org>,
"linux-audit@...hat.com" <linux-audit@...hat.com>,
"linux-security-module@...r.kernel.org"
<linux-security-module@...r.kernel.org>,
"linux-integrity@...r.kernel.org" <linux-integrity@...r.kernel.org>
Subject: Re: [RFC PATCH v7 14/16] scripts: add boot policy generation program
On 11/3/2021 9:53 AM, Roberto Sassu wrote:
>> From: Roberto Sassu [mailto:roberto.sassu@...wei.com]
>> Sent: Wednesday, November 3, 2021 5:43 PM
>>> From: deven.desai@...ux.microsoft.com
>>> [mailto:deven.desai@...ux.microsoft.com]
>>> From: Deven Bowers <deven.desai@...ux.microsoft.com>
>>>
>>> Enables an IPE policy to be enforced from kernel start, enabling access
>>> control based on trust from kernel startup. This is accomplished by
>>> transforming an IPE policy indicated by CONFIG_IPE_BOOT_POLICY into a
>>> c-string literal that is parsed at kernel startup as an unsigned policy.
>>>
>>> Signed-off-by: Deven Bowers <deven.desai@...ux.microsoft.com>
>>> ---
>>>
>>> Relevant changes since v6:
>>> * Move patch 01/12 to [14/16] of the series
>>>
>>> ---
>>> MAINTAINERS | 1 +
>>> scripts/Makefile | 1 +
>>> scripts/ipe/Makefile | 2 +
>>> scripts/ipe/polgen/.gitignore | 1 +
>>> scripts/ipe/polgen/Makefile | 6 ++
>>> scripts/ipe/polgen/polgen.c | 145 ++++++++++++++++++++++++++++++++++
>>> security/ipe/.gitignore | 1 +
>>> security/ipe/Kconfig | 10 +++
>>> security/ipe/Makefile | 13 +++
>>> security/ipe/ctx.c | 18 +++++
>>> 10 files changed, 198 insertions(+)
>>> create mode 100644 scripts/ipe/Makefile
>>> create mode 100644 scripts/ipe/polgen/.gitignore
>>> create mode 100644 scripts/ipe/polgen/Makefile
>>> create mode 100644 scripts/ipe/polgen/polgen.c
>>> create mode 100644 security/ipe/.gitignore
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index f1e76f791d47..a84ca781199b 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -9283,6 +9283,7 @@ INTEGRITY POLICY ENFORCEMENT (IPE)
>>> M: Deven Bowers <deven.desai@...ux.microsoft.com>
>>> M: Fan Wu <wufan@...ux.microsoft.com>
>>> S: Supported
>>> +F: scripts/ipe/
>>> F: security/ipe/
>>>
>>> INTEL 810/815 FRAMEBUFFER DRIVER
>>> diff --git a/scripts/Makefile b/scripts/Makefile
>>> index 9adb6d247818..a31da6d57a36 100644
>>> --- a/scripts/Makefile
>>> +++ b/scripts/Makefile
>>> @@ -41,6 +41,7 @@ targets += module.lds
>>> subdir-$(CONFIG_GCC_PLUGINS) += gcc-plugins
>>> subdir-$(CONFIG_MODVERSIONS) += genksyms
>>> subdir-$(CONFIG_SECURITY_SELINUX) += selinux
>>> +subdir-$(CONFIG_SECURITY_IPE) += ipe
>>>
>>> # Let clean descend into subdirs
>>> subdir- += basic dtc gdb kconfig mod
>>> diff --git a/scripts/ipe/Makefile b/scripts/ipe/Makefile
>>> new file mode 100644
>>> index 000000000000..e87553fbb8d6
>>> --- /dev/null
>>> +++ b/scripts/ipe/Makefile
>>> @@ -0,0 +1,2 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only
>>> +subdir-y := polgen
>>> diff --git a/scripts/ipe/polgen/.gitignore b/scripts/ipe/polgen/.gitignore
>>> new file mode 100644
>>> index 000000000000..80f32f25d200
>>> --- /dev/null
>>> +++ b/scripts/ipe/polgen/.gitignore
>>> @@ -0,0 +1 @@
>>> +polgen
>>> diff --git a/scripts/ipe/polgen/Makefile b/scripts/ipe/polgen/Makefile
>>> new file mode 100644
>>> index 000000000000..066060c22b4a
>>> --- /dev/null
>>> +++ b/scripts/ipe/polgen/Makefile
>>> @@ -0,0 +1,6 @@
>>> +# SPDX-License-Identifier: GPL-2.0
>>> +hostprogs-always-y := polgen
>>> +HOST_EXTRACFLAGS += \
>>> + -I$(srctree)/include \
>>> + -I$(srctree)/include/uapi \
>>> +
>>> diff --git a/scripts/ipe/polgen/polgen.c b/scripts/ipe/polgen/polgen.c
>>> new file mode 100644
>>> index 000000000000..73cf13e743f7
>>> --- /dev/null
>>> +++ b/scripts/ipe/polgen/polgen.c
>>> @@ -0,0 +1,145 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Copyright (C) Microsoft Corporation. All rights reserved.
>>> + */
>>> +
>>> +#include <stdlib.h>
>>> +#include <stddef.h>
>>> +#include <stdio.h>
>>> +#include <unistd.h>
>>> +#include <errno.h>
>>> +
>>> +static void usage(const char *const name)
>>> +{
>>> + printf("Usage: %s OutputFile (PolicyFile)\n", name);
>>> + exit(EINVAL);
>>> +}
>>> +
>>> +static int policy_to_buffer(const char *pathname, char **buffer, size_t *size)
>>> +{
>>> + int rc = 0;
>>> + FILE *fd;
>>> + char *lbuf;
>>> + size_t fsize;
>>> + size_t read;
>>> +
>>> + fd = fopen(pathname, "r");
>>> + if (!fd) {
>>> + rc = errno;
>>> + goto out;
>>> + }
>>> +
>>> + fseek(fd, 0, SEEK_END);
>>> + fsize = ftell(fd);
>>> + rewind(fd);
>>> +
>>> + lbuf = malloc(fsize);
>>> + if (!lbuf) {
>>> + rc = ENOMEM;
>>> + goto out_close;
>>> + }
>>> +
>>> + read = fread((void *)lbuf, sizeof(*lbuf), fsize, fd);
>>> + if (read != fsize) {
>>> + rc = -1;
>>> + goto out_free;
>>> + }
>>> +
>>> + *buffer = lbuf;
>>> + *size = fsize;
>>> + fclose(fd);
>>> +
>>> + return rc;
>>> +
>>> +out_free:
>>> + free(lbuf);
>>> +out_close:
>>> + fclose(fd);
>>> +out:
>>> + return rc;
>>> +}
>>> +
>>> +static int write_boot_policy(const char *pathname, const char *buf, size_t
>> size)
>>> +{
>>> + int rc = 0;
>>> + FILE *fd;
>>> + size_t i;
>>> +
>>> + fd = fopen(pathname, "w");
>>> + if (!fd) {
>>> + rc = errno;
>>> + goto err;
>>> + }
>>> +
>>> + fprintf(fd, "/* This file is automatically generated.");
>>> + fprintf(fd, " Do not edit. */\n");
>>> + fprintf(fd, "#include <stddef.h>\n");
>>> + fprintf(fd, "\nextern const char *const ipe_boot_policy;\n\n");
>>> + fprintf(fd, "const char *const ipe_boot_policy =\n");
>>> +
>>> + if (!buf || size == 0) {
>>> + fprintf(fd, "\tNULL;\n");
>>> + fclose(fd);
>>> + return 0;
>>> + }
>>> +
>>> + fprintf(fd, "\t\"");
>>> +
>>> + for (i = 0; i < size; ++i) {
>>> + switch (buf[i]) {
>>> + case '"':
>>> + fprintf(fd, "\\\"");
>>> + break;
>>> + case '\'':
>>> + fprintf(fd, "'");
>>> + break;
>>> + case '\n':
>>> + fprintf(fd, "\\n\"\n\t\"");
>>> + break;
>>> + case '\\':
>>> + fprintf(fd, "\\\\");
>>> + break;
>>> + case '\t':
>>> + fprintf(fd, "\\t");
>>> + break;
>>> + case '\?':
>>> + fprintf(fd, "\\?");
>>> + break;
>>> + default:
>>> + fprintf(fd, "%c", buf[i]);
>>> + }
>>> + }
>>> + fprintf(fd, "\";\n");
>>> + fclose(fd);
>>> +
>>> + return 0;
>>> +
>>> +err:
>>> + if (fd)
>>> + fclose(fd);
>>> + return rc;
>>> +}
>>> +
>>> +int main(int argc, const char *const argv[])
>>> +{
>>> + int rc = 0;
>>> + size_t len = 0;
>>> + char *policy = NULL;
>>> +
>>> + if (argc < 2)
>>> + usage(argv[0]);
>>> +
>>> + if (argc > 2) {
>>> + rc = policy_to_buffer(argv[2], &policy, &len);
>>> + if (rc != 0)
>>> + goto cleanup;
>>> + }
>>> +
>>> + rc = write_boot_policy(argv[1], policy, len);
>>> +cleanup:
>>> + if (policy)
>>> + free(policy);
>>> + if (rc != 0)
>>> + perror("An error occurred during policy conversion: ");
>>> + return rc;
>>> +}
>>> diff --git a/security/ipe/.gitignore b/security/ipe/.gitignore
>>> new file mode 100644
>>> index 000000000000..eca22ad5ed22
>>> --- /dev/null
>>> +++ b/security/ipe/.gitignore
>>> @@ -0,0 +1 @@
>>> +boot-policy.c
>>> \ No newline at end of file
>>> diff --git a/security/ipe/Kconfig b/security/ipe/Kconfig
>>> index fcf82a8152ec..39df680b67a2 100644
>>> --- a/security/ipe/Kconfig
>>> +++ b/security/ipe/Kconfig
>>> @@ -20,6 +20,16 @@ menuconfig SECURITY_IPE
>>>
>>> if SECURITY_IPE
>>>
>>> +config IPE_BOOT_POLICY
>>> + string "Integrity policy to apply on system startup"
>>> + help
>>> + This option specifies a filepath to a IPE policy that is compiled
>>> + into the kernel. This policy will be enforced until a policy update
>>> + is deployed via the $securityfs/ipe/policies/$policy_name/active
>>> + interface.
>>> +
>>> + If unsure, leave blank.
>>> +
>>> choice
>>> prompt "Hash algorithm used in auditing policies"
>>> default IPE_AUDIT_HASH_SHA1
>>> diff --git a/security/ipe/Makefile b/security/ipe/Makefile
>>> index 1e7b2d7fcd9e..89fec670f954 100644
>>> --- a/security/ipe/Makefile
>>> +++ b/security/ipe/Makefile
>>> @@ -7,7 +7,18 @@
>>>
>>> ccflags-y := -I$(srctree)/security/ipe/modules
>>>
>>> +quiet_cmd_polgen = IPE_POL $(2)
>>> + cmd_polgen = scripts/ipe/polgen/polgen security/ipe/boot-policy.c $(2)
>>> +
>>> +$(eval $(call config_filename,IPE_BOOT_POLICY))
>>> +
>>> +targets += boot-policy.c
>>> +
>>> +$(obj)/boot-policy.c: scripts/ipe/polgen/polgen
>>> $(IPE_BOOT_POLICY_FILENAME) FORCE
>>> + $(call if_changed,polgen,$(IPE_BOOT_POLICY_FILENAME))
>>> +
>>> obj-$(CONFIG_SECURITY_IPE) += \
>>> + boot-policy.o \
>>> ctx.o \
>>> eval.o \
>>> fs.o \
>>> @@ -21,3 +32,5 @@ obj-$(CONFIG_SECURITY_IPE) += \
>>> policyfs.o \
>>>
>>> obj-$(CONFIG_AUDIT) += audit.o
>>> +
>>> +clean-files := boot-policy.c \
>>> diff --git a/security/ipe/ctx.c b/security/ipe/ctx.c
>>> index fc9b8e467bc9..879acf4ceac5 100644
>>> --- a/security/ipe/ctx.c
>>> +++ b/security/ipe/ctx.c
>>> @@ -15,6 +15,7 @@
>>> #include <linux/spinlock.h>
>>> #include <linux/moduleparam.h>
>>>
>>> +extern const char *const ipe_boot_policy;
>>> static bool success_audit;
>>> static bool enforce = true;
>>>
>>> @@ -329,6 +330,7 @@ void ipe_put_ctx(struct ipe_context *ctx)
>>> int __init ipe_init_ctx(void)
>>> {
>>> int rc = 0;
>>> + struct ipe_policy *p = NULL;
>>> struct ipe_context *lns = NULL;
>>>
>>> lns = create_ctx();
>>> @@ -342,10 +344,26 @@ int __init ipe_init_ctx(void)
>>> WRITE_ONCE(lns->enforce, enforce);
>>> spin_unlock(&lns->lock);
>>>
>>> + if (ipe_boot_policy) {
>>> + p = ipe_new_policy(ipe_boot_policy, strlen(ipe_boot_policy),
>>> + NULL, 0);
>>> + if (IS_ERR(p)) {
>>> + rc = PTR_ERR(lns);
>> This should be:
>>
>> rc = PTR_ERR(p);
>>
>>> + goto err;
>>> + }
>>> +
>>> + ipe_add_policy(lns, p);
>>> + rc = ipe_set_active_pol(p);
>>> + if (!rc)
>> Here you need to set a non-zero value, so that ipe_init()
>> does not enable the LSM.
> Actually you probably should just check that rc is not zero
> and goto err.
>
> Roberto
>
Yeah, I actually noticed these two mistakes myself fairly recently.
Changes will be applied in v8.
>
>> I would set to 1 a new global variable, like ipe_lsm_enabled,
>> in ipe_init() just before security_add_hooks().
>>
>> Then, I would add a check of this variable in ipe_init_securityfs()
>> to avoid the kernel panic.
That's my plan.
-Deven
Powered by blists - more mailing lists