2017-08-02 21:24 GMT+02:00 Pascal Quantin <pascal.quan...@gmail.com>:
> Hi Hassan, > > 2017-08-02 1:05 GMT+02:00 Sultan, Hassan via Wireshark-dev < > 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 >> >> 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
