Skip to content

Commit 65b948d

Browse files
MarkusLassilanordicjm
MarkusLassila
authored and
nordicjm
committed
net: lib: ftp_client: Fix vulnerabilities
- sprintf -> snprintf (boundary checks). - Improve pasv_msg validation. - Add input parameter validation. - Fix race condition with PASV response being overwritten. Signed-off-by: Markus Lassila <markus.lassila@nordicsemi.no>
1 parent beb8ab3 commit 65b948d

File tree

1 file changed

+155
-44
lines changed

1 file changed

+155
-44
lines changed

‎subsys/net/lib/ftp_client/src/ftp_client.c‎

Lines changed: 155 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,13 @@ LOG_MODULE_REGISTER(ftp_client, CONFIG_FTP_CLIENT_LOG_LEVEL);
2222

2323
#define FTP_CODE_ANY 0
2424

25+
/* FTP parameter length limits */
26+
#define FTP_MAX_USERNAME 64 /* Unix/Linux username typical limit */
27+
#define FTP_MAX_PASSWORD 255 /* Allow longer passwords */
28+
#define FTP_MAX_FILENAME 255 /* Filename length limit */
29+
#define FTP_MAX_PATHNAME 255 /* Standard NAME_MAX */
30+
#define FTP_MAX_OPTIONS 32 /* FTP LIST options are typically short */
31+
2532
#define FTP_STACK_SIZE KB(2)
2633
#define FTP_PRIORITY K_LOWEST_APPLICATION_THREAD_PRIO
2734
static K_THREAD_STACK_DEFINE(ftp_stack_area, FTP_STACK_SIZE);
@@ -51,14 +58,32 @@ enum data_task_type {
5158
static struct data_task {
5259
struct k_work work;
5360
enum data_task_type task;
54-
char *ctrl_msg; /* PSAV resposne */
61+
char ctrl_msg[128]; /* PASV response copy */
5562
uint8_t *data; /* TX data */
5663
uint16_t length; /* TX length */
5764
} data_task_param;
5865

5966
static bool ftp_inactivity;
6067
static atomic_t data_task_running;
6168

69+
static int validate_ftp_param(const char *param, size_t max_len)
70+
{
71+
if (param == NULL) {
72+
return -EINVAL;
73+
}
74+
75+
if (strlen(param) > max_len) {
76+
return -EINVAL;
77+
}
78+
79+
/* Check for FTP command injection - CR/LF can inject additional commands */
80+
if (strchr(param, '\r') || strchr(param, '\n')) {
81+
return -EINVAL;
82+
}
83+
84+
return 0;
85+
}
86+
6287
static int parse_return_code(const uint8_t *message, int success_code)
6388
{
6489
if (success_code == FTP_CODE_ANY) {
@@ -92,7 +117,8 @@ static int parse_return_code(const uint8_t *message, int success_code)
92117
static int establish_data_channel(const char *pasv_msg)
93118
{
94119
int ret;
95-
char tmp[8];
120+
char *endptr;
121+
long port_byte;
96122
char *tmp1, *tmp2;
97123
uint16_t data_port;
98124

@@ -109,19 +135,33 @@ static int establish_data_channel(const char *pasv_msg)
109135
if (tmp2 == NULL) {
110136
return -EINVAL;
111137
}
112-
memset(tmp, 0x00, sizeof(tmp));
113-
strncpy(tmp, (const char *)(tmp2 + 1), (size_t)(tmp1 - tmp2 - 1));
114-
data_port = atoi(tmp);
138+
/* Validate that ')' appears after the last ',' */
139+
if (tmp1 <= tmp2) {
140+
return -EINVAL;
141+
}
142+
143+
/* Extract low byte of port (p2) */
144+
port_byte = strtol(tmp2 + 1, &endptr, 10);
145+
if (endptr != tmp1 || port_byte < 0 || port_byte > 255) {
146+
return -EINVAL;
147+
}
148+
data_port = (uint16_t)port_byte;
149+
150+
/* Find previous comma for high byte */
115151
tmp1 = tmp2 - 1;
116-
while (isdigit((int)(*tmp1))) {
152+
while (tmp1 > pasv_msg && isdigit((int)(*tmp1))) {
117153
tmp1--;
118154
}
119-
if (*tmp1 != ',') {
155+
if (tmp1 <= pasv_msg || *tmp1 != ',') {
120156
return -EINVAL;
121157
}
122-
memset(tmp, 0x00, sizeof(tmp));
123-
strncpy(tmp, (const char *)(tmp1 + 1), (size_t)(tmp2 - tmp1 - 1));
124-
data_port += atoi(tmp) << 8;
158+
159+
/* Extract high byte of port (p1) */
160+
port_byte = strtol(tmp1 + 1, &endptr, 10);
161+
if (endptr != tmp2 || port_byte < 0 || port_byte > 255) {
162+
return -EINVAL;
163+
}
164+
data_port += (uint16_t)(port_byte << 8);
125165
LOG_DBG("data port: %d", data_port);
126166

127167
/* open data socket */
@@ -171,22 +211,25 @@ static void close_connection(int code, int error)
171211
if (FTP_PROPRIETARY(code)) {
172212
switch (code) {
173213
case FTP_CODE_901:
174-
sprintf(ctrl_buf, "901 Disconnected(%d).\r\n", error);
214+
snprintf(ctrl_buf, sizeof(ctrl_buf), "901 Disconnected(%d).\r\n", error);
175215
break;
176216
case FTP_CODE_902:
177-
sprintf(ctrl_buf, "902 Connection aborted(%d).\r\n", error);
217+
snprintf(ctrl_buf, sizeof(ctrl_buf), "902 Connection aborted(%d).\r\n",
218+
error);
178219
break;
179220
case FTP_CODE_903:
180-
sprintf(ctrl_buf, "903 Poll error(%d).\r\n", error);
221+
snprintf(ctrl_buf, sizeof(ctrl_buf), "903 Poll error(%d).\r\n", error);
181222
break;
182223
case FTP_CODE_904:
183-
sprintf(ctrl_buf, "904 Unexpected poll event(%d).\r\n", error);
224+
snprintf(ctrl_buf, sizeof(ctrl_buf), "904 Unexpected poll event(%d).\r\n",
225+
error);
184226
break;
185227
case FTP_CODE_905:
186-
sprintf(ctrl_buf, "905 Network down (%d).\r\n", error);
228+
snprintf(ctrl_buf, sizeof(ctrl_buf), "905 Network down (%d).\r\n", error);
187229
break;
188230
default:
189-
sprintf(ctrl_buf, "900 Unknown error(%d).\r\n", -ENOEXEC);
231+
snprintf(ctrl_buf, sizeof(ctrl_buf), "900 Unknown error(%d).\r\n",
232+
-ENOEXEC);
190233
break;
191234
}
192235
client.ctrl_callback(ctrl_buf, strlen(ctrl_buf));
@@ -507,7 +550,7 @@ int ftp_open(const char *hostname, uint16_t port, int sec_tag)
507550
}
508551

509552
/* Send UTF8 option */
510-
sprintf(ctrl_buf, CMD_OPTS, "UTF8 ON");
553+
snprintf(ctrl_buf, sizeof(ctrl_buf), CMD_OPTS, "UTF8 ON");
511554
ret = do_ftp_send_ctrl(ctrl_buf, strlen(ctrl_buf));
512555
if (ret) {
513556
zsock_close(client.cmd_sock);
@@ -524,16 +567,26 @@ int ftp_login(const char *username, const char *password)
524567
int ret;
525568
int keepalive_time = CONFIG_FTP_CLIENT_KEEPALIVE_TIME;
526569

570+
/* Validate inputs */
571+
ret = validate_ftp_param(username, FTP_MAX_USERNAME);
572+
if (ret) {
573+
return ret;
574+
}
575+
ret = validate_ftp_param(password, FTP_MAX_PASSWORD);
576+
if (ret) {
577+
return ret;
578+
}
579+
527580
/* send username */
528-
sprintf(ctrl_buf, CMD_USER, username);
581+
snprintf(ctrl_buf, sizeof(ctrl_buf), CMD_USER, username);
529582
ret = do_ftp_send_ctrl(ctrl_buf, strlen(ctrl_buf));
530583
if (ret) {
531584
return ret;
532585
}
533586
ret = do_ftp_recv_ctrl(true, FTP_CODE_331);
534587
if (ret == FTP_CODE_331) {
535588
/* send password if requested */
536-
sprintf(ctrl_buf, CMD_PASS, password);
589+
snprintf(ctrl_buf, sizeof(ctrl_buf), CMD_PASS, password);
537590
ret = do_ftp_send_ctrl(ctrl_buf, strlen(ctrl_buf));
538591
if (ret) {
539592
return ret;
@@ -630,7 +683,16 @@ int ftp_pwd(void)
630683
int ftp_list(const char *options, const char *target)
631684
{
632685
int ret;
633-
char list_cmd[128];
686+
687+
/* Validate inputs */
688+
ret = validate_ftp_param(options, FTP_MAX_OPTIONS);
689+
if (ret) {
690+
return ret;
691+
}
692+
ret = validate_ftp_param(target, FTP_MAX_PATHNAME);
693+
if (ret) {
694+
return ret;
695+
}
634696

635697
/* Always set Passive mode to act as TCP client */
636698
ret = do_ftp_send_ctrl(CMD_PASV, sizeof(CMD_PASV) - 1);
@@ -641,24 +703,25 @@ int ftp_list(const char *options, const char *target)
641703
if (ret != FTP_CODE_227) {
642704
return ret;
643705
}
644-
data_task_param.ctrl_msg = ctrl_buf;
706+
strncpy(data_task_param.ctrl_msg, ctrl_buf, sizeof(data_task_param.ctrl_msg) - 1);
707+
data_task_param.ctrl_msg[sizeof(data_task_param.ctrl_msg) - 1] = '\0';
645708
data_task_param.task = TASK_RECEIVE;
646709

647710
/* Send LIST/NLST command in control channel */
648711
if (strlen(options) != 0) {
649712
if (strlen(target) != 0) {
650-
sprintf(list_cmd, CMD_LIST_OPT_FILE, options, target);
713+
snprintf(ctrl_buf, sizeof(ctrl_buf), CMD_LIST_OPT_FILE, options, target);
651714
} else {
652-
sprintf(list_cmd, CMD_LIST_OPT, options);
715+
snprintf(ctrl_buf, sizeof(ctrl_buf), CMD_LIST_OPT, options);
653716
}
654717
} else {
655718
if (strlen(target) != 0) {
656-
sprintf(list_cmd, CMD_LIST_FILE, target);
719+
snprintf(ctrl_buf, sizeof(ctrl_buf), CMD_LIST_FILE, target);
657720
} else {
658-
strcpy(list_cmd, CMD_NLST);
721+
snprintf(ctrl_buf, sizeof(ctrl_buf), CMD_NLST);
659722
}
660723
}
661-
ret = do_ftp_send_ctrl(list_cmd, strlen(list_cmd));
724+
ret = do_ftp_send_ctrl(ctrl_buf, strlen(ctrl_buf));
662725

663726
return run_data_task();
664727
}
@@ -667,10 +730,16 @@ int ftp_cwd(const char *folder)
667730
{
668731
int ret;
669732

733+
/* Validate input */
734+
ret = validate_ftp_param(folder, FTP_MAX_PATHNAME);
735+
if (ret) {
736+
return ret;
737+
}
738+
670739
if (strcmp(folder, "..") == 0) {
671740
ret = do_ftp_send_ctrl(CMD_CDUP, sizeof(CMD_CDUP) - 1);
672741
} else {
673-
sprintf(ctrl_buf, CMD_CWD, folder);
742+
snprintf(ctrl_buf, sizeof(ctrl_buf), CMD_CWD, folder);
674743
ret = do_ftp_send_ctrl(ctrl_buf, strlen(ctrl_buf));
675744
}
676745
if (ret == 0) {
@@ -684,7 +753,13 @@ int ftp_mkd(const char *folder)
684753
{
685754
int ret;
686755

687-
sprintf(ctrl_buf, CMD_MKD, folder);
756+
/* Validate input */
757+
ret = validate_ftp_param(folder, FTP_MAX_PATHNAME);
758+
if (ret) {
759+
return ret;
760+
}
761+
762+
snprintf(ctrl_buf, sizeof(ctrl_buf), CMD_MKD, folder);
688763
ret = do_ftp_send_ctrl(ctrl_buf, strlen(ctrl_buf));
689764
if (ret == 0) {
690765
ret = do_ftp_recv_ctrl(true, FTP_CODE_257);
@@ -697,7 +772,13 @@ int ftp_rmd(const char *folder)
697772
{
698773
int ret;
699774

700-
sprintf(ctrl_buf, CMD_RMD, folder);
775+
/* Validate input */
776+
ret = validate_ftp_param(folder, FTP_MAX_PATHNAME);
777+
if (ret) {
778+
return ret;
779+
}
780+
781+
snprintf(ctrl_buf, sizeof(ctrl_buf), CMD_RMD, folder);
701782
ret = do_ftp_send_ctrl(ctrl_buf, strlen(ctrl_buf));
702783
if (ret == 0) {
703784
ret = do_ftp_recv_ctrl(true, FTP_CODE_250);
@@ -710,7 +791,17 @@ int ftp_rename(const char *old_name, const char *new_name)
710791
{
711792
int ret;
712793

713-
sprintf(ctrl_buf, CMD_RNFR, old_name);
794+
/* Validate inputs */
795+
ret = validate_ftp_param(old_name, FTP_MAX_PATHNAME);
796+
if (ret) {
797+
return ret;
798+
}
799+
ret = validate_ftp_param(new_name, FTP_MAX_PATHNAME);
800+
if (ret) {
801+
return ret;
802+
}
803+
804+
snprintf(ctrl_buf, sizeof(ctrl_buf), CMD_RNFR, old_name);
714805
ret = do_ftp_send_ctrl(ctrl_buf, strlen(ctrl_buf));
715806
if (ret == 0) {
716807
ret = do_ftp_recv_ctrl(true, FTP_CODE_350);
@@ -719,7 +810,7 @@ int ftp_rename(const char *old_name, const char *new_name)
719810
return ret;
720811
}
721812

722-
sprintf(ctrl_buf, CMD_RNTO, new_name);
813+
snprintf(ctrl_buf, sizeof(ctrl_buf), CMD_RNTO, new_name);
723814
ret = do_ftp_send_ctrl(ctrl_buf, strlen(ctrl_buf));
724815
if (ret == 0) {
725816
ret = do_ftp_recv_ctrl(true, FTP_CODE_250);
@@ -732,7 +823,13 @@ int ftp_delete(const char *file)
732823
{
733824
int ret;
734825

735-
sprintf(ctrl_buf, CMD_DELE, file);
826+
/* Validate input */
827+
ret = validate_ftp_param(file, FTP_MAX_PATHNAME);
828+
if (ret) {
829+
return ret;
830+
}
831+
832+
snprintf(ctrl_buf, sizeof(ctrl_buf), CMD_DELE, file);
736833
ret = do_ftp_send_ctrl(ctrl_buf, strlen(ctrl_buf));
737834
if (ret == 0) {
738835
ret = do_ftp_recv_ctrl(true, FTP_CODE_250);
@@ -744,7 +841,12 @@ int ftp_delete(const char *file)
744841
int ftp_get(const char *file)
745842
{
746843
int ret;
747-
char get_cmd[128];
844+
845+
/* Validate input */
846+
ret = validate_ftp_param(file, FTP_MAX_FILENAME);
847+
if (ret) {
848+
return ret;
849+
}
748850

749851
/* Always set Passive mode to act as TCP client */
750852
ret = do_ftp_send_ctrl(CMD_PASV, sizeof(CMD_PASV) - 1);
@@ -755,12 +857,13 @@ int ftp_get(const char *file)
755857
if (ret != FTP_CODE_227) {
756858
return ret;
757859
}
758-
data_task_param.ctrl_msg = ctrl_buf;
860+
strncpy(data_task_param.ctrl_msg, ctrl_buf, sizeof(data_task_param.ctrl_msg) - 1);
861+
data_task_param.ctrl_msg[sizeof(data_task_param.ctrl_msg) - 1] = '\0';
759862
data_task_param.task = TASK_RECEIVE;
760863

761864
/* Send RETR command in control channel */
762-
sprintf(get_cmd, CMD_RETR, file);
763-
ret = do_ftp_send_ctrl(get_cmd, strlen(get_cmd));
865+
snprintf(ctrl_buf, sizeof(ctrl_buf), CMD_RETR, file);
866+
ret = do_ftp_send_ctrl(ctrl_buf, strlen(ctrl_buf));
764867
if (ret) {
765868
return -EIO;
766869
}
@@ -771,7 +874,6 @@ int ftp_get(const char *file)
771874
int ftp_put(const char *file, const uint8_t *data, uint16_t length, int type)
772875
{
773876
int ret;
774-
char put_cmd[128];
775877

776878
if (type != FTP_PUT_NORMAL && type != FTP_PUT_UNIQUE && type != FTP_PUT_APPEND) {
777879
return -EINVAL;
@@ -781,6 +883,14 @@ int ftp_put(const char *file, const uint8_t *data, uint16_t length, int type)
781883
return -EINVAL;
782884
}
783885

886+
/* Validate file parameter if provided */
887+
if (file != NULL) {
888+
ret = validate_ftp_param(file, FTP_MAX_FILENAME);
889+
if (ret) {
890+
return ret;
891+
}
892+
}
893+
784894
/** Typical sequence:
785895
* FTP 51 Request: PASV
786896
* FTP 96 Response: 227 Entering Passive Mode (90,130,70,73,105,177).
@@ -800,24 +910,25 @@ int ftp_put(const char *file, const uint8_t *data, uint16_t length, int type)
800910
if (ret != FTP_CODE_227) {
801911
return ret;
802912
}
803-
data_task_param.ctrl_msg = ctrl_buf;
913+
strncpy(data_task_param.ctrl_msg, ctrl_buf, sizeof(data_task_param.ctrl_msg) - 1);
914+
data_task_param.ctrl_msg[sizeof(data_task_param.ctrl_msg) - 1] = '\0';
804915
data_task_param.task = TASK_SEND;
805916
data_task_param.data = (uint8_t *)data;
806917
data_task_param.length = length;
807918
}
808919

809920
if (type == FTP_PUT_NORMAL) {
810921
/* Send STOR command in control channel */
811-
sprintf(put_cmd, CMD_STOR, file);
812-
ret = do_ftp_send_ctrl(put_cmd, strlen(put_cmd));
922+
snprintf(ctrl_buf, sizeof(ctrl_buf), CMD_STOR, file);
923+
ret = do_ftp_send_ctrl(ctrl_buf, strlen(ctrl_buf));
813924
} else if (type == FTP_PUT_UNIQUE) {
814925
/* Send STOU command in control channel */
815-
sprintf(put_cmd, CMD_STOU);
816-
ret = do_ftp_send_ctrl(put_cmd, strlen(put_cmd));
926+
snprintf(ctrl_buf, sizeof(ctrl_buf), CMD_STOU);
927+
ret = do_ftp_send_ctrl(ctrl_buf, strlen(ctrl_buf));
817928
} else {
818929
/* Send APPE command in control channel */
819-
sprintf(put_cmd, CMD_APPE, file);
820-
ret = do_ftp_send_ctrl(put_cmd, strlen(put_cmd));
930+
snprintf(ctrl_buf, sizeof(ctrl_buf), CMD_APPE, file);
931+
ret = do_ftp_send_ctrl(ctrl_buf, strlen(ctrl_buf));
821932
}
822933
if (ret != 0) {
823934
return ret;

0 commit comments

Comments
 (0)