Re: Hierarchy of fields & offsets again, more potential offenders

[Pascal Quantin <pascal.quantin () gmail com>]

2017-08-02 22:03 GMT+02:00 Sultan, Hassan <sultah () 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.quantin () gmail com]
> > Sent: Wednesday, August 02, 2017 12:41 PM
> > To: Developer support list for Wireshark <wireshark-dev () wireshark org>
> > Cc: Sultan, Hassan <sultah () 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.quantin () gmail com
> > <mailto:pascal.quantin () 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-request () wireshark org?subject=unsubscribe