Hi,
see the attached patch for ticket
https://fedorahosted.org/sssd/ticket/1697
I think this is a candidate to include in our
coding guidelines.
Thanks,
Michal
--
Senior Principal Intern
From e731298ddb68395322843e3b143ff04bdec3353f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michal=20=C5=BDidek?= mzi...@redhat.com
Date: Wed, 22 Jul 2015 16:35:35 +0200
Subject: [PATCH] sssd: incorrect checks on length values during packet
decoding
https://fedorahosted.org/sssd/ticket/1697
It is safer to isolate the checked (unknown/untrusted)
value on the left hand side in the conditions
to avoid overflows/underflows.
---
src/providers/ad/ad_gpo_child.c | 8
src/providers/ipa/selinux_child.c | 6 +++---
src/providers/krb5/krb5_child.c | 10 +-
src/providers/krb5/krb5_child_handler.c | 6 +++---
src/providers/ldap/ldap_child.c | 6 +++---
src/providers/ldap/sdap_child_helpers.c | 2 +-
src/sss_client/ssh/sss_ssh_client.c | 6 +++---
7 files changed, 22 insertions(+), 22 deletions(-)
diff --git a/src/providers/ad/ad_gpo_child.c b/src/providers/ad/ad_gpo_child.c
index 4750732..0355b46 100644
--- a/src/providers/ad/ad_gpo_child.c
+++ b/src/providers/ad/ad_gpo_child.c
@@ -69,7 +69,7 @@ unpack_buffer(uint8_t *buf,
if (len == 0) {
return EINVAL;
} else {
-if ((p + len ) size) return EINVAL;
+if (len size - p) return EINVAL;
ibuf-smb_server = talloc_strndup(ibuf, (char *)(buf + p), len);
if (ibuf-smb_server == NULL) return ENOMEM;
DEBUG(SSSDBG_TRACE_ALL, smb_server: %s\n, ibuf-smb_server);
@@ -82,7 +82,7 @@ unpack_buffer(uint8_t *buf,
if (len == 0) {
return EINVAL;
} else {
-if ((p + len ) size) return EINVAL;
+if (len size - p) return EINVAL;
ibuf-smb_share = talloc_strndup(ibuf, (char *)(buf + p), len);
if (ibuf-smb_share == NULL) return ENOMEM;
DEBUG(SSSDBG_TRACE_ALL, smb_share: %s\n, ibuf-smb_share);
@@ -95,7 +95,7 @@ unpack_buffer(uint8_t *buf,
if (len == 0) {
return EINVAL;
} else {
-if ((p + len ) size) return EINVAL;
+if (len size - p) return EINVAL;
ibuf-smb_path = talloc_strndup(ibuf, (char *)(buf + p), len);
if (ibuf-smb_path == NULL) return ENOMEM;
DEBUG(SSSDBG_TRACE_ALL, smb_path: %s\n, ibuf-smb_path);
@@ -108,7 +108,7 @@ unpack_buffer(uint8_t *buf,
if (len == 0) {
return EINVAL;
} else {
-if ((p + len ) size) return EINVAL;
+if (len size - p) return EINVAL;
ibuf-smb_cse_suffix = talloc_strndup(ibuf, (char *)(buf + p), len);
if (ibuf-smb_cse_suffix == NULL) return ENOMEM;
DEBUG(SSSDBG_TRACE_ALL, smb_cse_suffix: %s\n, ibuf-smb_cse_suffix);
diff --git a/src/providers/ipa/selinux_child.c b/src/providers/ipa/selinux_child.c
index 7c5731d..3a15e7f 100644
--- a/src/providers/ipa/selinux_child.c
+++ b/src/providers/ipa/selinux_child.c
@@ -53,7 +53,7 @@ static errno_t unpack_buffer(uint8_t *buf,
DEBUG(SSSDBG_TRACE_INTERNAL,
Empty SELinux user, will delete the mapping\n);
} else {
-if ((p + len ) size) return EINVAL;
+if (len size - p) return EINVAL;
ibuf-seuser = talloc_strndup(ibuf, (char *)(buf + p), len);
if (ibuf-seuser == NULL) return ENOMEM;
DEBUG(SSSDBG_TRACE_INTERNAL, seuser: %s\n, ibuf-seuser);
@@ -69,7 +69,7 @@ static errno_t unpack_buffer(uint8_t *buf,
return EINVAL;
}
} else {
-if ((p + len ) size) return EINVAL;
+if (len size - p) return EINVAL;
ibuf-mls_range = talloc_strndup(ibuf, (char *)(buf + p), len);
if (ibuf-mls_range == NULL) return ENOMEM;
DEBUG(SSSDBG_TRACE_INTERNAL, mls_range: %s\n, ibuf-mls_range);
@@ -83,7 +83,7 @@ static errno_t unpack_buffer(uint8_t *buf,
DEBUG(SSSDBG_CRIT_FAILURE, No username set!\n);
return EINVAL;
} else {
-if ((p + len ) size) return EINVAL;
+if (len size - p) return EINVAL;
ibuf-username = talloc_strndup(ibuf, (char *)(buf + p), len);
if (ibuf-username == NULL) return ENOMEM;
DEBUG(SSSDBG_TRACE_INTERNAL, username: %s\n, ibuf-username);
diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c
index 2c5e446..0d0b4c0 100644
--- a/src/providers/krb5/krb5_child.c
+++ b/src/providers/krb5/krb5_child.c
@@ -1813,7 +1813,7 @@ static errno_t unpack_buffer(uint8_t *buf, size_t size,
SAFEALIGN_COPY_UINT32_CHECK(use_enterprise_princ, buf + p, size, p);
kr-use_enterprise_princ = (use_enterprise_princ == 0) ? false : true;
SAFEALIGN_COPY_UINT32_CHECK(len, buf + p, size, p);
-if ((p + len ) size) return EINVAL;
+if (len size - p) return EINVAL;
kr-upn = talloc_strndup(pd, (char *)(buf + p), len);
if (kr-upn == NULL) return ENOMEM;
p += len;
@@ -1830,13 +1830,13 @@ static errno_t unpack_buffer(uint8_t *buf
