Wireshark · Wireshark-dev: [Wireshark-dev] I think the following patch fixes the longstanding bug in the SPNEGO dissector

[Richard Sharpe <realrichardsharpe@xxxxxxxxx>]

Hi folks,

Thanks to the hint from Thomas Kukosa, I now seem to have the SPNEGO
dissector doing the correct things. It now dissects the NegTokenInit2
in a NegotiateProtocol response and the NegTokenInit in a
SessionSetupRequest.

Attached is a patch. I will add the individual patch files to the bug
when I find it again.

-- 
Regards,
Richard Sharpe
(何以解憂？唯有杜康。--曹操)

diff --git a/asn1/spnego/packet-spnego-template.c b/asn1/spnego/packet-spnego-template.c
index ead29a3..ac2d9b1 100644
--- a/asn1/spnego/packet-spnego-template.c
+++ b/asn1/spnego/packet-spnego-template.c
@@ -96,13 +96,16 @@ static gint ett_spnego_krb5_cfx_flags = -1;
 #include "packet-spnego-ett.c"
 
 /*
- * Unfortunately, we have to have a forward declaration of this,
+ * Unfortunately, we have to have forward declarations of thess,
  * as the code generated by asn2wrs includes a call before the
  * definition.
  */
-static int dissect_spnego_PrincipalSeq(gboolean implicit_tag, tvbuff_t *tvb,
+static int dissect_spnego_NegTokenInit(gboolean implicit_tag, tvbuff_t *tvb,
                                        int offset, asn1_ctx_t *actx _U_,
                                        proto_tree *tree, int hf_index);
+static int dissect_spnego_NegTokenInit2(gboolean implicit_tag, tvbuff_t *tvb,
+                                        int offset, asn1_ctx_t *actx _U_,
+                                        proto_tree *tree, int hf_index);
 
 #include "packet-spnego-fn.c"
 /*
diff --git a/asn1/spnego/spnego.asn b/asn1/spnego/spnego.asn
index 190b3f1..99c119f 100644
--- a/asn1/spnego/spnego.asn
+++ b/asn1/spnego/spnego.asn
@@ -13,21 +13,11 @@ NegotiationToken ::= CHOICE {
 MechTypeList ::= SEQUENCE OF MechType
 
 --
--- In some cases, the mechListMIC is a sequence of GeneralString,
--- rather than an OCTET STRING.  We define that sequence here so
--- that we can call its dissector.
--- The IRC discussion at
+-- MS-SPNG tells us that the format of a negTokenInit is actually
+-- negTokenInit2 if a negTokenInit is seen in a response. It might need
+-- to be the first negTokenInit seen in a response, but I am not sure. 
+-- It will only occur in a NegotiateProtocol response in CIFS/SMB or SMB2.
 --
---	http://irc.vernstok.nl/samba-technical.dy
---
--- seems to suggest that it's a Kerberos principal of some sort, thanks
--- to some flavor of "embrace, extend, expectorate" sequence from
--- Microsoft.
---
-PrincipalSeq ::= SEQUENCE {
-	principal [0] GeneralString
-}
-
 NegTokenInit ::= SEQUENCE {
                             mechTypes       [0] MechTypeList  OPTIONAL,
                             reqFlags        [1] ContextFlags  OPTIONAL,
@@ -35,6 +25,19 @@ NegTokenInit ::= SEQUENCE {
                             mechListMIC     [3] OCTET STRING  OPTIONAL
                          }
 
+NegHints ::= SEQUENCE {
+        hintName        [0] GeneralString OPTIONAL,
+        hintAddress     [1] OCTET STRING OPTIONAL
+}
+
+NegTokenInit2 ::= SEQUENCE {
+        mechTypes       [0] MechTypeList OPTIONAL,
+        reqFlags        [1] ContextFlags OPTIONAL,
+        mechToken       [2] OCTET STRING OPTIONAL,
+        negHints        [3] NegHints OPTIONAL,
+        mechListMIC     [4] OCTET STRING OPTIONAL
+}
+
 ContextFlags ::= BIT STRING {
         delegFlag       (0),
         mutualFlag      (1),
diff --git a/asn1/spnego/spnego.cnf b/asn1/spnego/spnego.cnf
index a4fcc0b..5c633f5 100644
--- a/asn1/spnego/spnego.cnf
+++ b/asn1/spnego/spnego.cnf
@@ -8,11 +8,22 @@
 #.NO_EMIT ONLY_VALS
 NegotiationToken
 
-#.TYPE_RENAME
-NegTokenInit/mechListMIC T_NegTokenInit_mechListMIC
+#.FN_BODY NegotiationToken/negTokenInit
+  gboolean is_response = actx->pinfo->ptype == PT_TCP && 
+                         actx->pinfo->srcport < 1024;
 
-#.FIELD_RENAME
-NegTokenInit/mechListMIC negTokenInit_mechListMIC
+  /*
+   * We decode as negTokenInit2 or negTokenInit depending on whether or not
+   * we are in a response or a request. That is essentially what MS-SPNG
+   * says.
+   */
+  if (is_response) {
+    return dissect_spnego_NegTokenInit2(%(IMPLICIT_TAG)s, %(TVB)s, %(OFFSET)s,
+                                        %(ACTX)s, %(TREE)s, %(HF_INDEX)s);
+  } else {
+    return dissect_spnego_NegTokenInit(%(IMPLICIT_TAG)s, %(TVB)s, %(OFFSET)s,
+                                       %(ACTX)s, %(TREE)s, %(HF_INDEX)s);
+  }
 
 #.FN_PARS MechType
 
@@ -121,42 +132,28 @@ NegTokenInit/mechListMIC negTokenInit_mechListMIC
      call_dissector(next_level_value->handle, mechToken_tvb, actx->pinfo, tree);
 
 
+#.FN_PARS NegTokenInit/mechListMIC
+
+  VAL_PTR = &mechListMIC_tvb
+
 #.FN_BODY NegTokenInit/mechListMIC
 
-  gint8 ber_class;
-  gboolean pc;
-  gint32 tag;
   tvbuff_t *mechListMIC_tvb;
 
+
+%(DEFAULT_BODY)s
+
+
   /*
-   * There seems to be two different forms this can take,
-   * one as an octet string, and one as a general string in a
-   * sequence.
-   *
-   * Peek at the header, and then decide which it is we're seeing.
+   * Now, we should be able to dispatch, if we've gotten a tvbuff for
+   * the MIC and we have information on how to dissect its contents.
    */
-  get_ber_identifier(tvb, offset, &ber_class, &pc, &tag);
-  if (ber_class == BER_CLASS_UNI && pc && tag == BER_UNI_TAG_SEQUENCE) {
-    /*
-     * It's a sequence.
-     */
-    return dissect_spnego_PrincipalSeq(FALSE, tvb, offset, actx, tree,
-                                       hf_spnego_mechListMIC);
-  } else {
-    /*
-     * It's not a sequence, so dissect it as an octet string,
-     * which is what it's supposed to be; that'll cause the
-     * right error report if it's not an octet string, either.
-     */
-    offset = dissect_ber_octet_string(FALSE, actx, tree, tvb, offset,
-                                      hf_spnego_mechListMIC, &mechListMIC_tvb);
-
-    /*
-     * Now, we should be able to dispatch with that tvbuff.
-     */
-    if (mechListMIC_tvb && next_level_value)
-      call_dissector(next_level_value->handle, mechListMIC_tvb, actx->pinfo, tree);
-    return offset;
+  if (mechListMIC_tvb && (tvb_reported_length(mechListMIC_tvb) > 0) ){
+    gssapi_oid_value *value=next_level_value;
+
+    if(value){
+      call_dissector(value->handle, mechListMIC_tvb, actx->pinfo, tree);
+    }
   }
 
 #.FN_BODY NegTokenTarg/supportedMech