From 9f0bffebd070115ab47a92eadc6890a721c7b78d Mon Sep 17 00:00:00 2001 From: Michal Židek Date: Wed, 22 Jul 2015 16:35:35 +0200 Subject: 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 --- 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 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; } -- cgit