Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

protodetect: improve DCERPC UDP probing parser and simplify app-layer-detect-proto.c code #11541

Closed
wants to merge 2 commits into from
Closed

protodetect: improve DCERPC UDP probing parser and simplify app-layer-detect-proto.c code #11541

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6ea90c8
protodetect/dcerpc: improve dcerpc udp probe
ilya-bakhtin Jul 21, 2024
05bd7e9
protodetect: simplify app-layer-detect-proto.c code since DCERPC UDP …
ilya-bakhtin Jul 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion rust/src/dcerpc/dcerpc_udp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,9 +294,11 @@ pub unsafe extern "C" fn rs_dcerpc_udp_get_tx_cnt(vtx: *mut std::os::raw::c_void
/// Probe input to see if it looks like DCERPC.
fn probe(input: &[u8]) -> (bool, bool) {
match parser::parse_dcerpc_udp_header(input) {
Ok((_, hdr)) => {
Ok((leftover_bytes, hdr)) => {
let is_request = hdr.pkt_type == 0x00;
let is_dcerpc = hdr.rpc_vers == 0x04 &&
hdr.fragnum == 0 &&
leftover_bytes.len() >= hdr.fraglen as usize &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this condition leftover_bytes.len() >= hdr.fraglen as usize ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see the conventional parser code at rust/src/dcerpc/dcerpc_udp.rs:207
The probe must do at least the same since the DCERPC PM is extremely short.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok indeed, we are on UDP here :-)

(hdr.flags2 & 0xfc == 0) &&
(hdr.drep[0] & 0xee == 0) &&
(hdr.drep[1] <= 3);
Expand Down
25 changes: 5 additions & 20 deletions src/app-layer-detect-proto.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1401,7 +1401,6 @@ AppProto AppLayerProtoDetectGetProto(AppLayerProtoDetectThreadCtx *tctx, Flow *f
(flags & STREAM_TOSERVER) ? "toserver" : "toclient");

AppProto alproto = ALPROTO_UNKNOWN;
AppProto pm_alproto = ALPROTO_UNKNOWN;

if (!FLOW_IS_PM_DONE(f, flags)) {
AppProto pm_results[ALPROTO_MAX];
Expand All @@ -1419,38 +1418,24 @@ AppProto AppLayerProtoDetectGetProto(AppLayerProtoDetectThreadCtx *tctx, Flow *f
FLOW_RESET_PP_DONE(f, reverse_dir);
}
}

/* HACK: if detected protocol is dcerpc/udp, we run PP as well
* to avoid misdetecting DNS as DCERPC. */
if (!(ipproto == IPPROTO_UDP && alproto == ALPROTO_DCERPC))
goto end;

pm_alproto = alproto;

/* fall through */
SCReturnUInt(alproto);
}
}

if (!FLOW_IS_PP_DONE(f, flags)) {
bool rflow = false;
alproto = AppLayerProtoDetectPPGetProto(f, buf, buflen, ipproto, flags, &rflow);
*reverse_flow = false; /* to be safe if it was changed by PM */
alproto = AppLayerProtoDetectPPGetProto(f, buf, buflen, ipproto, flags, reverse_flow);
if (AppProtoIsValid(alproto)) {
if (rflow) {
*reverse_flow = true;
}
goto end;
SCReturnUInt(alproto);
}
}

/* Look if flow can be found in expectation list */
if (!FLOW_IS_PE_DONE(f, flags)) {
*reverse_flow = false; /* to be safe if it was changed by PP */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about this safety check... not sure it is needed...
Maybe we should rather have a DEBUG_VALIDATION like DEBUG_VALIDATE_BUG_ON(*reverse_flow); // should not be set to true, otherwise we returned a valid alproto already

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'll inspect if the reversed flag is never set by the previous kind of the proto detection in the case of an unknown/failed result

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made an inspection of the corresponding code. Direction may be reported reversed only in a case of valid protocol returned. So the "to be safe" lines are not necessary. Now i believe even debug validation is not necessary.

At the same time i found a case when incorrect direction may be technically reported by the PM.
It's not in the scope of the current work. And most likely the case is not possible whith the currently registered set of protocol detection patterns. I can not provide a reproducer. The following is a result of code review only.

The PM technically detects a set of protos but only first one is used. Let's see at app-layer-detect-proto.c from line 1407
uint16_t pm_matches = AppLayerProtoDetectPMGetProto(
tctx, f, buf, buflen, flags, pm_results, reverse_flow);
if (pm_matches > 0) {
DEBUG_VALIDATE_BUG_ON(pm_matches > 1);
alproto = pm_results[0];

DEBUG_VALIDATE_BUG_ON exists but the whole part of code looks confusing. Why do we use an array of matches when only first is expected? And i believe without debug a combination of patterns is possible when more than one is returned and some of the following damaged the 'reverse_flow'.

It looks a bit confusing why PMGetProtoInspect tries to traverse all patterns (from line 294 of app-layer-detect-proto.c) while its client expects the one and only. And it has even DEBUG_VALIDATE_BUG_ON check.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the investigation

Now i believe even debug validation is not necessary.

It may be useful in the future, if some new code tries to break this assertion

Why do we use an array of matches when only first is expected?

Because we use some generic code for multi-pattern matching. The debug validation is here so that fuzzing can find if there is one way to find mutliple protocols with patterns... which would lead likely to bad things...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see.
I prepared a version with the debug validation - #11644

Regarding the array of matches - I still don't quite understand why we do not break from the loop started at app-layer-detect-proto.c:294 when the pattern is found.
Well, I did not inspect the behavior fully and it's not in the scope of current change. Moreover, I hope the condition with multiple patterns detected is impossible with the current set of registered protocol detection patterns.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moreover, I hope the condition with multiple patterns detected is impossible with the current set of registered protocol detection patterns.

That is the important point indeed...

alproto = AppLayerProtoDetectPEGetProto(f, flags);
}

end:
if (!AppProtoIsValid(alproto))
alproto = pm_alproto;

SCReturnUInt(alproto);
}

Expand Down
Loading