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

[Pascal Quantin <pascal.quantin@xxxxxxxxx>]

2017-08-02 22:03 GMT+02:00 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 ?

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.quantin@gmail.com]
> 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@gmail.com> >:
>
>
>       Hi Hassan,
>
>
>       2017-08-02 1:05 GMT+02:00 Sultan, Hassan via Wireshark-dev
> <wireshark-dev@xxxxxxxxxxxxx <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.
>
>