Skip to contentSkip to conversation

Remove hardcoding of SHA1 in EVM signatures

PATCH In review Matthew Garrett 2 participants 10 messages Apr 17, 2018 – Jul 1, 2018 Keys-trusted

lore
Share & tools
lore

Matthew Garrett

author
#0 lore raw

PATCH 1/2 Remove hardcoding of SHA1 in EVM signatures

EVM signatures are always being generated with SHA1 even if the -a argument has been provided to evmctl. Fix this so the provided hash algorithm is used instead.

Signed-off-by Matthew Garrett <[email protected]>
---
 src/evmctl.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)
src/evmctl.c +17 -3
index 2ffee78..43d261f 100644
@@ -313,6 +313,7 @@ err:
 
 static int calc_evm_hash(const char *file, unsigned char *hash)
 {
+        const EVP_MD *md;
 	struct stat st;
 	int err;
 	uint32_t generation = 0;

@@ -374,7 +375,13 @@ static int calc_evm_hash(const char *file, unsigned char *hash)
 		return -1;
 	}
 
-	err = EVP_DigestInit(pctx, EVP_sha1());
+	md = EVP_get_digestbyname(params.hash_algo);
+	if (!md) {
+		log_err("EVP_get_digestbyname() failed\n");
+		return 1;
+	}
+
+	err = EVP_DigestInit(pctx, md);
 	if (!err) {
 		log_err("EVP_DigestInit() failed\n");
 		return 1;

@@ -498,7 +505,7 @@ static int sign_evm(const char *file, const char *key)
 	if (len <= 1)
 		return len;
 
-	len = sign_hash("sha1", hash, len, key, NULL, sig + 1);
+	len = sign_hash(params.hash_algo, hash, len, key, NULL, sig + 1);
 	if (len <= 1)
 		return len;
 

@@ -967,6 +974,7 @@ static int cmd_setxattr_ima(struct command *cmd)
 
 static int calc_evm_hmac(const char *file, const char *keyfile, unsigned char *hash)
 {
+        const EVP_MD *md;
 	struct stat st;
 	int err = -1;
 	uint32_t generation = 0;

@@ -1033,7 +1041,13 @@ static int calc_evm_hmac(const char *file, const char *keyfile, unsigned char *h
 		goto out;
 	}
 
-	err = !HMAC_Init_ex(pctx, evmkey, sizeof(evmkey), EVP_sha1(), NULL);
+	md = EVP_get_digestbyname(params.hash_algo);
+	if (!md) {
+		log_err("EVP_get_digestbyname() failed\n");
+		goto out;
+	}
+
+	err = !HMAC_Init_ex(pctx, evmkey, sizeof(evmkey), md, NULL);
 	if (err) {
 		log_err("HMAC_Init() failed\n");
 		goto out;
Signature · 1 line
2.17.0.484.g0c8726318c-goog
author
#1 lore raw

PATCH 2/2 Add security.apparmor to the set of extended attributes used by EVM

The kernel is taking security.apparmor into account when validating EVM, so evmctl should be doing the same.

Signed-off-by Matthew Garrett <[email protected]>
---
 src/evmctl.c | 2 ++
 1 file changed, 2 insertions(+)
src/evmctl.c +2 -0
index 43d261f..e350f69 100644
@@ -69,6 +69,7 @@
 static char *evm_default_xattrs[] = {
 	XATTR_NAME_SELINUX,
 	XATTR_NAME_SMACK,
+	XATTR_NAME_APPARMOR,
 	XATTR_NAME_IMA,
 	XATTR_NAME_CAPS,
 	NULL

@@ -80,6 +81,7 @@ static char *evm_extra_smack_xattrs[] = {
 	XATTR_NAME_SMACKEXEC,
 	XATTR_NAME_SMACKTRANSMUTE,
 	XATTR_NAME_SMACKMMAP,
+	XATTR_NAME_APPARMOR,
 	XATTR_NAME_IMA,
 	XATTR_NAME_CAPS,
 	NULL
Signature · 1 line
2.17.0.484.g0c8726318c-goog
#2 lore raw

Re: [USER] [PATCH 2/2] Add security.apparmor to the set of extended attributes used by EVM

Quoted 4 lines · Matthew Garrett

The kernel is taking security.apparmor into account when validating EVM, so evmctl should be doing the same.

Signed-off-by Matthew Garrett <[email protected]>

The XATTR_NAME_APPARMOR is dependent on the version of "/usr/include/linux/xattr.h". Without it defined, evmctl fails to build.

Mimi

Quoted 24 lines
---
 src/evmctl.c | 2 ++
 1 file changed, 2 insertions(+)
src/evmctl.c +2 -0
index 43d261f..e350f69 100644
@@ -69,6 +69,7 @@
 static char *evm_default_xattrs[] = {
 	XATTR_NAME_SELINUX,
 	XATTR_NAME_SMACK,
+	XATTR_NAME_APPARMOR,
 	XATTR_NAME_IMA,
 	XATTR_NAME_CAPS,
 	NULL

@@ -80,6 +81,7 @@ static char *evm_extra_smack_xattrs[] = {
 	XATTR_NAME_SMACKEXEC,
 	XATTR_NAME_SMACKTRANSMUTE,
 	XATTR_NAME_SMACKMMAP,
+	XATTR_NAME_APPARMOR,
 	XATTR_NAME_IMA,
 	XATTR_NAME_CAPS,
 	NULL
author
#3 lore raw

Re: [USER] [PATCH 2/2] Add security.apparmor to the set of extended attributes used by EVM

Quoted 10 lines · Mimi Zohar
Quoted 4 lines · Matthew Garrett

The kernel is taking security.apparmor into account when validating EVM, so evmctl should be doing the same.

Signed-off-by Matthew Garrett <[email protected]>

The XATTR_NAME_APPARMOR is dependent on the version of "/usr/include/linux/xattr.h". Without it defined, evmctl fails to build.

Hmm, true. Is it reasonable to just hardcode it rather than using the define?

#4 lore raw

Re: [USER] [PATCH 2/2] Add security.apparmor to the set of extended attributes used by EVM

Quoted 6 lines · Matthew Garrett
Quoted 5 lines · Mimi Zohar
Quoted 3 lines · Matthew Garrett

The kernel is taking security.apparmor into account when validating EVM, so evmctl should be doing the same.

Quoted 7 lines
Quoted 5 lines
Quoted 1 line
Signed-off-by Matthew Garrett <[email protected]>

The XATTR_NAME_APPARMOR is dependent on the version of "/usr/include/linux/xattr.h". Without it defined, evmctl fails to build.

Hmm, true. Is it reasonable to just hardcode it rather than using the define?

I'm not sure how difficult it would be to tie the package name/version to a specific kernel release. Commit 096b85464832 ("EVM: Include security.apparmor in EVM measurements") was upstreamed in linux-4.15.

Mimi

#5 lore raw

Re: [USER] [PATCH 2/2] Add security.apparmor to the set of extended attributes used by EVM

Quoted 13 lines · Mimi Zohar
Quoted 12 lines · Matthew Garrett
Quoted 10 lines · Mimi Zohar
Quoted 4 lines · Matthew Garrett

The kernel is taking security.apparmor into account when validating EVM, so evmctl should be doing the same.

Signed-off-by Matthew Garrett <[email protected]>

The XATTR_NAME_APPARMOR is dependent on the version of "/usr/include/linux/xattr.h". Without it defined, evmctl fails to build.

Quoted 5 lines
Quoted 1 line

Hmm, true. Is it reasonable to just hardcode it rather than using the define?

I'm not sure how difficult it would be to tie the package name/version to a specific kernel release. Commit 096b85464832 ("EVM: Include security.apparmor in EVM measurements") was upstreamed in linux-4.15.

Instead, I've made the following change and pushed it out to master.

+#ifndef XATTR_APPAARMOR_SUFFIX
+#define XATTR_APPARMOR_SUFFIX "apparmor"
+#define XATTR_NAME_APPARMOR XATTR_SECURITY_PREFIX XATTR_APPARMOR_SUFFIX +#endif
+

thanks,

Mimi

#6 lore raw
In reply to Matthew Garrett
Quoted 3 lines · Matthew Garrett

EVM signatures are always being generated with SHA1 even if the -a argument has been provided to evmctl. Fix this so the provided hash algorithm is used instead.

The kernel assumes both the HMAC and signature are sha1 as well.

static char * const evm_hmac = "hmac(sha1)";
static char * const evm_hash = "sha1";

Mimi

Quoted 64 lines
Signed-off-by Matthew Garrett <[email protected]>
---
 src/evmctl.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)
src/evmctl.c +17 -3
index 2ffee78..43d261f 100644
@@ -313,6 +313,7 @@ err:
 
 static int calc_evm_hash(const char *file, unsigned char *hash)
 {
+        const EVP_MD *md;
 	struct stat st;
 	int err;
 	uint32_t generation = 0;

@@ -374,7 +375,13 @@ static int calc_evm_hash(const char *file, unsigned char *hash)
 		return -1;
 	}
 
-	err = EVP_DigestInit(pctx, EVP_sha1());
+	md = EVP_get_digestbyname(params.hash_algo);
+	if (!md) {
+		log_err("EVP_get_digestbyname() failed\n");
+		return 1;
+	}
+
+	err = EVP_DigestInit(pctx, md);
 	if (!err) {
 		log_err("EVP_DigestInit() failed\n");
 		return 1;

@@ -498,7 +505,7 @@ static int sign_evm(const char *file, const char *key)
 	if (len <= 1)
 		return len;
 
-	len = sign_hash("sha1", hash, len, key, NULL, sig + 1);
+	len = sign_hash(params.hash_algo, hash, len, key, NULL, sig + 1);
 	if (len <= 1)
 		return len;
 

@@ -967,6 +974,7 @@ static int cmd_setxattr_ima(struct command *cmd)
 
 static int calc_evm_hmac(const char *file, const char *keyfile, unsigned char *hash)
 {
+        const EVP_MD *md;
 	struct stat st;
 	int err = -1;
 	uint32_t generation = 0;

@@ -1033,7 +1041,13 @@ static int calc_evm_hmac(const char *file, const char *keyfile, unsigned char *h
 		goto out;
 	}
 
-	err = !HMAC_Init_ex(pctx, evmkey, sizeof(evmkey), EVP_sha1(), NULL);
+	md = EVP_get_digestbyname(params.hash_algo);
+	if (!md) {
+		log_err("EVP_get_digestbyname() failed\n");
+		goto out;
+	}
+
+	err = !HMAC_Init_ex(pctx, evmkey, sizeof(evmkey), md, NULL);
 	if (err) {
 		log_err("HMAC_Init() failed\n");
 		goto out;
#7 lore raw
In reply to Matthew Garrett

src/evmctl.c static int calc_evm_hmac(const char *file, const char *keyfile, unsigned char *h · line 1044

Quoted 6 lines · Matthew Garrett
src/evmctl.c +1 -1
@@ -1033,7 +1041,13 @@ static int calc_evm_hmac(const char *file, const char *keyfile, unsigned char *h
 		goto out;
 	}
 
-	err = !HMAC_Init_ex(pctx, evmkey, sizeof(evmkey), EVP_sha1(), NULL);
+	md = EVP_get_digestbyname(params.hash_algo);

HMAC is still limited to sha1.

Quoted 9 lines
src/evmctl.c +6 -0
+	if (!md) {
+		log_err("EVP_get_digestbyname() failed\n");
+		goto out;
+	}
+
+	err = !HMAC_Init_ex(pctx, evmkey, sizeof(evmkey), md, NULL);
 	if (err) {
 		log_err("HMAC_Init() failed\n");
 		goto out;
author
#8 lore raw
Quoted 6 lines · Mimi Zohar

src/evmctl.c static int calc_evm_hmac(const char *file, const char *keyfile, unsigned char *h · line 1044

Quoted 2 lines · Matthew Garrett
src/evmctl.c +1 -1
-     err = !HMAC_Init_ex(pctx, evmkey, sizeof(evmkey), EVP_sha1(), NULL);
+     md = EVP_get_digestbyname(params.hash_algo);

HMAC is still limited to sha1.

Yes, let me revert that hunk and I'll resend.

#9 lore raw
In reply to Matthew Garrett

Hi Matthew,

Quoted 14 lines · Matthew Garrett

EVM signatures are always being generated with SHA1 even if the -a argument has been provided to evmctl. Fix this so the provided hash algorithm is used instead.

Signed-off-by Matthew Garrett <[email protected]>
---
 src/evmctl.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)
src/evmctl.c +0 -0
index 2ffee78..43d261f 100644

src/evmctl.c err: · line 314

Quoted 2 lines
src/evmctl.c +0 -0
@@ -313,6 +313,7 @@ err:
 
 static int calc_evm_hash(const char *file, unsigned char *hash)

Please make sure the caller provides a large enough buffer for the hash.

Mimi

Quoted 52 lines
src/evmctl.c +17 -3
 {
+        const EVP_MD *md;
 	struct stat st;
 	int err;
 	uint32_t generation = 0;

@@ -374,7 +375,13 @@ static int calc_evm_hash(const char *file, unsigned char *hash)
 		return -1;
 	}
 
-	err = EVP_DigestInit(pctx, EVP_sha1());
+	md = EVP_get_digestbyname(params.hash_algo);
+	if (!md) {
+		log_err("EVP_get_digestbyname() failed\n");
+		return 1;
+	}
+
+	err = EVP_DigestInit(pctx, md);
 	if (!err) {
 		log_err("EVP_DigestInit() failed\n");
 		return 1;

@@ -498,7 +505,7 @@ static int sign_evm(const char *file, const char *key)
 	if (len <= 1)
 		return len;
 
-	len = sign_hash("sha1", hash, len, key, NULL, sig + 1);
+	len = sign_hash(params.hash_algo, hash, len, key, NULL, sig + 1);
 	if (len <= 1)
 		return len;
 

@@ -967,6 +974,7 @@ static int cmd_setxattr_ima(struct command *cmd)
 
 static int calc_evm_hmac(const char *file, const char *keyfile, unsigned char *hash)
 {
+        const EVP_MD *md;
 	struct stat st;
 	int err = -1;
 	uint32_t generation = 0;

@@ -1033,7 +1041,13 @@ static int calc_evm_hmac(const char *file, const char *keyfile, unsigned char *h
 		goto out;
 	}
 
-	err = !HMAC_Init_ex(pctx, evmkey, sizeof(evmkey), EVP_sha1(), NULL);
+	md = EVP_get_digestbyname(params.hash_algo);
+	if (!md) {
+		log_err("EVP_get_digestbyname() failed\n");
+		goto out;
+	}
+
+	err = !HMAC_Init_ex(pctx, evmkey, sizeof(evmkey), md, NULL);
 	if (err) {
 		log_err("HMAC_Init() failed\n");
 		goto out;