tftp: Handle TFTP servers that don't switch ports

If we get into the dissect_tftp call, we must have either matched
a WRQ/RRQ at some point and created a wildcarded UDP conversation,
or we matched the TFTP port. While it is contrary to the spirit
of RFC 1350 for the server not to switch ports, it basically works
and the port is IANA assigned, so it doesn't do harm to process these.
In the heuristic dissector, of course, we don't do this.

The conversation code doesn't automatically fill in wildcarded
ports for UDP (since it's connectionless), and the wildcarded
find_conversation call in the TFTP dissector was twisted around
so it didn't actually fill in the second port before anyway.
Filling in the server port would make sense, but then the necessary
logic to find the right conversations would be more complicated.
(The default find_conversation logic prefers any conversation with
both ports to a wildcarded conversation, but the TFTP dissector would
then want the most recent conversation, whether wildcarded or with
both ports.)

These packets were handled prior to the 3.6 changes. Fix #18122

1 files changed, 30 insertions, 15 deletions

diff --git a/epan/dissectors/packet-tftp.c b/epan/dissectors/packet-tftp.c
index 6d1b0308bd..bb91c04647 100644
--- a/epan/dissectors/packet-tftp.c
+++ b/epan/dissectors/packet-tftp.c
@@ -918,26 +918,41 @@ dissect_tftp(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree, void* data _U_
   if (conversation == NULL)
   {
     /* Not the initial read or write request */
-    if (!PINFO_FD_VISITED(pinfo)) {
-      /* During first pass, look for conversation based upon client port */
-      conversation = find_conversation(pinfo->num, &pinfo->src, &pinfo->dst, ENDPOINT_UDP,
-                                       pinfo->destport, 0, NO_PORT_B);
-      if (conversation != NULL) {
-        /* Set other side of conversation (server port) */
-        if (pinfo->destport == conversation_key_port1(conversation->key_ptr))
-          conversation_set_port2(conversation, pinfo->srcport);
-        else
-          /* Direction of conv match must have been wrong - ignore! */
-          return 0;
-      }
-    }
-    if (conversation == NULL) {
+    /* Look for wildcarded conversation based upon client port */
+    if ((conversation = find_conversation(pinfo->num, &pinfo->dst, &pinfo->src, ENDPOINT_UDP,
+                                     pinfo->destport, 0, NO_PORT_B)) && conversation_get_dissector(conversation, pinfo->num) == tftp_handle) {
+#if 0
+      /* XXX: While setting the wildcarded port makes sense, if we do that,
+       * it's more complicated to find the correct conversation if ports are
+       * reused. (find_conversation with full information prefers any exact
+       * match, even with an earlier setup frame, to any wildcarded match.)
+       * We would want to find the most recent conversations with one wildcard
+       * and with both ports, and take the latest of those.
+       */
+      /* Set other side of conversation (server port) */
+      if (pinfo->destport == conversation_key_port1(conversation->key_ptr))
+        conversation_set_port2(conversation, pinfo->srcport);
+#endif
+    } else if ((conversation = find_conversation(pinfo->num, &pinfo->src, &pinfo->dst, ENDPOINT_UDP,
+                                     pinfo->srcport, 0, NO_PORT_B)) && conversation_get_dissector(conversation, pinfo->num) == tftp_handle) {
+
+    } else {
+      /* How did we get here? We must have matched one of the TFTP ports
+       * and missed the WRQ/RRQ. While it is contrary to the spirit of
+       * RFC 1350 for the server not to change ports, there appear to be
+       * such servers out there (issue #18122), and since the default port
+       * is IANA assigned it doesn't do harm to process it. Note that in
+       * that case the conversation won't have the tftp dissector set. */
       conversation = find_conversation_pinfo(pinfo, 0);
-      if (conversation == NULL || conversation_get_dissector(conversation, pinfo->num) != tftp_handle)
+      if (conversation == NULL) {
         return 0;
+      }
     }
   }
 
+  if (pinfo->num > conversation->last_frame) {
+    conversation->last_frame = pinfo->num;
+  }
   dissect_tftp_message(tftp_info_for_conversation(conversation), tvb, pinfo, tree);
   return tvb_captured_length(tvb);
 }