diff mbox series

[RFC,-next] ima: Make tpm hash configurable

Message ID 20230817061334.1910-1-guozihua@huawei.com (mailing list archive)
State New, archived
Headers show
Series [RFC,-next] ima: Make tpm hash configurable | expand

Commit Message

Guozihua (Scott) Aug. 17, 2023, 6:13 a.m. UTC
TPM2 chips supports algorithms other than SHA1. However, the original
IMA design hardcode template hash to be SHA1.

This patch added CONFIG_IMA_TEMPLATE_HASH as well as ima_tpm_hash=
cmdline argument for configurating template hash. The usage is simuliar
to CONFIG_IMA_DEFAULT_HASH and ima_hash=. The configured hash is checked
against TPM and make sure that the hash algorithm is supported by
ima_tpm_chip.

To accommodate the change, we must put a digest length into binary
measurement list items. The binary measurement list item format is
changed to this:
	16bit-le=pcr#
	16bit-le=template digest size
	char[n]=template digest
	32bit-le=template name size
	char[n]=template name
	[eventdata length]
	eventdata[n]=template specific data
The first element is now a 16bit pcr number and a 16bit template digest
size, instead of the original 32bit pcr number.

The format of ascii_measurement_list is also changed. For sha1 template
hash, the format is the same as before. For other hash algorithms, a
hash name is prepended as such:
"sha256:30ee3e25620478759600be00e06fda7b4fe23bbf575621d480400d536cf54f5b"

Signed-off-by: GUO Zihua <guozihua@huawei.com>
---
 .../admin-guide/kernel-parameters.txt         |  7 +++
 security/integrity/ima/Kconfig                | 36 ++++++++++++-
 security/integrity/ima/ima.h                  |  2 +
 security/integrity/ima/ima_crypto.c           | 51 ++++++++++---------
 security/integrity/ima/ima_fs.c               | 43 +++++++++++-----
 security/integrity/ima/ima_init.c             | 44 +++++++++++++++-
 security/integrity/ima/ima_main.c             |  1 +
 7 files changed, 144 insertions(+), 40 deletions(-)

Comments

Mimi Zohar Aug. 17, 2023, 2:19 p.m. UTC | #1
Hi Scott,

This patch changes the TPM PCR hash algorithm and value in the IMA
measurement list.  The Subject line doesn't convey that.

On Thu, 2023-08-17 at 14:13 +0800, GUO Zihua wrote:
> TPM2 chips supports algorithms other than SHA1. However, the original
> IMA design hardcode template hash to be SHA1.

True, IMA initially calculated and extended a SHA1 hash into the TPM,
but Roberto addressed that years ago.  Refer to commit  1ea973df6e21
("ima: Calculate and extend PCR with digests in ima_template_entry").

IMA now calculates and extends each of the enabled TPM banks with the
appropriate hash value.  The PCR value in the IMA measurement list
remains SHA1.  Attestation servers can first verify the SHA1 template
hash as stored in the measurement list.  Then it can walk the IMA
measurement list calculating the template data hash based on the per
TPM bank algorithm to verify the TPM bank PCR value. 

> 
> This patch added CONFIG_IMA_TEMPLATE_HASH as well as ima_tpm_hash=
> cmdline argument for configurating template hash. The usage is simuliar
> to CONFIG_IMA_DEFAULT_HASH and ima_hash=. The configured hash is checked
> against TPM and make sure that the hash algorithm is supported by
> ima_tpm_chip.
> 
> To accommodate the change, we must put a digest length into binary
> measurement list items. The binary measurement list item format is
> changed to this:
> 	16bit-le=pcr#
> 	16bit-le=template digest size
> 	char[n]=template digest
> 	32bit-le=template name size
> 	char[n]=template name
> 	[eventdata length]
> 	eventdata[n]=template specific data
> The first element is now a 16bit pcr number and a 16bit template digest
> size, instead of the original 32bit pcr number.
> 
> The format of ascii_measurement_list is also changed. For sha1 template
> hash, the format is the same as before. For other hash algorithms, a
> hash name is prepended as such:
> "sha256:30ee3e25620478759600be00e06fda7b4fe23bbf575621d480400d536cf54f5b"
> Signed-off-by: GUO Zihua <guozihua@huawei.com>

Other proposals have changed the hard coded hash algorithm and PCR
value from SHA1 to SHA256.  Both that proposal and this will break
existing userspace applications.

Before we can introduce this sort of change, we would need to introduce
an IMA measurement list version.  Perhaps its time to define an IMA
security critical-data record, which would include this and other
information.  The measurement list itself would need to include a
version number.
Guozihua (Scott) Aug. 18, 2023, 1:25 a.m. UTC | #2
Hi Mimi,

On 2023/8/17 22:19, Mimi Zohar wrote:
> Hi Scott,
> 
> This patch changes the TPM PCR hash algorithm and value in the IMA
> measurement list.  The Subject line doesn't convey that.
> 
> On Thu, 2023-08-17 at 14:13 +0800, GUO Zihua wrote:
>> TPM2 chips supports algorithms other than SHA1. However, the original
>> IMA design hardcode template hash to be SHA1.
> 
> True, IMA initially calculated and extended a SHA1 hash into the TPM,
> but Roberto addressed that years ago.  Refer to commit  1ea973df6e21
> ("ima: Calculate and extend PCR with digests in ima_template_entry").
> 
> IMA now calculates and extends each of the enabled TPM banks with the
> appropriate hash value.  The PCR value in the IMA measurement list
> remains SHA1.  Attestation servers can first verify the SHA1 template
> hash as stored in the measurement list.  Then it can walk the IMA
> measurement list calculating the template data hash based on the per
> TPM bank algorithm to verify the TPM bank PCR value. 

Yeah I noticed that. However the fact that only SHA1 digest is included
in the measurement list means that the file-data hash and file hint is
effectively "protected" by just a SHA1 digest, and at the end of the day
for remote attestation only SHA1 digest is available.

> 
>>
>> This patch added CONFIG_IMA_TEMPLATE_HASH as well as ima_tpm_hash=
>> cmdline argument for configurating template hash. The usage is simuliar
>> to CONFIG_IMA_DEFAULT_HASH and ima_hash=. The configured hash is checked
>> against TPM and make sure that the hash algorithm is supported by
>> ima_tpm_chip.
>>
>> To accommodate the change, we must put a digest length into binary
>> measurement list items. The binary measurement list item format is
>> changed to this:
>> 	16bit-le=pcr#
>> 	16bit-le=template digest size
>> 	char[n]=template digest
>> 	32bit-le=template name size
>> 	char[n]=template name
>> 	[eventdata length]
>> 	eventdata[n]=template specific data
>> The first element is now a 16bit pcr number and a 16bit template digest
>> size, instead of the original 32bit pcr number.
>>
>> The format of ascii_measurement_list is also changed. For sha1 template
>> hash, the format is the same as before. For other hash algorithms, a
>> hash name is prepended as such:
>> "sha256:30ee3e25620478759600be00e06fda7b4fe23bbf575621d480400d536cf54f5b"
>> Signed-off-by: GUO Zihua <guozihua@huawei.com>
> 
> Other proposals have changed the hard coded hash algorithm and PCR
> value from SHA1 to SHA256.  Both that proposal and this will break
> existing userspace applications.

This is the part I would like to "RFC" on, and thanks for the comment!
In deed this change should break userspace as well as all the existing
remote attestation implementation. It should be better to have a brand
new file for this.
> 
> Before we can introduce this sort of change, we would need to introduce
> an IMA measurement list version.  Perhaps its time to define an IMA
> security critical-data record, which would include this and other
> information.  The measurement list itself would need to include a
> version number.
> 
I guess one of the easy way to do it is to make a
ascii_runtime_measurements_ng and binary_runtime_measurements_ng, which
contains a changed template supporting configurable template hash. What
do you think?
Mimi Zohar Aug. 18, 2023, 11:17 p.m. UTC | #3
On Fri, 2023-08-18 at 09:25 +0800, Guozihua (Scott) wrote:
> On 2023/8/17 22:19, Mimi Zohar wrote:
> > On Thu, 2023-08-17 at 14:13 +0800, GUO Zihua wrote:
[...]
 
> > Other proposals have changed the hard coded hash algorithm and PCR
> > value from SHA1 to SHA256.  Both that proposal and this will break
> > existing userspace applications.
> 
> This is the part I would like to "RFC" on, and thanks for the comment!

Another proposal included all of the enabled TPM bank digests.

> In deed this change should break userspace as well as all the existing
> remote attestation implementation. It should be better to have a brand
> new file for this.

True SHA1 is being phased out due to hash collisions.  Verifying the
template data hash against the template data isn't necessary for the
attestation server to verify a TPM quote against any of the enabled TPM
banks.  The attestation server walks the measurement list calculating
the bank specific template data hash.  Breaking existing applications
is unreasonable.

> > 
> > Before we can introduce this sort of change, we would need to introduce
> > an IMA measurement list version.  Perhaps its time to define an IMA
> > security critical-dbata record, which would include this and other
> > information.  The measurement list itself would need to include a
> > version number.
> > 
> I guess one of the easy way to do it is to make a
> ascii_runtime_measurements_ng and binary_runtime_measurements_ng, which
> contains a changed template supporting configurable template hash. What
> do you think?

Defining additional pseudo filesystems would allow both the old and new
measurement list formats to be enabled at the same time.
Guozihua (Scott) Aug. 19, 2023, 8:43 a.m. UTC | #4
On 2023/8/19 7:17, Mimi Zohar wrote:
> On Fri, 2023-08-18 at 09:25 +0800, Guozihua (Scott) wrote:
>> On 2023/8/17 22:19, Mimi Zohar wrote:
>>> On Thu, 2023-08-17 at 14:13 +0800, GUO Zihua wrote:
> [...]
>  
>>> Other proposals have changed the hard coded hash algorithm and PCR
>>> value from SHA1 to SHA256.  Both that proposal and this will break
>>> existing userspace applications.
>>
>> This is the part I would like to "RFC" on, and thanks for the comment!
> 
> Another proposal included all of the enabled TPM bank digests.
> 
>> In deed this change should break userspace as well as all the existing
>> remote attestation implementation. It should be better to have a brand
>> new file for this.
> 
> True SHA1 is being phased out due to hash collisions.  Verifying the
> template data hash against the template data isn't necessary for the
> attestation server to verify a TPM quote against any of the enabled TPM
> banks.  The attestation server walks the measurement list calculating
> the bank specific template data hash.  Breaking existing applications
> is unreasonable.

I get what you mean. Attestation could still extract result of the other
PCR and do the verification ignoring the SHA1 hash in the measurement list.
> 
>>>
>>> Before we can introduce this sort of change, we would need to introduce
>>> an IMA measurement list version.  Perhaps its time to define an IMA
>>> security critical-dbata record, which would include this and other
>>> information.  The measurement list itself would need to include a
>>> version number.
>>>
>> I guess one of the easy way to do it is to make a
>> ascii_runtime_measurements_ng and binary_runtime_measurements_ng, which
>> contains a changed template supporting configurable template hash. What
>> do you think?
> 
> Defining additional pseudo filesystems would allow both the old and new
> measurement list formats to be enabled at the same time.
> 
Something like an IMAfs? Or maybe showing different format based on
flags when file is opened.
Ken Goldman Aug. 28, 2023, 8:35 p.m. UTC | #5
On 8/17/2023 2:13 AM, GUO Zihua wrote:
> TPM2 chips supports algorithms other than SHA1. However, the original
> IMA design hardcode template hash to be SHA1.
> 
> This patch added CONFIG_IMA_TEMPLATE_HASH as well as ima_tpm_hash=
> cmdline argument for configurating template hash. The usage is simuliar
> to CONFIG_IMA_DEFAULT_HASH and ima_hash=. The configured hash is checked
> against TPM and make sure that the hash algorithm is supported by
> ima_tpm_chip.
> 
> To accommodate the change, we must put a digest length into binary
> measurement list items. The binary measurement list item format is
> changed to this:
> 	16bit-le=pcr#
> 	16bit-le=template digest size
> 	char[n]=template digest
> 	32bit-le=template name size
> 	char[n]=template name
> 	[eventdata length]
> 	eventdata[n]=template specific data
> The first element is now a 16bit pcr number and a 16bit template digest
> size, instead of the original 32bit pcr number.
> 
> The format of ascii_measurement_list is also changed. For sha1 template
> hash, the format is the same as before. For other hash algorithms, a
> hash name is prepended as such:
> "sha256:30ee3e25620478759600be00e06fda7b4fe23bbf575621d480400d536cf54f5b"

I would not change the PCR handle to 16 bits.  The TPM supports NVRAM
based PCRs, and their handles would be 0x01xxxxxx. In the future, there
may be other 'first byte' values.

A template digest size does not describe the digest algorithm.  E.g.,
SM3 and SHA-256 are both 32 bytes.

If one wants to describe the digest algorithm in 2 bytes, a reasonable 
choice would be the values in the TCG Algorithm registry.  Se TPM Spec 
Part 2 Table 9 — Definition of (UINT16) TPM_ALG_ID Constants <IN/OUT, S>

E.g., SHA-256 is 000b and SM3 is 0012.
Ken Goldman Aug. 28, 2023, 9:05 p.m. UTC | #6
I'd like to present three possible cases, expanding on Mimi's
observation that the template hash is currently the hash of the
template data.

My vote is #2, but all have merits.

----------

1. Leave the SHA-1 template hash.

A. This does not break existing applications.

B. The template data is protected by the TPM quote, which does not
have to be SHA-1.

C. The SHA-1 digest provides some debug usefulness against a
non-malicious alteration of the template data. The application can
report which event record is incorrect.

----------

2. Include template hashes for all PCR banks.

A. This breaks existing applications on the attestor side, but could
be made backward compatible / deprecated at the verifier side.

B. The redundant data is an attack surface, in that the verifier must
remember to check the hashes against the quote AND against the
template data.

C. The digest provides debug usefulness against malicious attacks on
the template data.

D. This permits the use case where the template hash is NOT a hash of
the template data. In the UEFI event log (using IMA terms), the
template hash can be a digest of some other data and the template data
is a hint as to where and what that data is.

E.g., the UEFI event EV_CPU_MICROCODE template data field has a patch
descriptor, while the template hash is a digest of the patch itself.

----------

3. Include a template hash for the strongest hash algorithm.

A. It's not always clear what the strongest algorithm is.

Otherwise, this is the same as #2.

On 8/18/2023 7:17 PM, Mimi Zohar wrote:
> On Fri, 2023-08-18 at 09:25 +0800, Guozihua (Scott) wrote:
>> On 2023/8/17 22:19, Mimi Zohar wrote:
>>> On Thu, 2023-08-17 at 14:13 +0800, GUO Zihua wrote:
> 
> True SHA1 is being phased out due to hash collisions.  Verifying the
> template data hash against the template data isn't necessary for the
> attestation server to verify a TPM quote against any of the enabled TPM
> banks.  The attestation server walks the measurement list calculating
> the bank specific template data hash.  Breaking existing applications
> is unreasonable.
Ken Goldman Aug. 29, 2023, 2:09 p.m. UTC | #7
On 8/17/2023 10:19 AM, Mimi Zohar wrote:
> Before we can introduce this sort of change, we would need to introduce
> an IMA measurement list version.  Perhaps its time to define an IMA
> security critical-data record, which would include this and other
> information.  The measurement list itself would need to include a
> version number.

FYI, the TCG specified UEFI event log has a standard first record.  The 
events themselves do not have a version number.

That first record is itself versioned, so it can change over time.

The record has various version numbers, a count of PCR banks, a mapping 
from algorithm number to algorithm size, and a sized vendor specific area.
Guozihua (Scott) Aug. 30, 2023, 9:14 a.m. UTC | #8
On 2023/8/19 7:17, Mimi Zohar wrote:
> On Fri, 2023-08-18 at 09:25 +0800, Guozihua (Scott) wrote:
>> On 2023/8/17 22:19, Mimi Zohar wrote:
>>> On Thu, 2023-08-17 at 14:13 +0800, GUO Zihua wrote:
> [...]
>  
>>> Other proposals have changed the hard coded hash algorithm and PCR
>>> value from SHA1 to SHA256.  Both that proposal and this will break
>>> existing userspace applications.
>>
>> This is the part I would like to "RFC" on, and thanks for the comment!
> 
> Another proposal included all of the enabled TPM bank digests.
Will this introduce some performance issue? I don't think we should be
calculating various hashes on the same thing again and again.

If we are feeding digests for all slots of the same PCR bank, we could
do a cut-down or padding on the banks we don't care, avoid unnecessary
hash operations.
> 
>> In deed this change should break userspace as well as all the existing
>> remote attestation implementation. It should be better to have a brand
>> new file for this.
> 
> True SHA1 is being phased out due to hash collisions.  Verifying the
> template data hash against the template data isn't necessary for the
> attestation server to verify a TPM quote against any of the enabled TPM
> banks.  The attestation server walks the measurement list calculating
> the bank specific template data hash.  Breaking existing applications
> is unreasonable.
> 
>>>
>>> Before we can introduce this sort of change, we would need to introduce
>>> an IMA measurement list version.  Perhaps its time to define an IMA
>>> security critical-dbata record, which would include this and other
>>> information.  The measurement list itself would need to include a
>>> version number.
>>>
>> I guess one of the easy way to do it is to make a
>> ascii_runtime_measurements_ng and binary_runtime_measurements_ng, which
>> contains a changed template supporting configurable template hash. What
>> do you think?
> 
> Defining additional pseudo filesystems would allow both the old and new
> measurement list formats to be enabled at the same time.
>
Guozihua (Scott) Aug. 30, 2023, 9:32 a.m. UTC | #9
On 2023/8/29 5:05, Ken Goldman wrote:
> I'd like to present three possible cases, expanding on Mimi's
> observation that the template hash is currently the hash of the
> template data.
> 
> My vote is #2, but all have merits.
> 
> ----------
> 
> 1. Leave the SHA-1 template hash.
> 
> A. This does not break existing applications.
> 
> B. The template data is protected by the TPM quote, which does not
> have to be SHA-1.
> 
> C. The SHA-1 digest provides some debug usefulness against a
> non-malicious alteration of the template data. The application can
> report which event record is incorrect.

What the verifier can't do as of now is to tell which file has been
corrupted, given the attacker managed to achieve a SHA-1 collision while
keeping the measurement items reasonable.

Even worse, as the TPM quote contains only an accumulated digest, there
leaves a chance for the attacker to "fix" the TPM digest by generating
additional measurement items. Without the ability to verify each and
every individual measurement item verifier could produce a false
negative result.
> 
> ----------
> 
> 2. Include template hashes for all PCR banks.
> 
> A. This breaks existing applications on the attestor side, but could
> be made backward compatible / deprecated at the verifier side.
> 
> B. The redundant data is an attack surface, in that the verifier must
> remember to check the hashes against the quote AND against the
> template data.
> 
> C. The digest provides debug usefulness against malicious attacks on
> the template data.
> 
> D. This permits the use case where the template hash is NOT a hash of
> the template data. In the UEFI event log (using IMA terms), the
> template hash can be a digest of some other data and the template data
> is a hint as to where and what that data is.
> 
> E.g., the UEFI event EV_CPU_MICROCODE template data field has a patch
> descriptor, while the template hash is a digest of the patch itself.

As of now, IMA stores template hash with it's measurement item in the
memory, and storing digest value from all banks would make the digest
list N times bigger, where N would be an increasing number of supported
algorithms by TPM.
> 
> ----------
> 
> 3. Include a template hash for the strongest hash algorithm.
> 
> A. It's not always clear what the strongest algorithm is.
> 
> Otherwise, this is the same as #2.

"strongest" algorithm does not exist. I think it would be better to find
a extensible solution allowing for future upgrade.
> 
> On 8/18/2023 7:17 PM, Mimi Zohar wrote:
>> On Fri, 2023-08-18 at 09:25 +0800, Guozihua (Scott) wrote:
>>> On 2023/8/17 22:19, Mimi Zohar wrote:
>>>> On Thu, 2023-08-17 at 14:13 +0800, GUO Zihua wrote:
>>
>> True SHA1 is being phased out due to hash collisions.  Verifying the
>> template data hash against the template data isn't necessary for the
>> attestation server to verify a TPM quote against any of the enabled TPM
>> banks.  The attestation server walks the measurement list calculating
>> the bank specific template data hash.  Breaking existing applications
>> is unreasonable.
> 
>
Guozihua (Scott) Aug. 30, 2023, 9:38 a.m. UTC | #10
On 2023/8/29 4:35, Ken Goldman wrote:
> On 8/17/2023 2:13 AM, GUO Zihua wrote:
>> TPM2 chips supports algorithms other than SHA1. However, the original
>> IMA design hardcode template hash to be SHA1.
>>
>> This patch added CONFIG_IMA_TEMPLATE_HASH as well as ima_tpm_hash=
>> cmdline argument for configurating template hash. The usage is simuliar
>> to CONFIG_IMA_DEFAULT_HASH and ima_hash=. The configured hash is checked
>> against TPM and make sure that the hash algorithm is supported by
>> ima_tpm_chip.
>>
>> To accommodate the change, we must put a digest length into binary
>> measurement list items. The binary measurement list item format is
>> changed to this:
>>     16bit-le=pcr#
>>     16bit-le=template digest size
>>     char[n]=template digest
>>     32bit-le=template name size
>>     char[n]=template name
>>     [eventdata length]
>>     eventdata[n]=template specific data
>> The first element is now a 16bit pcr number and a 16bit template digest
>> size, instead of the original 32bit pcr number.
>>
>> The format of ascii_measurement_list is also changed. For sha1 template
>> hash, the format is the same as before. For other hash algorithms, a
>> hash name is prepended as such:
>> "sha256:30ee3e25620478759600be00e06fda7b4fe23bbf575621d480400d536cf54f5b"
> 
> I would not change the PCR handle to 16 bits.  The TPM supports NVRAM
> based PCRs, and their handles would be 0x01xxxxxx. In the future, there
> may be other 'first byte' values.
> 
> A template digest size does not describe the digest algorithm.  E.g.,
> SM3 and SHA-256 are both 32 bytes.

Oops that's a miss. We would need a brand new format for this I guess.
> 
> If one wants to describe the digest algorithm in 2 bytes, a reasonable
> choice would be the values in the TCG Algorithm registry.  Se TPM Spec
> Part 2 Table 9 — Definition of (UINT16) TPM_ALG_ID Constants <IN/OUT, S>
> 
> E.g., SHA-256 is 000b and SM3 is 0012.
>
Mimi Zohar Aug. 30, 2023, 12:27 p.m. UTC | #11
On Wed, 2023-08-30 at 17:14 +0800, Guozihua (Scott) wrote:
> On 2023/8/19 7:17, Mimi Zohar wrote:
> > On Fri, 2023-08-18 at 09:25 +0800, Guozihua (Scott) wrote:
> >> On 2023/8/17 22:19, Mimi Zohar wrote:
> >>> On Thu, 2023-08-17 at 14:13 +0800, GUO Zihua wrote:
> > [...]
> >  
> >>> Other proposals have changed the hard coded hash algorithm and PCR
> >>> value from SHA1 to SHA256.  Both that proposal and this will break
> >>> existing userspace applications.
> >>
> >> This is the part I would like to "RFC" on, and thanks for the comment!
> > 
> > Another proposal included all of the enabled TPM bank digests.
> Will this introduce some performance issue? I don't think we should be
> calculating various hashes on the same thing again and again.

Per TPM bank specific hashes are already being calculated and extended
into the TPM banks.  Refer to  ima_calc_field_array_hash().
Ken Goldman Aug. 30, 2023, 7:26 p.m. UTC | #12
On 8/30/2023 5:32 AM, Guozihua (Scott) wrote:
> On 2023/8/29 5:05, Ken Goldman wrote:
>> I'd like to present three possible cases, expanding on Mimi's
>> observation that the template hash is currently the hash of the
>> template data.
>>
>> My vote is #2, but all have merits.
>>
>> ----------
>>
>> 1. Leave the SHA-1 template hash.
>>
>> A. This does not break existing applications.
>>
>> B. The template data is protected by the TPM quote, which does not
>> have to be SHA-1.
>>
>> C. The SHA-1 digest provides some debug usefulness against a
>> non-malicious alteration of the template data. The application can
>> report which event record is incorrect.
> 
> What the verifier can't do as of now is to tell which file has been
> corrupted, given the attacker managed to achieve a SHA-1 collision while
> keeping the measurement items reasonable.

That's why I said "non-malicious".  A collision in that case is unlikely.

It's just a debug aid. The quote failed.  Including a SHA-1 hash
can help in software debug.

> 
> Even worse, as the TPM quote contains only an accumulated digest, there
> leaves a chance for the attacker to "fix" the TPM digest by generating
> additional measurement items. Without the ability to verify each and
> every individual measurement item verifier could produce a false
> negative result.

The quote uses SHA-256 or better, which the attacker cannot fix.  In #1, 
the SHA-1 hash is for debug, not for security.

>>
>> ----------
>>
>> 2. Include template hashes for all PCR banks.
>>
>> A. This breaks existing applications on the attestor side, but could
>> be made backward compatible / deprecated at the verifier side.
>>
>> B. The redundant data is an attack surface, in that the verifier must
>> remember to check the hashes against the quote AND against the
>> template data.
>>
>> C. The digest provides debug usefulness against malicious attacks on
>> the template data.
>>
>> D. This permits the use case where the template hash is NOT a hash of
>> the template data. In the UEFI event log (using IMA terms), the
>> template hash can be a digest of some other data and the template data
>> is a hint as to where and what that data is.
>>
>> E.g., the UEFI event EV_CPU_MICROCODE template data field has a patch
>> descriptor, while the template hash is a digest of the patch itself.
> 
> As of now, IMA stores template hash with it's measurement item in the
> memory, and storing digest value from all banks would make the digest
> list N times bigger, where N would be an increasing number of supported
> algorithms by TPM.

N  is not the number of supported algorithms. It's the number of 
allocated PCR banks.

E.g., a typical TPM might support SHA-1, 256, 384, but only allocate 
SHA-256 and SHA-384.  The IMA log would hold those two.

In addition, while the digest list may be N times bigger, the record is 
not, because most of the record is the template name, template data, ...

>>
>> ----------
>>
>> 3. Include a template hash for the strongest hash algorithm.
>>
>> A. It's not always clear what the strongest algorithm is.
>>
>> Otherwise, this is the same as #2.
> 
> "strongest" algorithm does not exist. I think it would be better to find
> a extensible solution allowing for future upgrade.

Sometimes yes (SHA-256 vs. SHA-384).  I agree that A. is the drawback of 
#3. I voted for #2.
diff mbox series

Patch

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 647a55fcba27..33ab98d52062 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1987,6 +1987,13 @@ 
 			The list of supported hash algorithms is defined
 			in crypto/hash_info.h.
 
+	ima_tpm_hash=	[IMA]
+			Format: { sha1 | sha256 | sha512 | ... }
+			default: "sha1"
+
+			The algorithm configured will be checked against the
+			supported algorithm of the TPM chip should it exists.
+
 	ima_policy=	[IMA]
 			The builtin policies to load during IMA setup.
 			Format: "tcb | appraise_tcb | secure_boot |
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index c17660bf5f34..b0e6da3ee257 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -121,9 +121,43 @@  config IMA_DEFAULT_HASH
 	default "sha1" if IMA_DEFAULT_HASH_SHA1
 	default "sha256" if IMA_DEFAULT_HASH_SHA256
 	default "sha512" if IMA_DEFAULT_HASH_SHA512
-	default "wp512" if IMA_DEFAULT_HASH_WP512
 	default "sm3" if IMA_DEFAULT_HASH_SM3
 
+choice
+	prompt "Default template hash algorithm"
+	default IMA_TEMPLATE_HASH_SHA1
+	depends on IMA
+	help
+	   Select the hash algorithm used for the template hash,
+	   and PCR extension.  The compiled default hash algorithm
+	   can be overwritten using the kernel command line
+	   'ima_tpm_hash=' option. The configured algorithm is
+	   checked against the TPM chip used.
+
+	config IMA_TEMPLATE_HASH_SHA1
+		bool "SHA1 (default)"
+		depends on CRYPTO_SHA1=y
+
+	config IMA_TEMPLATE_HASH_SHA256
+		bool "SHA256"
+		depends on CRYPTO_SHA256=y
+
+	config IMA_TEMPLATE_HASH_SHA512
+		bool "SHA512"
+		depends on CRYPTO_SHA512=y
+
+	config IMA_TEMPLATE_HASH_SM3
+		bool "SM3"
+		depends on CRYPTO_SM3_GENERIC=y
+endchoice
+
+config IMA_TEMPLATE_HASH
+	string
+	depends on IMA
+	default "sha1" if IMA_TEMPLATE_HASH_SHA1
+	default "sha256" if IMA_TEMPLATE_HASH_SHA256
+	default "sha512" if IMA_TEMPLATE_HASH_SHA512
+
 config IMA_WRITE_POLICY
 	bool "Enable multiple writes to the IMA policy"
 	depends on IMA
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index c29db699c996..a280062abbf8 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -51,8 +51,10 @@  extern atomic_t ima_setxattr_allowed_hash_algorithms;
 
 /* set during initialization */
 extern int ima_hash_algo __ro_after_init;
+extern int ima_tpm_algo __ro_after_init;
 extern int ima_sha1_idx __ro_after_init;
 extern int ima_hash_algo_idx __ro_after_init;
+extern int ima_tpm_algo_idx __ro_after_init;
 extern int ima_extra_slots __ro_after_init;
 extern int ima_appraise;
 extern struct tpm_chip *ima_tpm_chip;
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 51ad29940f05..0c748646d000 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -64,6 +64,7 @@  struct ima_algo_desc {
 
 int ima_sha1_idx __ro_after_init;
 int ima_hash_algo_idx __ro_after_init;
+int ima_tpm_algo_idx __ro_after_init;
 /*
  * Additional number of slots reserved, as needed, for SHA1
  * and IMA default algo.
@@ -122,22 +123,22 @@  int __init ima_init_crypto(void)
 	if (rc)
 		return rc;
 
-	ima_sha1_idx = -1;
 	ima_hash_algo_idx = -1;
+	ima_tpm_algo_idx = -1;
 
 	for (i = 0; i < NR_BANKS(ima_tpm_chip); i++) {
 		algo = ima_tpm_chip->allocated_banks[i].crypto_id;
-		if (algo == HASH_ALGO_SHA1)
-			ima_sha1_idx = i;
-
 		if (algo == ima_hash_algo)
 			ima_hash_algo_idx = i;
+
+		if (algo == ima_tpm_algo)
+			ima_tpm_algo_idx = i;
 	}
 
-	if (ima_sha1_idx < 0) {
-		ima_sha1_idx = NR_BANKS(ima_tpm_chip) + ima_extra_slots++;
+	if (ima_tpm_algo_idx < 0) {
+		ima_tpm_algo_idx = NR_BANKS(ima_tpm_chip) + ima_extra_slots++;
 		if (ima_hash_algo == HASH_ALGO_SHA1)
-			ima_hash_algo_idx = ima_sha1_idx;
+			ima_hash_algo_idx = ima_tpm_algo_idx;
 	}
 
 	if (ima_hash_algo_idx < 0)
@@ -160,6 +161,8 @@  int __init ima_init_crypto(void)
 
 		if (algo == ima_hash_algo) {
 			ima_algo_array[i].tfm = ima_shash_tfm;
+			if (ima_tpm_algo == ima_hash_algo)
+				ima_algo_array[i].tfm = ima_shash_tfm;
 			continue;
 		}
 
@@ -175,23 +178,24 @@  int __init ima_init_crypto(void)
 		}
 	}
 
-	if (ima_sha1_idx >= NR_BANKS(ima_tpm_chip)) {
-		if (ima_hash_algo == HASH_ALGO_SHA1) {
-			ima_algo_array[ima_sha1_idx].tfm = ima_shash_tfm;
+	if (ima_tpm_algo_idx >= NR_BANKS(ima_tpm_chip)) {
+		if (ima_tpm_algo == ima_hash_algo) {
+			ima_algo_array[ima_tpm_algo_idx].tfm = ima_shash_tfm;
 		} else {
-			ima_algo_array[ima_sha1_idx].tfm =
-						ima_alloc_tfm(HASH_ALGO_SHA1);
-			if (IS_ERR(ima_algo_array[ima_sha1_idx].tfm)) {
-				rc = PTR_ERR(ima_algo_array[ima_sha1_idx].tfm);
+			ima_algo_array[ima_tpm_algo_idx].tfm =
+						ima_alloc_tfm(ima_tpm_algo);
+			if (IS_ERR(ima_algo_array[ima_tpm_algo_idx].tfm)) {
+				rc = PTR_ERR(
+					ima_algo_array[ima_tpm_algo_idx].tfm);
 				goto out_array;
 			}
 		}
 
-		ima_algo_array[ima_sha1_idx].algo = HASH_ALGO_SHA1;
+		ima_algo_array[ima_tpm_algo_idx].algo = ima_tpm_algo;
 	}
 
 	if (ima_hash_algo_idx >= NR_BANKS(ima_tpm_chip) &&
-	    ima_hash_algo_idx != ima_sha1_idx) {
+	    ima_hash_algo_idx != ima_tpm_algo_idx) {
 		ima_algo_array[ima_hash_algo_idx].tfm = ima_shash_tfm;
 		ima_algo_array[ima_hash_algo_idx].algo = ima_hash_algo;
 	}
@@ -630,26 +634,25 @@  int ima_calc_field_array_hash(struct ima_field_data *field_data,
 	u16 alg_id;
 	int rc, i;
 
-	rc = ima_calc_field_array_hash_tfm(field_data, entry, ima_sha1_idx);
+	rc = ima_calc_field_array_hash_tfm(field_data, entry, ima_tpm_algo_idx);
 	if (rc)
 		return rc;
 
-	entry->digests[ima_sha1_idx].alg_id = TPM_ALG_SHA1;
-
 	for (i = 0; i < NR_BANKS(ima_tpm_chip) + ima_extra_slots; i++) {
-		if (i == ima_sha1_idx)
-			continue;
 
 		if (i < NR_BANKS(ima_tpm_chip)) {
 			alg_id = ima_tpm_chip->allocated_banks[i].alg_id;
 			entry->digests[i].alg_id = alg_id;
 		}
 
-		/* for unmapped TPM algorithms digest is still a padded SHA1 */
+		if (i == ima_tpm_algo_idx)
+			continue;
+
+		/* for unmapped TPM algorithms digest full length digest */
 		if (!ima_algo_array[i].tfm) {
 			memcpy(entry->digests[i].digest,
-			       entry->digests[ima_sha1_idx].digest,
-			       TPM_DIGEST_SIZE);
+			       entry->digests[ima_tpm_algo_idx].digest,
+			       TPM_MAX_DIGEST_SIZE);
 			continue;
 		}
 
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index cd1683dad3bf..036eb80c338b 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -117,8 +117,9 @@  void ima_putc(struct seq_file *m, void *data, int datalen)
 }
 
 /* print format:
- *       32bit-le=pcr#
- *       char[20]=template digest
+ *       16bit-le=pcr#
+ *       16bit-le=template digest size
+ *       char[n]=template digest
  *       32bit-le=template name size
  *       char[n]=template name
  *       [eventdata length]
@@ -130,7 +131,8 @@  int ima_measurements_show(struct seq_file *m, void *v)
 	struct ima_queue_entry *qe = v;
 	struct ima_template_entry *e;
 	char *template_name;
-	u32 pcr, namelen, template_data_len; /* temporary fields */
+	u32 namelen, template_data_len; /* temporary fields */
+	u16 pcr, template_hash_len;
 	bool is_ima_template = false;
 	int i;
 
@@ -147,21 +149,30 @@  int ima_measurements_show(struct seq_file *m, void *v)
 	 * PCR used defaults to the same (config option) in
 	 * little-endian format, unless set in policy
 	 */
-	pcr = !ima_canonical_fmt ? e->pcr : (__force u32)cpu_to_le32(e->pcr);
-	ima_putc(m, &pcr, sizeof(e->pcr));
-
-	/* 2nd: template digest */
-	ima_putc(m, e->digests[ima_sha1_idx].digest, TPM_DIGEST_SIZE);
-
-	/* 3rd: template name size */
+	pcr = !ima_canonical_fmt ? (u16)e->pcr :
+				   (__force u16)cpu_to_le16(e->pcr);
+	ima_putc(m, &pcr, sizeof(pcr));
+
+	/* 2nd: template digest size */
+	template_hash_len = (u16)hash_digest_size[ima_tpm_algo];
+	if (ima_canonical_fmt)
+		template_hash_len =
+			(__force u16)cpu_to_le16(template_hash_len);
+	ima_putc(m, &template_hash_len, sizeof(template_hash_len));
+
+	/* 3rd: template digest */
+	ima_putc(m, e->digests[ima_tpm_algo_idx].digest,
+		 hash_digest_size[ima_tpm_algo]);
+
+	/* 4th: template name size */
 	namelen = !ima_canonical_fmt ? strlen(template_name) :
 		(__force u32)cpu_to_le32(strlen(template_name));
 	ima_putc(m, &namelen, sizeof(namelen));
 
-	/* 4th:  template name */
+	/* 5th:  template name */
 	ima_putc(m, template_name, strlen(template_name));
 
-	/* 5th:  template length (except for 'ima' template) */
+	/* 6th:  template length (except for 'ima' template) */
 	if (strcmp(template_name, IMA_TEMPLATE_IMA_NAME) == 0)
 		is_ima_template = true;
 
@@ -171,7 +182,7 @@  int ima_measurements_show(struct seq_file *m, void *v)
 		ima_putc(m, &template_data_len, sizeof(e->template_data_len));
 	}
 
-	/* 6th:  template specific data */
+	/* 7th:  template specific data */
 	for (i = 0; i < e->template_desc->num_fields; i++) {
 		enum ima_show_type show = IMA_SHOW_BINARY;
 		const struct ima_template_field *field =
@@ -233,8 +244,12 @@  static int ima_ascii_measurements_show(struct seq_file *m, void *v)
 	/* 1st: PCR used (config option) */
 	seq_printf(m, "%2d ", e->pcr);
 
+	if (ima_tpm_algo != HASH_ALGO_SHA1)
+		seq_printf(m, "%s:", hash_algo_name[ima_tpm_algo]);
+
 	/* 2nd: SHA1 template hash */
-	ima_print_digest(m, e->digests[ima_sha1_idx].digest, TPM_DIGEST_SIZE);
+	ima_print_digest(m, e->digests[ima_tpm_algo_idx].digest,
+			 hash_digest_size[ima_tpm_algo]);
 
 	/* 3th:  template name */
 	seq_printf(m, " %s", template_name);
diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index 63979aefc95f..cc54229cb5ae 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -23,6 +23,40 @@ 
 /* name for boot aggregate entry */
 const char boot_aggregate_name[] = "boot_aggregate";
 struct tpm_chip *ima_tpm_chip;
+static int ima_tpm_algo_tmp = -1;
+
+static bool __init ima_tpm_support_algo(int algo)
+{
+	int j;
+
+	if (algo < 0 || algo > HASH_ALGO__LAST)
+		return false;
+
+	for (j = 0; j < NR_BANKS(ima_tpm_chip); j++) {
+		if (ima_tpm_chip->allocated_banks[j].crypto_id == algo)
+			return true;
+	}
+	return false;
+}
+
+/*
+ * This funciton setup hash algorithm used by TPM extension.
+ * The hash algorithm is checked against supported algorithms
+ * of the chip.
+ *
+ * This function would be called by cmdline handling and ima_init.
+ * A possible situation is when the first time this is called,
+ * ima_tpm_chip has not been initialized yet.
+ */
+static int __init tpm_hash_setup(char *str)
+{
+	ima_tpm_algo_tmp = match_string(hash_algo_name, HASH_ALGO__LAST, str);
+	if (ima_tpm_algo_tmp < 0)
+		pr_err("invalid template hash algorithm \"%s\"", str);
+
+	return 1;
+}
+__setup("ima_tpm_hash=", tpm_hash_setup);
 
 /* Add the boot aggregate to the IMA measurement list and extend
  * the PCR register.
@@ -117,9 +151,17 @@  int __init ima_init(void)
 {
 	int rc;
 
+	if (ima_tpm_algo_tmp < 0)
+		tpm_hash_setup(CONFIG_IMA_TEMPLATE_HASH);
+
 	ima_tpm_chip = tpm_default_chip();
-	if (!ima_tpm_chip)
+	if (!ima_tpm_chip) {
 		pr_info("No TPM chip found, activating TPM-bypass!\n");
+		ima_tpm_algo = ima_tpm_algo_tmp;
+	} else if (ima_tpm_support_algo(ima_tpm_algo_tmp))
+		ima_tpm_algo = ima_tpm_algo_tmp;
+	else
+		ima_tpm_algo = HASH_ALGO_SHA1;
 
 	rc = integrity_init_keyring(INTEGRITY_KEYRING_IMA);
 	if (rc)
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 365db0e43d7c..fa4f6c261cd7 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -35,6 +35,7 @@  int ima_appraise;
 #endif
 
 int __ro_after_init ima_hash_algo = HASH_ALGO_SHA1;
+int __ro_after_init ima_tpm_algo = HASH_ALGO_SHA1;
 static int hash_setup_done;
 
 static struct notifier_block ima_lsm_policy_notifier = {