[PATCH] net: tftp: fix option validation as per RFCs
RFC2348, RFC2349: - Option string is case in-sensitive. - Client must generate ERR pkt in case option value mismatch in server OACK - Fix debug print for options
Signed-off-by: Ravik Hasija rahasij@linux.microsoft.com ---
net/tftp.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 52 insertions(+), 7 deletions(-)
diff --git a/net/tftp.c b/net/tftp.c index be24e63075..14621e2441 100644 --- a/net/tftp.c +++ b/net/tftp.c @@ -68,6 +68,7 @@ enum { TFTP_ERR_UNEXPECTED_OPCODE = 4, TFTP_ERR_UNKNOWN_TRANSFER_ID = 5, TFTP_ERR_FILE_ALREADY_EXISTS = 6, + TFTP_ERR_OPTION_NEGOTIATION = 8, };
static struct in_addr tftp_remote_ip; @@ -111,6 +112,7 @@ static int tftp_put_final_block_sent; #define STATE_OACK 5 #define STATE_RECV_WRQ 6 #define STATE_SEND_WRQ 7 +#define STATE_INVALID_OPTION 8
/* default TFTP block size */ #define TFTP_BLOCK_SIZE 512 @@ -313,6 +315,7 @@ static void tftp_send(void) uchar *xp; int len = 0; ushort *s; + bool err_pkt = false;
/* * We will always be sending some sort of packet, so @@ -383,6 +386,7 @@ static void tftp_send(void) strcpy((char *)pkt, "File too large"); pkt += 14 /*strlen("File too large")*/ + 1; len = pkt - xp; + err_pkt = true; break;
case STATE_BAD_MAGIC: @@ -394,11 +398,28 @@ static void tftp_send(void) strcpy((char *)pkt, "File has bad magic"); pkt += 18 /*strlen("File has bad magic")*/ + 1; len = pkt - xp; + err_pkt = true; + break; + + case STATE_INVALID_OPTION: + xp = pkt; + s = (ushort *)pkt; + *s++ = htons(TFTP_ERROR); + *s++ = htons(TFTP_ERR_OPTION_NEGOTIATION); + pkt = (uchar *)s; + strcpy((char *)pkt, "Option Negotiation Failed"); + /* strlen("Option Negotiation Failed") + NULL*/ + pkt += 25 + 1; + len = pkt - xp; + err_pkt = true; break; }
net_send_udp_packet(net_server_ethaddr, tftp_remote_ip, tftp_remote_port, tftp_our_port, len); + + if (err_pkt) + net_set_state(NETLOOP_FAIL); }
#ifdef CONFIG_CMD_TFTPPUT @@ -419,6 +440,7 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip, __be16 proto; __be16 *s; int i; + u16 timeout_val_rcvd;
if (dest != tftp_our_port) { return; @@ -475,8 +497,14 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip, #endif
case TFTP_OACK: - debug("Got OACK: %s %s\n", - pkt, pkt + strlen((char *)pkt) + 1); + debug("Got OACK: "); + for (i = 0; i < len; i++) { + if (pkt[i] == '\0') + debug(" "); + else + debug("%c", pkt[i]); + } + debug("\n"); tftp_state = STATE_OACK; tftp_remote_port = src; /* @@ -485,15 +513,32 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip, * something like "len-8" may give a *huge* number */ for (i = 0; i+8 < len; i++) { - if (strcmp((char *)pkt + i, "blksize") == 0) { + if (strcasecmp((char *)pkt + i, "blksize") == 0) { tftp_block_size = (unsigned short) simple_strtoul((char *)pkt + i + 8, NULL, 10); - debug("Blocksize ack: %s, %d\n", + debug("Blocksize oack: %s, %d\n", (char *)pkt + i + 8, tftp_block_size); + if (tftp_block_size > tftp_block_size_option) { + printf("Invalid blk size(=%d)\n", + tftp_block_size); + tftp_state = STATE_INVALID_OPTION; + } + } + if (strcasecmp((char *)pkt + i, "timeout") == 0) { + timeout_val_rcvd = (unsigned short) + simple_strtoul((char *)pkt + i + 8, + NULL, 10); + debug("Timeout oack: %s, %d\n", + (char *)pkt + i + 8, timeout_val_rcvd); + if (timeout_val_rcvd != (timeout_ms / 1000)) { + printf("Invalid timeout val(=%d s)\n", + timeout_val_rcvd); + tftp_state = STATE_INVALID_OPTION; + } } #ifdef CONFIG_TFTP_TSIZE - if (strcmp((char *)pkt+i, "tsize") == 0) { + if (strcasecmp((char *)pkt + i, "tsize") == 0) { tftp_tsize = simple_strtoul((char *)pkt + i + 6, NULL, 10); debug("size = %s, %d\n", @@ -502,7 +547,7 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip, #endif } #ifdef CONFIG_CMD_TFTPPUT - if (tftp_put_active) { + if (tftp_put_active && tftp_state == STATE_OACK) { /* Get ready to send the first block */ tftp_state = STATE_DATA; tftp_cur_block++; @@ -519,7 +564,7 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip, update_block_number();
if (tftp_state == STATE_SEND_RRQ) - debug("Server did not acknowledge timeout option!\n"); + debug("Server did not acknowledge any options!\n");
if (tftp_state == STATE_SEND_RRQ || tftp_state == STATE_OACK || tftp_state == STATE_RECV_WRQ) {
On Tue, May 19, 2020 at 7:36 AM Ravik Hasija rahasij@linux.microsoft.com wrote:
RFC2348, RFC2349:
- Option string is case in-sensitive.
- Client must generate ERR pkt in case option value mismatch in server OACK
- Fix debug print for options
Signed-off-by: Ravik Hasija rahasij@linux.microsoft.com
net/tftp.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 52 insertions(+), 7 deletions(-)
diff --git a/net/tftp.c b/net/tftp.c index be24e63075..14621e2441 100644 --- a/net/tftp.c +++ b/net/tftp.c @@ -68,6 +68,7 @@ enum { TFTP_ERR_UNEXPECTED_OPCODE = 4, TFTP_ERR_UNKNOWN_TRANSFER_ID = 5, TFTP_ERR_FILE_ALREADY_EXISTS = 6,
TFTP_ERR_OPTION_NEGOTIATION = 8,};
static struct in_addr tftp_remote_ip; @@ -111,6 +112,7 @@ static int tftp_put_final_block_sent; #define STATE_OACK 5 #define STATE_RECV_WRQ 6 #define STATE_SEND_WRQ 7 +#define STATE_INVALID_OPTION 8
/* default TFTP block size */ #define TFTP_BLOCK_SIZE 512 @@ -313,6 +315,7 @@ static void tftp_send(void) uchar *xp; int len = 0; ushort *s;
bool err_pkt = false; /* * We will always be sending some sort of packet, so@@ -383,6 +386,7 @@ static void tftp_send(void) strcpy((char *)pkt, "File too large"); pkt += 14 /*strlen("File too large")*/ + 1; len = pkt - xp;
err_pkt = true; break; case STATE_BAD_MAGIC:@@ -394,11 +398,28 @@ static void tftp_send(void) strcpy((char *)pkt, "File has bad magic"); pkt += 18 /*strlen("File has bad magic")*/ + 1; len = pkt - xp;
err_pkt = true;break;case STATE_INVALID_OPTION:xp = pkt;s = (ushort *)pkt;*s++ = htons(TFTP_ERROR);*s++ = htons(TFTP_ERR_OPTION_NEGOTIATION);pkt = (uchar *)s;strcpy((char *)pkt, "Option Negotiation Failed");/* strlen("Option Negotiation Failed") + NULL*/pkt += 25 + 1;len = pkt - xp;err_pkt = true; break; } net_send_udp_packet(net_server_ethaddr, tftp_remote_ip, tftp_remote_port, tftp_our_port, len);if (err_pkt)net_set_state(NETLOOP_FAIL);}
#ifdef CONFIG_CMD_TFTPPUT @@ -419,6 +440,7 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip, __be16 proto; __be16 *s; int i;
u16 timeout_val_rcvd; if (dest != tftp_our_port) { return;@@ -475,8 +497,14 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip, #endif
case TFTP_OACK:
debug("Got OACK: %s %s\n",pkt, pkt + strlen((char *)pkt) + 1);
debug("Got OACK: ");for (i = 0; i < len; i++) {if (pkt[i] == '\0')debug(" ");elsedebug("%c", pkt[i]);}debug("\n"); tftp_state = STATE_OACK; tftp_remote_port = src; /*@@ -485,15 +513,32 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip, * something like "len-8" may give a *huge* number */ for (i = 0; i+8 < len; i++) {
if (strcmp((char *)pkt + i, "blksize") == 0) {
if (strcasecmp((char *)pkt + i, "blksize") == 0) { tftp_block_size = (unsigned short) simple_strtoul((char *)pkt + i + 8, NULL, 10);
debug("Blocksize ack: %s, %d\n",
debug("Blocksize oack: %s, %d\n", (char *)pkt + i + 8, tftp_block_size);if (tftp_block_size > tftp_block_size_option) {printf("Invalid blk size(=%d)\n",tftp_block_size);tftp_state = STATE_INVALID_OPTION;}}if (strcasecmp((char *)pkt + i, "timeout") == 0) {timeout_val_rcvd = (unsigned short)simple_strtoul((char *)pkt + i + 8,NULL, 10);debug("Timeout oack: %s, %d\n",(char *)pkt + i + 8, timeout_val_rcvd);if (timeout_val_rcvd != (timeout_ms / 1000)) {printf("Invalid timeout val(=%d s)\n",timeout_val_rcvd);tftp_state = STATE_INVALID_OPTION;} }#ifdef CONFIG_TFTP_TSIZE
if (strcmp((char *)pkt+i, "tsize") == 0) {
if (strcasecmp((char *)pkt + i, "tsize") == 0) { tftp_tsize = simple_strtoul((char *)pkt + i + 6, NULL, 10); debug("size = %s, %d\n",@@ -502,7 +547,7 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip, #endif } #ifdef CONFIG_CMD_TFTPPUT
if (tftp_put_active) {
if (tftp_put_active && tftp_state == STATE_OACK) { /* Get ready to send the first block */ tftp_state = STATE_DATA; tftp_cur_block++;@@ -519,7 +564,7 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip, update_block_number();
if (tftp_state == STATE_SEND_RRQ)
debug("Server did not acknowledge timeout option!\n");
debug("Server did not acknowledge any options!\n"); if (tftp_state == STATE_SEND_RRQ || tftp_state == STATE_OACK || tftp_state == STATE_RECV_WRQ) {-- 2.17.1
Reviewed-By: Ramon Fried rfried.dev@gmail.com
On Mon, May 18, 2020 at 09:35:43PM -0700, Ravik Hasija wrote:
RFC2348, RFC2349:
- Option string is case in-sensitive.
- Client must generate ERR pkt in case option value mismatch in server OACK
- Fix debug print for options
Signed-off-by: Ravik Hasija rahasij@linux.microsoft.com Reviewed-By: Ramon Fried rfried.dev@gmail.com
Applied to u-boot/next, thanks!
participants (3)
-
Ramon Fried -
Ravik Hasija -
Tom Rini