Wireshark-dev: Re: [Wireshark-dev] Hierarchy of fields & offsets again, more potential offender
From: Pascal Quantin <pascal.quantin@xxxxxxxxx>
Date: Wed, 2 Aug 2017 22:13:00 +0200
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.
>
>
- References:
- [Wireshark-dev] Hierarchy of fields & offsets again, more potential offenders
- From: Sultan, Hassan
- Re: [Wireshark-dev] Hierarchy of fields & offsets again, more potential offenders
- From: Pascal Quantin
- Re: [Wireshark-dev] Hierarchy of fields & offsets again, more potential offenders
- From: Pascal Quantin
- Re: [Wireshark-dev] Hierarchy of fields & offsets again, more potential offenders
- From: Sultan, Hassan
- [Wireshark-dev] Hierarchy of fields & offsets again, more potential offenders
- Prev by Date: Re: [Wireshark-dev] Setting to disable all expert info
- Next by Date: Re: [Wireshark-dev] Setting to disable all expert info
- Previous by thread: Re: [Wireshark-dev] Hierarchy of fields & offsets again, more potential offenders
- Next by thread: Re: [Wireshark-dev] Hierarchy of fields & offsets again, more potential offenders
- Index(es):