Project

General

Profile

Actions

Security #4857

closed

ftp: SEGV at flow cleanup due to protocol confusion

Added by Kirill Rassokhin over 2 years ago. Updated over 1 year ago.

Status:
Closed
Priority:
High
Target version:
Affected Versions:
Label:
CVE:
Git IDs:
Severity:
MODERATE
Disclosure Date:

Description

Hello,

Segmentation fault occurs when scanning traffic
Suricata version 6.0.3

Error:
Segmentation fault (core dumped)

pcap file in attachment


Files

test.pcap (17 KB) test.pcap Kirill Rassokhin, 11/28/2021 10:07 PM

Related issues 3 (1 open2 closed)

Related to Suricata - Bug #4940: ftp-data: protocol misclassification if the file begins with a protocol patternNewPhilippe AntoineActions
Copied to Suricata - Security #4888: ftp: SEGV at flow cleanup due to protocol confusionClosedShivani BhardwajActions
Copied to Suricata - Security #4889: ftp: SEGV at flow cleanup due to protocol confusionClosedJeff LucovskyActions
Actions #1

Updated by Victor Julien over 2 years ago

  • Private changed from No to Yes
  • Affected Versions 6.0.4 added
Actions #2

Updated by Victor Julien over 2 years ago

  • Tracker changed from Bug to Security
  • Subject changed from Segmentation fault occurs when scanning traffic to ftp: SEGV at flow cleanup due to protocol confusion
  • Status changed from New to Assigned
  • Assignee set to Philippe Antoine
  • Priority changed from Normal to High
  • Target version set to 7.0.0-beta1
  • Severity set to MODERATE
  • Affected Versions git master added
  • Label Needs backport to 5.0, Needs backport to 6.0 added

It looks like we have an issue with protocol detection here. The pattern based protocol detection detects the ftp data body as HTTP, but somehow the expectation based protocol detect later overrides it, but it does not handle the case where the protocol was already set to HTTP, and the state was already initialized as HtpState. Thus later on the Flow::alstate is freed using FTPDataStateFree.

At

#0  AppLayerExpectationHandle (f=0x3501f00, flags=7 '\a') at app-layer-expectation.c:342
#1  0x00000000009a84af in AppLayerProtoDetectPEGetProto (f=0x3501f00, ipproto=6 '\006', flags=7 '\a') at app-layer-detect-proto.c:469
#2  0x00000000009a7242 in AppLayerProtoDetectGetProto (tctx=0x7ffff0500480, f=0x3501f00, buf=0x0, buflen=0, ipproto=6 '\006', flags=7 '\a', reverse_flow=0x7ffff62114f7) at app-layer-detect-proto.c:1581
#3  0x000000000097a60a in TCPProtoDetect (tv=0x3572480, ra_ctx=0x7ffff0500400, app_tctx=0x7ffff0500430, p=0x7ffff04c7840, f=0x3501f00, ssn=0x7ffff05dfc00, stream=0x7ffff6211640, data=0x0, data_len=0, 
    flags=7 '\a') at app-layer.c:330
#4  0x0000000000979fbe in AppLayerHandleTCPData (tv=0x3572480, ra_ctx=0x7ffff0500400, p=0x7ffff04c7840, f=0x3501f00, ssn=0x7ffff05dfc00, stream=0x7ffff6211640, data=0x0, data_len=0, flags=7 '\a')
    at app-layer.c:645
#5  0x0000000000d8d74c in StreamTcpReassembleAppLayer (tv=0x3572480, ra_ctx=0x7ffff0500400, ssn=0x7ffff05dfc00, stream=0x7ffff05dfc98, p=0x7ffff04c7840, dir=UPDATE_DIR_PACKET) at stream-tcp-reassemble.c:1243
#6  0x0000000000d90870 in StreamTcpReassembleHandleSegment (tv=0x3572480, ra_ctx=0x7ffff0500400, ssn=0x7ffff05dfc00, stream=0x7ffff05dfc98, p=0x7ffff04c7840, pq=0x7ffff04e82d0) at stream-tcp-reassemble.c:1923
#7  0x0000000000d4111f in StreamTcpPacket (tv=0x3572480, p=0x7ffff04c7840, stt=0x7ffff05000f0, pq=0x7ffff04e82d0) at stream-tcp.c:4940
#8  0x0000000000d44e86 in StreamTcp (tv=0x3572480, p=0x7ffff04c7840, data=0x7ffff05000f0, pq=0x7ffff04e82d0) at stream-tcp.c:5312
#9  0x0000000000cdf616 in FlowWorkerStreamTCPUpdate (tv=0x3572480, fw=0x7ffff04e82a0, p=0x7ffff04c7840, detect_thread=0x0, timeout=true) at flow-worker.c:370
#10 0x0000000000ce0255 in FlowWorkerFlowTimeout (tv=0x3572480, p=0x7ffff04c7840, fw=0x7ffff04e82a0, detect_thread=0x0) at flow-worker.c:413
#11 0x0000000000ce014c in FlowFinish (tv=0x3572480, f=0x3501f00, fw=0x7ffff04e82a0, detect_thread=0x0) at flow-worker.c:154
#12 0x0000000000cdfde5 in CheckWorkQueue (tv=0x3572480, fw=0x7ffff04e82a0, detect_thread=0x0, counters=0x7ffff6211940, fq=0x7ffff6211960) at flow-worker.c:177
#13 0x0000000000cdfb0f in FlowWorkerProcessInjectedFlows (tv=0x3572480, fw=0x7ffff04e82a0, p=0x3a3b870, detect_thread=0x0) at flow-worker.c:459
#14 0x0000000000cdf22b in FlowWorker (tv=0x3572480, p=0x3a3b870, data=0x7ffff04e82a0) at flow-worker.c:588
#15 0x00000000008ff96f in TmThreadsSlotVarRun (tv=0x3572480, p=0x3a3b870, slot=0x2467130) at tm-threads.c:117
#16 0x0000000000905f1b in TmThreadsSlotProcessPkt (tv=0x3572480, s=0x2467130, p=0x3a3b870) at ./tm-threads.h:195
#17 0x0000000000905ddd in TmThreadTimeoutLoop (tv=0x3572480, s=0x247b5f0) at tm-threads.c:177
#18 0x00000000009056cd in TmThreadsSlotPktAcqLoop (td=0x3572480) at tm-threads.c:330
#19 0x00007ffff7d5c609 in start_thread (arg=<optimized out>) at pthread_create.c:477
#20 0x00007ffff71e4293 in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

we have:
$5 = {src = {address = {address_un_data32 = {3757650186, 0, 0, 0}, address_un_data16 = {12554, 57337, 0, 0, 0, 0, 0, 0}, address_un_data8 = "\n1\371\337", '\000' <repeats 11 times>}}, dst = {address = {
      address_un_data32 = {63907382, 0, 0, 0}, address_un_data16 = {9782, 975, 0, 0, 0, 0, 0, 0}, address_un_data8 = "6&\317\003", '\000' <repeats 11 times>}}, {sp = 49168, icmp_s = {type = 16 '\020', 
      code = 192 '\300'}, esp = {spi = 49168}}, {dp = 51048, icmp_d = {type = 104 'h', code = 199 '\307'}}, proto = 6 '\006', recursion_level = 0 '\000', vlan_id = {0, 0}, use_cnt = 2, vlan_idx = 0 '\000', {{
      ffr_ts = 1 '\001', ffr_tc = 1 '\001'}, ffr = 17 '\021'}, timeout_at = 1624314947, thread_id = {1, 1}, next = 0x0, livedev = 0x0, flow_hash = 1396070381, lastts = {tv_sec = 1624314347, tv_usec = 378567}, 
  timeout_policy = 600, flow_state = 1, tenant_id = 0, probing_parser_toserver_alproto_masks = 0, probing_parser_toclient_alproto_masks = 0, flags = 10076163, file_flags = 1023, protodetect_dp = 0, 
  parent_id = 0, m = {__data = {__lock = 1, __count = 0, __owner = 287858, __nusers = 1, __kind = 0, __spins = 0, __elision = 0, __list = {__prev = 0x0, __next = 0x0}}, 
    __size = "\001\000\000\000\000\000\000\000rd\004\000\001", '\000' <repeats 26 times>, __align = 1}, protoctx = 0x7ffff05dfc00, protomap = 0 '\000', flow_end_flags = 80 'P', alproto = 1, alproto_ts = 17, 
  alproto_tc = 17, alproto_orig = 0, alproto_expect = 0, de_ctx_version = 0, min_ttl_toserver = 128 '\200', max_ttl_toserver = 128 '\200', min_ttl_toclient = 64 '@', max_ttl_toclient = 64 '@', 
  alparser = 0x7ffff0780480, alstate = 0x7ffff07800f0, sgh_toclient = 0x0, sgh_toserver = 0x0, flowvar = 0x0, fb = 0x0, startts = {tv_sec = 1624314327, tv_usec = 277618}, todstpktcnt = 3, tosrcpktcnt = 3, 
  todstbytecnt = 186, tosrcbytecnt = 642}
(gdb) p *(HtpState *)f->alstate
$6 = {connp = 0x7ffff0781de0, conn = 0x7ffff0781f80, f = 0x3501f00, transaction_cnt = 1, store_tx_id = 0, files_ts = 0x0, files_tc = 0x7ffff0780370, cfg = 0x2400cf8 <cfglist>, flags = 0, events = 1, 
  htp_messages_offset = 1, file_track_id = 1, file_range = 0x0, last_request_data_stamp = 0, last_response_data_stamp = 468}

Note: alproto = 1 which is HTTP

Actions #3

Updated by Victor Julien over 2 years ago

The app layer expectation logic assumes that a flow calls the AppLayerExpectationHandle function on the very first packet for that flow. However it seems that we've see traffic in both directions here before it is called. I think this may be because there is just data on the flow in one direction, so it still wants to do protocol detection in the other direction. Only then it considers the expectation list. I wonder if we can solve this w/o checking the expectation list first, which would likely result in a serious perf penalty.

Actions #4

Updated by Victor Julien over 2 years ago

Thinking a bit more, I wonder if having a separate expectation list makes sense at all. Instead of an expectation we can also just set up a flow that already has the correct protocol set up. That will be a bigger project though.

Actions #5

Updated by Philippe Antoine over 2 years ago

  • Status changed from Assigned to In Review

Let's fix the segfault first :
https://github.com/OISF/suricata/pull/6651

Thinking a bit more, I wonder if having a separate expectation list makes sense at all. Instead of an expectation we can also just set up a flow that already has the correct protocol set up. That will be a bigger project though.

Sounds good.
Would we log this flow even if we do not see real packets ?
Could it lead to some evasion ? Like I will pretend waiting for ftp-data on port 80 and then do regular HTTP ?

Actions #6

Updated by Victor Julien over 2 years ago

I think I overlooked something. We don't know the full 5 tuple for the expected flow I think. We know 4 of the fields: proto, src_ip, dest_ip, dest_port, but not the source port.

Actions #7

Updated by Philippe Antoine over 2 years ago

Victor Julien wrote in #note-6:

I think I overlooked something. We don't know the full 5 tuple for the expected flow I think. We know 4 of the fields: proto, src_ip, dest_ip, dest_port, but not the source port.

Indeed.

We could have an array of the 65k ports pointing to expectations lists...
So, in most cases, it would be just adding one check to make sure expectation is not for a new flow...

Actions #8

Updated by Jeff Lucovsky over 2 years ago

  • Copied to Security #4888: ftp: SEGV at flow cleanup due to protocol confusion added
Actions #9

Updated by Jeff Lucovsky over 2 years ago

  • Copied to Security #4889: ftp: SEGV at flow cleanup due to protocol confusion added
Actions #10

Updated by Philippe Antoine over 2 years ago

By the way, this is the perfect example of a bug not found by fuzzing, and I do not know why.
The code is clearly reachable by fuzz target like fuzz_predef. And it would simply be taking an element of the seed corpus with ftp data, with one mutation being to insert one of the protocol detection patterns...

Actions #11

Updated by Philippe Antoine over 2 years ago

Segfault is fixed there : https://github.com/OISF/suricata/pull/6651
Should we create another ticket for the problem of the misclassification ?

Actions #12

Updated by Victor Julien over 2 years ago

  • Status changed from In Review to Closed

Yeah lets create a ticket for that.

Actions #13

Updated by Philippe Antoine over 2 years ago

Victor Julien wrote in #note-12:

Yeah lets create a ticket for that.

Should we wait for the security back ports first ?

Actions #14

Updated by Jeff Lucovsky over 2 years ago

  • Label deleted (Needs backport to 5.0, Needs backport to 6.0)
Actions #15

Updated by Philippe Antoine over 2 years ago

By the way, this is not reproducible by fuzzing.
Because the fuzz target never calls CheckWorkQueue from FlowWorkerProcessInjectedFlows (and so never calls FlowFinish which generates the virtual ending packet which triggers protocol detection on the unknown side...)

Actions #16

Updated by Philippe Antoine about 2 years ago

  • Related to Bug #4940: ftp-data: protocol misclassification if the file begins with a protocol pattern added
Actions #17

Updated by Victor Julien over 1 year ago

  • Private changed from Yes to No
Actions

Also available in: Atom PDF