Hello Humberto, I applaud your effort but the use of dynamically allocated buffers verse statically allocated buffers does not alleviate the buffer-overflow problem. It mitigates trash-writing to somewhere else. Additionally, using dynamically allocated buffers does slow the system down and, potentially, generate a Kannel PANIC.
Also, some of the code you pointed to uses information from the Kannel configuration file (e.g. the serial device filename for the emi_x25). I feel we should document the length limitation in the userguide and let the person configuration shoot himself/herself in the foot if they go beyond it. If we are kind, then we can provide '%s' precision to limit their inputted value to conform to the stated acceptable length. But, I do agree with you in the code sections were the information is coming from outside our control. For those sections, we should use '%s' precision and maybe even snprintf() for that "belt-and-suspender" feeling. Did you get your ideas for the changes through an automated buffer-overflow checking application? It appears that many of the suggestions didn't take into effect the coding around the proposed fix. More comments inline... See ya... d.c. **>Date: Wed, 10 May 2006 20:38:38 -0400 **>From: "Humberto Figuera" <[EMAIL PROTECTED]> **>To: [email protected] **>Subject: [PATH] Possible buffer overflow gateway/gw/smsc/smsc_emi_x25.c **> **>Hi, **> **>I've been using kannel since few months ago, looking the code found **>possibles buffer overflow. **> **>* fixed size local buffer : they are prime targets for buffer overflow **>attacks. **>* sprintf : the format string could contain `%s' without precision **>that could result in a buffer overflow. **>* strcpy : if the argument 2 passed to this function copy more data **>than can be handled, resulting in a buffer overflow. **> **>I would since today follow looking all source tree and submit the patch. **> **>I hope the list can appreciate it and it to be committed to CVS ;p **> **>--- tmp/gateway/gw/smsc/smsc_emi_x25.c 2005-02-11 11:35:48.000000000 -0400 **>+++ gateway/gw/smsc/smsc_emi_x25.c 2006-05-10 19:38:00.000000000 -0400 **>@@ -137,11 +137,16 @@ **>*/ **>static int emi_open_connection(SMSCenter *smsc) **>{ **>- char tmpbuff[1024]; **>+ char *tmpbuff; **>+ **>+ tmpbuff = gw_malloc(1024); **>+ memset(tmpbuff, 0, 1024); **> **>- sprintf(tmpbuff, "/dev/%s", smsc->emi_serialdevice); **>+ snprintf(tmpbuff, 1024 - 1, "/dev/%s", smsc->emi_serialdevice); **> smsc->emi_fd = at_dial(tmpbuff, smsc->emi_phonenum, "ATD", 30); **> **>+ gw_free(tmpbuff); **>+ **> if (smsc->emi_fd <= 0) **> return -1; smsc->emi_serialdevice comes from the "device=" definition in the configuration file. If the user tries to generate a 1020 byte device filename, then fine, let him. gw_malloc()'ing this tmpbuf just creates runtime performance overheads. Additionally, gw_malloc()'ing memory could lead to a Kannel PANIC if there isn't enough free memory to fulfill the request. In this instance, it' not a problem since it would happen at the very beginning of Kannel startup. **> **>@@ -170,7 +175,7 @@ **> if (emi_open_connection(smsc) < 0) **> goto error; **> **>- sprintf(smsc->name, "EMI:%s:%s", smsc->emi_phonenum, **>+ snprintf(smsc->name, 1024 - 1, "EMI:%s:%s", smsc->emi_phonenum, **> smsc->emi_username); **> return smsc; smsc->emi_phonenum, comes from the "phone=" definition in the configuration file. Again, I say document the size limit they should follow and let the person shoot himself/herself in the foot if they ignore us. **> **>@@ -224,20 +229,33 @@ **> **>static int emi_open_session(SMSCenter *smsc) **>{ **>- char message_whole [1024]; **>- char message_body [1024]; **>- char message_header [50]; **>- char message_footer [10]; **>- char my_buffer [1024]; **>+ char *message_whole; **>+ char *message_body; **>+ char *message_header; **>+ char *message_footer; **>+ char *my_buffer; **> int length; **> **>- memset(message_whole, 0, sizeof(message_whole)); **>- memset(message_body, 0, sizeof(message_body)); **>- memset(message_header, 0, sizeof(message_header)); **>- memset(message_footer, 0, sizeof(message_footer)); **>+ message_whole = gw_malloc(1024); **>+ message_body = gw_malloc(1024); **>+ message_header = gw_malloc(50); **>+ message_footer = gw_malloc(10); **>+ my_buffer = gw_malloc(1024); **>+ **>+ memset(message_whole, 0, 1024); **>+ memset(message_body, 0, 1024); **>+ memset(message_header, 0, 50); **>+ memset(message_footer, 0, 10); **>+ memset(my_buffer, 0, 1024); **> **> if (emi_fill_ucp60_login(message_body, smsc->emi_username, **>smsc->emi_password) < 0) { **> error(0, "emi_fill_ucp60_login failed"); **>+ gw_free(message_whole); **>+ gw_free(message_body); **>+ gw_free(message_header); **>+ gw_free(message_footer); **>+ gw_free(my_buffer); **>+ **> return -1; **> } **> **>@@ -246,21 +264,27 @@ **> length += 2; /* footer (fixed) */ **> length += 2; /* slashes between header, body, footer */ **> **>- sprintf(message_header, "%02i/%05i/O/60", **>+ snprintf(message_header, 50 - 1, "%02i/%05i/O/60", **> (smsc->emi_current_msg_number++ % 100), length); **> **> /* FOOTER */ **> **>- sprintf(my_buffer, "%s/%s/", message_header, message_body); **>+ snprintf(my_buffer, 1024 - 1, "%s/%s/", message_header, message_body); **> generate_checksum(my_buffer, message_footer); **> **>- sprintf(message_whole, "\x02%s/%s/%s\x03", message_header, **>+ snprintf(message_whole, 1024 - 1, "\x02%s/%s/%s\x03", message_header, **> message_body, message_footer); **> **> debug("bb.sms.emi", 0, "final UCP60 msg: <%s>", message_whole); **> **> put_data(smsc, message_whole, strlen(message_whole), 0); **> **>+ gw_free(message_whole); **>+ gw_free(message_body); **>+ gw_free(message_header); **>+ gw_free(message_footer); **>+ gw_free(my_buffer); **>+ **> if (!wait_for_ack(smsc, 60)) { **> info(0, "emi_open_session: wait for ack failed!"); **> return -1; With the exception of smsc->emi_username and smsc->emi_password (both obtained from the configuration file "smsc-username" and "smsc-password", respectively), everything else in message_body, message_header, message_footer, message_whole, are of finite length. message_body's length is generated by emi_fill_ucp60_login(). I haven't ever seen a UCP60 Protocol Login that was 9999 characters in length. The message_header, "%02i/%05i/0/60", is of fixed size since smsc->emi_current_msg_number cannot be more that 99 (the part that goes into %02i). The length (%05i), is calculated from above and can only go beyond 5 digits if the message_body generated by emi_fill_ucp60_login() is greater than 9999 characters in size...very doubtful. The length of message_footer is finite since it is a checksum generated by us. And, message_whole is the accumulation of message_header, message_body, and message_footer. Since none of the three section should go beyond a certain point, the message_whole should be safe. **>@@ -471,7 +495,7 @@ **> **>static int at_dial(char *device, char *phonenum, char *at_prefix, **>time_t how_long) **>{ **>- char tmpbuff[1024]; **>+ char *tmpbuff; **> int howmanyread = 0; **> int thistime = 0; **> int redial; **>@@ -502,6 +526,9 @@ **> tios.c_cflag |= (HUPCL | CREAD | CRTSCTS); **> tcsetattr(fd, TCSANOW, &tios); **> **>+ tmpbuff = gw_malloc(1024); **>+ memset(tmpbuff, 0, 1024); **>+ **> /* Dial using an AT command string. */ **> for (redial = 1; redial; ) { **> info(0, "at_dial: dialing <%s> on <%s> for <%i> seconds", **>@@ -510,9 +537,9 @@ **> **> /* Send AT dial request. */ **> howmanyread = 0; **>- sprintf(tmpbuff, "%s%s\r\n", at_prefix, phonenum); **>+ snprintf(tmpbuff, 1024 - 1, "%s%s\r\n", at_prefix, phonenum); phonenum comes from the from the "phone=" definition in the configuration file. at_prefix is "ATD" as specified in emi_open_connection(). **> ret = write(fd, tmpbuff, strlen(tmpbuff)); /* errors... -mg */ **>- memset(&tmpbuff, 0, sizeof(tmpbuff)); **>+ memset(tmpbuff, 0, 1024); **> **> /* Read the answer to the AT command and react accordingly. */ **> for (; ; ) { **>@@ -521,7 +548,7 @@ **> goto timeout; **> **> /* We don't need more space for dialout */ **>- if (howmanyread >= (int) sizeof(tmpbuff)) **>+ if (howmanyread >= 1024) **> goto error; **> **> /* We read 1 char a time so that we don't **>@@ -580,15 +607,18 @@ **> } /* End of dial loop. */ **> **> debug("bb.sms.emi", 0, "at_dial: done with dialing"); **>+ gw_free(tmpbuff); **> return fd; **> **>timeout: **> error(0, "at_dial timed out"); **>+ gw_free(tmpbuff); **> close(fd); **> return -1; **> **>error: **> error(0, "at_dial failed"); **>+ gw_free(tmpbuff); **> close(fd); **> return -1; **>} **>@@ -807,9 +837,15 @@ **>*/ **>static int memorybuffer_has_rawmessage(SMSCenter *smsc, int type, char auth) **>{ **>- char tmpbuff[1024], tmpbuff2[1024]; **>+ char *tmpbuff, *tmpbuff2; **> char *stx, *etx; **> **>+ tmpbuff = gw_malloc(1024); **>+ tmpbuff2 = gw_malloc(1024); **>+ **>+ memset(tmpbuff, 0, 1024); **>+ memset(tmpbuff2, 0, 1024); **>+ **> stx = memchr(smsc->buffer, '\2', smsc->buflen); **> etx = memchr(smsc->buffer, '\3', smsc->buflen); **> **>@@ -817,15 +853,19 @@ **> strncpy(tmpbuff, stx, etx - stx + 1); **> tmpbuff[etx - stx + 1] = '\0'; **> if (auth) **>- sprintf(tmpbuff2, "/%c/%02i/", auth, type); **>+ snprintf(tmpbuff2, 1024 - 1, "/%c/%02i/", auth, type); **> else **>- sprintf(tmpbuff2, "/%02i/", type); **>+ snprintf(tmpbuff2, 1024 - 1, "/%02i/", type); **> **> if (strstr(tmpbuff, tmpbuff2) != NULL) { **> debug("bb.sms.emi", 0, "found message <%c/%02i>...msg **><%s>", auth, type, tmpbuff); **>+ gw_free(tmpbuff); **>+ gw_free(tmpbuff); **> return 1; **> } **> } **>+ gw_free(tmpbuff); **>+ gw_free(tmpbuff); **> return 0; **> **>} Take a look at the code. tmpbuff2 can only hold the results of "/%c/%02i/" or "/%02i/". 'auth' is a single character. 'type', even if it was a full 32 32-bit signed integer would not cause it to more than 11 characters. So, the largest tmpbuff2 could be is "/" + sizeof(char) + "/" + 11 + "/", or 15 characters + 1 NULL byte. **>@@ -880,13 +920,14 @@ **> **> char emivars[128][1024]; **> char *leftslash, *rightslash; **>- char isotext[2048]; **>+ char *isotext; **> int msgnbr; **> int tmpint; **> **> msgnbr = -1; **> **>- memset(isotext, 0, sizeof(isotext)); **>+ isotext = gw_malloc(2048); **>+ memset(isotext, 0, 2048); **> **> strncpy(isotext, rawmessage, length); **> leftslash = isotext; **>@@ -901,25 +942,25 @@ **> break; **> **> *rightslash = '\0'; **>- strcpy(emivars[tmpint], leftslash + 1); **>+ strncpy(emivars[tmpint], leftslash + 1, sizeof(emivars[tmpint]) - **>1); **> leftslash = rightslash; **> } I agree with you about using strncpy(). If the a right slash or '\3' character could not be found within 1023 bytes from the previous slash or '\3', then you will have a buffer-overflow. The string copy to emivars[] from the raw message should be capped at 1023 with a NULL byte added to the last position of emivars[]. **> **> if (strcmp(emivars[3], "01") == 0) { **> if (strcmp(emivars[7], "2") == 0) { **>- strcpy(isotext, emivars[8]); **>+ strncpy(isotext, emivars[8], 2048 - 1); This will not be needed if you limited the stuffed copied during parsing from above. If you limited emivars[] to store only 1023 characters, then you don't need to worry about isotext's 2K buffer being blown out. **> } else if (strcmp(emivars[7], "3") == 0) { **>- parse_emi_to_iso88591(emivars[8], isotext, sizeof(isotext), **>+ parse_emi_to_iso88591(emivars[8], isotext, 2048, **> smsc->alt_charset); **> } else { **> error(0, "Unknown 01-type EMI SMS (%s)", emivars[7]); **> strcpy(isotext, ""); **> } **> } else if (strcmp(emivars[3], "51") == 0) { **>- parse_emi_to_iso88591(emivars[24], isotext, sizeof(isotext), **>+ parse_emi_to_iso88591(emivars[24], isotext, 2048, **> smsc->alt_charset); **> } else if (strcmp(emivars[3], "52") == 0) { **>- parse_emi_to_iso88591(emivars[24], isotext, sizeof(isotext), **>+ parse_emi_to_iso88591(emivars[24], isotext, 2048, **> smsc->alt_charset); **> } else { **> error(0, "HEY WE SHOULD NOT BE HERE!! Type = %s", emivars[3]); **>@@ -934,9 +975,11 @@ **> (*msg)->sms.msgdata = octstr_create(isotext); **> (*msg)->sms.udhdata = NULL; **> **>+ gw_free(isotext); **> return msgnbr; **> **>error: **>+ gw_free(isotext); **> return -1; **>} **> **>@@ -948,19 +991,25 @@ **>{ **> **> char emivars[128][1024]; **>- char timestamp[2048], sender[2048], receiver[2048]; **>- char emitext[2048], isotext[2048]; **>+ char *timestamp, *sender, *receiver; **>+ char *emitext, *isotext; **> char *leftslash, *rightslash; **> int msgnbr; **> int tmpint; **> int is_backup = 0; **> **>+ sender = gw_malloc(2048); **>+ receiver = gw_malloc(2048); **>+ emitext = gw_malloc(2048); **>+ isotext = gw_malloc(2048); **>+ timestamp = gw_malloc(2048); **>+ **> msgnbr = -1; **>- memset(&sender, 0, sizeof(sender)); **>- memset(&receiver, 0, sizeof(receiver)); **>- memset(&emitext, 0, sizeof(emitext)); **>- memset(&isotext, 0, sizeof(isotext)); **>- memset(×tamp, 0, sizeof(timestamp)); **>+ memset(sender, 0, 2048); **>+ memset(receiver, 0, 2048); **>+ memset(emitext, 0, 2048); **>+ memset(isotext, 0, 2048); **>+ memset(timestamp, 0, 2048); **> **> strncpy(isotext, rawmessage, length); **> leftslash = isotext; **>@@ -978,31 +1027,36 @@ **> break; **> **> *rightslash = '\0'; **>- strcpy(emivars[tmpint], leftslash + 1); **>+ strncpy(emivars[tmpint], leftslash + 1, 1024 - 1); **> leftslash = rightslash; **> } Same response as above. I agree we should limit the amount copied into emivars[] to 1023 and tack on a NULL byte at the last position. **> **> /* BODY */ **>- sprintf(isotext, "A//%s:%s", emivars[4], emivars[18]); **>- sprintf(isotext, "A//%s:", emivars[5]); **>+ snprintf(isotext, 2048 - 1, "A//%s:%s", emivars[4], emivars[18]); I agree to use snprintf() here since emivars[] could have up to 1023 worth of data which can potentially blow out the isotext buffer. Actually, we should really just check and see if: strlen(emivars[4]) + strlen(emivars[18]) > (sizeof(isotext)-5) If it is then we will generate an invalid EMI packet. We should just through the whole thing away instead of trying to generate a truncated version of the EMI packet. **>+ snprintf(isotext, 2048 - 1, "A//%s:", emivars[5]); I don't think we need it here. emivars should never be more than 1023 if we limited it during parsing. **> is_backup = 0; **> **> /* HEADER */ **> **> debug("bb.sms.emi", 0, "acknowledge: type = '%s'", emivars[3]); **> **>- sprintf(emitext, "%s/%05i/%s/%s", emivars[0], (int) strlen(isotext) + **>17, **>+ snprintf(emitext, 2048 - 1, "%s/%05i/%s/%s", emivars[0], (int) **>strlen(isotext) + 17, **> "R", emivars[3]); Yes, use snprintf() to limit the size of text going into emitext to 2047. Or, just through it away if strlen(emivar[0]) + strlen(emivars[3]) + 11 + 1 + 3 > sizeof(emitext) **> **> smsc->emi_current_msg_number = atoi(emivars[0]) + 1; **> **> /* FOOTER */ **>- sprintf(timestamp, "%s/%s/", emitext, isotext); **>+ snprintf(timestamp, 2048 - 1, "%s/%s/", emitext, isotext); **> generate_checksum(timestamp, receiver); **> **>- sprintf(sender, "%c%s/%s/%s%c", 0x02, emitext, isotext, receiver, **>0x03); **>+ snprintf(sender, 2048 - 1, "%c%s/%s/%s%c", 0x02, emitext, **>isotext, receiver, 0x03); **> put_data(smsc, sender, strlen(sender), is_backup); **> **>+ gw_free(sender); **>+ gw_free(receiver); **>+ gw_free(emitext); **>+ gw_free(isotext); **>+ gw_free(timestamp); **> return msgnbr; **> **>} Yes, use snprintf() to limit the size of text going into timestamp, and sender. Actually, thinking about this, what should really happen is that as soon as you enter acknowledge_from_rawmessage(), it should check the size of the rawmessage. If it's bigger than what we can handle, we should return an error. Otherwise, we we will be trying to generate an acknowledgement with invalid data. **>@@ -1013,36 +1067,51 @@ **>*/ **>static int parse_msg_to_rawmessage(SMSCenter *smsc, Msg *msg, char **>*rawmessage, int rawmessage_length) **>{ **>- char message_whole[10*1024]; **>- char message_body[10*1024]; **>- char message_header[1024]; **>- char message_footer[1024]; **>- **>- char my_buffer[10*1024]; **>- char my_buffer2[10*1024]; **>- char msgtext[1024]; **>+ char *message_whole; **>+ char *message_body; **>+ char *message_header; **>+ char *message_footer; **>+ **>+ char *my_buffer; **>+ char *my_buffer2; **>+ char *msgtext; **> int length; **> char mt; **>- char mcl[20]; **>- char snumbits[20]; **>- char xser[1024]; **>+ char *mcl; **>+ char *snumbits; **>+ char *xser; **> int udh_len; **> **>- memset(&message_whole, 0, sizeof(message_whole)); **>- memset(&message_body, 0, sizeof(message_body)); **>- memset(&message_header, 0, sizeof(message_header)); **>- memset(&message_footer, 0, sizeof(message_footer)); **>- memset(&my_buffer, 0, sizeof(my_buffer)); **>- memset(&my_buffer2, 0, sizeof(my_buffer2)); **>+ message_whole = gw_malloc(10 * 1024); **>+ message_body = gw_malloc(10 * 1024); **>+ message_header = gw_malloc(1024); **>+ message_footer = gw_malloc(1024); **>+ my_buffer = gw_malloc(10 * 1024); **>+ my_buffer2 = gw_malloc(10 * 1024); **>+ msgtext = gw_malloc(1024); **>+ mcl = gw_malloc(20); **>+ snumbits = gw_malloc(20); **>+ xser = gw_malloc(1024); **>+ **>+ memset(message_whole, 0, 10 * 1024); **>+ memset(message_body, 0, 10 * 1024); **>+ memset(message_header, 0, 1024); **>+ memset(message_footer, 0, 1024); **>+ memset(my_buffer, 0, 10 * 1024); **>+ memset(my_buffer2, 0, 10 * 1024); **> mt = '3'; **>- memset(&snumbits, 0, sizeof(snumbits)); **>- memset(&xser, 0, sizeof(xser)); **>+ memset(msgtext, 0, 1024); **>+ memset(mcl, 0, 20); **>+ memset(snumbits, 0, 20); **>+ memset(xser, 0, 1024); **> **> /* XXX parse_iso88591_to_emi shouldn't use NUL terminated **> * strings, but Octstr directly, or a char* and a length. **> */ **> if (octstr_len(msg->sms.udhdata)) { **>- char xserbuf[258]; **>+ char *xserbuf; **>+ xserbuf = gw_malloc(258); **>+ memset(xserbuf, 0, 258); **> /* we need a properly formated UDH here, there first byte **>contains his length **> * this will be formatted in the xser field of the EMI Protocol **> */ **>@@ -1051,6 +1120,7 @@ **> xserbuf[1] = udh_len; **> octstr_get_many_chars(&xserbuf[2], msg->sms.udhdata, 0, udh_len); **> parse_binary_to_emi(xserbuf, xser, udh_len + 2); **>+ gw_free(xserbuf); **> } else { **> udh_len = 0; **> } **>@@ -1070,7 +1140,7 @@ **> **> parse_binary_to_emi(msgtext, my_buffer2, **> octstr_len(msg->sms.msgdata)); **> **>- sprintf(snumbits, "%04ld", octstr_len(msg->sms.msgdata)*8); **>+ snprintf(snumbits, 20 - 1, "%04ld", **>octstr_len(msg->sms.msgdata)*8); **> mt = '4'; **> strcpy(mcl, "1"); **> } **>@@ -1079,7 +1149,7 @@ **> * Please someone encode it with fields_to_dcs **> */ **> **>- sprintf(message_body, **>+ snprintf(message_body, 10 * 1024 - 1, **> "%s/%s/%s/%s/%s//%s////////////%c/%s/%s////%s//////%s//", **> octstr_get_cstr(msg->sms.receiver), **> msg->sms.sender ? octstr_get_cstr(msg->sms.sender) : "", I fully agree that we should use snprintf() here. msg->sms.receiver and msg->sms.sender both come from the SMSC. We have no idea how large it could be coming from the SMSC. What we could do use to use '%s' precision to limit the size inputted into our message_body. We could base it on 2 * MAXSIZE(E.164). **>@@ -1100,14 +1170,14 @@ **> length += 2; /* footer (fixed) */ **> length += 2; /* slashes between header, body, footer */ **> **>- sprintf(message_header, "%02i/%05i/%s/%s", **>(smsc->emi_current_msg_number++ % 100), length, "O", "51"); **>+ snprintf(message_header, 1024 - 1, "%02i/%05i/%s/%s", **>(smsc->emi_current_msg_number++ % 100), length, "O", "51"); Once again, everything put into message_header is of finite size and could not possible blow out a 1024 buffer. **> **> /* FOOTER */ **> **>- sprintf(my_buffer, "%s/%s/", message_header, message_body); **>+ snprintf(my_buffer, 10 * 1024 - 1, "%s/%s/", message_header, **>message_body); If message_header and message_body are limited to no more than 1023 each, then there's no worry about overflowing a 10K buffer. **> generate_checksum(my_buffer, message_footer); **> **>- sprintf(message_whole, "%c%s/%s/%s%c", 0x02, message_header, **>message_body, message_footer, 0x03); **>+ snprintf(message_whole, 10 * 1024 - 1, "%c%s/%s/%s%c", 0x02, **>message_header, message_body, message_footer, 0x03); Again, message_header, message_body, and message_footer should be limited to no more than 1023 in size each. Theoretically, message_whole can be no more than: 1 + 1023 + 1 + 1023 + 1 + 1023 + 1 = 3073. **> **> strncpy(rawmessage, message_whole, rawmessage_length); **> **>@@ -1117,6 +1187,17 @@ **> } **> debug("bb.sms.emi", 0, "emi %d message %s", **> smsc->emi_current_msg_number, rawmessage); **>+ **>+ gw_free(message_whole); **>+ gw_free(message_body); **>+ gw_free(message_header); **>+ gw_free(message_footer); **>+ gw_free(my_buffer); **>+ gw_free(my_buffer2); **>+ gw_free(msgtext); **>+ gw_free(mcl); **>+ gw_free(snumbits); **>+ gw_free(xser); **> return strlen(rawmessage); **>} **> **>@@ -1128,7 +1209,10 @@ **>{ **> int hmtg = 0; **> unsigned int mychar; **>- char tmpbuff[128]; **>+ char *tmpbuff; **>+ **>+ tmpbuff = gw_malloc(128); **>+ memset(tmpbuff, 0, 128); **> **> for (hmtg = 0; hmtg <= (int)strlen(from); hmtg += 2) { **> strncpy(tmpbuff, from + hmtg, 2); **>@@ -1138,6 +1222,7 @@ **> **> to[(hmtg / 2)-1] = '\0'; **> **>+ gw_free(tmpbuff); **> return 0; **> **>} **>@@ -1148,22 +1233,25 @@ **>static int parse_iso88591_to_emi(char *from, char *to, **> int length, int alt_charset) **>{ **>- char buf[10]; **>+ char *buf; **> unsigned char tmpchar; **> char *ptr; **> **> if (!from || !to || length <= 0) **> return -1; **> **>+ buf = gw_malloc(10); **>+ memset(buf, 0, 10); **> *to = '\0'; **> **> debug("bb.sms.emi", 0, "emi parsing <%s> to emi, length %d", from, **> length); **> **> for (ptr = from; length > 0; ptr++, length--) { **> tmpchar = char_iso_to_sms(*ptr, alt_charset); **>- sprintf(buf, "%02X", tmpchar); **>+ snprintf(buf, 10 - 1, "%02X", tmpchar); **> strncat(to, buf, 2); **> } **>+ gw_free(buf); **> return 0; **>} buf is being used to store the hexadecimal representation of tmpchar a single byte. As a result, it shouldn't overflow a 10 byte buffer. If you really want the "belt-and-suspender" method: sprintf(buf, "%02X", tmpchar&0xFF); **> **>@@ -1172,19 +1260,22 @@ **>*/ **>static int parse_binary_to_emi(char *from, char *to, int length) **>{ **>- char buf[10]; **>+ char *buf; **> char *ptr; **> **> if (!from || !to || length <= 0) **> return -1; **> **>+ buf = gw_malloc(10); **>+ memset(buf, 0, 10); **> *to = '\0'; **> **> for (ptr = from; length > 0; ptr++, length--) { **>- sprintf(buf, "%02X", (unsigned char)*ptr); **>+ snprintf(buf, 10 - 1, "%02X", (unsigned char)*ptr); **> strncat(to, buf, 2); **> } **> **>+ gw_free(buf); **> return 0; **>} **> Same as above. This is only a hexadecimal conversion with a fixed length value. snprintf() is overkill. **> **>-- **>Humberto Figuera - Using Linux 2.6.16 **>Usuario GNU/Linux 369709 **>Caracas - Venezuela **>GPG Key Fingerprint = 5AAC DF0C 00F4 2834 28BA 37AD 3364 01D1 74CA 0603 **> **>
