{"action":"opened","issue":{"url":"https://api.github.com/repos/videolabs/libdsm/issues/117","repository_url":"https://api.github.com/repos/videolabs/libdsm","labels_url":"https://api.github.com/repos/videolabs/libdsm/issues/117/labels{/name}","comments_url":"https://api.github.com/repos/videolabs/libdsm/issues/117/comments","events_url":"https://api.github.com/repos/videolabs/libdsm/issues/117/events","html_url":"https://github.com/videolabs/libdsm/issues/117","id":257682192,"number":117,"title":"Security Issue: check message boundary","user":{"login":"Kxuan","id":10725111,"avatar_url":"https://avatars1.githubusercontent.com/u/10725111?v=4","gravatar_id":"","url":"https://api.github.com/users/Kxuan","html_url":"https://github.com/Kxuan","followers_url":"https://api.github.com/users/Kxuan/followers","following_url":"https://api.github.com/users/Kxuan/following{/other_user}","gists_url":"https://api.github.com/users/Kxuan/gists{/gist_id}","starred_url":"https://api.github.com/users/Kxuan/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Kxuan/subscriptions","organizations_url":"https://api.github.com/users/Kxuan/orgs","repos_url":"https://api.github.com/users/Kxuan/repos","events_url":"https://api.github.com/users/Kxuan/events{/privacy}","received_events_url":"https://api.github.com/users/Kxuan/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":0,"created_at":"2017-09-14T11:14:29Z","updated_at":"2017-09-14T11:14:29Z","closed_at":null,"author_association":"CONTRIBUTOR","body":"We have not check the boundary of SMB message!! The server may attack us by sending a malformed SMB message. \r\n\r\nFor example:  \r\nsmb_trans2.c:480\r\n```C\r\n480     tr2_resp  = (smb_trans2_resp *)reply.packet->payload;\r\n481     info      = (smb_tr2_path_info *)(tr2_resp->payload + 4); //+4 is padding\r\n482     file      = calloc(1, sizeof(smb_file));\r\n483     if (!file)\r\n484         return NULL;\r\n485 \r\n486     file->name_len  = smb_from_utf16((const char *)info->name, info->name_len,\r\n487                                      &file->name);\r\n488     file->name[info->name_len / 2] = 0;\r\n```\r\nWe trust the server that it sends a corrent `info->name_len` to us. But it may be incorrect! if name_len is larger than the real size of `info->name`, the code will trying to write data to an unexpected address!\r\n\r\nThere are many codes that has this issue:\r\nsmb_file.c\r\n```C\r\n220     if (!smb_session_recv_msg(s, &resp_msg))\r\n221         return -1;\r\n222     if (!smb_session_check_nt_status(s, &resp_msg))\r\n223         return -1;\r\n224 \r\n225     resp = (smb_read_resp *)resp_msg.packet->payload;\r\n226     if (buf)\r\n227         memcpy(buf, (char *)resp_msg.packet + resp->data_offset, resp->data_len);\r\n```\r\n`resp->data_offset` & `resp->data_len` is trusted. But if the server send an incorrect `resp->data_len` or `resp->data_offset` to us, the code may overwrite the unexpected memory area around `buf`.\r\n\r\nsmb_share.c\r\n```C\r\n318     // Is the server throwing pile of shit back at me ?\r\n319     res = smb_session_recv_msg(s, &resp);\r\n320     if (!res || resp.packet->payload[68])\r\n321     {\r\n322         BDSM_dbg(\"Bind call failed: 0x%hhx (reason = 0x%hhx)\\n\",\r\n323                  resp.packet->payload[68], resp.packet->payload[70]);\r\n324         ret = DSM_ERROR_NETWORK;\r\n325         goto error;\r\n326     }\r\n```\r\n`resp.packet->payload[68]` The size of payload may small than 69.\r\n\r\nsmb_share.c\r\n```C\r\n159 static ssize_t  smb_share_parse_enum(smb_message *msg, char ***list)\r\n160 {\r\n161     uint32_t          share_count, i;\r\n162     uint8_t           *data, *eod;\r\n163     uint32_t          *p_share_count;\r\n164 \r\n165     assert(msg != NULL && list != NULL);\r\n166     // Let's skip smb parameters and DCE/RPC stuff until we are at the begginning of\r\n167     // NetShareCtrl\r\n168 \r\n169     p_share_count = (uint32_t *)(msg->packet->payload + 60);\r\n170     share_count = *p_share_count;\r\n171     data        = msg->packet->payload + 72 + share_count * 12;\r\n172     eod         = msg->packet->payload + msg->payload_size;\r\n173 \r\n174     *list       = calloc(share_count + 1, sizeof(char *));\r\n175     if (!(*list))\r\n176         return -1;\r\n177 \r\n178     for (i = 0; i < share_count && data < eod; i++)\r\n179     {\r\n180         uint32_t name_len, com_len;\r\n181 \r\n182         name_len = *((uint32_t *)data);   // Read 'Max Count', make it a multiple of 2\r\n183         data    += 3 * sizeof(uint32_t);  // Move pointer to beginning of Name.\r\n184 \r\n185         smb_from_utf16((const char *)data, name_len * 2, (*list) + i);\r\n186 \r\n187         if (name_len % 2) name_len += 1;  // Align next move\r\n188         data    += name_len * 2;          // Move the pointer to Comment 'Max count'\r\n189 \r\n190         com_len  = *((uint32_t *)data);\r\n191         data    += 3 * sizeof(uint32_t);  // Move pointer to beginning of Comment.\r\n192         if (com_len % 2) com_len += 1;    // Align next move\r\n193         data    += com_len * 2;           // Move the pointer to next item\r\n194     }\r\n195 \r\n196     return i;\r\n197 }\r\n\r\n```\r\n`msg` are totally trusted, but `p_share_count` may incorrect. The line 178 limits  `data < eod`, but `data` + `name_len` may excess the eod.\r\n\r\nPlease fix this issue!!"}}
{"actor":{"display_login":"Kxuan"}}