Wireshark · Wireshark-commits: [Wireshark-commits] master 6e4caf3: BOOTP BSDP: Allow "pad" and "end" suboptions.

[Wireshark code review <code-review-do-not-reply@xxxxxxxxxxxxx>]

URL: https://code.wireshark.org/review/gitweb?p=wireshark.git;a=commit;h=6e4caf3d90862c36c451f48abe7533fa92ecf58b
Submitter: Anders Broman (a.broman58@xxxxxxxxx)
Changed: branch: master
Repository: wireshark

Commits:

6e4caf3 by Darius Davis (darius@xxxxxxxxxx):

    BOOTP BSDP: Allow "pad" and "end" suboptions.
    
    Apple bsdpd uses the same routine to parse BSDP suboptions as it uses to parse
    the DHCP options, which means that the "pad" (0) and "end" (255) options (as
    described in RFC 2132) are also accepted as BSDP suboptions.  Just like when
    used as DHCP options, they do not follow the usual TLV template: They do not
    have a length field and do not have any value, so they always consume exactly
    one byte.
    
    This change enhances the BSDP suboption dissector to accept the "pad" (0) and
    "end" (255) suboptions, without any stored length or value.
    
    Apple firmware/software does not issue BSDP "pad" or "end" suboptions, but will
    tolerate them in received packets.  At least one 3rd-party BSDP implementation
    (the Dell KACE K2000 appliance) includes a BSDP "end" suboption in packets it
    sends.  Prior to this fix, function dissect_vendor_bsdp_suboption was expecting
    a length for these suboptions, leading to dissection failing with error
    "Suboption 255: no room left in option for suboption length".
    
    For further discussion -- in which the exact same issue is found to affect
    VMware virtual machine firmware -- refer to the VMware Communities forum thread
    at https://communities.vmware.com/message/2459144#2459144 .
    
    Interestingly, when Apple's bsdpd finds an "end" BSDP suboption, it simply
    records that an "end" was encountered, and continues parsing until the whole of
    the vendor options blob is consumed.  The BSDP suboption dissector required no
    modification to match that behavior.
    
    Testing Done: Built Wireshark on Linux amd64.  Loaded a BSDP ACK[LIST] from a
       Dell KACE K2000 appliance; Previously it would issue an error about there
       being insufficient room for the length of the "end" suboption, and now it
       parses correctly.  Modified the packet to include a string of "0" and "255"
       suboptions, and observed that they were parsed as expected: One byte each,
       no subtree, no length, and parsing continues afterwards.  200,000 iterations
       with tools/fuzz-test.sh using the original BSDP packet, 4,000 of which were
       under Valgrind.
    
    Change-Id: I1786414b2ef0b8726d989a566d0e8a3525d516b8
    Reviewed-on: https://code.wireshark.org/review/27210
    Reviewed-by: Alexis La Goutte <alexis.lagoutte@xxxxxxxxx>
    Petri-Dish: Alexis La Goutte <alexis.lagoutte@xxxxxxxxx>
    Tested-by: Petri Dish Buildbot
    Reviewed-by: Anders Broman <a.broman58@xxxxxxxxx>
    

Actions performed:

    from  b768386   gsm_r_uus1: Fix Dead Store (Dead assignement/Dead increment) Warning found by Clang
    adds  6e4caf3   BOOTP BSDP: Allow "pad" and "end" suboptions.


Summary of changes:
 epan/dissectors/packet-bootp.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)