aiden
/
suricata
mirror of https://github.com/OISF/suricata
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

rfb: never return error on unknown traffic

Browse Source 
We only try to parse a small subset of what is possible in
RFB. Currently we only understand some standard auth schemes
and stop parsing when the server-client handshake is complete.
Since in IPS mode returning an error from the parser causes
drops that are likely uncalled for, we do not want to return
errors when we simply do not understand what happens in the
traffic. This addresses Redmine #5912.

Bug: #5912.
pull/9083/head
Sascha Steinbiss 2 years ago committed by Victor Julien
parent 836fff3679
commit 1f8a5874fb
7 changed files with 202 additions and 69 deletions
Show Stats Download Patch File Download Diff File

1
rules/Makefile.am
Unescape Escape View File

@ -17,6 +17,7 @@ mqtt-events.rules \
nfs-events.rules \
ntp-events.rules \
quic-events.rules \
rfb-events.rules \
smb-events.rules \
smtp-events.rules \
ssh-events.rules \

10
rules/rfb-events.rules
Unescape Escape View File

@ -0,0 +1,10 @@
# RFB app-layer event rules.
#
# These SIDs fall in the 2233000+ range. See:
# http://doc.emergingthreats.net/bin/view/Main/SidAllocation and
# https://redmine.openinfosecfoundation.org/projects/suricata/wiki/AppLayer
alert rfb any any -> any any (msg:"SURICATA RFB Malformed or unknown message"; app-layer-event:rfb.malformed_message; classtype:protocol-command-decode; sid:2233000; rev:1;)
alert rfb any any -> any any (msg:"SURICATA RFB Unimplemented security type"; app-layer-event:rfb.unimplemented_security_type; classtype:protocol-command-decode; sid:2233001; rev:1;)
alert rfb any any -> any any (msg:"SURICATA RFB Unknown security result"; app-layer-event:rfb.unknown_security_result; classtype:protocol-command-decode; sid:2233002; rev:1;)
alert rfb any any -> any any (msg:"SURICATA RFB Unexpected State in Parser"; app-layer-event:rfb.confused_state; classtype:protocol-command-decode; sid:2233003; rev:1;)

14
rust/src/rfb/detect.rs
Unescape Escape View File

@ -22,9 +22,7 @@ use std::ptr;
#[no_mangle]
pub unsafe extern "C" fn rs_rfb_tx_get_name(
tx: &mut RFBTransaction,
buffer: *mut *const u8,
buffer_len: *mut u32,
tx: &mut RFBTransaction, buffer: *mut *const u8, buffer_len: *mut u32,
) -> u8 {
if let Some(ref r) = tx.tc_server_init {
let p = &r.name;
@ -42,10 +40,7 @@ pub unsafe extern "C" fn rs_rfb_tx_get_name(
}
#[no_mangle]
pub unsafe extern "C" fn rs_rfb_tx_get_sectype(
tx: &mut RFBTransaction,
sectype: *mut u32,
) -> u8 {
pub unsafe extern "C" fn rs_rfb_tx_get_sectype(tx: &mut RFBTransaction, sectype: *mut u32) -> u8 {
if let Some(ref r) = tx.chosen_security_type {
*sectype = *r;
return 1;
@ -58,8 +53,7 @@ pub unsafe extern "C" fn rs_rfb_tx_get_sectype(
#[no_mangle]
pub unsafe extern "C" fn rs_rfb_tx_get_secresult(
tx: &mut RFBTransaction,
secresult: *mut u32,
tx: &mut RFBTransaction, secresult: *mut u32,
) -> u8 {
if let Some(ref r) = tx.tc_security_result {
*secresult = r.status;
@ -67,4 +61,4 @@ pub unsafe extern "C" fn rs_rfb_tx_get_secresult(
}
return 0;
}
}

37
rust/src/rfb/logger.rs
Unescape Escape View File

@ -17,10 +17,10 @@
// Author: Frank Honza <frank.honza@dcso.de>
use std;
use std::fmt::Write;
use super::rfb::RFBTransaction;
use crate::jsonbuilder::{JsonBuilder, JsonError};
use std;
use std::fmt::Write;
fn log_rfb(tx: &RFBTransaction, js: &mut JsonBuilder) -> Result<(), JsonError> {
js.open_object("rfb")?;
@ -64,15 +64,17 @@ fn log_rfb(tx: &RFBTransaction, js: &mut JsonBuilder) -> Result<(), JsonError> {
}
js.close()?;
}
_ => ()
_ => (),
}
if let Some(security_result) = &tx.tc_security_result {
let _ = match security_result.status {
0 => js.set_string("security_result", "OK")?,
1 => js.set_string("security-result", "FAIL")?,
2 => js.set_string("security_result", "TOOMANY")?,
_ => js.set_string("security_result",
&format!("UNKNOWN ({})", security_result.status))?,
_ => js.set_string(
"security_result",
&format!("UNKNOWN ({})", security_result.status),
)?,
};
}
js.close()?; // Close authentication.
@ -92,15 +94,27 @@ fn log_rfb(tx: &RFBTransaction, js: &mut JsonBuilder) -> Result<(), JsonError> {
js.set_string_from_bytes("name", &tc_server_init.name)?;
js.open_object("pixel_format")?;
js.set_uint("bits_per_pixel", tc_server_init.pixel_format.bits_per_pixel as u64)?;
js.set_uint(
"bits_per_pixel",
tc_server_init.pixel_format.bits_per_pixel as u64,
)?;
js.set_uint("depth", tc_server_init.pixel_format.depth as u64)?;
js.set_bool("big_endian", tc_server_init.pixel_format.big_endian_flag != 0)?;
js.set_bool("true_color", tc_server_init.pixel_format.true_colour_flag != 0)?;
js.set_bool(
"big_endian",
tc_server_init.pixel_format.big_endian_flag != 0,
)?;
js.set_bool(
"true_color",
tc_server_init.pixel_format.true_colour_flag != 0,
)?;
js.set_uint("red_max", tc_server_init.pixel_format.red_max as u64)?;
js.set_uint("green_max", tc_server_init.pixel_format.green_max as u64)?;
js.set_uint("blue_max", tc_server_init.pixel_format.blue_max as u64)?;
js.set_uint("red_shift", tc_server_init.pixel_format.red_shift as u64)?;
js.set_uint("green_shift", tc_server_init.pixel_format.green_shift as u64)?;
js.set_uint(
"green_shift",
tc_server_init.pixel_format.green_shift as u64,
)?;
js.set_uint("blue_shift", tc_server_init.pixel_format.blue_shift as u64)?;
js.close()?;
@ -113,8 +127,9 @@ fn log_rfb(tx: &RFBTransaction, js: &mut JsonBuilder) -> Result<(), JsonError> {
}
#[no_mangle]
pub unsafe extern "C" fn rs_rfb_logger_log(tx: *mut std::os::raw::c_void,
js: &mut JsonBuilder) -> bool {
pub unsafe extern "C" fn rs_rfb_logger_log(
tx: *mut std::os::raw::c_void, js: &mut JsonBuilder,
) -> bool {
let tx = cast_pointer!(tx, RFBTransaction);
log_rfb(tx, js).is_ok()
}

2
rust/src/rfb/mod.rs
Unescape Escape View File

@ -20,4 +20,4 @@
pub mod detect;
pub mod logger;
pub mod parser;
pub mod rfb;
pub mod rfb;

8
rust/src/rfb/parser.rs
Unescape Escape View File

@ -17,15 +17,15 @@
// Author: Frank Honza <frank.honza@dcso.de>
use nom7::bytes::streaming::take;
use nom7::bytes::streaming::tag;
use nom7::bytes::streaming::take;
use nom7::combinator::map_res;
use nom7::number::streaming::*;
use nom7::*;
use std::fmt;
use std::str;
#[derive(Debug,PartialEq)]
#[derive(Debug, PartialEq)]
pub enum RFBGlobalState {
TCServerProtocolVersion,
TCSupportedSecurityTypes,
@ -38,7 +38,7 @@ pub enum RFBGlobalState {
TSVncResponse,
TCSecurityResult,
TSClientInit,
Message,
Skip,
}
impl fmt::Display for RFBGlobalState {
@ -55,7 +55,7 @@ impl fmt::Display for RFBGlobalState {
RFBGlobalState::TCSecurityResult => write!(f, "TCSecurityResult"),
RFBGlobalState::TCServerSecurityType => write!(f, "TCServerSecurityType"),
RFBGlobalState::TSClientInit => write!(f, "TSClientInit"),
RFBGlobalState::Message => write!(f, "Message"),
RFBGlobalState::Skip => write!(f, "Skip"),
}
}
}

199
rust/src/rfb/rfb.rs
Unescape Escape View File

@ -29,6 +29,14 @@ use std::ffi::CString;
static mut ALPROTO_RFB: AppProto = ALPROTO_UNKNOWN;
#[derive(FromPrimitive, Debug, AppLayerEvent)]
pub enum RFBEvent {
UnimplementedSecurityType,
UnknownSecurityResult,
MalformedMessage,
ConfusedState,
}
#[derive(AppLayerFrameType)]
pub enum RFBFrameType {
Pdu,
@ -87,6 +95,10 @@ impl RFBTransaction {
tx_data: applayer::AppLayerTxData::new(),
}
}
fn set_event(&mut self, event: RFBEvent) {
self.tx_data.set_event(event as u8);
}
}
pub struct RFBState {
@ -196,7 +208,9 @@ impl RFBState {
if let Some(current_transaction) = self.get_current_tx() {
current_transaction.ts_client_protocol_version = Some(request);
} else {
return AppLayerResult::err();
debug_validate_fail!(
"no transaction set at protocol selection stage"
);
}
}
Err(Err::Incomplete(_)) => {
@ -206,6 +220,7 @@ impl RFBState {
);
}
Err(_) => {
// We even failed to parse the protocol version.
return AppLayerResult::err();
}
}
@ -228,7 +243,18 @@ impl RFBState {
match chosen_security_type {
2 => self.state = parser::RFBGlobalState::TCVncChallenge,
1 => self.state = parser::RFBGlobalState::TSClientInit,
_ => return AppLayerResult::err(),
_ => {
if let Some(current_transaction) = self.get_current_tx() {
current_transaction
.set_event(RFBEvent::UnimplementedSecurityType);
}
// We have just have seen a security type we don't know about.
// This is not bad per se, it might just mean this is a
// proprietary one not in the spec.
// Continue the flow but stop trying to map the protocol.
self.state = parser::RFBGlobalState::Skip;
return AppLayerResult::ok();
}
}
if let Some(current_transaction) = self.get_current_tx() {
@ -236,7 +262,7 @@ impl RFBState {
current_transaction.chosen_security_type =
Some(chosen_security_type as u32);
} else {
return AppLayerResult::err();
debug_validate_fail!("no transaction set at security type stage");
}
}
Err(Err::Incomplete(_)) => {
@ -246,7 +272,13 @@ impl RFBState {
);
}
Err(_) => {
return AppLayerResult::err();
if let Some(current_transaction) = self.get_current_tx() {
current_transaction.set_event(RFBEvent::MalformedMessage);
}
// We failed to parse the security type.
// Continue the flow but stop trying to map the protocol.
self.state = parser::RFBGlobalState::Skip;
return AppLayerResult::ok();
}
}
}
@ -268,7 +300,7 @@ impl RFBState {
if let Some(current_transaction) = self.get_current_tx() {
current_transaction.ts_vnc_response = Some(request);
} else {
return AppLayerResult::err();
debug_validate_fail!("no transaction set at security result stage");
}
}
Err(Err::Incomplete(_)) => {
@ -278,7 +310,12 @@ impl RFBState {
);
}
Err(_) => {
return AppLayerResult::err();
if let Some(current_transaction) = self.get_current_tx() {
current_transaction.set_event(RFBEvent::MalformedMessage);
}
// Continue the flow but stop trying to map the protocol.
self.state = parser::RFBGlobalState::Skip;
return AppLayerResult::ok();
}
},
parser::RFBGlobalState::TSClientInit => match parser::parse_client_init(current) {
@ -299,7 +336,7 @@ impl RFBState {
if let Some(current_transaction) = self.get_current_tx() {
current_transaction.ts_client_init = Some(request);
} else {
return AppLayerResult::err();
debug_validate_fail!("no transaction set at client init stage");
}
}
Err(Err::Incomplete(_)) => {
@ -309,20 +346,34 @@ impl RFBState {
);
}
Err(_) => {
return AppLayerResult::err();
if let Some(current_transaction) = self.get_current_tx() {
current_transaction.set_event(RFBEvent::MalformedMessage);
}
// We failed to parse the client init.
// Continue the flow but stop trying to map the protocol.
self.state = parser::RFBGlobalState::Skip;
return AppLayerResult::ok();
}
},
parser::RFBGlobalState::Message => {
//todo implement RFB messages, for now we stop here
return AppLayerResult::err();
}
parser::RFBGlobalState::TCServerProtocolVersion => {
SCLogDebug!("Reversed traffic, expected response.");
return AppLayerResult::err();
parser::RFBGlobalState::Skip => {
// End of parseable handshake reached, skip rest of traffic
return AppLayerResult::ok();
}
_ => {
SCLogDebug!("Invalid state for request {}", self.state);
current = b"";
// We have gotten out of sync with the expected state flow.
// This could happen since we use a global state (i.e. that
// is used for both directions), but if traffic can not be
// parsed as expected elsewhere, we might not have advanced
// a state for one direction but received data in the
// "unexpected" direction, causing the parser to end up
// here. Let's stop trying to parse the traffic but still
// accept it.
SCLogDebug!("Invalid state for request: {}", self.state);
if let Some(current_transaction) = self.get_current_tx() {
current_transaction.set_event(RFBEvent::ConfusedState);
}
self.state = parser::RFBGlobalState::Skip;
return AppLayerResult::ok();
}
}
}
@ -368,7 +419,7 @@ impl RFBState {
if let Some(current_transaction) = self.get_current_tx() {
current_transaction.tc_server_protocol_version = Some(request);
} else {
return AppLayerResult::err();
debug_validate_fail!("no transaction set but we just set one");
}
}
Err(Err::Incomplete(_)) => {
@ -378,6 +429,7 @@ impl RFBState {
);
}
Err(_) => {
// We even failed to parse the protocol version.
return AppLayerResult::err();
}
}
@ -415,7 +467,7 @@ impl RFBState {
if let Some(current_transaction) = self.get_current_tx() {
current_transaction.tc_supported_security_types = Some(request);
} else {
return AppLayerResult::err();
debug_validate_fail!("no transaction set at security type stage");
}
}
Err(Err::Incomplete(_)) => {
@ -425,7 +477,12 @@ impl RFBState {
);
}
Err(_) => {
return AppLayerResult::err();
if let Some(current_transaction) = self.get_current_tx() {
current_transaction.set_event(RFBEvent::MalformedMessage);
}
// Continue the flow but stop trying to map the protocol.
self.state = parser::RFBGlobalState::Skip;
return AppLayerResult::ok();
}
}
}
@ -451,8 +508,20 @@ impl RFBState {
1 => self.state = parser::RFBGlobalState::TSClientInit,
2 => self.state = parser::RFBGlobalState::TCVncChallenge,
_ => {
// TODO Event unknown security type
return AppLayerResult::err();
if let Some(current_transaction) = self.get_current_tx() {
current_transaction
.set_event(RFBEvent::UnimplementedSecurityType);
} else {
debug_validate_fail!(
"no transaction set at security type stage"
);
}
// We have just have seen a security type we don't know about.
// This is not bad per se, it might just mean this is a
// proprietary one not in the spec.
// Continue the flow but stop trying to map the protocol.
self.state = parser::RFBGlobalState::Skip;
return AppLayerResult::ok();
}
}
@ -461,7 +530,7 @@ impl RFBState {
current_transaction.chosen_security_type =
Some(chosen_security_type);
} else {
return AppLayerResult::err();
debug_validate_fail!("no transaction set at security type stage");
}
}
Err(Err::Incomplete(_)) => {
@ -471,7 +540,12 @@ impl RFBState {
);
}
Err(_) => {
return AppLayerResult::err();
if let Some(current_transaction) = self.get_current_tx() {
current_transaction.set_event(RFBEvent::MalformedMessage);
}
// Continue the flow but stop trying to map the protocol.
self.state = parser::RFBGlobalState::Skip;
return AppLayerResult::ok();
}
}
}
@ -493,7 +567,7 @@ impl RFBState {
if let Some(current_transaction) = self.get_current_tx() {
current_transaction.tc_vnc_challenge = Some(request);
} else {
return AppLayerResult::err();
debug_validate_fail!("no transaction set at auth stage");
}
}
Err(Err::Incomplete(_)) => {
@ -503,7 +577,12 @@ impl RFBState {
);
}
Err(_) => {
return AppLayerResult::err();
if let Some(current_transaction) = self.get_current_tx() {
current_transaction.set_event(RFBEvent::MalformedMessage);
}
// Continue the flow but stop trying to map the protocol.
self.state = parser::RFBGlobalState::Skip;
return AppLayerResult::ok();
}
},
parser::RFBGlobalState::TCSecurityResult => {
@ -526,12 +605,19 @@ impl RFBState {
if let Some(current_transaction) = self.get_current_tx() {
current_transaction.tc_security_result = Some(request);
} else {
return AppLayerResult::err();
debug_validate_fail!(
"no transaction set at security result stage"
);
}
} else if request.status == 1 {
self.state = parser::RFBGlobalState::TCFailureReason;
} else {
// TODO: Event: unknown security result value
if let Some(current_transaction) = self.get_current_tx() {
current_transaction.set_event(RFBEvent::UnknownSecurityResult);
}
// Continue the flow but stop trying to map the protocol.
self.state = parser::RFBGlobalState::Skip;
return AppLayerResult::ok();
}
}
Err(Err::Incomplete(_)) => {
@ -541,7 +627,12 @@ impl RFBState {
);
}
Err(_) => {
return AppLayerResult::err();
if let Some(current_transaction) = self.get_current_tx() {
current_transaction.set_event(RFBEvent::MalformedMessage);
}
// Continue the flow but stop trying to map the protocol.
self.state = parser::RFBGlobalState::Skip;
return AppLayerResult::ok();
}
}
}
@ -551,9 +642,9 @@ impl RFBState {
if let Some(current_transaction) = self.get_current_tx() {
current_transaction.tc_failure_reason = Some(request);
} else {
return AppLayerResult::err();
debug_validate_fail!("no transaction set at failure reason stage");
}
return AppLayerResult::err();
return AppLayerResult::ok();
}
Err(Err::Incomplete(_)) => {
return AppLayerResult::incomplete(
@ -562,7 +653,12 @@ impl RFBState {
);
}
Err(_) => {
return AppLayerResult::err();
if let Some(current_transaction) = self.get_current_tx() {
current_transaction.set_event(RFBEvent::MalformedMessage);
}
// Continue the flow but stop trying to map the protocol.
self.state = parser::RFBGlobalState::Skip;
return AppLayerResult::ok();
}
}
}
@ -580,14 +676,14 @@ impl RFBState {
current = rem;
self.state = parser::RFBGlobalState::Message;
self.state = parser::RFBGlobalState::Skip;
if let Some(current_transaction) = self.get_current_tx() {
current_transaction.tc_server_init = Some(request);
// connection initialization is complete and parsed
current_transaction.complete = true;
} else {
return AppLayerResult::err();
debug_validate_fail!("no transaction set at server init stage");
}
}
Err(Err::Incomplete(_)) => {
@ -597,17 +693,34 @@ impl RFBState {
);
}
Err(_) => {
return AppLayerResult::err();
if let Some(current_transaction) = self.get_current_tx() {
current_transaction.set_event(RFBEvent::MalformedMessage);
}
// Continue the flow but stop trying to map the protocol.
self.state = parser::RFBGlobalState::Skip;
return AppLayerResult::ok();
}
}
}
parser::RFBGlobalState::Message => {
parser::RFBGlobalState::Skip => {
//todo implement RFB messages, for now we stop here
return AppLayerResult::err();
return AppLayerResult::ok();
}
_ => {
SCLogDebug!("Invalid state for response");
return AppLayerResult::err();
// We have gotten out of sync with the expected state flow.
// This could happen since we use a global state (i.e. that
// is used for both directions), but if traffic can not be
// parsed as expected elsewhere, we might not have advanced
// a state for one direction but received data in the
// "unexpected" direction, causing the parser to end up
// here. Let's stop trying to parse the traffic but still
// accept it.
SCLogDebug!("Invalid state for response: {}", self.state);
if let Some(current_transaction) = self.get_current_tx() {
current_transaction.set_event(RFBEvent::ConfusedState);
}
self.state = parser::RFBGlobalState::Skip;
return AppLayerResult::ok();
}
}
}
@ -713,8 +826,8 @@ pub unsafe extern "C" fn rs_rfb_register_parser() {
tx_comp_st_ts: 1,
tx_comp_st_tc: 1,
tx_get_progress: rs_rfb_tx_get_alstate_progress,
get_eventinfo: None,
get_eventinfo_byid: None,
get_eventinfo: Some(RFBEvent::get_event_info),
get_eventinfo_byid: Some(RFBEvent::get_event_info_by_id),
localstorage_new: None,
localstorage_free: None,
get_tx_files: None,
@ -757,7 +870,7 @@ mod test {
0x67, 0x6c, 0x65, 0x73, 0x40, 0x6c, 0x6f, 0x63, 0x61, 0x6c, 0x68, 0x6f, 0x73, 0x74,
0x2e, 0x6c, 0x6f, 0x63, 0x61, 0x6c, 0x64, 0x6f, 0x6d, 0x61, 0x69, 0x6e,
];
let r = state.parse_request(
let r = state.parse_response(
std::ptr::null(),
StreamSlice::from_slice(buf, STREAM_START, 0),
);
@ -862,7 +975,7 @@ mod test {
std::ptr::null(),
StreamSlice::from_slice(&buf[36..90], STREAM_START, 0),
);
ok_state = parser::RFBGlobalState::Message;
ok_state = parser::RFBGlobalState::Skip;
assert_eq!(init_state.state, ok_state);
}
}

Write Preview
Loading…
Cancel
Save