[1/1] security/integrity: Harden against malformed xattrs
diff mbox

Message ID 1470057550-58098-2-git-send-email-seth.forshee@canonical.com
State New
Headers show

Commit Message

Seth Forshee Aug. 1, 2016, 1:19 p.m. UTC
In general the handling of IMA/EVM xattrs is good, but I found
a few locations where either the xattr size or the value of the
type field in the xattr are not checked. Add a few simple checks
to these locations to prevent malformed or malicious xattrs from
causing problems.

Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
 security/integrity/digsig.c           | 2 +-
 security/integrity/evm/evm_main.c     | 4 ++++
 security/integrity/ima/ima_appraise.c | 5 ++++-
 3 files changed, 9 insertions(+), 2 deletions(-)

Comments

Seth Forshee Aug. 29, 2016, 6:31 p.m. UTC | #1
On Mon, Aug 01, 2016 at 08:19:10AM -0500, Seth Forshee wrote:
> In general the handling of IMA/EVM xattrs is good, but I found
> a few locations where either the xattr size or the value of the
> type field in the xattr are not checked. Add a few simple checks
> to these locations to prevent malformed or malicious xattrs from
> causing problems.
> 
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>

Bump. I haven't seen any feedback on this patch, but I also don't think
it's been applied anywhere, so I suspect it might have been forgotten.

Thanks,
Seth

--
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
Mimi Zohar Aug. 29, 2016, 8:17 p.m. UTC | #2
On Mon, 2016-08-29 at 13:31 -0500, Seth Forshee wrote:
> On Mon, Aug 01, 2016 at 08:19:10AM -0500, Seth Forshee wrote:
> > In general the handling of IMA/EVM xattrs is good, but I found
> > a few locations where either the xattr size or the value of the
> > type field in the xattr are not checked. Add a few simple checks
> > to these locations to prevent malformed or malicious xattrs from
> > causing problems.
> > 
> > Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> 
> Bump. I haven't seen any feedback on this patch, but I also don't think
> it's been applied anywhere, so I suspect it might have been forgotten.

Thanks for the reminder.

Mimi

--
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

Patch
diff mbox

diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 4304372b323f..106e855e2d9d 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -51,7 +51,7 @@  static bool init_keyring __initdata;
 int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
 			    const char *digest, int digestlen)
 {
-	if (id >= INTEGRITY_KEYRING_MAX)
+	if (id >= INTEGRITY_KEYRING_MAX || siglen < 2)
 		return -EINVAL;
 
 	if (!keyring[id]) {
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index b9e26288d30c..35ab453ce861 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -145,6 +145,10 @@  static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 	/* check value type */
 	switch (xattr_data->type) {
 	case EVM_XATTR_HMAC:
+		if (xattr_len != sizeof(struct evm_ima_xattr_data)) {
+			evm_status = INTEGRITY_FAIL;
+			goto out;
+		}
 		rc = evm_calc_hmac(dentry, xattr_name, xattr_value,
 				   xattr_value_len, calc.digest);
 		if (rc)
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 1bcbc12e03d9..11a17073e8a2 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -130,6 +130,7 @@  enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
 				 int xattr_len)
 {
 	struct signature_v2_hdr *sig;
+	enum hash_algo ret;
 
 	if (!xattr_value || xattr_len < 2)
 		/* return default hash algo */
@@ -143,7 +144,9 @@  enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
 		return sig->hash_algo;
 		break;
 	case IMA_XATTR_DIGEST_NG:
-		return xattr_value->digest[0];
+		ret = xattr_value->digest[0];
+		if (ret < HASH_ALGO__LAST)
+			return ret;
 		break;
 	case IMA_XATTR_DIGEST:
 		/* this is for backward compatibility */