[Bro-Dev] SMB Analyzer code factorization

[Bencteux Jeffrey]

Hi all,


As I looked into SMBv1 analyzer, I found that most of the files
describing SMB messages have code duplication. According to the SMB
specification ([MS-CIFS]), SMB messages are composed of a fixed-length
header (defined as SMB_Header in smb1-protocol.pac for Bro) and then of
two "blocks" : the parameter block (section 2.2.3.2) and the data block
(section 2.2.3.3). Every message of the protocol I saw in the
specification or encountered in trafic always followed that scheme.
However, in Bro's SMBv1 analyzer, each .pac file describing a particular
type of message reimplement the two blocks common fields.


The SMB PDU is defined as such :


type SMB_PDU(...) = record {

    header: SMB_Header(...)

    message: case msg_len of {

        35 -> no_msg: SMB_No_Message;

        default -> msg: SMB_Message(...);

    };

};


SMB_Message is just a switch between SMB_Message_request and
SMB_Message_Response. Then these two are defined as a switch on command
type :


type SMB_Message_Request(...) = case command of {

    SMB_COM_READ_ANDX -> read_andx: SMB1_read_andx_request(...);

    ...

};


And then every command is implemented like this one :


type SMB_read_andx_request(...) = record {

    word_count: uint8

    ... specific fields ...

    byte_count: uint16;

    ... specific fields ...

};


I feel like it would be a good idea to abstract the two blocks : it
would factorize code. Also, there is currently no check on the value of
word_count field or byte_count field for the rest of the payload. This
could lead to protocol violations from BinPAC if you have a byte_count
set to 0 and then a following field trying to parse a SMB_String for
example (see SMB1_transaction_request record in smb1-com-transaction.pac
for an example of that).


A solution could be to change SMB_Message_* to something closer to what
the specification describe by dividing every message in two half
structures :


type SMB_Message_Request(...) = record {

    parameter_block: SMB_Request_Parameters(...);

    data_block: SMB_Request_Data(...);

};


type SMB_Request_Parameters(...) = record {

    word_count: uint8;

    # Maybe a check on word_count here? Some messages seems to have a
predefined value for this field

    words: SMB_Request_Words(...);

};


type SMB_Request_Data(...) = record {

    byte_count: uint16;

    data_or_not: case byte_count of {

        0 -> none: empty;

        default -> bytes: SMB_Request_Bytes(...);

    };

};


And then SMB_Request_Words and SMB_Request_Bytes would be switch on
different messages types :


type SMB_Request_Words(...) = case command of {

    SMB_COM_READ_ANDX -> words_read_and_x:
SMB1_Words_read_andx_request(...);

    ...

};


type SMB_Request_Bytes(...) = case command of {

    SMB_COM_READ_ANDX -> bytes_read_and_x:
SMB1_Bytes_read_andx_request(...);

    ...

};


Of course, it means splitting every existing message and re factoring a
lot of code of the analyzer, but it would be easier then to address
problems such as the one quoted as example above. What do you think of it ?


Regards,


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
Url : http://mailman.icsi.berkeley.edu/pipermail/bro-dev/attachments/20180608/2a9c29dc/attachment.bin