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

[Pascal Quantin <pascal.quantin@xxxxxxxxx>]

2017-08-02 21:24 GMT+02:00 Pascal Quantin <pascal.quantin@xxxxxxxxx>:

Hi Hassan,

2017-08-02 1:05 GMT+02:00 Sultan, Hassan via Wireshark-dev <wireshark-dev@xxxxxxxxxxxxx>:

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="">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.