Weak hash algorithms allowed with DIGEST_NG

Weak hash algorithms allowed with DIGEST_NG

[THOBY Simon]

Hello,
first of all I'm sorry if this isn't the best mailing list to discuss this, but the "users" mailing list on sourceforge seems dead since around 2017. Maybe there is some other mailing I missed ?

When using evmctl and IMA-EVM in hash+HMAC mode (no digital signatures involved), I was suprised to see that IMA didn't complain if a file was hashed with an algorithm "weaker" than the one specified on the command line.
Of course I suppose EVM should stop downgrade attacks (by that I mean an offline attacker changing the hash of a legitimate file from sha256 to sha1 or md5). However, if files were already hashed as md5 - or more probably sha1 because that is still the default value for evmctl - then the attacker could potentially perform collision attacks against the weakly hashed file. The user may believe himself protected against collision attacks because of the 'ima_hash=sha256' command line parameter (with or without 'ima_template=ima-ng').

Here is an example on a CentOS 8 box with a vanilla 5.13 kernel:

[root@localhost ~]# cat /proc/cmdline
root=UUID=xxxxxxxx ro rd.luks.uuid=luks-xxxxxxxx  ima_hash=sha256 ima_template=ima-ng
[root@localhost ~]# cat /sys/kernel/security/ima/policy
dont_measure fsmagic=0x9fa0
[...]
dont_appraise fsmagic=0x6e736673
appraise func=BPRM_CHECK fowner=0
appraise func=BPRM_CHECK euid=0
appraise func=MMAP_CHECK mask=MAY_EXEC fowner=0
appraise func=MMAP_CHECK mask=MAY_EXEC euid=0
appraise func=MODULE_CHECK
appraise func=FIRMWARE_CHECK
appraise func=POLICY_CHECK
[root@localhost ~]# cat > /tmp/a.c <<EOF
int main() {
printf("helo\n");
}
EOF
[root@localhost ~]# gcc /tmp/a.c -o testfile
[root@localhost ~]# evmctl ima_hash -a md5 -d testfile
hash(md5): 0108f04837b9336d40b8ec4f78df320143
[root@localhost ~]# getfattr -e hex -m . -d testfile
# file: testfile
security.evm=0x02a0cb5aab84522b5bc9d662998786676ad27ad142
security.ima=0x0108f04837b9336d40b8ec4f78df320143
security.selinux=0x756e636f6e66696e65645f753a6f626a6563745f723a61646d696e5f686f6d655f743a733000
[root@localhost ~]# ./testfile
helo

By the size if the security.ima xattr (smaller than a sha1 hash), the value of the first byte (0x1 so this is a DIGEST, not a DIGEST_NG) and the fact that nothing was pushed to the audit log, we can confirm that md5 is truly used for validation.

In addition, I am sure that IMA is enabled and working, as can be shown in this example where I craft an invalid IMA xattr):

[root@localhost ~]# cat /sys/kernel/security/evm
1[root@localhost ~]#
[root@localhost ~]# setfattr -n security.ima -v 0x01aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa testfile
[root@localhost ~]# getfattr -e hex -m . -d testfile
# file: testfile
security.evm=0x028fa94dc0950002fdfb2674ae94a7677fc5fb4ff4
security.ima=0x01aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
security.selinux=0x756e636f6e66696e65645f753a6f626a6563745f723a61646d696e5f686f6d655f743a733000
[root@localhost ~]# ./testfile
-bash: ./testfile: Permission denied
and I get the expected audti log message:
type=INTEGRITY_DATA msg=audit(...): pid=1428 uid=0 auid=0 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=appraise_data cause=invalid-hash comm="bash" name="/root/testfile" dev="dm-0" ino=2224735 res=0 errno=0UID="root" AUID="root"

So I took a quick glance at kernel source code and indeed, when looking at ima_get_hash_algo (in security/integrity/ima/ima_appraise.c), there is no validation for the algorithm used:
	case IMA_XATTR_DIGEST_NG:
		/* first byte contains algorithm id */
		ret = xattr_value->data[0];
		if (ret < HASH_ALGO__LAST)
			return ret;
		break;

That hash is then used as-is in process_measurement (in security/integrity/ima/ima_main.c):
	hash_algo = ima_get_hash_algo(xattr_value, xattr_len);

	rc = ima_collect_measurement(iint, file, buf, size, hash_algo, modsig);

ima_collect_measurement (defined in security/integrity/ima/ima_api.c) then proceeds to use the hash algorithm without further ado:
	hash.hdr.algo = algo;

	/* Initialize hash digest to 0's in case of failure */
	memset(&hash.digest, 0, sizeof(hash.digest));

	if (buf)
		result = ima_calc_buffer_hash(buf, size, &hash.hdr);
	else
		result = ima_calc_file_hash(file, &hash.hdr);

To confirm that there was no validation, I took what appeared to be the weakest hash to my non-cryptographer eyes, MD4, and I decided to hash my file wiht it.
It appears evmctl was not really conceived to use md4 (and it shouldn't be either, so all is well), and we need to apply a tiny trick here:

[root@localhost ~]# evmctl ima_hash -a md4 -d testfile
hash(md4): 013785074a0decdf72f189fce55ff3c743
# We need to drop the 01 prefix (that indicated that the IMA type is DIGEST) because the DIGEST type uses the digest size to determine the algorithme, and only supports SHA1 and MD5.
# To use MD4, we must use the newer digest type (digest_ng instead of digest) to use the oldest algorithm. I dare say this made me smile a bit.
# We will prepend 04 (DIGEST_NG) and the index of the target hash algorithm in the hash_algo_name list: 00 because HASH_ALGO_MD4 is the first in the list (see crypto/hash_info.c).
# This gives 013785074a0decdf72f189fce55ff3c743 -> 3785074a0decdf72f189fce55ff3c743 -> 04003785074a0decdf72f189fce55ff3c743
[root@localhost ~]# setfattr -n security.ima -v 0x04003785074a0decdf72f189fce55ff3c743 testfile
[root@localhost ~]# getfattr -e hex -m . -d testfile
# file: testfile
security.evm=0x02aaa230ee23a3d82169c346711eea31573c861348
security.ima=0x04003785074a0decdf72f189fce55ff3c743
security.selinux=0x756e636f6e66696e65645f753a6f626a6563745f723a61646d696e5f686f6d655f743a733000
[root@localhost ~]# ./testfile
helo

Is there any way to enforce the use of the hash specified in the 'ima_hash' cmdline parameter ?
I couldn't find any glancing at the code, but I didn't read all of it and I understood even less, so I secretly hope to have missed a small yet critical check/option.
And if there is no such way, would you be opposed to a patch adding an option (something like 'ima_enforce_hash_alg') that only allows digest hashed with the values supplied in the 'ima_hash' parameter ?


Thanks for your consideration,
Have a nice day,
Simon Thoby

Re: Weak hash algorithms allowed with DIGEST_NG

[Mimi Zohar]

Hi Simon,

On Wed, 2021-07-07 at 09:54 +0000, THOBY Simon wrote:

<snip>

> When using evmctl and IMA-EVM in hash+HMAC mode (no digital
> signatures involved), I was suprised to see that IMA didn't complain
> if a file was hashed with an algorithm "weaker" than the one
> specified on the command line.
> Of course I suppose EVM should stop downgrade attacks (by that I mean
> an offline attacker changing the hash of a legitimate file from
> sha256 to sha1 or md5). However, if files were already hashed as md5
> - or more probably sha1 because that is still the default value for
> evmctl - then the attacker could potentially perform collision
> attacks against the weakly hashed file. The user may believe himself
> protected against collision attacks because of the 'ima_hash=sha256'
> command line parameter (with or without 'ima_template=ima-ng').

Before allowing the EVM HMAC to be updated, EVM verifies the existing
HMAC to protect against an offline attack.  It doesn't prevent online
changes.  Additional support to prevent crypto downgrade would need to
be added.

<snip>

> Is there any way to enforce the use of the hash specified in the
> 'ima_hash' cmdline parameter ?

The cmdline parameter overrides the compile time default hash algorithm
used for (re-)calculating the file hash.

> I couldn't find any glancing at the code, but I didn't read all of it
> and I understood even less, so I secretly hope to have missed a small
> yet critical check/option.
> And if there is no such way, would you be opposed to a patch adding
> an option (something like 'ima_enforce_hash_alg') that only allows
> digest hashed with the values supplied in the 'ima_hash' parameter ?

Please keep in mind that:
- depending on which file is not properly signed with the required
hash, the system might not boot.
- limiting the hash algorithm to a single algorithm would prevent
migrating to a stronger algorithm.

For embedded/IoT, these concerns might not be a problem.

thanks,

Mimi

RE: Weak hash algorithms allowed with DIGEST_NG

[THOBY Simon]

Hello again,

From : Mimi Zohar <zohar@linux.ibm.com>
> Before allowing the EVM HMAC to be updated, EVM verifies the existing
> HMAC to protect against an offline attack.  It doesn't prevent online
> changes.  Additional support to prevent crypto downgrade would need to
> be added.
> 

Yes, I wasn't really worrying about EVM, but only about IMA itself. Because if a
critical file (let's say bash) was hashed by the rightful user, but with a weak
algorithm, one can imagine an attacker finding a collision (another file,
carefully crafted by the attacker to have the same hash), and replacing offline
the legitimate file buy the "malicious" one. As far as I understand, the new
file would share the same hash, so the security.ima attribute wouldn't change,
and the security.evm wouldn't either because no xattr or inode number changed.
Of course this issue isn't critical because even if people hash their files
by calling evmctl, the default is SHA1 and it should be fairly hard to find
collisions (comparatively to e.g. MD4 or MD5), so nobody in pratice
should use a very weak algorithm where collisions would be "easy".

A careful owner/device producer should have no issue, but this highlights
the value of sane defaults, and I think evmctl certainly could benefit from
defaulting to sha256 in 2021 (but there may be compatibility issues I'm
not aware of that prevent such change).

> <snip>
> 
> > Is there any way to enforce the use of the hash specified in the
> > 'ima_hash' cmdline parameter ?
> 
> The cmdline parameter overrides the compile time default hash algorithm
> used for (re-)calculating the file hash.
> 

Yes, but that only applies to the hashes performed automatically by the kernel,
not to a user relabelling his whole / with
find / \( -fstype rootfs -o -fstype ext4 \) -type f -uid 0 -exec evmctl ima_hash '{}' 2> /dev/null \;
and forgetting to specify a stronger algorithm (that's how I learned of this pitfall myself).

> > I couldn't find any glancing at the code, but I didn't read all of it
> > and I understood even less, so I secretly hope to have missed a small
> > yet critical check/option.
> > And if there is no such way, would you be opposed to a patch adding
> > an option (something like 'ima_enforce_hash_alg') that only allows
> > digest hashed with the values supplied in the 'ima_hash' parameter ?
> 
> Please keep in mind that:
> - depending on which file is not properly signed with the required
> hash, the system might not boot.

Yes, yet in a sense that is also true when deploying IMA on a system, so it shouldn't
change much is such a scenario. This could definitely break a working system however,
so I have no doubt that if such option were to exit, it should be opt-in and not opt-out
(and to think I was talking of "sane defaults" a few lines above :)).

> - limiting the hash algorithm to a single algorithm would prevent
> migrating to a stronger algorithm.
> 

Indeed, I fear migrating the system online with such an option would be quite complex.

> For embedded/IoT, these concerns might not be a problem.
> 
> thanks,
> 
> Mimi
> 

Simon

Re: Weak hash algorithms allowed with DIGEST_NG

[Mimi Zohar]

On Wed, 2021-07-07 at 15:10 +0000, THOBY Simon wrote:

> > > Is there any way to enforce the use of the hash specified in the
> > > 'ima_hash' cmdline parameter ?
> > 
> > The cmdline parameter overrides the compile time default hash algorithm
> > used for (re-)calculating the file hash.
> > 
> 
> Yes, but that only applies to the hashes performed automatically by the kernel,
> not to a user relabelling his whole / with
> find / \( -fstype rootfs -o -fstype ext4 \) -type f -uid 0 -exec evmctl ima_hash '{}' 2> /dev/null \;
> and forgetting to specify a stronger algorithm (that's how I learned of this pitfall myself).

If you were interested in limiting which algorithms could be used, the
change would be made in ima_inode_setxattr().  I'd be interested in
seeing what you come up with.

thanks,

Mimi