[PATCH 1/2] ima: split ima_add_digest_entry() function

[PATCH 1/2] ima: split ima_add_digest_entry() function

[Roberto Sassu]

[-- Attachment #1: Type: text/plain, Size: 3038 bytes --]

The ima_add_digest_entry() function has been split in order to avoid
adding an entry in the measurements list for which the PCR extend
operation subsequently fails. Required memory is allocated earlier in the
new function ima_prepare_template_entry() and the template entry is added
after ima_pcr_extend().

Signed-off-by: Roberto Sassu <roberto.sassu@polito.it>
---
 security/integrity/ima/ima_queue.c |   35 ++++++++++++++++++++++++++---------
 1 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index 8e28f04..ddc7e18 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -59,30 +59,41 @@ static struct ima_queue_entry *ima_lookup_digest_entry(u8 *digest_value)
 	return ret;
 }
 
-/* ima_add_template_entry helper function:
- * - Add template entry to measurement list and hash table.
+/* ima_prepare_template_entry helper function:
+ * - prepare template entry before adding it to the measurement list.
  *
  * (Called with ima_extend_list_mutex held.)
  */
-static int ima_add_digest_entry(struct ima_template_entry *entry)
+static struct ima_queue_entry *ima_prepare_template_entry(
+					struct ima_template_entry *entry)
 {
 	struct ima_queue_entry *qe;
-	unsigned int key;
 
 	qe = kmalloc(sizeof(*qe), GFP_KERNEL);
 	if (qe == NULL) {
 		pr_err("IMA: OUT OF MEMORY ERROR creating queue entry.\n");
-		return -ENOMEM;
+		goto out;
 	}
 	qe->entry = entry;
+out:
+	return qe;
+}
+
+/* ima_add_digest_entry helper function:
+ * - Add template entry to measurement list and hash table.
+ *
+ * (Called with ima_extend_list_mutex held.)
+ */
+static void ima_add_digest_entry(struct ima_queue_entry *qe)
+{
+	unsigned int key;
 
 	INIT_LIST_HEAD(&qe->later);
 	list_add_tail_rcu(&qe->later, &ima_measurements);
 
 	atomic_long_inc(&ima_htable.len);
-	key = ima_hash_key(entry->digest);
+	key = ima_hash_key(qe->entry->digest);
 	hlist_add_head_rcu(&qe->hnext, &ima_htable.queue[key]);
-	return 0;
 }
 
 static int ima_pcr_extend(const u8 *hash)
@@ -104,6 +115,7 @@ static int ima_pcr_extend(const u8 *hash)
 int ima_add_template_entry(struct ima_template_entry *entry, int violation,
 			   const char *op, struct inode *inode)
 {
+	struct ima_queue_entry *qe;
 	u8 digest[IMA_DIGEST_SIZE];
 	const char *audit_cause = "hash_added";
 	int audit_info = 1;
@@ -118,10 +130,11 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
 		}
 	}
 
-	result = ima_add_digest_entry(entry);
-	if (result < 0) {
+	qe = ima_prepare_template_entry(entry);
+	if (qe == NULL) {
 		audit_cause = "ENOMEM";
 		audit_info = 0;
+		result = -ENOMEM;
 		goto out;
 	}
 
@@ -132,7 +145,11 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
 	if (result != 0) {
 		audit_cause = "TPM error";
 		audit_info = 0;
+		kfree(qe);
+		goto out;
 	}
+
+	ima_add_digest_entry(qe);
 out:
 	mutex_unlock(&ima_extend_list_mutex);
 	integrity_audit_msg(AUDIT_INTEGRITY_PCR, inode,
-- 
1.7.6.4


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 2061 bytes --]

[PATCH 2/2] ima: free memory of unused template entries

[Roberto Sassu]

[-- Attachment #1: Type: text/plain, Size: 2060 bytes --]

This patch solves the following issues:
- free the memory allocated for template entries that are already present
  in the measurements list;
- since template entries are added to the measurements list after the
  PCR extend operation (with the previous patch), they can be now safely
  removed if the latter fails.

Signed-off-by: Roberto Sassu <roberto.sassu@polito.it>
---
 security/integrity/ima/ima_api.c   |    4 ++--
 security/integrity/ima/ima_queue.c |    5 ++++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 0d50df0..88a2788 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -178,8 +178,8 @@ void ima_store_measurement(struct integrity_iint_cache *iint,
 	strncpy(entry->template.file_name, filename, IMA_EVENT_NAME_LEN_MAX);
 
 	result = ima_store_template(entry, violation, inode);
-	if (!result)
+	if (!result || result == -EEXIST)
 		iint->flags |= IMA_MEASURED;
-	else
+	if (result < 0)
 		kfree(entry);
 }
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index ddc7e18..7ae1e9a 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -105,7 +105,8 @@ static int ima_pcr_extend(const u8 *hash)
 
 	result = tpm_pcr_extend(TPM_ANY_NUM, CONFIG_IMA_MEASURE_PCR_IDX, hash);
 	if (result != 0)
-		pr_err("IMA: Error Communicating to TPM chip\n");
+		pr_err("IMA: Error Communicating to TPM chip, result: %d\n",
+		       result);
 	return result;
 }
 
@@ -126,6 +127,7 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
 		memcpy(digest, entry->digest, sizeof digest);
 		if (ima_lookup_digest_entry(digest)) {
 			audit_cause = "hash_exists";
+			result = -EEXIST;
 			goto out;
 		}
 	}
@@ -145,6 +147,7 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
 	if (result != 0) {
 		audit_cause = "TPM error";
 		audit_info = 0;
+		result = -ECOMM;
 		kfree(qe);
 		goto out;
 	}
-- 
1.7.6.4


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 2061 bytes --]

Re: [PATCH 1/2] ima: split ima_add_digest_entry() function

[Mimi Zohar]

On Wed, 2011-11-16 at 11:10 +0100, Roberto Sassu wrote:
> The ima_add_digest_entry() function has been split in order to avoid
> adding an entry in the measurements list for which the PCR extend
> operation subsequently fails. Required memory is allocated earlier in the
> new function ima_prepare_template_entry() and the template entry is added
> after ima_pcr_extend().
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@polito.it>

Not adding a measurement would certainly resolve the problems with
validating the measurement list against the PCR, but poses a risk that
something was read/executed that wasn't included in the measurement
list.  One solution would be for IMA to queue these measurements for the
TPM, but, I think, a better solution would be for the tpm driver to
queue them.

thanks,

Mimi

> ---
>  security/integrity/ima/ima_queue.c |   35 ++++++++++++++++++++++++++---------
>  1 files changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
> index 8e28f04..ddc7e18 100644
> --- a/security/integrity/ima/ima_queue.c
> +++ b/security/integrity/ima/ima_queue.c
> @@ -59,30 +59,41 @@ static struct ima_queue_entry *ima_lookup_digest_entry(u8 *digest_value)
>  	return ret;
>  }
> 
> -/* ima_add_template_entry helper function:
> - * - Add template entry to measurement list and hash table.
> +/* ima_prepare_template_entry helper function:
> + * - prepare template entry before adding it to the measurement list.
>   *
>   * (Called with ima_extend_list_mutex held.)
>   */
> -static int ima_add_digest_entry(struct ima_template_entry *entry)
> +static struct ima_queue_entry *ima_prepare_template_entry(
> +					struct ima_template_entry *entry)
>  {
>  	struct ima_queue_entry *qe;
> -	unsigned int key;
> 
>  	qe = kmalloc(sizeof(*qe), GFP_KERNEL);
>  	if (qe == NULL) {
>  		pr_err("IMA: OUT OF MEMORY ERROR creating queue entry.\n");
> -		return -ENOMEM;
> +		goto out;
>  	}
>  	qe->entry = entry;
> +out:
> +	return qe;
> +}
> +
> +/* ima_add_digest_entry helper function:
> + * - Add template entry to measurement list and hash table.
> + *
> + * (Called with ima_extend_list_mutex held.)
> + */
> +static void ima_add_digest_entry(struct ima_queue_entry *qe)
> +{
> +	unsigned int key;
> 
>  	INIT_LIST_HEAD(&qe->later);
>  	list_add_tail_rcu(&qe->later, &ima_measurements);
> 
>  	atomic_long_inc(&ima_htable.len);
> -	key = ima_hash_key(entry->digest);
> +	key = ima_hash_key(qe->entry->digest);
>  	hlist_add_head_rcu(&qe->hnext, &ima_htable.queue[key]);
> -	return 0;
>  }
> 
>  static int ima_pcr_extend(const u8 *hash)
> @@ -104,6 +115,7 @@ static int ima_pcr_extend(const u8 *hash)
>  int ima_add_template_entry(struct ima_template_entry *entry, int violation,
>  			   const char *op, struct inode *inode)
>  {
> +	struct ima_queue_entry *qe;
>  	u8 digest[IMA_DIGEST_SIZE];
>  	const char *audit_cause = "hash_added";
>  	int audit_info = 1;
> @@ -118,10 +130,11 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
>  		}
>  	}
> 
> -	result = ima_add_digest_entry(entry);
> -	if (result < 0) {
> +	qe = ima_prepare_template_entry(entry);
> +	if (qe == NULL) {
>  		audit_cause = "ENOMEM";
>  		audit_info = 0;
> +		result = -ENOMEM;
>  		goto out;
>  	}
> 
> @@ -132,7 +145,11 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
>  	if (result != 0) {
>  		audit_cause = "TPM error";
>  		audit_info = 0;
> +		kfree(qe);
> +		goto out;
>  	}
> +
> +	ima_add_digest_entry(qe);
>  out:
>  	mutex_unlock(&ima_extend_list_mutex);
>  	integrity_audit_msg(AUDIT_INTEGRITY_PCR, inode,

Re: [PATCH 1/2] ima: split ima_add_digest_entry() function

[Roberto Sassu]

On 11/16/2011 02:38 PM, Mimi Zohar wrote:
> On Wed, 2011-11-16 at 11:10 +0100, Roberto Sassu wrote:
>> The ima_add_digest_entry() function has been split in order to avoid
>> adding an entry in the measurements list for which the PCR extend
>> operation subsequently fails. Required memory is allocated earlier in the
>> new function ima_prepare_template_entry() and the template entry is added
>> after ima_pcr_extend().
>>
>> Signed-off-by: Roberto Sassu<roberto.sassu@polito.it>
>

Hi Mimi

i don't know if this condition can happen, but suppose that
for whatever reason the PCR extend fails. In this case, since
the PCR is not extended, the measurements list can be modified,
by removing the non-measured entry, without this fact being
detected by the verifier. So, probably we can avoid to display
the entry.


> Not adding a measurement would certainly resolve the problems with
> validating the measurement list against the PCR, but poses a risk that
> something was read/executed that wasn't included in the measurement
> list.  One solution would be for IMA to queue these measurements for the
> TPM, but, I think, a better solution would be for the tpm driver to
> queue them.
>

I think this solution will not prevent that some measurements
are not included in the list, because, if i understand correctly,
queuing an operation means that the latter can be executed after
the list mutex is released. After that, a malicious program may
corrupt the queue, hiding its presence to the verifier.

There is also another issue to solve (but i don't know if this
can really happen): a process requests a system call but, at the
time IMA is invoked, there is not enough memory for allocating
a template entry. However, it may continue its execution if,
meanwhile, some memory resources are freed.

In my opinion, a viable solution may be to return an error in
the IMA hooks when an inode cannot be properly measured (exactly
as the ima-appraisal extension does). For compatibility reasons,
this feature can be disabled by introducing a new kernel parameter
that tell IMA to ignore errors.

Thanks

Roberto Sassu


> thanks,
>
> Mimi
>
>> ---
>>   security/integrity/ima/ima_queue.c |   35 ++++++++++++++++++++++++++---------
>>   1 files changed, 26 insertions(+), 9 deletions(-)
>>
>> diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
>> index 8e28f04..ddc7e18 100644
>> --- a/security/integrity/ima/ima_queue.c
>> +++ b/security/integrity/ima/ima_queue.c
>> @@ -59,30 +59,41 @@ static struct ima_queue_entry *ima_lookup_digest_entry(u8 *digest_value)
>>   	return ret;
>>   }
>>
>> -/* ima_add_template_entry helper function:
>> - * - Add template entry to measurement list and hash table.
>> +/* ima_prepare_template_entry helper function:
>> + * - prepare template entry before adding it to the measurement list.
>>    *
>>    * (Called with ima_extend_list_mutex held.)
>>    */
>> -static int ima_add_digest_entry(struct ima_template_entry *entry)
>> +static struct ima_queue_entry *ima_prepare_template_entry(
>> +					struct ima_template_entry *entry)
>>   {
>>   	struct ima_queue_entry *qe;
>> -	unsigned int key;
>>
>>   	qe = kmalloc(sizeof(*qe), GFP_KERNEL);
>>   	if (qe == NULL) {
>>   		pr_err("IMA: OUT OF MEMORY ERROR creating queue entry.\n");
>> -		return -ENOMEM;
>> +		goto out;
>>   	}
>>   	qe->entry = entry;
>> +out:
>> +	return qe;
>> +}
>> +
>> +/* ima_add_digest_entry helper function:
>> + * - Add template entry to measurement list and hash table.
>> + *
>> + * (Called with ima_extend_list_mutex held.)
>> + */
>> +static void ima_add_digest_entry(struct ima_queue_entry *qe)
>> +{
>> +	unsigned int key;
>>
>>   	INIT_LIST_HEAD(&qe->later);
>>   	list_add_tail_rcu(&qe->later,&ima_measurements);
>>
>>   	atomic_long_inc(&ima_htable.len);
>> -	key = ima_hash_key(entry->digest);
>> +	key = ima_hash_key(qe->entry->digest);
>>   	hlist_add_head_rcu(&qe->hnext,&ima_htable.queue[key]);
>> -	return 0;
>>   }
>>
>>   static int ima_pcr_extend(const u8 *hash)
>> @@ -104,6 +115,7 @@ static int ima_pcr_extend(const u8 *hash)
>>   int ima_add_template_entry(struct ima_template_entry *entry, int violation,
>>   			   const char *op, struct inode *inode)
>>   {
>> +	struct ima_queue_entry *qe;
>>   	u8 digest[IMA_DIGEST_SIZE];
>>   	const char *audit_cause = "hash_added";
>>   	int audit_info = 1;
>> @@ -118,10 +130,11 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
>>   		}
>>   	}
>>
>> -	result = ima_add_digest_entry(entry);
>> -	if (result<  0) {
>> +	qe = ima_prepare_template_entry(entry);
>> +	if (qe == NULL) {
>>   		audit_cause = "ENOMEM";
>>   		audit_info = 0;
>> +		result = -ENOMEM;
>>   		goto out;
>>   	}
>>
>> @@ -132,7 +145,11 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
>>   	if (result != 0) {
>>   		audit_cause = "TPM error";
>>   		audit_info = 0;
>> +		kfree(qe);
>> +		goto out;
>>   	}
>> +
>> +	ima_add_digest_entry(qe);
>>   out:
>>   	mutex_unlock(&ima_extend_list_mutex);
>>   	integrity_audit_msg(AUDIT_INTEGRITY_PCR, inode,
>
>

Re: [PATCH 1/2] ima: split ima_add_digest_entry() function

[Rajiv Andrade]

Thanks, Rajiv Andrade Security Development IBM Linux Technology Center

On 16-11-2011 12:37, Roberto Sassu wrote:
> On 11/16/2011 02:38 PM, Mimi Zohar wrote:
>> On Wed, 2011-11-16 at 11:10 +0100, Roberto Sassu wrote:
>>> The ima_add_digest_entry() function has been split in order to avoid
>>> adding an entry in the measurements list for which the PCR extend
>>> operation subsequently fails. Required memory is allocated earlier 
>>> in the
>>> new function ima_prepare_template_entry() and the template entry is 
>>> added
>>> after ima_pcr_extend().
>>>
>>> Signed-off-by: Roberto Sassu<roberto.sassu@polito.it>
>>
>
> Hi Mimi
>
> i don't know if this condition can happen, but suppose that
> for whatever reason the PCR extend fails. In this case, since
> the PCR is not extended, the measurements list can be modified,
> by removing the non-measured entry, without this fact being
> detected by the verifier. So, probably we can avoid to display
> the entry.
>
>
Hi Roberto,

IMA's trustworthiness is built on the assumption that the TPM underneath can
be trusted. If that can't be, the eventlog alone doesn't provide us any 
security.
It's the TPM device driver's job though to workaround any HW bug so that 
in the
end all its stakeholders have their commands processed successfully, as 
we've
pursued in some changes here:

http://marc.info/?l=linux-kernel&m=132144742019589&w=2 
<http://marc.info/?l=linux-kernel&m=132144742019589&w=2>

What you're doing is to indeed move part of that trust to the software 
stack,
assuming that in case the TPM fails to process a command, you could fall 
back to
the event log anyways. It isn't a matter of it's a right or wrong 
software engineering
decision, but inside the trusted computing scope, it breaks the model.

Rajiv

Re: [PATCH 1/2] ima: split ima_add_digest_entry() function

[Roberto Sassu]

On 11/16/2011 07:52 PM, Rajiv Andrade wrote:
>
> Thanks, Rajiv Andrade Security Development IBM Linux Technology Center
>
> On 16-11-2011 12:37, Roberto Sassu wrote:
>> On 11/16/2011 02:38 PM, Mimi Zohar wrote:
>>> On Wed, 2011-11-16 at 11:10 +0100, Roberto Sassu wrote:
>>>> The ima_add_digest_entry() function has been split in order to avoid
>>>> adding an entry in the measurements list for which the PCR extend
>>>> operation subsequently fails. Required memory is allocated earlier
>>>> in the
>>>> new function ima_prepare_template_entry() and the template entry is
>>>> added
>>>> after ima_pcr_extend().
>>>>
>>>> Signed-off-by: Roberto Sassu<roberto.sassu@polito.it>
>>>
>>
>> Hi Mimi
>>
>> i don't know if this condition can happen, but suppose that
>> for whatever reason the PCR extend fails. In this case, since
>> the PCR is not extended, the measurements list can be modified,
>> by removing the non-measured entry, without this fact being
>> detected by the verifier. So, probably we can avoid to display
>> the entry.
>>
>>
> Hi Roberto,
>
> IMA's trustworthiness is built on the assumption that the TPM underneath
> can
> be trusted. If that can't be, the eventlog alone doesn't provide us any
> security.
> It's the TPM device driver's job though to workaround any HW bug so that
> in the
> end all its stakeholders have their commands processed successfully, as
> we've
> pursued in some changes here:
>

Hi Rajiv

thanks for your comments.

I absolutely agree that we have to trust the TPM for the correct
execution of IMA.

I think the principle that has been used to build IMA (according
to the TGC specifications) is that we can trust the eventlog
as long as the measurement infrastructure is reliable or it is
possible to detect a threat from previous measurements.

For this reason, a system call is never executed before the
inode measurement is inserted in the eventlog and the PCR
is extended. Since these operations must be considered as
atomic, their execution is protected by a mutex, that is
released only after all tasks have been performed. This
ensures that we begin with a measured kernel and we can
reliably measure all further interactions. This also explains,
in my view, why delaying the PCR extend operation may lead
to security risks.

About my patch, i did not move out the protected region any
of the above described operations. Instead, i'm preventing
measurements for which the PCR extend failed to be added to
the measurements list, because in any case it is impossible
for a verifier to detect their removal from the list.

As i mentioned in the previous mail, one solution to overcome
this issue is to deny, on the platform running IMA, the execution
of those system calls for which the measurement process ended
with an error.

Regards

Roberto Sassu


> http://marc.info/?l=linux-kernel&m=132144742019589&w=2
> <http://marc.info/?l=linux-kernel&m=132144742019589&w=2>
>
> What you're doing is to indeed move part of that trust to the software
> stack,
> assuming that in case the TPM fails to process a command, you could fall
> back to
> the event log anyways. It isn't a matter of it's a right or wrong
> software engineering
> decision, but inside the trusted computing scope, it breaks the model.
>
> Rajiv
>

Re: [PATCH 1/2] ima: split ima_add_digest_entry() function

[Mimi Zohar]

On Thu, 2011-11-17 at 11:57 +0100, Roberto Sassu wrote:
> On 11/16/2011 07:52 PM, Rajiv Andrade wrote:
> >
> > Thanks, Rajiv Andrade Security Development IBM Linux Technology Center
> >
> > On 16-11-2011 12:37, Roberto Sassu wrote:
> >> On 11/16/2011 02:38 PM, Mimi Zohar wrote:
> >>> On Wed, 2011-11-16 at 11:10 +0100, Roberto Sassu wrote:
> >>>> The ima_add_digest_entry() function has been split in order to avoid
> >>>> adding an entry in the measurements list for which the PCR extend
> >>>> operation subsequently fails. Required memory is allocated earlier
> >>>> in the
> >>>> new function ima_prepare_template_entry() and the template entry is
> >>>> added
> >>>> after ima_pcr_extend().
> >>>>
> >>>> Signed-off-by: Roberto Sassu<roberto.sassu@polito.it>
> >>>
> >>
> >> Hi Mimi
> >>
> >> i don't know if this condition can happen, but suppose that
> >> for whatever reason the PCR extend fails. In this case, since
> >> the PCR is not extended, the measurements list can be modified,
> >> by removing the non-measured entry, without this fact being
> >> detected by the verifier. So, probably we can avoid to display
> >> the entry.
> >>
> >>
> > Hi Roberto,
> >
> > IMA's trustworthiness is built on the assumption that the TPM underneath
> > can
> > be trusted. If that can't be, the eventlog alone doesn't provide us any
> > security.
> > It's the TPM device driver's job though to workaround any HW bug so that
> > in the
> > end all its stakeholders have their commands processed successfully, as
> > we've
> > pursued in some changes here:
> >
> 
> Hi Rajiv
> 
> thanks for your comments.
> 
> I absolutely agree that we have to trust the TPM for the correct
> execution of IMA.
> 
> I think the principle that has been used to build IMA (according
> to the TGC specifications) is that we can trust the eventlog
> as long as the measurement infrastructure is reliable or it is
> possible to detect a threat from previous measurements.
> 
> For this reason, a system call is never executed before the
> inode measurement is inserted in the eventlog and the PCR
> is extended. Since these operations must be considered as
> atomic, their execution is protected by a mutex, that is
> released only after all tasks have been performed. This
> ensures that we begin with a measured kernel and we can
> reliably measure all further interactions. This also explains,
> in my view, why delaying the PCR extend operation may lead
> to security risks.
> 
> About my patch, i did not move out the protected region any
> of the above described operations. Instead, i'm preventing
> measurements for which the PCR extend failed to be added to
> the measurements list, because in any case it is impossible
> for a verifier to detect their removal from the list.
> 
> As i mentioned in the previous mail, one solution to overcome
> this issue is to deny, on the platform running IMA, the execution
> of those system calls for which the measurement process ended
> with an error.
> 
> Regards
> 
> Roberto Sassu

True, if the TPM failed to extend the PCR, a malicious user would be
able to remove the measurement from the measurement list without it
being detected.  However, according to the TPM specs, the PCR extend
operation is always suppose to succeed, even in the case when the TPM is
not enabled.

More importantly we need to be able to detect when the PCR has not been
extended appropriately in order to address it.  Otherwise we're just
covering it up.

thanks,

Mimi

Re: [PATCH 1/2] ima: split ima_add_digest_entry() function

[Roberto Sassu]

On 11/17/2011 10:15 PM, Mimi Zohar wrote:
> On Thu, 2011-11-17 at 11:57 +0100, Roberto Sassu wrote:
>> On 11/16/2011 07:52 PM, Rajiv Andrade wrote:
>>>
>>> Thanks, Rajiv Andrade Security Development IBM Linux Technology Center
>>>
>>> On 16-11-2011 12:37, Roberto Sassu wrote:
>>>> On 11/16/2011 02:38 PM, Mimi Zohar wrote:
>>>>> On Wed, 2011-11-16 at 11:10 +0100, Roberto Sassu wrote:
>>>>>> The ima_add_digest_entry() function has been split in order to avoid
>>>>>> adding an entry in the measurements list for which the PCR extend
>>>>>> operation subsequently fails. Required memory is allocated earlier
>>>>>> in the
>>>>>> new function ima_prepare_template_entry() and the template entry is
>>>>>> added
>>>>>> after ima_pcr_extend().
>>>>>>
>>>>>> Signed-off-by: Roberto Sassu<roberto.sassu@polito.it>
>>>>>
>>>>
>>>> Hi Mimi
>>>>
>>>> i don't know if this condition can happen, but suppose that
>>>> for whatever reason the PCR extend fails. In this case, since
>>>> the PCR is not extended, the measurements list can be modified,
>>>> by removing the non-measured entry, without this fact being
>>>> detected by the verifier. So, probably we can avoid to display
>>>> the entry.
>>>>
>>>>
>>> Hi Roberto,
>>>
>>> IMA's trustworthiness is built on the assumption that the TPM underneath
>>> can
>>> be trusted. If that can't be, the eventlog alone doesn't provide us any
>>> security.
>>> It's the TPM device driver's job though to workaround any HW bug so that
>>> in the
>>> end all its stakeholders have their commands processed successfully, as
>>> we've
>>> pursued in some changes here:
>>>
>>
>> Hi Rajiv
>>
>> thanks for your comments.
>>
>> I absolutely agree that we have to trust the TPM for the correct
>> execution of IMA.
>>
>> I think the principle that has been used to build IMA (according
>> to the TGC specifications) is that we can trust the eventlog
>> as long as the measurement infrastructure is reliable or it is
>> possible to detect a threat from previous measurements.
>>
>> For this reason, a system call is never executed before the
>> inode measurement is inserted in the eventlog and the PCR
>> is extended. Since these operations must be considered as
>> atomic, their execution is protected by a mutex, that is
>> released only after all tasks have been performed. This
>> ensures that we begin with a measured kernel and we can
>> reliably measure all further interactions. This also explains,
>> in my view, why delaying the PCR extend operation may lead
>> to security risks.
>>
>> About my patch, i did not move out the protected region any
>> of the above described operations. Instead, i'm preventing
>> measurements for which the PCR extend failed to be added to
>> the measurements list, because in any case it is impossible
>> for a verifier to detect their removal from the list.
>>
>> As i mentioned in the previous mail, one solution to overcome
>> this issue is to deny, on the platform running IMA, the execution
>> of those system calls for which the measurement process ended
>> with an error.
>>
>> Regards
>>
>> Roberto Sassu
>
> True, if the TPM failed to extend the PCR, a malicious user would be
> able to remove the measurement from the measurement list without it
> being detected.  However, according to the TPM specs, the PCR extend
> operation is always suppose to succeed, even in the case when the TPM is
> not enabled.
>

Hi Mimi

i don't know if this can happen, but, since the TPM always returns
a result in its response, there may be particular conditions under
which the PCR extend fails.

After quickly looking at the code in 'drivers/char/tpm.c' there are
some additional cases where the operation may fail. For instance,
an operation may have been cancelled or it may have just reached the
defined time limit.


> More importantly we need to be able to detect when the PCR has not been
> extended appropriately in order to address it.  Otherwise we're just
> covering it up.
>

Probably i did not understand this point, but do the return code of 
tpm_pcr_extend() is not sufficient to determine if the operation was
completed successfully?

Roberto Sassu


> thanks,
>
> Mimi
>

Re: [PATCH 1/2] ima: split ima_add_digest_entry() function

[Mimi Zohar]

On Fri, 2011-11-18 at 11:27 +0100, Roberto Sassu wrote:
> On 11/17/2011 10:15 PM, Mimi Zohar wrote:
> > On Thu, 2011-11-17 at 11:57 +0100, Roberto Sassu wrote:
> >> On 11/16/2011 07:52 PM, Rajiv Andrade wrote:
> >>>
> >>> Thanks, Rajiv Andrade Security Development IBM Linux Technology Center
> >>>
> >>> On 16-11-2011 12:37, Roberto Sassu wrote:
> >>>> On 11/16/2011 02:38 PM, Mimi Zohar wrote:
> >>>>> On Wed, 2011-11-16 at 11:10 +0100, Roberto Sassu wrote:
> >>>>>> The ima_add_digest_entry() function has been split in order to avoid
> >>>>>> adding an entry in the measurements list for which the PCR extend
> >>>>>> operation subsequently fails. Required memory is allocated earlier
> >>>>>> in the
> >>>>>> new function ima_prepare_template_entry() and the template entry is
> >>>>>> added
> >>>>>> after ima_pcr_extend().
> >>>>>>
> >>>>>> Signed-off-by: Roberto Sassu<roberto.sassu@polito.it>
> >>>>>
> >>>>
> >>>> Hi Mimi
> >>>>
> >>>> i don't know if this condition can happen, but suppose that
> >>>> for whatever reason the PCR extend fails. In this case, since
> >>>> the PCR is not extended, the measurements list can be modified,
> >>>> by removing the non-measured entry, without this fact being
> >>>> detected by the verifier. So, probably we can avoid to display
> >>>> the entry.
> >>>>
> >>>>
> >>> Hi Roberto,
> >>>
> >>> IMA's trustworthiness is built on the assumption that the TPM underneath
> >>> can
> >>> be trusted. If that can't be, the eventlog alone doesn't provide us any
> >>> security.
> >>> It's the TPM device driver's job though to workaround any HW bug so that
> >>> in the
> >>> end all its stakeholders have their commands processed successfully, as
> >>> we've
> >>> pursued in some changes here:
> >>>
> >>
> >> Hi Rajiv
> >>
> >> thanks for your comments.
> >>
> >> I absolutely agree that we have to trust the TPM for the correct
> >> execution of IMA.
> >>
> >> I think the principle that has been used to build IMA (according
> >> to the TGC specifications) is that we can trust the eventlog
> >> as long as the measurement infrastructure is reliable or it is
> >> possible to detect a threat from previous measurements.
> >>
> >> For this reason, a system call is never executed before the
> >> inode measurement is inserted in the eventlog and the PCR
> >> is extended. Since these operations must be considered as
> >> atomic, their execution is protected by a mutex, that is
> >> released only after all tasks have been performed. This
> >> ensures that we begin with a measured kernel and we can
> >> reliably measure all further interactions. This also explains,
> >> in my view, why delaying the PCR extend operation may lead
> >> to security risks.
> >>
> >> About my patch, i did not move out the protected region any
> >> of the above described operations. Instead, i'm preventing
> >> measurements for which the PCR extend failed to be added to
> >> the measurements list, because in any case it is impossible
> >> for a verifier to detect their removal from the list.
> >>
> >> As i mentioned in the previous mail, one solution to overcome
> >> this issue is to deny, on the platform running IMA, the execution
> >> of those system calls for which the measurement process ended
> >> with an error.
> >>
> >> Regards
> >>
> >> Roberto Sassu
> >
> > True, if the TPM failed to extend the PCR, a malicious user would be
> > able to remove the measurement from the measurement list without it
> > being detected.  However, according to the TPM specs, the PCR extend
> > operation is always suppose to succeed, even in the case when the TPM is
> > not enabled.
> >
> 
> Hi Mimi
> 
> i don't know if this can happen, but, since the TPM always returns
> a result in its response, there may be particular conditions under
> which the PCR extend fails.
> 
> After quickly looking at the code in 'drivers/char/tpm.c' there are
> some additional cases where the operation may fail. For instance,
> an operation may have been cancelled or it may have just reached the
> defined time limit.
> 
> 
> > More importantly we need to be able to detect when the PCR has not been
> > extended appropriately in order to address it.  Otherwise we're just
> > covering it up.
> >
> 
> Probably i did not understand this point, but do the return code of 
> tpm_pcr_extend() is not sufficient to determine if the operation was
> completed successfully?
> 
> Roberto Sassu

Your concern, that if the PCR extend fails, the PCR value will not match
the measurement list, is a valid concern. I'm not disagreeing with you
on this point.  At the same time, the IMA measurement policy defines
which files to include in the measurement list.  This policy is
independent of the TPM, whether it is enabled or not, or even if the TPM
is failing for some reason.  IMA goes into TPM by-pass mode and
maintains the measurement list on systems without a TPM.

Removing measurements from the measurement list, so that we can verify
the PCR value, to me at least, is the wrong approach.  Maybe a better
approach would be to go into TPM by-pass mode.

Mimi

Re: [PATCH 1/2] ima: split ima_add_digest_entry() function

[Roberto Sassu]

On 11/18/2011 06:31 PM, Mimi Zohar wrote:
> On Fri, 2011-11-18 at 11:27 +0100, Roberto Sassu wrote:
>> On 11/17/2011 10:15 PM, Mimi Zohar wrote:
>>> On Thu, 2011-11-17 at 11:57 +0100, Roberto Sassu wrote:
>>>> On 11/16/2011 07:52 PM, Rajiv Andrade wrote:
>>>>>
>>>>> Thanks, Rajiv Andrade Security Development IBM Linux Technology Center
>>>>>
>>>>> On 16-11-2011 12:37, Roberto Sassu wrote:
>>>>>> On 11/16/2011 02:38 PM, Mimi Zohar wrote:
>>>>>>> On Wed, 2011-11-16 at 11:10 +0100, Roberto Sassu wrote:
>>>>>>>> The ima_add_digest_entry() function has been split in order to avoid
>>>>>>>> adding an entry in the measurements list for which the PCR extend
>>>>>>>> operation subsequently fails. Required memory is allocated earlier
>>>>>>>> in the
>>>>>>>> new function ima_prepare_template_entry() and the template entry is
>>>>>>>> added
>>>>>>>> after ima_pcr_extend().
>>>>>>>>
>>>>>>>> Signed-off-by: Roberto Sassu<roberto.sassu@polito.it>
>>>>>>>
>>>>>>
>>>>>> Hi Mimi
>>>>>>
>>>>>> i don't know if this condition can happen, but suppose that
>>>>>> for whatever reason the PCR extend fails. In this case, since
>>>>>> the PCR is not extended, the measurements list can be modified,
>>>>>> by removing the non-measured entry, without this fact being
>>>>>> detected by the verifier. So, probably we can avoid to display
>>>>>> the entry.
>>>>>>
>>>>>>
>>>>> Hi Roberto,
>>>>>
>>>>> IMA's trustworthiness is built on the assumption that the TPM underneath
>>>>> can
>>>>> be trusted. If that can't be, the eventlog alone doesn't provide us any
>>>>> security.
>>>>> It's the TPM device driver's job though to workaround any HW bug so that
>>>>> in the
>>>>> end all its stakeholders have their commands processed successfully, as
>>>>> we've
>>>>> pursued in some changes here:
>>>>>
>>>>
>>>> Hi Rajiv
>>>>
>>>> thanks for your comments.
>>>>
>>>> I absolutely agree that we have to trust the TPM for the correct
>>>> execution of IMA.
>>>>
>>>> I think the principle that has been used to build IMA (according
>>>> to the TGC specifications) is that we can trust the eventlog
>>>> as long as the measurement infrastructure is reliable or it is
>>>> possible to detect a threat from previous measurements.
>>>>
>>>> For this reason, a system call is never executed before the
>>>> inode measurement is inserted in the eventlog and the PCR
>>>> is extended. Since these operations must be considered as
>>>> atomic, their execution is protected by a mutex, that is
>>>> released only after all tasks have been performed. This
>>>> ensures that we begin with a measured kernel and we can
>>>> reliably measure all further interactions. This also explains,
>>>> in my view, why delaying the PCR extend operation may lead
>>>> to security risks.
>>>>
>>>> About my patch, i did not move out the protected region any
>>>> of the above described operations. Instead, i'm preventing
>>>> measurements for which the PCR extend failed to be added to
>>>> the measurements list, because in any case it is impossible
>>>> for a verifier to detect their removal from the list.
>>>>
>>>> As i mentioned in the previous mail, one solution to overcome
>>>> this issue is to deny, on the platform running IMA, the execution
>>>> of those system calls for which the measurement process ended
>>>> with an error.
>>>>
>>>> Regards
>>>>
>>>> Roberto Sassu
>>>
>>> True, if the TPM failed to extend the PCR, a malicious user would be
>>> able to remove the measurement from the measurement list without it
>>> being detected.  However, according to the TPM specs, the PCR extend
>>> operation is always suppose to succeed, even in the case when the TPM is
>>> not enabled.
>>>
>>
>> Hi Mimi
>>
>> i don't know if this can happen, but, since the TPM always returns
>> a result in its response, there may be particular conditions under
>> which the PCR extend fails.
>>
>> After quickly looking at the code in 'drivers/char/tpm.c' there are
>> some additional cases where the operation may fail. For instance,
>> an operation may have been cancelled or it may have just reached the
>> defined time limit.
>>
>>
>>> More importantly we need to be able to detect when the PCR has not been
>>> extended appropriately in order to address it.  Otherwise we're just
>>> covering it up.
>>>
>>
>> Probably i did not understand this point, but do the return code of
>> tpm_pcr_extend() is not sufficient to determine if the operation was
>> completed successfully?
>>
>> Roberto Sassu
>
> Your concern, that if the PCR extend fails, the PCR value will not match
> the measurement list, is a valid concern. I'm not disagreeing with you
> on this point.  At the same time, the IMA measurement policy defines
> which files to include in the measurement list.  This policy is
> independent of the TPM, whether it is enabled or not, or even if the TPM
> is failing for some reason.  IMA goes into TPM by-pass mode and
> maintains the measurement list on systems without a TPM.
>

Hi Mimi

i think that if at some point IMA goes in TPM by-pass mode, this allows
an attacker to hide malicious programs if they are measured after the
switch.

So, i agree that an IMA user wants the complete measurements list for
the corresponding policy, but i think it is also necessary to give
verifiers the proof that the list contains exactly all events occurred
in the platform to be attested, not only those for which the
measurement process ended successfully.

For this reason, i implemented a feature that denies the execution of
a requested system call if the measurement cannot be taken. I think
this will not affect users in their normal work, since the events that
cause this behaviour are very rare. Further, even if some measurements
were not taken, the Administrator can become aware of occurred errors by 
checking the audit log.

By default, this feature is deactivated so that IMA will behave as
usual. Instead, if a verifier needs the proof mentioned above, he can
check whether the newly introduced parameter that enables the feature
is present in the kernel command line.

Roberto Sassu


> Removing measurements from the measurement list, so that we can verify
> the PCR value, to me at least, is the wrong approach.  Maybe a better
> approach would be to go into TPM by-pass mode.
>
> Mimi
>

Re: [PATCH 1/2] ima: split ima_add_digest_entry() function

[Mimi Zohar]

On Mon, 2011-11-21 at 15:52 +0100, Roberto Sassu wrote:
> On 11/18/2011 06:31 PM, Mimi Zohar wrote:
> > On Fri, 2011-11-18 at 11:27 +0100, Roberto Sassu wrote:
> >> On 11/17/2011 10:15 PM, Mimi Zohar wrote:
> >>> On Thu, 2011-11-17 at 11:57 +0100, Roberto Sassu wrote:
> >>>> On 11/16/2011 07:52 PM, Rajiv Andrade wrote:
> >>>>>
> >>>>> Thanks, Rajiv Andrade Security Development IBM Linux Technology Center
> >>>>>
> >>>>> On 16-11-2011 12:37, Roberto Sassu wrote:
> >>>>>> On 11/16/2011 02:38 PM, Mimi Zohar wrote:
> >>>>>>> On Wed, 2011-11-16 at 11:10 +0100, Roberto Sassu wrote:
> >>>>>>>> The ima_add_digest_entry() function has been split in order to avoid
> >>>>>>>> adding an entry in the measurements list for which the PCR extend
> >>>>>>>> operation subsequently fails. Required memory is allocated earlier
> >>>>>>>> in the
> >>>>>>>> new function ima_prepare_template_entry() and the template entry is
> >>>>>>>> added
> >>>>>>>> after ima_pcr_extend().
> >>>>>>>>
> >>>>>>>> Signed-off-by: Roberto Sassu<roberto.sassu@polito.it>
> >>>>>>>
> >>>>>>
> >>>>>> Hi Mimi
> >>>>>>
> >>>>>> i don't know if this condition can happen, but suppose that
> >>>>>> for whatever reason the PCR extend fails. In this case, since
> >>>>>> the PCR is not extended, the measurements list can be modified,
> >>>>>> by removing the non-measured entry, without this fact being
> >>>>>> detected by the verifier. So, probably we can avoid to display
> >>>>>> the entry.
> >>>>>>
> >>>>>>
> >>>>> Hi Roberto,
> >>>>>
> >>>>> IMA's trustworthiness is built on the assumption that the TPM underneath
> >>>>> can
> >>>>> be trusted. If that can't be, the eventlog alone doesn't provide us any
> >>>>> security.
> >>>>> It's the TPM device driver's job though to workaround any HW bug so that
> >>>>> in the
> >>>>> end all its stakeholders have their commands processed successfully, as
> >>>>> we've
> >>>>> pursued in some changes here:
> >>>>>
> >>>>
> >>>> Hi Rajiv
> >>>>
> >>>> thanks for your comments.
> >>>>
> >>>> I absolutely agree that we have to trust the TPM for the correct
> >>>> execution of IMA.
> >>>>
> >>>> I think the principle that has been used to build IMA (according
> >>>> to the TGC specifications) is that we can trust the eventlog
> >>>> as long as the measurement infrastructure is reliable or it is
> >>>> possible to detect a threat from previous measurements.
> >>>>
> >>>> For this reason, a system call is never executed before the
> >>>> inode measurement is inserted in the eventlog and the PCR
> >>>> is extended. Since these operations must be considered as
> >>>> atomic, their execution is protected by a mutex, that is
> >>>> released only after all tasks have been performed. This
> >>>> ensures that we begin with a measured kernel and we can
> >>>> reliably measure all further interactions. This also explains,
> >>>> in my view, why delaying the PCR extend operation may lead
> >>>> to security risks.
> >>>>
> >>>> About my patch, i did not move out the protected region any
> >>>> of the above described operations. Instead, i'm preventing
> >>>> measurements for which the PCR extend failed to be added to
> >>>> the measurements list, because in any case it is impossible
> >>>> for a verifier to detect their removal from the list.
> >>>>
> >>>> As i mentioned in the previous mail, one solution to overcome
> >>>> this issue is to deny, on the platform running IMA, the execution
> >>>> of those system calls for which the measurement process ended
> >>>> with an error.
> >>>>
> >>>> Regards
> >>>>
> >>>> Roberto Sassu
> >>>
> >>> True, if the TPM failed to extend the PCR, a malicious user would be
> >>> able to remove the measurement from the measurement list without it
> >>> being detected.  However, according to the TPM specs, the PCR extend
> >>> operation is always suppose to succeed, even in the case when the TPM is
> >>> not enabled.
> >>>
> >>
> >> Hi Mimi
> >>
> >> i don't know if this can happen, but, since the TPM always returns
> >> a result in its response, there may be particular conditions under
> >> which the PCR extend fails.
> >>
> >> After quickly looking at the code in 'drivers/char/tpm.c' there are
> >> some additional cases where the operation may fail. For instance,
> >> an operation may have been cancelled or it may have just reached the
> >> defined time limit.
> >>
> >>
> >>> More importantly we need to be able to detect when the PCR has not been
> >>> extended appropriately in order to address it.  Otherwise we're just
> >>> covering it up.
> >>>
> >>
> >> Probably i did not understand this point, but do the return code of
> >> tpm_pcr_extend() is not sufficient to determine if the operation was
> >> completed successfully?
> >>
> >> Roberto Sassu
> >
> > Your concern, that if the PCR extend fails, the PCR value will not match
> > the measurement list, is a valid concern. I'm not disagreeing with you
> > on this point.  At the same time, the IMA measurement policy defines
> > which files to include in the measurement list.  This policy is
> > independent of the TPM, whether it is enabled or not, or even if the TPM
> > is failing for some reason.  IMA goes into TPM by-pass mode and
> > maintains the measurement list on systems without a TPM.
> >
> 
> Hi Mimi
> 
> i think that if at some point IMA goes in TPM by-pass mode, this allows
> an attacker to hide malicious programs if they are measured after the
> switch.
> 
> So, i agree that an IMA user wants the complete measurements list for
> the corresponding policy, but i think it is also necessary to give
> verifiers the proof that the list contains exactly all events occurred
> in the platform to be attested, not only those for which the
> measurement process ended successfully.

Ok, so we agree that the IMA measurement list must include all
measurements based on policy and that either the verifier needs to be
able to detect TPM failure to extend the PCR with a measurement, or
that, as you suggest, the read/execute request itself is denied.

Unlike IMA-appraisal, which enforces file integrity, base IMA does not
interfere with the running system and should not deny read/execute
requests.

To validate the PCR against the measurement list, the verifier requests
a PCR quote and the measurement list. In the case of a peristent TPM
failure, the verifier won't be able to get a PCR quote.  For the
intermittent TPM failure case, in addition to going into TPM by-pass
mode, we could also prevent the TPM from quoting the IMA PCR.  This
would require some minor changes to the TPM device driver.

Mimi

Re: [PATCH 1/2] ima: split ima_add_digest_entry() function

[Roberto Sassu]

On 12/05/2011 12:36 AM, Mimi Zohar wrote:
> On Mon, 2011-11-21 at 15:52 +0100, Roberto Sassu wrote:
>> On 11/18/2011 06:31 PM, Mimi Zohar wrote:
>>> On Fri, 2011-11-18 at 11:27 +0100, Roberto Sassu wrote:
>>>> On 11/17/2011 10:15 PM, Mimi Zohar wrote:
>>>>> On Thu, 2011-11-17 at 11:57 +0100, Roberto Sassu wrote:
>>>>>> On 11/16/2011 07:52 PM, Rajiv Andrade wrote:
>>>>>>>
>>>>>>> Thanks, Rajiv Andrade Security Development IBM Linux Technology Center
>>>>>>>
>>>>>>> On 16-11-2011 12:37, Roberto Sassu wrote:
>>>>>>>> On 11/16/2011 02:38 PM, Mimi Zohar wrote:
>>>>>>>>> On Wed, 2011-11-16 at 11:10 +0100, Roberto Sassu wrote:
>>>>>>>>>> The ima_add_digest_entry() function has been split in order to avoid
>>>>>>>>>> adding an entry in the measurements list for which the PCR extend
>>>>>>>>>> operation subsequently fails. Required memory is allocated earlier
>>>>>>>>>> in the
>>>>>>>>>> new function ima_prepare_template_entry() and the template entry is
>>>>>>>>>> added
>>>>>>>>>> after ima_pcr_extend().
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Roberto Sassu<roberto.sassu@polito.it>
>>>>>>>>>
>>>>>>>>
>>>>>>>> Hi Mimi
>>>>>>>>
>>>>>>>> i don't know if this condition can happen, but suppose that
>>>>>>>> for whatever reason the PCR extend fails. In this case, since
>>>>>>>> the PCR is not extended, the measurements list can be modified,
>>>>>>>> by removing the non-measured entry, without this fact being
>>>>>>>> detected by the verifier. So, probably we can avoid to display
>>>>>>>> the entry.
>>>>>>>>
>>>>>>>>
>>>>>>> Hi Roberto,
>>>>>>>
>>>>>>> IMA's trustworthiness is built on the assumption that the TPM underneath
>>>>>>> can
>>>>>>> be trusted. If that can't be, the eventlog alone doesn't provide us any
>>>>>>> security.
>>>>>>> It's the TPM device driver's job though to workaround any HW bug so that
>>>>>>> in the
>>>>>>> end all its stakeholders have their commands processed successfully, as
>>>>>>> we've
>>>>>>> pursued in some changes here:
>>>>>>>
>>>>>>
>>>>>> Hi Rajiv
>>>>>>
>>>>>> thanks for your comments.
>>>>>>
>>>>>> I absolutely agree that we have to trust the TPM for the correct
>>>>>> execution of IMA.
>>>>>>
>>>>>> I think the principle that has been used to build IMA (according
>>>>>> to the TGC specifications) is that we can trust the eventlog
>>>>>> as long as the measurement infrastructure is reliable or it is
>>>>>> possible to detect a threat from previous measurements.
>>>>>>
>>>>>> For this reason, a system call is never executed before the
>>>>>> inode measurement is inserted in the eventlog and the PCR
>>>>>> is extended. Since these operations must be considered as
>>>>>> atomic, their execution is protected by a mutex, that is
>>>>>> released only after all tasks have been performed. This
>>>>>> ensures that we begin with a measured kernel and we can
>>>>>> reliably measure all further interactions. This also explains,
>>>>>> in my view, why delaying the PCR extend operation may lead
>>>>>> to security risks.
>>>>>>
>>>>>> About my patch, i did not move out the protected region any
>>>>>> of the above described operations. Instead, i'm preventing
>>>>>> measurements for which the PCR extend failed to be added to
>>>>>> the measurements list, because in any case it is impossible
>>>>>> for a verifier to detect their removal from the list.
>>>>>>
>>>>>> As i mentioned in the previous mail, one solution to overcome
>>>>>> this issue is to deny, on the platform running IMA, the execution
>>>>>> of those system calls for which the measurement process ended
>>>>>> with an error.
>>>>>>
>>>>>> Regards
>>>>>>
>>>>>> Roberto Sassu
>>>>>
>>>>> True, if the TPM failed to extend the PCR, a malicious user would be
>>>>> able to remove the measurement from the measurement list without it
>>>>> being detected.  However, according to the TPM specs, the PCR extend
>>>>> operation is always suppose to succeed, even in the case when the TPM is
>>>>> not enabled.
>>>>>
>>>>
>>>> Hi Mimi
>>>>
>>>> i don't know if this can happen, but, since the TPM always returns
>>>> a result in its response, there may be particular conditions under
>>>> which the PCR extend fails.
>>>>
>>>> After quickly looking at the code in 'drivers/char/tpm.c' there are
>>>> some additional cases where the operation may fail. For instance,
>>>> an operation may have been cancelled or it may have just reached the
>>>> defined time limit.
>>>>
>>>>
>>>>> More importantly we need to be able to detect when the PCR has not been
>>>>> extended appropriately in order to address it.  Otherwise we're just
>>>>> covering it up.
>>>>>
>>>>
>>>> Probably i did not understand this point, but do the return code of
>>>> tpm_pcr_extend() is not sufficient to determine if the operation was
>>>> completed successfully?
>>>>
>>>> Roberto Sassu
>>>
>>> Your concern, that if the PCR extend fails, the PCR value will not match
>>> the measurement list, is a valid concern. I'm not disagreeing with you
>>> on this point.  At the same time, the IMA measurement policy defines
>>> which files to include in the measurement list.  This policy is
>>> independent of the TPM, whether it is enabled or not, or even if the TPM
>>> is failing for some reason.  IMA goes into TPM by-pass mode and
>>> maintains the measurement list on systems without a TPM.
>>>
>>
>> Hi Mimi
>>
>> i think that if at some point IMA goes in TPM by-pass mode, this allows
>> an attacker to hide malicious programs if they are measured after the
>> switch.
>>
>> So, i agree that an IMA user wants the complete measurements list for
>> the corresponding policy, but i think it is also necessary to give
>> verifiers the proof that the list contains exactly all events occurred
>> in the platform to be attested, not only those for which the
>> measurement process ended successfully.
>
> Ok, so we agree that the IMA measurement list must include all
> measurements based on policy and that either the verifier needs to be
> able to detect TPM failure to extend the PCR with a measurement, or
> that, as you suggest, the read/execute request itself is denied.
>
> Unlike IMA-appraisal, which enforces file integrity, base IMA does not
> interfere with the running system and should not deny read/execute
> requests.
>
> To validate the PCR against the measurement list, the verifier requests
> a PCR quote and the measurement list. In the case of a peristent TPM
> failure, the verifier won't be able to get a PCR quote.  For the
> intermittent TPM failure case, in addition to going into TPM by-pass
> mode, we could also prevent the TPM from quoting the IMA PCR.  This
> would require some minor changes to the TPM device driver.
>

Hi Mimi

i think moving this logic to the TPM driver (or in general, delaying
the action after the list mutex is unlocked) is not safe, because in
this way you are relying on the kernel trustworthiness to protect
itself and IMA against unmeasured potential attacks. So, the verifier
is unable to detect a kernel tampering that removed the limitation
on the TPM Quote operation.

What i'm proposing in the patch:

https://lkml.org/lkml/2011/11/21/202

is in fact a new extension, which is triggered by a new kernel
parameter, so that the behaviour of the base IMA is not modified.

Instead, regardless of this patch, we should fix the memory leaks and
the memory reference errors as it has been proposed in the patch 2/2 of 
this set.

Roberto Sassu


> Mimi
>

Re: [PATCH 1/2] ima: split ima_add_digest_entry() function

[Mimi Zohar]

On Mon, 2011-12-05 at 11:04 +0100, Roberto Sassu wrote:

> Hi Mimi
> 
> i think moving this logic to the TPM driver (or in general, delaying
> the action after the list mutex is unlocked) is not safe, because in
> this way you are relying on the kernel trustworthiness to protect
> itself and IMA against unmeasured potential attacks. So, the verifier
> is unable to detect a kernel tampering that removed the limitation
> on the TPM Quote operation.
> 
> What i'm proposing in the patch:
> 
> https://lkml.org/lkml/2011/11/21/202
> 
> is in fact a new extension, which is triggered by a new kernel
> parameter, so that the behaviour of the base IMA is not modified.

How/why the TPM fails is important.  If the TPM fails because of an
intermittent problem, then your solution of denying read/execute could
work, but what would happen if it was persistent?  Would you be able to
quiesce the system?

As there is no way of differentiating a persistent from intermittent
failure, both need to be addressed in the same manor.  For persistent
TPM failure, we can not access the TPM to modify the PCR.  So what
options do we have left?  My suggestion, though not optimal, prevents
the IMA PCR from being quoted.

>From ima_queue.c: ima_add_template_entry()

        result = ima_pcr_extend(digest);
        if (result != 0) {
                audit_cause = "TPM error";
                audit_info = 0;
        }
out:
        mutex_unlock(&ima_extend_list_mutex);

Either in ima_pcr_extend() itself or as a new call, set a flag of some
sort to prevent the TPM dd from quoting the IMA PCR.

I'm open to other suggestions.

> Instead, regardless of this patch, we should fix the memory leaks and
> the memory reference errors as it has been proposed in the patch 2/2 of 
> this set.
> 
> Roberto Sassu

Please separate bug fixes from any other changes.

thanks,

Mimi

Re: [PATCH 1/2] ima: split ima_add_digest_entry() function

[Roberto Sassu]

On 12/05/2011 02:03 PM, Mimi Zohar wrote:
> On Mon, 2011-12-05 at 11:04 +0100, Roberto Sassu wrote:
>
>> Hi Mimi
>>
>> i think moving this logic to the TPM driver (or in general, delaying
>> the action after the list mutex is unlocked) is not safe, because in
>> this way you are relying on the kernel trustworthiness to protect
>> itself and IMA against unmeasured potential attacks. So, the verifier
>> is unable to detect a kernel tampering that removed the limitation
>> on the TPM Quote operation.
>>
>> What i'm proposing in the patch:
>>
>> https://lkml.org/lkml/2011/11/21/202
>>
>> is in fact a new extension, which is triggered by a new kernel
>> parameter, so that the behaviour of the base IMA is not modified.
>
> How/why the TPM fails is important.  If the TPM fails because of an
> intermittent problem, then your solution of denying read/execute could
> work, but what would happen if it was persistent?  Would you be able to
> quiesce the system?
>
> As there is no way of differentiating a persistent from intermittent
> failure, both need to be addressed in the same manor.  For persistent
> TPM failure, we can not access the TPM to modify the PCR.  So what
> options do we have left?  My suggestion, though not optimal, prevents
> the IMA PCR from being quoted.
>

Hi Mimi

the solution you are proposing is reasonable as the default
behaviour, because not all IMA users need the high confidence
in the measurements, as ensured by denying the execution of
system calls.

However, during the IMA initialization the TPM is tested
by issuing a PCR read (the test procedure may be extended
to better detect existing errors in advance). So, this means
that a TPM failure when the system is already powered on is
very unlikely and may cause serious issues as it could happen
if other devices are involved.

For this reason, also my extension seems helpful especially
in the situations where all events need to be measured properly.
In this case, IMA users are aware that a TPM failure could hang
their systems, because they need to manually insert the required
kernel parameter.


>  From ima_queue.c: ima_add_template_entry()
>
>          result = ima_pcr_extend(digest);
>          if (result != 0) {
>                  audit_cause = "TPM error";
>                  audit_info = 0;
>          }
> out:
>          mutex_unlock(&ima_extend_list_mutex);
>
> Either in ima_pcr_extend() itself or as a new call, set a flag of some
> sort to prevent the TPM dd from quoting the IMA PCR.
>
> I'm open to other suggestions.
>
>> Instead, regardless of this patch, we should fix the memory leaks and
>> the memory reference errors as it has been proposed in the patch 2/2 of
>> this set.
>>
>> Roberto Sassu
>
> Please separate bug fixes from any other changes.
>

Ok, i will modify the patch 2/2 to work without the 1/2.

Thanks

Roberto Sassu


> thanks,
>
> Mimi
>

Re: [PATCH 1/2] ima: split ima_add_digest_entry() function

[Mimi Zohar]

On Mon, 2011-12-05 at 14:56 +0100, Roberto Sassu wrote:
> On 12/05/2011 02:03 PM, Mimi Zohar wrote:
> > On Mon, 2011-12-05 at 11:04 +0100, Roberto Sassu wrote:
> >
> >> Hi Mimi
> >>
> >> i think moving this logic to the TPM driver (or in general, delaying
> >> the action after the list mutex is unlocked) is not safe, because in
> >> this way you are relying on the kernel trustworthiness to protect
> >> itself and IMA against unmeasured potential attacks. So, the verifier
> >> is unable to detect a kernel tampering that removed the limitation
> >> on the TPM Quote operation.
> >>
> >> What i'm proposing in the patch:
> >>
> >> https://lkml.org/lkml/2011/11/21/202
> >>
> >> is in fact a new extension, which is triggered by a new kernel
> >> parameter, so that the behaviour of the base IMA is not modified.
> >
> > How/why the TPM fails is important.  If the TPM fails because of an
> > intermittent problem, then your solution of denying read/execute could
> > work, but what would happen if it was persistent?  Would you be able to
> > quiesce the system?
> >
> > As there is no way of differentiating a persistent from intermittent
> > failure, both need to be addressed in the same manor.  For persistent
> > TPM failure, we can not access the TPM to modify the PCR.  So what
> > options do we have left?  My suggestion, though not optimal, prevents
> > the IMA PCR from being quoted.
> >
> 
> Hi Mimi
> 
> the solution you are proposing is reasonable as the default
> behaviour, because not all IMA users need the high confidence
> in the measurements, as ensured by denying the execution of
> system calls.
> 
> However, during the IMA initialization the TPM is tested
> by issuing a PCR read (the test procedure may be extended
> to better detect existing errors in advance). So, this means
> that a TPM failure when the system is already powered on is
> very unlikely and may cause serious issues as it could happen
> if other devices are involved.
> 
> For this reason, also my extension seems helpful especially
> in the situations where all events need to be measured properly.
> In this case, IMA users are aware that a TPM failure could hang
> their systems, because they need to manually insert the required
> kernel parameter.

As you said a TPM failure is very unlikely, what type of attack are you
trying to defend against, that could possibly warrant causing the system
to hang?

Mimi

Re: [PATCH 1/2] ima: split ima_add_digest_entry() function

[Roberto Sassu]

On 12/05/2011 09:57 PM, Mimi Zohar wrote:
> On Mon, 2011-12-05 at 14:56 +0100, Roberto Sassu wrote:
>> On 12/05/2011 02:03 PM, Mimi Zohar wrote:
>>> On Mon, 2011-12-05 at 11:04 +0100, Roberto Sassu wrote:
>>>
>>>> Hi Mimi
>>>>
>>>> i think moving this logic to the TPM driver (or in general, delaying
>>>> the action after the list mutex is unlocked) is not safe, because in
>>>> this way you are relying on the kernel trustworthiness to protect
>>>> itself and IMA against unmeasured potential attacks. So, the verifier
>>>> is unable to detect a kernel tampering that removed the limitation
>>>> on the TPM Quote operation.
>>>>
>>>> What i'm proposing in the patch:
>>>>
>>>> https://lkml.org/lkml/2011/11/21/202
>>>>
>>>> is in fact a new extension, which is triggered by a new kernel
>>>> parameter, so that the behaviour of the base IMA is not modified.
>>>
>>> How/why the TPM fails is important.  If the TPM fails because of an
>>> intermittent problem, then your solution of denying read/execute could
>>> work, but what would happen if it was persistent?  Would you be able to
>>> quiesce the system?
>>>
>>> As there is no way of differentiating a persistent from intermittent
>>> failure, both need to be addressed in the same manor.  For persistent
>>> TPM failure, we can not access the TPM to modify the PCR.  So what
>>> options do we have left?  My suggestion, though not optimal, prevents
>>> the IMA PCR from being quoted.
>>>
>>
>> Hi Mimi
>>
>> the solution you are proposing is reasonable as the default
>> behaviour, because not all IMA users need the high confidence
>> in the measurements, as ensured by denying the execution of
>> system calls.
>>
>> However, during the IMA initialization the TPM is tested
>> by issuing a PCR read (the test procedure may be extended
>> to better detect existing errors in advance). So, this means
>> that a TPM failure when the system is already powered on is
>> very unlikely and may cause serious issues as it could happen
>> if other devices are involved.
>>
>> For this reason, also my extension seems helpful especially
>> in the situations where all events need to be measured properly.
>> In this case, IMA users are aware that a TPM failure could hang
>> their systems, because they need to manually insert the required
>> kernel parameter.
>
> As you said a TPM failure is very unlikely, what type of attack are you
> trying to defend against, that could possibly warrant causing the system
> to hang?
>

I don't know if this can really happen, but an attacker may issue
a lot of commands to the TPM, so that the timeout limit is reached
when IMA is trying to extend the PCR.

Roberto Sassu


> Mimi
>

Re: [PATCH 1/2] ima: split ima_add_digest_entry() function

[Mimi Zohar]

On Tue, 2011-12-06 at 11:27 +0100, Roberto Sassu wrote:
> On 12/05/2011 09:57 PM, Mimi Zohar wrote:
> > On Mon, 2011-12-05 at 14:56 +0100, Roberto Sassu wrote:
> >> On 12/05/2011 02:03 PM, Mimi Zohar wrote:
> >>> On Mon, 2011-12-05 at 11:04 +0100, Roberto Sassu wrote:
> >>>
> >>>> Hi Mimi
> >>>>
> >>>> i think moving this logic to the TPM driver (or in general, delaying
> >>>> the action after the list mutex is unlocked) is not safe, because in
> >>>> this way you are relying on the kernel trustworthiness to protect
> >>>> itself and IMA against unmeasured potential attacks. So, the verifier
> >>>> is unable to detect a kernel tampering that removed the limitation
> >>>> on the TPM Quote operation.
> >>>>
> >>>> What i'm proposing in the patch:
> >>>>
> >>>> https://lkml.org/lkml/2011/11/21/202
> >>>>
> >>>> is in fact a new extension, which is triggered by a new kernel
> >>>> parameter, so that the behaviour of the base IMA is not modified.
> >>>
> >>> How/why the TPM fails is important.  If the TPM fails because of an
> >>> intermittent problem, then your solution of denying read/execute could
> >>> work, but what would happen if it was persistent?  Would you be able to
> >>> quiesce the system?
> >>>
> >>> As there is no way of differentiating a persistent from intermittent
> >>> failure, both need to be addressed in the same manor.  For persistent
> >>> TPM failure, we can not access the TPM to modify the PCR.  So what
> >>> options do we have left?  My suggestion, though not optimal, prevents
> >>> the IMA PCR from being quoted.
> >>>
> >>
> >> Hi Mimi
> >>
> >> the solution you are proposing is reasonable as the default
> >> behaviour, because not all IMA users need the high confidence
> >> in the measurements, as ensured by denying the execution of
> >> system calls.
> >>
> >> However, during the IMA initialization the TPM is tested
> >> by issuing a PCR read (the test procedure may be extended
> >> to better detect existing errors in advance). So, this means
> >> that a TPM failure when the system is already powered on is
> >> very unlikely and may cause serious issues as it could happen
> >> if other devices are involved.
> >>
> >> For this reason, also my extension seems helpful especially
> >> in the situations where all events need to be measured properly.
> >> In this case, IMA users are aware that a TPM failure could hang
> >> their systems, because they need to manually insert the required
> >> kernel parameter.
> >
> > As you said a TPM failure is very unlikely, what type of attack are you
> > trying to defend against, that could possibly warrant causing the system
> > to hang?
> >
> 
> I don't know if this can really happen, but an attacker may issue
> a lot of commands to the TPM, so that the timeout limit is reached
> when IMA is trying to extend the PCR.
> 
> Roberto Sassu

Processing lots of commands isn't an issue, as IMA takes the
ima_extend_list_mutex to synchronize adding the measurement to the
measurement list and extending the PCR.  The TPM device driver takes the
tpm_mutex, in tpm_transmit(), before transmitting the command.

So the issue remains whether an individual PCR extend can timeout/fail.
As you previously said, this is highly unlikely.

Mimi

Re: [PATCH 1/2] ima: split ima_add_digest_entry() function

[Roberto Sassu]

On 12/06/2011 03:24 PM, Mimi Zohar wrote:
> On Tue, 2011-12-06 at 11:27 +0100, Roberto Sassu wrote:
>> On 12/05/2011 09:57 PM, Mimi Zohar wrote:
>>> On Mon, 2011-12-05 at 14:56 +0100, Roberto Sassu wrote:
>>>> On 12/05/2011 02:03 PM, Mimi Zohar wrote:
>>>>> On Mon, 2011-12-05 at 11:04 +0100, Roberto Sassu wrote:
>>>>>
>>>>>> Hi Mimi
>>>>>>
>>>>>> i think moving this logic to the TPM driver (or in general, delaying
>>>>>> the action after the list mutex is unlocked) is not safe, because in
>>>>>> this way you are relying on the kernel trustworthiness to protect
>>>>>> itself and IMA against unmeasured potential attacks. So, the verifier
>>>>>> is unable to detect a kernel tampering that removed the limitation
>>>>>> on the TPM Quote operation.
>>>>>>
>>>>>> What i'm proposing in the patch:
>>>>>>
>>>>>> https://lkml.org/lkml/2011/11/21/202
>>>>>>
>>>>>> is in fact a new extension, which is triggered by a new kernel
>>>>>> parameter, so that the behaviour of the base IMA is not modified.
>>>>>
>>>>> How/why the TPM fails is important.  If the TPM fails because of an
>>>>> intermittent problem, then your solution of denying read/execute could
>>>>> work, but what would happen if it was persistent?  Would you be able to
>>>>> quiesce the system?
>>>>>
>>>>> As there is no way of differentiating a persistent from intermittent
>>>>> failure, both need to be addressed in the same manor.  For persistent
>>>>> TPM failure, we can not access the TPM to modify the PCR.  So what
>>>>> options do we have left?  My suggestion, though not optimal, prevents
>>>>> the IMA PCR from being quoted.
>>>>>
>>>>
>>>> Hi Mimi
>>>>
>>>> the solution you are proposing is reasonable as the default
>>>> behaviour, because not all IMA users need the high confidence
>>>> in the measurements, as ensured by denying the execution of
>>>> system calls.
>>>>
>>>> However, during the IMA initialization the TPM is tested
>>>> by issuing a PCR read (the test procedure may be extended
>>>> to better detect existing errors in advance). So, this means
>>>> that a TPM failure when the system is already powered on is
>>>> very unlikely and may cause serious issues as it could happen
>>>> if other devices are involved.
>>>>
>>>> For this reason, also my extension seems helpful especially
>>>> in the situations where all events need to be measured properly.
>>>> In this case, IMA users are aware that a TPM failure could hang
>>>> their systems, because they need to manually insert the required
>>>> kernel parameter.
>>>
>>> As you said a TPM failure is very unlikely, what type of attack are you
>>> trying to defend against, that could possibly warrant causing the system
>>> to hang?
>>>
>>
>> I don't know if this can really happen, but an attacker may issue
>> a lot of commands to the TPM, so that the timeout limit is reached
>> when IMA is trying to extend the PCR.
>>
>> Roberto Sassu
>
> Processing lots of commands isn't an issue, as IMA takes the
> ima_extend_list_mutex to synchronize adding the measurement to the
> measurement list and extending the PCR.  The TPM device driver takes the
> tpm_mutex, in tpm_transmit(), before transmitting the command.
>

I mean issuing a lot of TPM commands, so that the TPM is unable
to process the IMA request.


> So the issue remains whether an individual PCR extend can timeout/fail.
> As you previously said, this is highly unlikely.
>

I think the question is whether or not an attacker can cause the
TPM to reach the timeout limit. If this is feasible and it cannot
be clearly detected by inspecting the measurements list, denying
the system call for which the measurement cannot be taken may be a solution.

Roberto Sassu


> Mimi
>

Re: [PATCH 1/2] ima: split ima_add_digest_entry() function

[Kasatkin, Dmitry]

On Tue, Dec 6, 2011 at 4:50 PM, Roberto Sassu <roberto.sassu@polito.it> wrote:
> On 12/06/2011 03:24 PM, Mimi Zohar wrote:
>>
>> On Tue, 2011-12-06 at 11:27 +0100, Roberto Sassu wrote:
>>>
>>> On 12/05/2011 09:57 PM, Mimi Zohar wrote:
>>>>
>>>> On Mon, 2011-12-05 at 14:56 +0100, Roberto Sassu wrote:
>>>>>
>>>>> On 12/05/2011 02:03 PM, Mimi Zohar wrote:
>>>>>>
>>>>>> On Mon, 2011-12-05 at 11:04 +0100, Roberto Sassu wrote:
>>>>>>
>>>>>>> Hi Mimi
>>>>>>>
>>>>>>> i think moving this logic to the TPM driver (or in general, delaying
>>>>>>> the action after the list mutex is unlocked) is not safe, because in
>>>>>>> this way you are relying on the kernel trustworthiness to protect
>>>>>>> itself and IMA against unmeasured potential attacks. So, the verifier
>>>>>>> is unable to detect a kernel tampering that removed the limitation
>>>>>>> on the TPM Quote operation.
>>>>>>>
>>>>>>> What i'm proposing in the patch:
>>>>>>>
>>>>>>> https://lkml.org/lkml/2011/11/21/202
>>>>>>>
>>>>>>> is in fact a new extension, which is triggered by a new kernel
>>>>>>> parameter, so that the behaviour of the base IMA is not modified.
>>>>>>
>>>>>>
>>>>>> How/why the TPM fails is important.  If the TPM fails because of an
>>>>>> intermittent problem, then your solution of denying read/execute could
>>>>>> work, but what would happen if it was persistent?  Would you be able
>>>>>> to
>>>>>> quiesce the system?
>>>>>>
>>>>>> As there is no way of differentiating a persistent from intermittent
>>>>>> failure, both need to be addressed in the same manor.  For persistent
>>>>>> TPM failure, we can not access the TPM to modify the PCR.  So what
>>>>>> options do we have left?  My suggestion, though not optimal, prevents
>>>>>> the IMA PCR from being quoted.
>>>>>>
>>>>>
>>>>> Hi Mimi
>>>>>
>>>>> the solution you are proposing is reasonable as the default
>>>>> behaviour, because not all IMA users need the high confidence
>>>>> in the measurements, as ensured by denying the execution of
>>>>> system calls.
>>>>>
>>>>> However, during the IMA initialization the TPM is tested
>>>>> by issuing a PCR read (the test procedure may be extended
>>>>> to better detect existing errors in advance). So, this means
>>>>> that a TPM failure when the system is already powered on is
>>>>> very unlikely and may cause serious issues as it could happen
>>>>> if other devices are involved.
>>>>>
>>>>> For this reason, also my extension seems helpful especially
>>>>> in the situations where all events need to be measured properly.
>>>>> In this case, IMA users are aware that a TPM failure could hang
>>>>> their systems, because they need to manually insert the required
>>>>> kernel parameter.
>>>>
>>>>
>>>> As you said a TPM failure is very unlikely, what type of attack are you
>>>> trying to defend against, that could possibly warrant causing the system
>>>> to hang?
>>>>
>>>
>>> I don't know if this can really happen, but an attacker may issue
>>> a lot of commands to the TPM, so that the timeout limit is reached
>>> when IMA is trying to extend the PCR.
>>>
>>> Roberto Sassu
>>
>>
>> Processing lots of commands isn't an issue, as IMA takes the
>> ima_extend_list_mutex to synchronize adding the measurement to the
>> measurement list and extending the PCR.  The TPM device driver takes the
>> tpm_mutex, in tpm_transmit(), before transmitting the command.
>>
>
> I mean issuing a lot of TPM commands, so that the TPM is unable
> to process the IMA request.
>
>
>
>> So the issue remains whether an individual PCR extend can timeout/fail.
>> As you previously said, this is highly unlikely.
>>
>
> I think the question is whether or not an attacker can cause the
> TPM to reach the timeout limit. If this is feasible and it cannot
> be clearly detected by inspecting the measurements list, denying
> the system call for which the measurement cannot be taken may be a solution.
>
> Roberto Sassu
>
>
>
>> Mimi
>>

If TPM is a separate HW module, it is often possible to make HW attack
and modify parameters and return values,
by accessing the bus.

- Dmitry

>
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] ima: split ima_add_digest_entry() function

[Roberto Sassu]

On 12/07/2011 02:33 PM, Kasatkin, Dmitry wrote:
> On Tue, Dec 6, 2011 at 4:50 PM, Roberto Sassu<roberto.sassu@polito.it>  wrote:
>> On 12/06/2011 03:24 PM, Mimi Zohar wrote:
>>>
>>> On Tue, 2011-12-06 at 11:27 +0100, Roberto Sassu wrote:
>>>>
>>>> On 12/05/2011 09:57 PM, Mimi Zohar wrote:
>>>>>
>>>>> On Mon, 2011-12-05 at 14:56 +0100, Roberto Sassu wrote:
>>>>>>
>>>>>> On 12/05/2011 02:03 PM, Mimi Zohar wrote:
>>>>>>>
>>>>>>> On Mon, 2011-12-05 at 11:04 +0100, Roberto Sassu wrote:
>>>>>>>
>>>>>>>> Hi Mimi
>>>>>>>>
>>>>>>>> i think moving this logic to the TPM driver (or in general, delaying
>>>>>>>> the action after the list mutex is unlocked) is not safe, because in
>>>>>>>> this way you are relying on the kernel trustworthiness to protect
>>>>>>>> itself and IMA against unmeasured potential attacks. So, the verifier
>>>>>>>> is unable to detect a kernel tampering that removed the limitation
>>>>>>>> on the TPM Quote operation.
>>>>>>>>
>>>>>>>> What i'm proposing in the patch:
>>>>>>>>
>>>>>>>> https://lkml.org/lkml/2011/11/21/202
>>>>>>>>
>>>>>>>> is in fact a new extension, which is triggered by a new kernel
>>>>>>>> parameter, so that the behaviour of the base IMA is not modified.
>>>>>>>
>>>>>>>
>>>>>>> How/why the TPM fails is important.  If the TPM fails because of an
>>>>>>> intermittent problem, then your solution of denying read/execute could
>>>>>>> work, but what would happen if it was persistent?  Would you be able
>>>>>>> to
>>>>>>> quiesce the system?
>>>>>>>
>>>>>>> As there is no way of differentiating a persistent from intermittent
>>>>>>> failure, both need to be addressed in the same manor.  For persistent
>>>>>>> TPM failure, we can not access the TPM to modify the PCR.  So what
>>>>>>> options do we have left?  My suggestion, though not optimal, prevents
>>>>>>> the IMA PCR from being quoted.
>>>>>>>
>>>>>>
>>>>>> Hi Mimi
>>>>>>
>>>>>> the solution you are proposing is reasonable as the default
>>>>>> behaviour, because not all IMA users need the high confidence
>>>>>> in the measurements, as ensured by denying the execution of
>>>>>> system calls.
>>>>>>
>>>>>> However, during the IMA initialization the TPM is tested
>>>>>> by issuing a PCR read (the test procedure may be extended
>>>>>> to better detect existing errors in advance). So, this means
>>>>>> that a TPM failure when the system is already powered on is
>>>>>> very unlikely and may cause serious issues as it could happen
>>>>>> if other devices are involved.
>>>>>>
>>>>>> For this reason, also my extension seems helpful especially
>>>>>> in the situations where all events need to be measured properly.
>>>>>> In this case, IMA users are aware that a TPM failure could hang
>>>>>> their systems, because they need to manually insert the required
>>>>>> kernel parameter.
>>>>>
>>>>>
>>>>> As you said a TPM failure is very unlikely, what type of attack are you
>>>>> trying to defend against, that could possibly warrant causing the system
>>>>> to hang?
>>>>>
>>>>
>>>> I don't know if this can really happen, but an attacker may issue
>>>> a lot of commands to the TPM, so that the timeout limit is reached
>>>> when IMA is trying to extend the PCR.
>>>>
>>>> Roberto Sassu
>>>
>>>
>>> Processing lots of commands isn't an issue, as IMA takes the
>>> ima_extend_list_mutex to synchronize adding the measurement to the
>>> measurement list and extending the PCR.  The TPM device driver takes the
>>> tpm_mutex, in tpm_transmit(), before transmitting the command.
>>>
>>
>> I mean issuing a lot of TPM commands, so that the TPM is unable
>> to process the IMA request.
>>
>>
>>
>>> So the issue remains whether an individual PCR extend can timeout/fail.
>>> As you previously said, this is highly unlikely.
>>>
>>
>> I think the question is whether or not an attacker can cause the
>> TPM to reach the timeout limit. If this is feasible and it cannot
>> be clearly detected by inspecting the measurements list, denying
>> the system call for which the measurement cannot be taken may be a solution.
>>
>> Roberto Sassu
>>
>>
>>
>>> Mimi
>>>
>
> If TPM is a separate HW module, it is often possible to make HW attack
> and modify parameters and return values,
> by accessing the bus.
>

Hi Dmitry

yes, this is another class of attacks that may also be used to
delay the execution of a command, so that the timeout limit will
be reached. However, probably we can do nothing if an attacker
can modify parameters or return values by accessing the hardware.

Roberto Sassu


> - Dmitry
>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe
>> linux-security-module" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html