2017-08-02 22:03 GMT+02:00 Sultan, Hassan <sul...@amazon.com>: > 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 ? > That's what I use for my own code. But breaking long existing behavior must have a good justification (like for the tcp.payload field). > > > -----Original Message----- > > From: Pascal Quantin [mailto:pascal.quan...@gmail.com] > > Sent: Wednesday, August 02, 2017 12:41 PM > > To: Developer support list for Wireshark <wireshark-dev@wireshark.org> > > Cc: Sultan, Hassan <sul...@amazon.com> > > Subject: Re: [Wireshark-dev] Hierarchy of fields & offsets again, more > potential > > offenders > > > > > > > > 2017-08-02 21:24 GMT+02:00 Pascal Quantin <pascal.quan...@gmail.com > > <mailto:pascal.quan...@gmail.com> >: > > > > > > Hi Hassan, > > > > > > 2017-08-02 1:05 GMT+02:00 Sultan, Hassan via Wireshark-dev > > <wireshark-dev@wireshark.org <mailto:wireshark-dev@wireshark.org> >: > > > > > > 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. > > > > > >
___________________________________________________________________________ Sent via: Wireshark-dev mailing list <wireshark-dev@wireshark.org> Archives: https://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
