Wireshark · Wireshark-dev: Re: [Wireshark-dev] Hierarchy of fields & offsets again, more potential offenders

["Sultan, Hassan" <sultah@xxxxxxxxxx>]

Thanks for the patch Pascal !

Regarding tcp.payload, I don't think tcp.payload in itself has any problems. I think the issue lies in tcp showing a length of 32 only, even though it has tcp.payload as its child.
To me either tcp.payload shouldn't be a child of tcp, or tcp should reflect the length of all its children.

For the SMB ones, I'll wait for others to chime in, but am I wrong in assuming that a parent node should reflect the length/offset of all its children ?

> -----Original Message-----
> From: Pascal Quantin [mailto:pascal.quantin@xxxxxxxxx]
> Sent: Wednesday, August 02, 2017 12:41 PM
> To: Developer support list for Wireshark <wireshark-dev@xxxxxxxxxxxxx>
> Cc: Sultan, Hassan <sultah@xxxxxxxxxx>
> Subject: Re: [Wireshark-dev] Hierarchy of fields & offsets again, more potential
> offenders
> 
> 
> 
> 2017-08-02 21:24 GMT+02:00 Pascal Quantin <pascal.quantin@xxxxxxxxx
> <mailto:pascal.quantin@xxxxxxxxx> >:
> 
> 
> 	Hi Hassan,
> 
> 
> 	2017-08-02 1:05 GMT+02:00 Sultan, Hassan via Wireshark-dev
> <wireshark-dev@xxxxxxxxxxxxx <mailto:wireshark-dev@xxxxxxxxxxxxx> >:
> 
> 
> 		Hi all,
> 
> 		So I've started adding checks to my wrapper and am finding
> some interesting cases (they all look like issues that need to be fixed to me, but
> again, I might be missing something), would someone please take a look and tell
> me which you think are ok / bugs so I can start sending patches to fix them ?
> 
> 		The below is the output from processing the file
> https://wiki.wireshark.org/SampleCaptures?action=AttachFile&do=get&target=
> smb3-aes-128-ccm.pcap
> <https://wiki.wireshark.org/SampleCaptures?action=AttachFile&do=get&target
> =smb3-aes-128-ccm.pcap>
> 
> 		Hopefully I'll soon enough grasp the intricacies and won't need
> the repeated validation from you guys. My plan longer term is to have this as an
> automated test to process capture files so that we can catch these issues at
> build time unless anyone has an objection.
> 
> 		Thx,
> 
> 		Hassan
> 
> 		Reminder, format of the output is :
> 
> 		<ftype> <offset> <name>(<length>):
> 		        [children]
> 
> 		 FT_PROTOCOL 0 frame(172) :
> 		[...]
> 		 FT_PROTOCOL 34 tcp(32) :
> 		         FT_UINT16 34 tcp.srcport(2) : 38166
> 		         FT_UINT16 36 tcp.dstport(2) : 445
> 		        [...]
> 		         FT_BYTES 66 tcp.payload(106) :
> 		VIOLATION 2 : Child tcp.payload has an end offset past the end
> of its parent
> 		VIOLATION 3 : Child tcp.payload has a length longer than its
> parent
> 
> 
> 
> 	This is done on purpose: we separate the tvb for the TCP header, and
> the tvb for the payload. tcp.payload field gives you the length of the payload
> and highlights it in the bytes view. Most probably not something we want to
> change.
> 
> 
> 
> 		 FT_PROTOCOL 0 frame(318) :
> 		[...]
> 		 FT_PROTOCOL 66 nbss(252) :
> 		         FT_UINT8 66 nbss.type(1) : 0x00000000
> 		         FT_UINT24 67 nbss.length(3) : 248
> 		 FT_PROTOCOL 70 smb2(248) :
> 		         FT_NONE 70 [SMB2 Header](64) :
> 		        [...]
> 		         FT_NONE 134 [Session Setup Response (0x01)](184) :
> 		                [...]
> 		                 FT_BYTES 142 smb2.security_blob(176) :
> 		                         FT_PROTOCOL 142 ntlmssp(176) :
> 		                                 FT_STRING 142 ntlmssp.identifier(8) :
> NTLMSSP
> 		                                 FT_UINT32 150 ntlmssp.messagetype(4) :
> 0x00000002
> 		                                 FT_STRING 198
> ntlmssp.challenge.target_name(8) : SUSE
> 		                                         FT_UINT16 154 ntlmssp.string.length(2) :
> 8
> 		VIOLATION 1 : Child ntlmssp.string.length has an offset lower
> than its parent
> 		                                         FT_UINT16 156 ntlmssp.string.maxlen(2)
> : 8
> 		VIOLATION 1 : Child ntlmssp.string.maxlen has an offset lower
> than its parent
> 		                                         FT_UINT32 158 ntlmssp.string.offset(4) :
> 56
> 		VIOLATION 1 : Child ntlmssp.string.offset has an offset lower
> than its parent
> 		                                 FT_UINT32 162 ntlmssp.negotiateflags(4) :
> 0xe2890235
> 		                                 FT_BYTES 166 ntlmssp.ntlmserverchallenge(8)
> : 01:15:18:13:d2:89:8c:cd
> 		                                 FT_BYTES 174 ntlmssp.reserved(8) :
> 00:00:00:00:00:00:00:00
> 		                                 FT_NONE 206
> ntlmssp.challenge.target_info(112) :
> 		                                         FT_UINT16 182
> ntlmssp.challenge.target_info.length(2) : 112
> 		VIOLATION 1 : Child ntlmssp.challenge.target_info.length has
> an offset lower than its parent
> 		                                         FT_UINT16 184
> ntlmssp.challenge.target_info.maxlen(2) : 112
> 		VIOLATION 1 : Child ntlmssp.challenge.target_info.maxlen has
> an offset lower than its parent
> 		                                         FT_UINT32 186
> ntlmssp.challenge.target_info.offset(4) : 64
> 		VIOLATION 1 : Child ntlmssp.challenge.target_info.offset has an
> offset lower than its parent
> 		                                        [...]
> 
> 
> 		 FT_PROTOCOL 0 frame(430) :
> 		[...]
> 		 FT_PROTOCOL 66 nbss(364) :
> 		         FT_UINT8 66 nbss.type(1) : 0x00000000
> 		         FT_UINT24 67 nbss.length(3) : 360
> 		 FT_PROTOCOL 70 smb2(360) :
> 		         FT_NONE 70 [SMB2 Header](64) :
> 		                 [...]
> 		         FT_NONE 134 [Session Setup Request (0x01)](296) :
> 		                 [...]
> 		                 FT_BYTES 158 smb2.security_blob(272) :
> 		                         FT_PROTOCOL 158 ntlmssp(272) :
> 		                                 FT_STRING 158 ntlmssp.identifier(8) :
> NTLMSSP
> 		                                 FT_UINT32 166 ntlmssp.messagetype(4) :
> 0x00000003
> 		                                 FT_BYTES 170 ntlmssp.auth.lmresponse(8) :
> 00:00:00:00:40:00:00:00
> 		                                 FT_BYTES 222 ntlmssp.auth.ntresponse(156) :
> 		                                         FT_UINT16 178 ntlmssp.blob.length(2) :
> 156
> 		VIOLATION 1 : Child ntlmssp.blob.length has an offset lower
> than its parent
> 		                                         FT_UINT16 180 ntlmssp.blob.maxlen(2) :
> 156
> 		VIOLATION 1 : Child ntlmssp.blob.maxlen has an offset lower
> than its parent
> 		                                         FT_UINT32 182 ntlmssp.blob.offset(4) :
> 64
> 		VIOLATION 1 : Child ntlmssp.blob.offset has an offset lower
> than its parent
> 
> 
> 
> 	It looks like some fields describing the blob position (and present before
> the blob itself) were put in a subtree of the blob. Whether this is to improve
> readability is left to someone knowing NTLM Server Challenge protocol (so not
> me).
> 
> 
> 
> 
> 		                                         FT_BYTES 222
> ntlmssp.ntlmv2_response(156) :
> 		                                                 FT_BYTES 222
> ntlmssp.ntlmv2_response.ntproofstr(16) :
> 39:db:db:eb:1b:dd:29:b0:7a:5d:20:c8:f8:2f:2c:b7
> 		                                                 [...]
> 		                                 FT_STRING 378 ntlmssp.auth.domain(8) :
> SUSE
> 		                                         FT_UINT16 186 ntlmssp.string.length(2) :
> 8
> 		VIOLATION 1 : Child ntlmssp.string.length has an offset lower
> than its parent
> 		                                         FT_UINT16 188 ntlmssp.string.maxlen(2)
> : 8
> 		VIOLATION 1 : Child ntlmssp.string.maxlen has an offset lower
> than its parent
> 		                                         FT_UINT32 190 ntlmssp.string.offset(4) :
> 220
> 		VIOLATION 1 : Child ntlmssp.string.offset has an offset lower
> than its parent
> 
> 
> 
> 	It looks like some fields describing the string position (and present
> before the string) were put in a subtree of the string. Whether this is to improve
> readability is left to someone knowing NTLM Server Challenge protocol (so not
> me).
> 
> 
> 
> 
> 		                                 FT_STRING 386 ntlmssp.auth.username(26) :
> administrator
> 		                                         FT_UINT16 194 ntlmssp.string.length(2) :
> 26
> 		VIOLATION 1 : Child ntlmssp.string.length has an offset lower
> than its parent
> 		                                         FT_UINT16 196 ntlmssp.string.maxlen(2)
> : 26
> 		VIOLATION 1 : Child ntlmssp.string.maxlen has an offset lower
> than its parent
> 		                                         FT_UINT32 198 ntlmssp.string.offset(4) :
> 228
> 		VIOLATION 1 : Child ntlmssp.string.offset has an offset lower
> than its parent
> 
> 
> 
> 	Same things as above
> 
> 
> 
> 
> 		                                 FT_STRING 202 ntlmssp.auth.hostname(8) :
> NULL
> 		                                 FT_BYTES 414 ntlmssp.auth.sesskey(16) :
> b2:e8:76:55:9c:9c:58:b0:34:4b:d5:a9:9f:8e:98:55
> 		                                         FT_UINT16 210 ntlmssp.blob.length(2) :
> 16
> 		VIOLATION 1 : Child ntlmssp.blob.length has an offset lower
> than its parent
> 		                                         FT_UINT16 212 ntlmssp.blob.maxlen(2) :
> 16
> 		VIOLATION 1 : Child ntlmssp.blob.maxlen has an offset lower
> than its parent
> 		                                         FT_UINT32 214 ntlmssp.blob.offset(4) :
> 256
> 		VIOLATION 1 : Child ntlmssp.blob.offset has an offset lower
> than its parent
> 
> 
> 
> 	Same thing as above
> 
> 
> 
> 
> 		                                 FT_UINT32 218 ntlmssp.negotiateflags(4) :
> 0xe0880235
> 
> 
> 		 FT_PROTOCOL 0 frame(180) :
> 		[...]
> 		 FT_PROTOCOL 70 smb2(110) :
> 		         FT_NONE 70 [SMB2 Header](64) :
> 		                 [...]
> 		         FT_NONE 134 [Tree Connect Request (0x03)](44) :
> 		                 FT_UINT16 134 smb2.buffer_code(2) : 0x00000009
> 		                 FT_BYTES 136 smb2.reserved(2) : 00:00
> 		                 FT_STRING 142 smb2.tree(36) : \\WS2016\encrypted
> 		                         FT_UINT32 138 smb2.olb.offset(2) : 0x00000048
> 		VIOLATION 1 : Child smb2.olb.offset has an offset lower than its
> parent
> 		                         FT_UINT32 140 smb2.olb.length(2) : 36
> 		VIOLATION 1 : Child smb2.olb.length has an offset lower than
> its parent
> 
> 
> 
> 	Same thing as NTLM Server Challenge, but for SMB2. Probably the same
> code author.
> 
> 
> 
> 
> 
> 
> 		 FT_PROTOCOL 0 frame(268) :
> 		[...]
> 		 FT_PROTOCOL 70 smb2(198) :
> 		         FT_NONE 70 [SMB2 Transform Header](0) :
> 		                 FT_NONE 70
> smb2.server_component_smb2_transform(4) : FD534D42
> 		VIOLATION 2 : Child smb2.server_component_smb2_transform
> has an end offset past the end of its parent
> 		VIOLATION 3 : Child smb2.server_component_smb2_transform
> has a length longer than its parent
> 		                 FT_BYTES 74 smb2.header.transform.signature(16) :
> 76:17:6b:19:56:ed:2c:9c:5a:cf:00:a3:0c:04:85:bc
> 		VIOLATION 2 : Child smb2.header.transform.signature has an
> end offset past the end of its parent
> 		VIOLATION 3 : Child smb2.header.transform.signature has a
> length longer than its parent
> 		VIOLATION 4 : Child smb2.header.transform.signature has a
> start offset past the end of its parent
> 		                 FT_BYTES 90 smb2.header.transform.nonce(16) :
> 3a:aa:96:8f:18:ae:61:e6:e7:6f:1f:00:00:00:00:00
> 		VIOLATION 2 : Child smb2.header.transform.nonce has an end
> offset past the end of its parent
> 		VIOLATION 3 : Child smb2.header.transform.nonce has a length
> longer than its parent
> 		VIOLATION 4 : Child smb2.header.transform.nonce has a start
> offset past the end of its parent
> 		                 FT_UINT32 106 smb2.header.transform.msg_size(4) :
> 146
> 		VIOLATION 2 : Child smb2.header.transform.msg_size has an
> end offset past the end of its parent
> 		VIOLATION 3 : Child smb2.header.transform.msg_size has a
> length longer than its parent
> 		VIOLATION 4 : Child smb2.header.transform.msg_size has a
> start offset past the end of its parent
> 		                 FT_BYTES 110 smb2.header.transform.reserved(2) :
> 00:00
> 		VIOLATION 2 : Child smb2.header.transform.reserved has an
> end offset past the end of its parent
> 		VIOLATION 3 : Child smb2.header.transform.reserved has a
> length longer than its parent
> 		VIOLATION 4 : Child smb2.header.transform.reserved has a
> start offset past the end of its parent
> 		                 FT_UINT16 112
> smb2.header.transform.encryption_alg(2) : 0x00000001
> 		VIOLATION 2 : Child smb2.header.transform.encryption_alg has
> an end offset past the end of its parent
> 		VIOLATION 3 : Child smb2.header.transform.encryption_alg has
> a length longer than its parent
> 		VIOLATION 4 : Child smb2.header.transform.encryption_alg has
> a start offset past the end of its parent
> 		                 FT_UINT64 114 smb2.sesid(8) : 0x000048009400003d
> 		VIOLATION 2 : Child smb2.sesid has an end offset past the end
> of its parent
> 		VIOLATION 3 : Child smb2.sesid has a length longer than its
> parent
> 		VIOLATION 4 : Child smb2.sesid has a start offset past the end
> of its parent
> 
> 
> 
> 	The SMB2 Transform Header text item is inserted in the tree using
> proto_tree_add_subtree() function using the -1 length parameter (which usually
> means till the end of the tvb, at least for some field types). This function is
> internally calling proto_tree_add_text_valist_internal() that does:
> 
> 	    if (length == -1) {
> 	        /* If we're fetching until the end of the TVB, only validate
> 	         * that the offset is within range.
> 	         */
> 	        length = 0;
> 
> 	    }
> 
> 
> 	This seems wrong. It might be replaced by something like:
> 
> 	    if (length == -1) {
> 	        /* If we're fetching until the end of the TVB, only validate
> 	         * that the offset is within range.
> 	         */
> 	       length = tvb_captured_length(tvb) ?
> tvb_ensure_captured_length_remaining(tvb, start) : 0;
> 	    }
> 
> 
> 
> See https://code.wireshark.org/review/22931
> 
> 
> 
> 
> 
> 
> 	Pascal.
> 
>