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.

Reviewed-by: Petr Cech <pcech@redhat.com>

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 03951af04..6547f9c05 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 7c5731d66..3a15e7f51 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 e5f48b713..1edf10ab8 100644
--- a/src/providers/krb5/krb5_child.c
+++ b/src/providers/krb5/krb5_child.c
@@ -1808,7 +1808,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;
@@ -1825,13 +1825,13 @@ static errno_t unpack_buffer(uint8_t *buf, size_t size,
         pd->cmd == SSS_CMD_RENEW ||
         pd->cmd == SSS_PAM_CHAUTHTOK_PRELIM || pd->cmd == SSS_PAM_CHAUTHTOK) {
         SAFEALIGN_COPY_UINT32_CHECK(&len, buf + p, size, &p);
-        if ((p + len ) > size) return EINVAL;
+        if (len > size - p) return EINVAL;
         kr->ccname = talloc_strndup(pd, (char *)(buf + p), len);
         if (kr->ccname == NULL) return ENOMEM;
         p += len;
 
         SAFEALIGN_COPY_UINT32_CHECK(&len, buf + p, size, &p);
-        if ((p + len ) > size) return EINVAL;
+        if (len > size - p) return EINVAL;
 
         if (len > 0) {
             kr->old_ccname = talloc_strndup(pd, (char *)(buf + p), len);
@@ -1842,7 +1842,7 @@ static errno_t unpack_buffer(uint8_t *buf, size_t size,
         }
 
         SAFEALIGN_COPY_UINT32_CHECK(&len, buf + p, size, &p);
-        if ((p + len ) > size) return EINVAL;
+        if (len > size - p) return EINVAL;
         kr->keytab = talloc_strndup(pd, (char *)(buf + p), len);
         if (kr->keytab == NULL) return ENOMEM;
         p += len;
@@ -1875,7 +1875,7 @@ static errno_t unpack_buffer(uint8_t *buf, size_t size,
 
     if (pd->cmd == SSS_PAM_ACCT_MGMT) {
         SAFEALIGN_COPY_UINT32_CHECK(&len, buf + p, size, &p);
-        if ((p + len ) > size) return EINVAL;
+        if (len > size - p) return EINVAL;
         pd->user = talloc_strndup(pd, (char *)(buf + p), len);
         if (pd->user == NULL) return ENOMEM;
         p += len;
diff --git a/src/providers/krb5/krb5_child_handler.c b/src/providers/krb5/krb5_child_handler.c
index 4e453b02d..fa1055eb7 100644
--- a/src/providers/krb5/krb5_child_handler.c
+++ b/src/providers/krb5/krb5_child_handler.c
@@ -532,9 +532,9 @@ parse_krb5_child_response(TALLOC_CTX *mem_ctx, uint8_t *buf, ssize_t len,
         DEBUG(SSSDBG_TRACE_LIBS, "child response [%d][%d][%d].\n",
               msg_status, msg_type, msg_len);
 
-        if ((p + msg_len) > len) {
-            DEBUG(SSSDBG_CRIT_FAILURE, "message format error [%zu] > [%zd].\n",
-                  p+msg_len, len);
+        if (msg_len > len - p) {
+            DEBUG(SSSDBG_CRIT_FAILURE, "message format error [%d] > [%zu].\n",
+                  msg_len, len - p);
             return EINVAL;
         }
 
diff --git a/src/providers/ldap/ldap_child.c b/src/providers/ldap/ldap_child.c
index 191d5bc65..7ce8d4e6c 100644
--- a/src/providers/ldap/ldap_child.c
+++ b/src/providers/ldap/ldap_child.c
@@ -66,7 +66,7 @@ static errno_t unpack_buffer(uint8_t *buf, size_t size,
 
     DEBUG(SSSDBG_TRACE_LIBS, "realm_str size: %d\n", len);
     if (len) {
-        if ((p + len ) > size) return EINVAL;
+        if (len > size - p) return EINVAL;
         ibuf->realm_str = talloc_strndup(ibuf, (char *)(buf + p), len);
         DEBUG(SSSDBG_TRACE_LIBS, "got realm_str: %s\n", ibuf->realm_str);
         if (ibuf->realm_str == NULL) return ENOMEM;
@@ -78,7 +78,7 @@ static errno_t unpack_buffer(uint8_t *buf, size_t size,
 
     DEBUG(SSSDBG_TRACE_LIBS, "princ_str size: %d\n", len);
     if (len) {
-        if ((p + len ) > size) return EINVAL;
+        if (len > size - p) return EINVAL;
         ibuf->princ_str = talloc_strndup(ibuf, (char *)(buf + p), len);
         DEBUG(SSSDBG_TRACE_LIBS, "got princ_str: %s\n", ibuf->princ_str);
         if (ibuf->princ_str == NULL) return ENOMEM;
@@ -90,7 +90,7 @@ static errno_t unpack_buffer(uint8_t *buf, size_t size,
 
     DEBUG(SSSDBG_TRACE_LIBS, "keytab_name size: %d\n", len);
     if (len) {
-        if ((p + len ) > size) return EINVAL;
+        if (len > size - p) return EINVAL;
         ibuf->keytab_name = talloc_strndup(ibuf, (char *)(buf + p), len);
         DEBUG(SSSDBG_TRACE_LIBS, "got keytab_name: %s\n", ibuf->keytab_name);
         if (ibuf->keytab_name == NULL) return ENOMEM;
diff --git a/src/providers/ldap/sdap_child_helpers.c b/src/providers/ldap/sdap_child_helpers.c
index afe6351e9..90330f13f 100644
--- a/src/providers/ldap/sdap_child_helpers.c
+++ b/src/providers/ldap/sdap_child_helpers.c
@@ -222,7 +222,7 @@ static int parse_child_response(TALLOC_CTX *mem_ctx,
     /* ccache name size */
     SAFEALIGN_COPY_UINT32_CHECK(&len, buf + p, size, &p);
 
-    if ((p + len ) > size) return EINVAL;
+    if (len > size - p) return EINVAL;
 
     ccn = talloc_size(mem_ctx, sizeof(char) * (len + 1));
     if (ccn == NULL) {
diff --git a/src/sss_client/ssh/sss_ssh_client.c b/src/sss_client/ssh/sss_ssh_client.c
index 245a02056..e5097337f 100644
--- a/src/sss_client/ssh/sss_ssh_client.c
+++ b/src/sss_client/ssh/sss_ssh_client.c
@@ -171,7 +171,7 @@ sss_ssh_get_ent(TALLOC_CTX *mem_ctx,
 
     /* parse reply */
     c = 0;
-    if (rep_len-c < 2*sizeof(uint32_t)) {
+    if (rep_len < c + 2*sizeof(uint32_t)) {
         ret = EINVAL;
         goto done;
     }
@@ -214,7 +214,7 @@ sss_ssh_get_ent(TALLOC_CTX *mem_ctx,
 
         SAFEALIGN_COPY_UINT32(&len, rep+c, &c);
 
-        if (rep_len-c < len + sizeof(uint32_t)) {
+        if (len > rep_len - c - sizeof(uint32_t)) {
             ret = EINVAL;
             goto done;
         }
@@ -237,7 +237,7 @@ sss_ssh_get_ent(TALLOC_CTX *mem_ctx,
 
         SAFEALIGN_COPY_UINT32(&len, rep+c, &c);
 
-        if (rep_len-c < len) {
+        if (len > rep_len - c) {
             ret = EINVAL;
             goto done;
         }