Re: [PATCH] net: tftp: fix option validation as per RFCs

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(" ");
  

•                   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) {
  

-- 2.17.1

Reviewed-By: Ramon Fried rfried.dev@gmail.com