| Message ID | 20211130160654.1418231-9-stefanb@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ima: Namespace IMA with audit support in IMA-ns | expand |
On Tue, 2021-11-30 at 11:06 -0500, Stefan Berger wrote: > Move measurement list related variables into the ima_namespace. This > way a > front-end like SecurityFS can show the measurement list inside an IMA > namespace. > > Implement ima_free_measurements() to free a list of measurements > and call it when an IMA namespace is deleted. This one worries me quite a lot. What seems to be happening in this code: > @@ -107,7 +100,7 @@ static int ima_add_digest_entry(struct > ima_namespace *ns, > qe->entry = entry; > > INIT_LIST_HEAD(&qe->later); > - list_add_tail_rcu(&qe->later, &ima_measurements); > + list_add_tail_rcu(&qe->later, &ns->ima_measurements); > > atomic_long_inc(&ns->ima_htable.len); > if (update_htable) { > is that we now only add the measurements to the namespace list, but that list is freed when the namespace dies. However, the measurement is still extended through the PCRs meaning we have incomplete information for a replay after the namespace dies? I tend to think the way this should work is that until we have a way of attesting inside the namespace, all measurements should go into the physical log, so that replay is always complete for the PCRs, so effectively the visible log of the namespace would always have to be a subset of the physical log. James
On 12/2/21 07:46, James Bottomley wrote: > On Tue, 2021-11-30 at 11:06 -0500, Stefan Berger wrote: >> Move measurement list related variables into the ima_namespace. This >> way a >> front-end like SecurityFS can show the measurement list inside an IMA >> namespace. >> >> Implement ima_free_measurements() to free a list of measurements >> and call it when an IMA namespace is deleted. > This one worries me quite a lot. What seems to be happening in this > code: > >> @@ -107,7 +100,7 @@ static int ima_add_digest_entry(struct >> ima_namespace *ns, >> qe->entry = entry; >> >> INIT_LIST_HEAD(&qe->later); >> - list_add_tail_rcu(&qe->later, &ima_measurements); >> + list_add_tail_rcu(&qe->later, &ns->ima_measurements); >> >> atomic_long_inc(&ns->ima_htable.len); >> if (update_htable) { >> > is that we now only add the measurements to the namespace list, but > that list is freed when the namespace dies. However, the measurement > is still extended through the PCRs meaning we have incomplete > information for a replay after the namespace dies? *Not at all.* The measurement list of the namespace is independent of the host. The cover letter states: "The following lines added to a suitable IMA policy on the host would cause the execution of the commands inside the container (by uid 1000) to be measured and audited as well on the host, thus leading to two auditing messages for the 'busybox cat' above and log entries in IMA's system log. echo -e "measure func=BPRM_CHECK mask=MAY_EXEC uid=1000\n" \ "audit func=BPRM_CHECK mask=MAY_EXEC uid=1000\n" \ > /sys/kernel/security/ima/policy " So even now, with only auditing support in the namespace, you would get measurements in the host log with an appropriately written IMA policy. The measurements in the host log won't go away when the namespace dies. The intention is to provide flexibility that allows for writing the IMA policy of the host in such a way - that file accesses occurring in namespaces get measured on the host - that file accesses occurring in the namespace do NOT get measured on the host and protect the host log from ever growing or actions in namespaces intentionally growing the host log There would be a namespace policy that would allow for logging inside the namespace. Combine this with the policy on the host and you can have no measurements of the namespace file access, measurements in either the host log or the namespace log or both. What I would be worried about is if the flexibility wasn't there. > > I tend to think the way this should work is that until we have a way of > attesting inside the namespace, all measurements should go into the > physical log, so that replay is always complete for the PCRs, so > effectively the visible log of the namespace would always have to be a > subset of the physical log. Per the cover letter description this is already possible today. Stefan > > James > >
On Thu, 2021-12-02 at 08:41 -0500, Stefan Berger wrote: > On 12/2/21 07:46, James Bottomley wrote: > > On Tue, 2021-11-30 at 11:06 -0500, Stefan Berger wrote: > > > Move measurement list related variables into the ima_namespace. > > > This > > > way a > > > front-end like SecurityFS can show the measurement list inside an > > > IMA > > > namespace. > > > > > > Implement ima_free_measurements() to free a list of measurements > > > and call it when an IMA namespace is deleted. > > This one worries me quite a lot. What seems to be happening in > > this > > code: > > > > > @@ -107,7 +100,7 @@ static int ima_add_digest_entry(struct > > > ima_namespace *ns, > > > qe->entry = entry; > > > > > > INIT_LIST_HEAD(&qe->later); > > > - list_add_tail_rcu(&qe->later, &ima_measurements); > > > + list_add_tail_rcu(&qe->later, &ns->ima_measurements); > > > > > > atomic_long_inc(&ns->ima_htable.len); > > > if (update_htable) { > > > > > is that we now only add the measurements to the namespace list, but > > that list is freed when the namespace dies. However, the > > measurement > > is still extended through the PCRs meaning we have incomplete > > information for a replay after the namespace dies? > > *Not at all.* The measurement list of the namespace is independent > of > the host. > > The cover letter states: I get that the host can set up a policy to log everything in the namespace, but that wasn't my question. My question is can the guest set up a policy to log something that doesn't go into the host log (because the host hasn't asked for it to be logged) but extends a PCR anyway, thus destroying the ability of the host to do log replay. James
On 12/2/21 11:29, James Bottomley wrote: > On Thu, 2021-12-02 at 08:41 -0500, Stefan Berger wrote: >> On 12/2/21 07:46, James Bottomley wrote: >>> On Tue, 2021-11-30 at 11:06 -0500, Stefan Berger wrote: >>>> Move measurement list related variables into the ima_namespace. >>>> This >>>> way a >>>> front-end like SecurityFS can show the measurement list inside an >>>> IMA >>>> namespace. >>>> >>>> Implement ima_free_measurements() to free a list of measurements >>>> and call it when an IMA namespace is deleted. >>> This one worries me quite a lot. What seems to be happening in >>> this >>> code: >>> >>>> @@ -107,7 +100,7 @@ static int ima_add_digest_entry(struct >>>> ima_namespace *ns, >>>> qe->entry = entry; >>>> >>>> INIT_LIST_HEAD(&qe->later); >>>> - list_add_tail_rcu(&qe->later, &ima_measurements); >>>> + list_add_tail_rcu(&qe->later, &ns->ima_measurements); >>>> >>>> atomic_long_inc(&ns->ima_htable.len); >>>> if (update_htable) { >>>> >>> is that we now only add the measurements to the namespace list, but >>> that list is freed when the namespace dies. However, the >>> measurement >>> is still extended through the PCRs meaning we have incomplete >>> information for a replay after the namespace dies? >> *Not at all.* The measurement list of the namespace is independent >> of >> the host. >> >> The cover letter states: > I get that the host can set up a policy to log everything in the > namespace, but that wasn't my question. My question is can the guest > set up a policy to log something that doesn't go into the host log > (because the host hasn't asked for it to be logged) but extends a PCR > anyway, thus destroying the ability of the host to do log replay. host log goes with host TPM and vice versa guest log goes with (optional) vTPM and vice version Extending the PCR of the host's TPM would require the data to be logged in the host log as well. So, no, it's not possible. Stefan > > James > >
On Thu, 2021-12-02 at 11:45 -0500, Stefan Berger wrote: > On 12/2/21 11:29, James Bottomley wrote: > > On Thu, 2021-12-02 at 08:41 -0500, Stefan Berger wrote: > > > On 12/2/21 07:46, James Bottomley wrote: > > > > On Tue, 2021-11-30 at 11:06 -0500, Stefan Berger wrote: > > > > > Move measurement list related variables into the > > > > > ima_namespace. > > > > > This > > > > > way a > > > > > front-end like SecurityFS can show the measurement list > > > > > inside an > > > > > IMA > > > > > namespace. > > > > > > > > > > Implement ima_free_measurements() to free a list of > > > > > measurements > > > > > and call it when an IMA namespace is deleted. > > > > This one worries me quite a lot. What seems to be happening in > > > > this > > > > code: > > > > > > > > > @@ -107,7 +100,7 @@ static int ima_add_digest_entry(struct > > > > > ima_namespace *ns, > > > > > qe->entry = entry; > > > > > > > > > > INIT_LIST_HEAD(&qe->later); > > > > > - list_add_tail_rcu(&qe->later, &ima_measurements); > > > > > + list_add_tail_rcu(&qe->later, &ns->ima_measurements); > > > > > > > > > > atomic_long_inc(&ns->ima_htable.len); > > > > > if (update_htable) { > > > > > > > > > is that we now only add the measurements to the namespace list, > > > > but > > > > that list is freed when the namespace dies. However, the > > > > measurement > > > > is still extended through the PCRs meaning we have incomplete > > > > information for a replay after the namespace dies? > > > *Not at all.* The measurement list of the namespace is > > > independent > > > of > > > the host. > > > > > > The cover letter states: > > I get that the host can set up a policy to log everything in the > > namespace, but that wasn't my question. My question is can the > > guest > > set up a policy to log something that doesn't go into the host log > > (because the host hasn't asked for it to be logged) but extends a > > PCR > > anyway, thus destroying the ability of the host to do log replay. > > host log goes with host TPM and vice versa > > guest log goes with (optional) vTPM and vice version But that's what doesn't seem to happen ... ima_pcr_extend isn't virtualized and it's always called from ima_add_template_entry() meaning the physical TPM is always extended even for a namespace only entry. > Extending the PCR of the host's TPM would require the data to be > logged in the host log as well. So, no, it's not possible. Well, exactly: if you don't have or want a vTPM per container the only way to attest is via the physical TPM which means all entries in the namespace must be in the host log, so the host owner can quote and reply and they can split the attested log and give assurance to the namespaces that their entries are correct. James
On 12/2/21 12:44, James Bottomley wrote: > On Thu, 2021-12-02 at 11:45 -0500, Stefan Berger wrote: >> On 12/2/21 11:29, James Bottomley wrote: >>> On Thu, 2021-12-02 at 08:41 -0500, Stefan Berger wrote: >>>> On 12/2/21 07:46, James Bottomley wrote: >>>>> On Tue, 2021-11-30 at 11:06 -0500, Stefan Berger wrote: >>>>>> Move measurement list related variables into the >>>>>> ima_namespace. >>>>>> This >>>>>> way a >>>>>> front-end like SecurityFS can show the measurement list >>>>>> inside an >>>>>> IMA >>>>>> namespace. >>>>>> >>>>>> Implement ima_free_measurements() to free a list of >>>>>> measurements >>>>>> and call it when an IMA namespace is deleted. >>>>> This one worries me quite a lot. What seems to be happening in >>>>> this >>>>> code: >>>>> >>>>>> @@ -107,7 +100,7 @@ static int ima_add_digest_entry(struct >>>>>> ima_namespace *ns, >>>>>> qe->entry = entry; >>>>>> >>>>>> INIT_LIST_HEAD(&qe->later); >>>>>> - list_add_tail_rcu(&qe->later, &ima_measurements); >>>>>> + list_add_tail_rcu(&qe->later, &ns->ima_measurements); >>>>>> >>>>>> atomic_long_inc(&ns->ima_htable.len); >>>>>> if (update_htable) { >>>>>> >>>>> is that we now only add the measurements to the namespace list, >>>>> but >>>>> that list is freed when the namespace dies. However, the >>>>> measurement >>>>> is still extended through the PCRs meaning we have incomplete >>>>> information for a replay after the namespace dies? >>>> *Not at all.* The measurement list of the namespace is >>>> independent >>>> of >>>> the host. >>>> >>>> The cover letter states: >>> I get that the host can set up a policy to log everything in the >>> namespace, but that wasn't my question. My question is can the >>> guest >>> set up a policy to log something that doesn't go into the host log >>> (because the host hasn't asked for it to be logged) but extends a >>> PCR >>> anyway, thus destroying the ability of the host to do log replay. >> host log goes with host TPM and vice versa >> >> guest log goes with (optional) vTPM and vice version > But that's what doesn't seem to happen ... ima_pcr_extend isn't > virtualized and it's always called from ima_add_template_entry() > meaning the physical TPM is always extended even for a namespace only > entry. You cannot set a measurement rule in the namespace. That is prevented per 9/20: ima: Only accept AUDIT rules for IMA non-init_ima_ns namespaces for now. Also, with the tests that I have done with IMA namespaces I have not seen any 'evmctl ima_measurement ...' failures. Have you been able to cause the IMA namespace to do measurements? It would be an easy thing to move the tpm_chip into the ima_namespace as well, but per 9/20 this shouldn't be necessary at this point. > > >> Extending the PCR of the host's TPM would require the data to be >> logged in the host log as well. So, no, it's not possible. > Well, exactly: if you don't have or want a vTPM per container the only > way to attest is via the physical TPM which means all entries in the > namespace must be in the host log, so the host owner can quote and > reply and they can split the attested log and give assurance to the > namespaces that their entries are correct. Yes, this series allows you to log into the system log and along with this extend the TPM PCR. > > James > >
On Thu, 2021-12-02 at 13:03 -0500, Stefan Berger wrote: > On 12/2/21 12:44, James Bottomley wrote: > > On Thu, 2021-12-02 at 11:45 -0500, Stefan Berger wrote: > > > On 12/2/21 11:29, James Bottomley wrote: > > > > On Thu, 2021-12-02 at 08:41 -0500, Stefan Berger wrote: > > > > > On 12/2/21 07:46, James Bottomley wrote: > > > > > > On Tue, 2021-11-30 at 11:06 -0500, Stefan Berger wrote: > > > > > > > Move measurement list related variables into the > > > > > > > ima_namespace. This way a front-end like SecurityFS can > > > > > > > show the measurement list inside an IMA > > > > > > > namespace. > > > > > > > > > > > > > > Implement ima_free_measurements() to free a list of > > > > > > > measurements and call it when an IMA namespace is > > > > > > > deleted. > > > > > > This one worries me quite a lot. What seems to be > > > > > > happening in this code: > > > > > > > > > > > > > @@ -107,7 +100,7 @@ static int > > > > > > > ima_add_digest_entry(struct > > > > > > > ima_namespace *ns, > > > > > > > qe->entry = entry; > > > > > > > > > > > > > > INIT_LIST_HEAD(&qe->later); > > > > > > > - list_add_tail_rcu(&qe->later, &ima_measurements); > > > > > > > + list_add_tail_rcu(&qe->later, &ns- > > > > > > > >ima_measurements); > > > > > > > > > > > > > > atomic_long_inc(&ns->ima_htable.len); > > > > > > > if (update_htable) { > > > > > > > > > > > > > is that we now only add the measurements to the namespace > > > > > > list, but that list is freed when the namespace > > > > > > dies. However, the measurement is still extended through > > > > > > the PCRs meaning we have incomplete information for a > > > > > > replay after the namespace dies? > > > > > *Not at all.* The measurement list of the namespace is > > > > > independent of the host. > > > > > > > > > > The cover letter states: > > > > I get that the host can set up a policy to log everything in > > > > the namespace, but that wasn't my question. My question is can > > > > the guest set up a policy to log something that doesn't go into > > > > the host log (because the host hasn't asked for it to be > > > > logged) but extends a PCR anyway, thus destroying the ability > > > > of the host to do log replay. > > > host log goes with host TPM and vice versa > > > > > > guest log goes with (optional) vTPM and vice version > > But that's what doesn't seem to happen ... ima_pcr_extend isn't > > virtualized and it's always called from ima_add_template_entry() > > meaning the physical TPM is always extended even for a namespace > > only entry. > > You cannot set a measurement rule in the namespace. That is > prevented per 9/20: ima: Only accept AUDIT rules for IMA non- > init_ima_ns namespaces for now. Ah, OK, so the answer is nothing ever traverses this code for the non- root namespace, so no measurement ever get logged inside a namespace. Got it. James
diff --git a/include/linux/ima.h b/include/linux/ima.h index 96254dfacfa0..850a513834d2 100644 --- a/include/linux/ima.h +++ b/include/linux/ima.h @@ -260,6 +260,8 @@ struct ima_namespace { int ima_policy_flag; struct ima_h_table ima_htable; + struct list_head ima_measurements; + unsigned long binary_runtime_size; }; extern struct ima_namespace init_ima_ns; diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index a7e6c8fb152a..bb9763cd5fb1 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -104,7 +104,6 @@ struct ima_queue_entry { struct list_head later; /* place in ima_measurements list */ struct ima_template_entry *entry; }; -extern struct list_head ima_measurements; /* list of all measurements */ /* Some details preceding the binary serialized measurement list */ struct ima_kexec_hdr { @@ -168,8 +167,9 @@ int ima_restore_measurement_entry(struct ima_namespace *ns, struct ima_template_entry *entry); int ima_restore_measurement_list(struct ima_namespace *ns, loff_t bufsize, void *buf); +void ima_free_measurements(struct ima_namespace *ns); int ima_measurements_show(struct seq_file *m, void *v); -unsigned long ima_get_binary_runtime_size(void); +unsigned long ima_get_binary_runtime_size(struct ima_namespace *ns); int ima_init_template(void); void ima_init_template_list(void); int __init ima_init_digests(void); diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c index 9df8648ad64d..c35e15fb313f 100644 --- a/security/integrity/ima/ima_fs.c +++ b/security/integrity/ima/ima_fs.c @@ -80,12 +80,13 @@ static const struct file_operations ima_measurements_count_ops = { /* returns pointer to hlist_node */ static void *ima_measurements_start(struct seq_file *m, loff_t *pos) { + struct ima_namespace *ns = get_current_ns(); loff_t l = *pos; struct ima_queue_entry *qe; /* we need a lock since pos could point beyond last element */ rcu_read_lock(); - list_for_each_entry_rcu(qe, &ima_measurements, later) { + list_for_each_entry_rcu(qe, &ns->ima_measurements, later) { if (!l--) { rcu_read_unlock(); return qe; @@ -97,6 +98,7 @@ static void *ima_measurements_start(struct seq_file *m, loff_t *pos) static void *ima_measurements_next(struct seq_file *m, void *v, loff_t *pos) { + struct ima_namespace *ns = get_current_ns(); struct ima_queue_entry *qe = v; /* lock protects when reading beyond last element @@ -107,7 +109,7 @@ static void *ima_measurements_next(struct seq_file *m, void *v, loff_t *pos) rcu_read_unlock(); (*pos)++; - return (&qe->later == &ima_measurements) ? NULL : qe; + return (&qe->later == &ns->ima_measurements) ? NULL : qe; } static void ima_measurements_stop(struct seq_file *m, void *v) diff --git a/security/integrity/ima/ima_init_ima_ns.c b/security/integrity/ima/ima_init_ima_ns.c index e13adc3287ed..57e46a10c001 100644 --- a/security/integrity/ima/ima_init_ima_ns.c +++ b/security/integrity/ima/ima_init_ima_ns.c @@ -43,6 +43,11 @@ int ima_init_namespace(struct ima_namespace *ns) atomic_long_set(&ns->ima_htable.len, 0); atomic_long_set(&ns->ima_htable.violations, 0); memset(&ns->ima_htable.queue, 0, sizeof(ns->ima_htable.queue)); + INIT_LIST_HEAD(&ns->ima_measurements); + if (IS_ENABLED(CONFIG_IMA_KEXEC) && ns == &init_ima_ns) + ns->binary_runtime_size = 0; + else + ns->binary_runtime_size = ULONG_MAX; return 0; } diff --git a/security/integrity/ima/ima_ns.c b/security/integrity/ima/ima_ns.c index 709db86f285f..e4f4cf84a6b5 100644 --- a/security/integrity/ima/ima_ns.c +++ b/security/integrity/ima/ima_ns.c @@ -69,6 +69,7 @@ static void destroy_ima_ns(struct ima_namespace *ns) pr_debug("DESTROY ima_ns: 0x%p\n", ns); ima_free_policy_rules(ns); free_ns_status_cache(ns); + ima_free_measurements(ns); kmem_cache_free(imans_cachep, ns); } diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c index 373154039b91..f15f776918ec 100644 --- a/security/integrity/ima/ima_queue.c +++ b/security/integrity/ima/ima_queue.c @@ -25,13 +25,6 @@ /* pre-allocated array of tpm_digest structures to extend a PCR */ static struct tpm_digest *digests; -LIST_HEAD(ima_measurements); /* list of all measurements */ -#ifdef CONFIG_IMA_KEXEC -static unsigned long binary_runtime_size; -#else -static unsigned long binary_runtime_size = ULONG_MAX; -#endif - /* mutex protects atomicity of extending measurement list * and extending the TPM PCR aggregate. Since tpm_extend can take * long (and the tpm driver uses a mutex), we can't use the spinlock. @@ -107,7 +100,7 @@ static int ima_add_digest_entry(struct ima_namespace *ns, qe->entry = entry; INIT_LIST_HEAD(&qe->later); - list_add_tail_rcu(&qe->later, &ima_measurements); + list_add_tail_rcu(&qe->later, &ns->ima_measurements); atomic_long_inc(&ns->ima_htable.len); if (update_htable) { @@ -116,12 +109,12 @@ static int ima_add_digest_entry(struct ima_namespace *ns, } else INIT_HLIST_NODE(&qe->hnext); - if (binary_runtime_size != ULONG_MAX) { + if (ns->binary_runtime_size != ULONG_MAX) { int size; size = get_binary_runtime_size(entry); - binary_runtime_size = (binary_runtime_size < ULONG_MAX - size) ? - binary_runtime_size + size : ULONG_MAX; + ns->binary_runtime_size = (ns->binary_runtime_size < ULONG_MAX - size) ? + ns->binary_runtime_size + size : ULONG_MAX; } return 0; } @@ -131,12 +124,12 @@ static int ima_add_digest_entry(struct ima_namespace *ns, * entire binary_runtime_measurement list, including the ima_kexec_hdr * structure. */ -unsigned long ima_get_binary_runtime_size(void) +unsigned long ima_get_binary_runtime_size(struct ima_namespace *ns) { - if (binary_runtime_size >= (ULONG_MAX - sizeof(struct ima_kexec_hdr))) + if (ns->binary_runtime_size >= (ULONG_MAX - sizeof(struct ima_kexec_hdr))) return ULONG_MAX; else - return binary_runtime_size + sizeof(struct ima_kexec_hdr); + return ns->binary_runtime_size + sizeof(struct ima_kexec_hdr); } static int ima_pcr_extend(struct tpm_digest *digests_arg, int pcr) @@ -217,6 +210,18 @@ int ima_restore_measurement_entry(struct ima_namespace *ns, return result; } +void ima_free_measurements(struct ima_namespace *ns) +{ + struct ima_queue_entry *qe, *tmp; + + list_for_each_entry_safe(qe, tmp, &ns->ima_measurements, later) { + list_del(&qe->later); + if (!hlist_unhashed(&qe->hnext)) + hlist_del(&qe->hnext); + kfree(qe); + } +} + int __init ima_init_digests(void) { u16 digest_size;
Move measurement list related variables into the ima_namespace. This way a front-end like SecurityFS can show the measurement list inside an IMA namespace. Implement ima_free_measurements() to free a list of measurements and call it when an IMA namespace is deleted. Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- include/linux/ima.h | 2 ++ security/integrity/ima/ima.h | 4 +-- security/integrity/ima/ima_fs.c | 6 +++-- security/integrity/ima/ima_init_ima_ns.c | 5 ++++ security/integrity/ima/ima_ns.c | 1 + security/integrity/ima/ima_queue.c | 33 ++++++++++++++---------- 6 files changed, 33 insertions(+), 18 deletions(-)