From: Miroslav Lichvar Date: Wed, 31 Jul 2013 13:01:15 +0000 (+0200) Subject: Fix buffer overflow when processing crafted command packets X-Git-Tag: 1.29~3 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=7712455d9aa33d0db0945effaa07e900b85987b1;p=thirdparty%2Fchrony.git Fix buffer overflow when processing crafted command packets When the length of the REQ_SUBNETS_ACCESSED, REQ_CLIENT_ACCESSES command requests and the RPY_SUBNETS_ACCESSED, RPY_CLIENT_ACCESSES, RPY_CLIENT_ACCESSES_BY_INDEX, RPY_MANUAL_LIST command replies is calculated, the number of items stored in the packet is not validated. A crafted command request/reply can be used to crash the server/client. Only clients allowed by cmdallow (by default only localhost) can crash the server. With chrony versions 1.25 and 1.26 this bug has a smaller security impact as the server requires the clients to be authenticated in order to process the subnet and client accesses commands. In 1.27 and 1.28, however, the invalid calculated length is included also in the authentication check which may cause another crash. --- diff --git a/client.c b/client.c index 595d9f4a..370b0827 100644 --- a/client.c +++ b/client.c @@ -1371,7 +1371,8 @@ submit_request(CMD_Request *request, CMD_Reply *reply, int *reply_auth_ok) read_length = recvfrom_status; expected_length = PKL_ReplyLength(reply); - bad_length = (read_length < expected_length); + bad_length = (read_length < expected_length || + expected_length < offsetof(CMD_Reply, data)); bad_sender = (where_from.u.sa_family != his_addr.u.sa_family || (where_from.u.sa_family == AF_INET && (where_from.in4.sin_addr.s_addr != his_addr.in4.sin_addr.s_addr || diff --git a/cmdmon.c b/cmdmon.c index 75bc4646..e4f73491 100644 --- a/cmdmon.c +++ b/cmdmon.c @@ -1781,29 +1781,10 @@ read_from_cmd_socket(void *anything) } read_length = status; - expected_length = PKL_CommandLength(&rx_message); - rx_command = ntohs(rx_message.command); LCL_ReadRawTime(&now); LCL_CookTime(&now, &cooked_now, NULL); - tx_message.version = PROTO_VERSION_NUMBER; - tx_message.pkt_type = PKT_TYPE_CMD_REPLY; - tx_message.res1 = 0; - tx_message.res2 = 0; - tx_message.command = rx_message.command; - tx_message.sequence = rx_message.sequence; - tx_message.reply = htons(RPY_NULL); - tx_message.number = htons(1); - tx_message.total = htons(1); - tx_message.pad1 = 0; - tx_message.utoken = htonl(utoken); - /* Set this to a default (invalid) value. This protects against the - token field being set to an arbitrary value if we reject the - message, e.g. due to the host failing the access check. */ - tx_message.token = htonl(0xffffffffUL); - memset(&tx_message.auth, 0, sizeof(tx_message.auth)); - switch (where_from.u.sa_family) { case AF_INET: remote_ip.family = IPADDR_INET4; @@ -1830,7 +1811,14 @@ read_from_cmd_socket(void *anything) allowed = ADF_IsAllowed(access_auth_table, &remote_ip) || localhost; - if (read_length < offsetof(CMD_Request, data) || + /* Message size sanity check */ + if (read_length >= offsetof(CMD_Request, data)) { + expected_length = PKL_CommandLength(&rx_message); + } else { + expected_length = 0; + } + + if (expected_length < offsetof(CMD_Request, data) || rx_message.pkt_type != PKT_TYPE_CMD_REQUEST || rx_message.res1 != 0 || rx_message.res2 != 0) { @@ -1842,6 +1830,25 @@ read_from_cmd_socket(void *anything) return; } + rx_command = ntohs(rx_message.command); + + tx_message.version = PROTO_VERSION_NUMBER; + tx_message.pkt_type = PKT_TYPE_CMD_REPLY; + tx_message.res1 = 0; + tx_message.res2 = 0; + tx_message.command = rx_message.command; + tx_message.sequence = rx_message.sequence; + tx_message.reply = htons(RPY_NULL); + tx_message.number = htons(1); + tx_message.total = htons(1); + tx_message.pad1 = 0; + tx_message.utoken = htonl(utoken); + /* Set this to a default (invalid) value. This protects against the + token field being set to an arbitrary value if we reject the + message, e.g. due to the host failing the access check. */ + tx_message.token = htonl(0xffffffffUL); + memset(&tx_message.auth, 0, sizeof(tx_message.auth)); + if (rx_message.version != PROTO_VERSION_NUMBER) { tx_message.status = htons(STT_NOHOSTACCESS); if (!LOG_RateLimited()) { diff --git a/pktlength.c b/pktlength.c index a2682f7e..68d0bda9 100644 --- a/pktlength.c +++ b/pktlength.c @@ -127,6 +127,8 @@ PKL_CommandLength(CMD_Request *r) { unsigned long ns; ns = ntohl(r->data.subnets_accessed.n_subnets); + if (ns > MAX_SUBNETS_ACCESSED) + return 0; return (offsetof(CMD_Request, data.subnets_accessed.subnets) + ns * sizeof(REQ_SubnetsAccessed_Subnet)); } @@ -134,6 +136,8 @@ PKL_CommandLength(CMD_Request *r) { unsigned long nc; nc = ntohl(r->data.client_accesses.n_clients); + if (nc > MAX_CLIENT_ACCESSES) + return 0; return (offsetof(CMD_Request, data.client_accesses.client_ips) + nc * sizeof(unsigned long)); } @@ -197,6 +201,8 @@ PKL_ReplyLength(CMD_Reply *r) { unsigned long ns = ntohl(r->data.subnets_accessed.n_subnets); if (r->status == htons(STT_SUCCESS)) { + if (ns > MAX_SUBNETS_ACCESSED) + return 0; return (offsetof(CMD_Reply, data.subnets_accessed.subnets) + ns * sizeof(RPY_SubnetsAccessed_Subnet)); } else { @@ -207,6 +213,8 @@ PKL_ReplyLength(CMD_Reply *r) { unsigned long nc = ntohl(r->data.client_accesses.n_clients); if (r->status == htons(STT_SUCCESS)) { + if (nc > MAX_CLIENT_ACCESSES) + return 0; return (offsetof(CMD_Reply, data.client_accesses.clients) + nc * sizeof(RPY_ClientAccesses_Client)); } else { @@ -217,6 +225,8 @@ PKL_ReplyLength(CMD_Reply *r) { unsigned long nc = ntohl(r->data.client_accesses_by_index.n_clients); if (r->status == htons(STT_SUCCESS)) { + if (nc > MAX_CLIENT_ACCESSES) + return 0; return (offsetof(CMD_Reply, data.client_accesses_by_index.clients) + nc * sizeof(RPY_ClientAccesses_Client)); } else { @@ -226,6 +236,8 @@ PKL_ReplyLength(CMD_Reply *r) case RPY_MANUAL_LIST: { unsigned long ns = ntohl(r->data.manual_list.n_samples); + if (ns > MAX_MANUAL_LIST_SAMPLES) + return 0; if (r->status == htons(STT_SUCCESS)) { return (offsetof(CMD_Reply, data.manual_list.samples) + ns * sizeof(RPY_ManualListSample));