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>

2 files changed, 8 insertions, 8 deletions

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