[riot-notifications] [RIOT-OS/RIOT] gnrc_tftp: Fix out-of-bounds memory access when comparing modes (#11737)

Sebastian Meiling notifications at github.com
Tue Jun 25 14:59:29 CEST 2019


smlng requested changes on this pull request.

generally looks good

> @@ -233,7 +233,7 @@ static tftp_state _tftp_send_error(tftp_context_t *ctxt, gnrc_pktsnip_t *buf, tf
 static tftp_state _tftp_send(gnrc_pktsnip_t *buf, tftp_context_t *ctxt, size_t len);
 
 /* decode the default TFTP start packet */
-static int _tftp_decode_start(tftp_context_t *ctxt, uint8_t *buf, gnrc_pktsnip_t *outbuf);
+static int _tftp_decode_start(tftp_context_t *ctxt, uint8_t *buf, size_t len, gnrc_pktsnip_t *outbuf);

I would rather change the function to

```
static int _tftp_decode_start(tftp_context_t *ctxt, uint8_t *buf,  gnrc_pktsnip_t *inpkt, gnrc_pktsnip_t *outbuf);
```

and in that function have

```
size_t len = inpkt->len
uint8_t *buf = inpkt->buf
```

what do you think?

> @@ -1047,6 +1047,9 @@ int _tftp_decode_start(tftp_context_t *ctxt, uint8_t *buf, gnrc_pktsnip_t *outbu
 
     /* decode the TFTP transfer mode */
     for (uint32_t idx = 0; idx < ARRAY_LEN(_tftp_modes); ++idx) {
+        if (_tftp_modes[idx].len > (len - sizeof(*hdr) - fnlen))

coding style requires braces even for one-line `if`s

> @@ -233,7 +233,7 @@ static tftp_state _tftp_send_error(tftp_context_t *ctxt, gnrc_pktsnip_t *buf, tf
 static tftp_state _tftp_send(gnrc_pktsnip_t *buf, tftp_context_t *ctxt, size_t len);
 
 /* decode the default TFTP start packet */
-static int _tftp_decode_start(tftp_context_t *ctxt, uint8_t *buf, gnrc_pktsnip_t *outbuf);
+static int _tftp_decode_start(tftp_context_t *ctxt, uint8_t *buf, size_t len, gnrc_pktsnip_t *outbuf);

or instead of the latter cast directly

```
tftp_header_t *hdr = (tftp_header_t *)inpkt->buf;
```

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/11737#pullrequestreview-253983527
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190625/bd4e8e15/attachment.html>


More information about the notifications mailing list