smb: fix SMB_COM_WRITE_ANDX record parser - v3

[b1tg]

Bug: #6008

• I have read the contributing guide lines at https://suricata.readthedocs.io/en/latest/devguide/codebase/contributing/contribution-process.html
• I have signed the Open Information Security Foundation contribution agreement at https://suricata.io/about/contribution-agreement/
• I have updated the user guide (in doc/userguide/) to reflect the changes made (if applicable)

ticket:
https://redmine.openinfosecfoundation.org/issues/6008

Previous PR: #8845

Changes from last PR:

• use smaller blob for test
• do not parse one byte padding which is possible missing
• update SV

SV_BRANCH=pr/1203

[catenacyber]

Thanks, looks like the one-line change to the offset definition is enough for the tests :

-        offset: high_offset.map(|ho| (ho as u64) << 32 | offset as u64).unwrap_or(0),
+        offset: ((high_offset.unwrap_or(0) as u64) << 32) | offset as u64,

Could we just use this and not change the complex padding logic ?
Or do you have another test case that shows another problem ?

[codecov]

Codecov Report

Merging #8889 (98c0673) into master (c30fff8) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8889      +/-   ##
==========================================
- Coverage   82.32%   82.27%   -0.05%     
==========================================
  Files         969      969              
  Lines      273352   273364      +12     
==========================================
- Hits       225025   224912     -113     
- Misses      48327    48452     +125     

Flag	Coverage Δ
fuzzcorpus	64.54% <100.00%> (-0.07%)	⬇️
suricata-verify	60.42% <100.00%> (-0.02%)	⬇️
unittests	62.96% <100.00%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

[b1tg]

Or do you have another test case that shows another problem ?

@catenacyber

In the origin packet, data_length == bcc == 20, if we use a proxy to change data_length to 17, Windows still accept it and write 17 bytes to file, but the original parse_smb1_write_andx_request_record will take 3 bytes padding to make record.data inconsistent with it in Windows.

#[test]
fn test_parse_smb1_write_andx_request_record_padding() {
    let data = hex::decode("0eff000000054000000000ff00000008001400000011003f000000000014004142434445464748494a4b4c4d4e4f5051520a0a").unwrap();
    let result = parse_smb1_write_andx_request_record(&data, SMB1_HEADER_SIZE);
	assert!(result.is_ok());
    let record = result.unwrap().1;
    assert_eq!(record.offset, 0);
    assert_eq!(record.len, 17);
    assert_eq!(record.data.len(), 17);
    // assert_eq!(record.data, b"DEFGHIJKLMNOPQR\n\n"); // origin `parse_smb1_write_andx_request_record` output this
    assert_eq!(record.data, b"ABCDEFGHIJKLMNOPQ"); // Windows server write this 17 bytes to file
}

pcap added to https://redmine.openinfosecfoundation.org/issues/6008

[victorjulien]

Is that pcap also part of a SV test? If not, could you add it as well? Thanks!

[catenacyber]

@b1tg running smb_bug_padding.pcap through suricata with master,
then `jq 'select(.event_type=="fileinfo") | .fileinfo.size' log/eve.json

I see both 20 and 17
That looks like the expected result, so no need to patch, right ?

I think there is some post-processing after unit parsing to limit the file data length...
And the root cause of this complexity is that we do not want to wait for the full NBSS/SMB write record before streaming the data into the file... For reference, you can look at https://redmine.openinfosecfoundation.org/issues/5786 and https://redmine.openinfosecfoundation.org/issues/5770

[b1tg]

Yes, smb_bug_padding.pcap include two tcp stream, i use a proxy to change data_length as 17 in the second stream, the issue here is not the fileinfo.size, but the file data which is not correct parsed.

[catenacyber]

Ok, so Suricata and Wireshark show "DEFGHIJKLMNOPQR\n\n" but Windows gets "ABCDEFGHIJKLMNOPQ"...
Do I get it right ?

If so, there are 2 bugs : the high_offset one and this one. I guess each bug deserves its own commit/ticket/test...

[b1tg]

Yes, and there is another case that may need to be considered, If proxy change data_length to 0x3c, Windows write follow data to file, i guess this can also be a possible evasion

00 14 00 41 42 43 44 45 46 47 48 49 4A 4B 4C 4D
4E 4F 50 51

[catenacyber]

If proxy change data_length to 0x3c

Not sure I understand this one.

The data gets bounded by the NBSS record, right ?

[b1tg]

I did some reverse engineer on srv.sys , the follow code might be helpful to understand.

__int64 __fastcall SrvSmbWriteAndX(__int64 this, __int64 a2, __int64 a3, __int64 a4)
  // [...]
  data_offset = *(unsigned __int16 *)(smb_record_data + 0x17);
  data_offset1 = data_offset;
  unknow = *(_QWORD *)(this + 184);
  smb_record_end_offset = *(_DWORD *)(unknow + 0x20);
  // Windows accept 0x3C as data_offset
  if ( smb_record_end_offset < data_offset || data_offset < 0x3C )// check data_offset
  {
    v10 = 2645;
    goto LABEL_189; // report error
  }
  v17 = smb_record_end_offset - data_offset;
  data_length_real = *(unsigned __int16 *)(smb_record_data + 0x15);
  if ( data_length_real >= v17 )
    data_length_real = v17;
  data_ptr = *(_QWORD *)(this + 208) + data_offset;// pointer to data

[catenacyber]

Thanks for these patches.
Would you split the commit in 2, one for each bug fix ? (the offset one, and the padding one)

[b1tg]

Is that pcap also part of a SV test? If not, could you add it as well? Thanks!

Added in OISF/suricata-verify#1211

[b1tg]

use #8905