diff --git a/doc/userguide/configuration/suricata-yaml.rst b/doc/userguide/configuration/suricata-yaml.rst index b2b20d08f497..5b374bbe2d35 100644 --- a/doc/userguide/configuration/suricata-yaml.rst +++ b/doc/userguide/configuration/suricata-yaml.rst @@ -2015,6 +2015,19 @@ default is 1 MB. mqtt: max-msg-length: 1mb +RFB +~~~ + +RFB can have some strings whose maximum length according to the RFC is 4GiB. +In order to limit ram consumption and log output, a configuration parameter ``max-string-length`` is available. +This limit will also apply during detection. +An event ``rfb.too_long_string`` will be emitted when a string exceeds the limit. The default is 4 KiB. + +:: + + rfb: + max-string-length: 4 KiB + SMTP ~~~~~~ diff --git a/doc/userguide/firewall/firewall-design.rst b/doc/userguide/firewall/firewall-design.rst index b2eb9d38c721..351cd19f25d5 100644 --- a/doc/userguide/firewall/firewall-design.rst +++ b/doc/userguide/firewall/firewall-design.rst @@ -122,6 +122,14 @@ alert action in firewall rules. The effect will be the creation of an alert event when the firewall rule matches. +config +~~~~~~ + +``config`` is a primary firewall action used to apply the setting of the ``config`` +rule keyword when the rule matches, see :doc:`../rules/config`. +The ``config`` action does not issue a verdict for the packet or the flow, so the +other tables are still evaluated. It is not available as a secondary action. + Multi action rules ~~~~~~~~~~~~~~~~~~ diff --git a/rules/rfb-events.rules b/rules/rfb-events.rules index 866a23851bb0..294d8b74bf9d 100644 --- a/rules/rfb-events.rules +++ b/rules/rfb-events.rules @@ -6,3 +6,4 @@ alert rfb any any -> any any (msg:"SURICATA RFB Malformed or unknown message"; a 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;) +alert rfb any any -> any any (msg:"SURICATA RFB too long string"; app-layer-event:rfb.too_long_string; classtype:protocol-command-decode; sid:2233004; rev:1;) diff --git a/rust/Cargo.lock.in b/rust/Cargo.lock.in index 9c80e26f9bb1..520ebf71c943 100644 --- a/rust/Cargo.lock.in +++ b/rust/Cargo.lock.in @@ -1169,9 +1169,9 @@ dependencies = [ [[package]] name = "psl" -version = "2.1.215" +version = "2.1.228" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "caff7403e06671f170c65dc7bf475ed31d6e108c7e0d2440fb4df8ba56cfded6" +checksum = "158b8294dde3909df5a4ec68a35f8c39720151834b1f87050b87b37435150011" dependencies = [ "psl-types", ] diff --git a/rust/src/rfb/logger.rs b/rust/src/rfb/logger.rs index 9934aae05733..d677bef6947d 100644 --- a/rust/src/rfb/logger.rs +++ b/rust/src/rfb/logger.rs @@ -81,7 +81,10 @@ fn log_rfb(tx: &RFBTransaction, js: &mut JsonBuilder) -> Result<(), JsonError> { js.close()?; // Close authentication. if let Some(ref reason) = tx.tc_failure_reason { - js.set_string("server_security_failure_reason", &reason.reason_string)?; + js.set_string( + "server_security_failure_reason", + &String::from_utf8_lossy(&reason.reason_string), + )?; } // Client/Server init diff --git a/rust/src/rfb/parser.rs b/rust/src/rfb/parser.rs index 2faaaaaa6b8f..30a735e66c9b 100644 --- a/rust/src/rfb/parser.rs +++ b/rust/src/rfb/parser.rs @@ -93,7 +93,8 @@ pub struct SecurityResult { } pub struct FailureReason { - pub reason_string: String, + pub reason_string: Vec, + pub to_skip: u32, } pub struct VncAuth { @@ -123,6 +124,7 @@ pub struct ServerInit { pub pixel_format: PixelFormat, pub name_length: u32, pub name: Vec, + pub to_skip: u32, } pub fn parse_protocol_version(i: &[u8]) -> IResult<&[u8], ProtocolVersion> { @@ -177,13 +179,15 @@ pub fn parse_security_result(i: &[u8]) -> IResult<&[u8], SecurityResult> { Ok((i, SecurityResult { status })) } -pub fn parse_failure_reason(i: &[u8]) -> IResult<&[u8], FailureReason> { +pub fn parse_failure_reason(i: &[u8], max_len: u32) -> IResult<&[u8], FailureReason> { let (i, reason_length) = be_u32(i)?; - let (i, reason_string) = map_res(take(reason_length as usize), str::from_utf8).parse(i)?; + let to_skip = reason_length.saturating_sub(max_len); + let (i, reason_string) = take((reason_length - to_skip) as usize).parse(i)?; Ok(( i, FailureReason { - reason_string: reason_string.to_string(), + reason_string: reason_string.to_vec(), + to_skip, }, )) } @@ -220,18 +224,21 @@ pub fn parse_pixel_format(i: &[u8]) -> IResult<&[u8], PixelFormat> { Ok((i, format)) } -pub fn parse_server_init(i: &[u8]) -> IResult<&[u8], ServerInit> { +pub fn parse_server_init(i: &[u8], max_len: u32) -> IResult<&[u8], ServerInit> { let (i, width) = be_u16(i)?; let (i, height) = be_u16(i)?; let (i, pixel_format) = parse_pixel_format(i)?; let (i, name_length) = be_u32(i)?; - let (i, name) = take(name_length as usize)(i)?; + let to_skip = name_length.saturating_sub(max_len); + + let (i, name) = take((name_length - to_skip) as usize)(i)?; let init = ServerInit { width, height, pixel_format, name_length, name: name.to_vec(), + to_skip, }; Ok((i, init)) } @@ -273,7 +280,7 @@ mod tests { 0x2e, 0x6c, 0x6f, 0x63, 0x61, 0x6c, 0x64, 0x6f, 0x6d, 0x61, 0x69, 0x6e, ]; - let result = parse_server_init(&buf); + let result = parse_server_init(&buf, 4096); match result { Ok((remainder, message)) => { // Check the first message. diff --git a/rust/src/rfb/rfb.rs b/rust/src/rfb/rfb.rs index 1decbf3c6933..931d2feace97 100644 --- a/rust/src/rfb/rfb.rs +++ b/rust/src/rfb/rfb.rs @@ -21,6 +21,7 @@ use super::parser; use crate::applayer; use crate::applayer::*; +use crate::conf::{conf_get, get_memval}; use crate::core::{ sc_app_layer_parser_trigger_raw_stream_inspection, ALPROTO_UNKNOWN, IPPROTO_TCP, }; @@ -38,6 +39,9 @@ use suricata_sys::sys::{ }; pub(super) static mut ALPROTO_RFB: AppProto = ALPROTO_UNKNOWN; +// Maximum strings length in bytes. +// If some string exceeds this length, it will be truncated. +static mut MAX_STR_LEN: u32 = 4096; #[derive(FromPrimitive, Debug, AppLayerEvent)] pub enum RFBEvent { @@ -45,6 +49,7 @@ pub enum RFBEvent { UnknownSecurityResult, MalformedMessage, ConfusedState, + TooLongString, } #[derive(AppLayerFrameType)] @@ -116,6 +121,7 @@ pub struct RFBState { tx_id: u64, transactions: Vec, state: parser::RFBGlobalState, + to_skip_tc: u32, } impl State for RFBState { @@ -141,6 +147,7 @@ impl RFBState { tx_id: 0, transactions: Vec::new(), state: parser::RFBGlobalState::TCServerProtocolVersion, + to_skip_tc: 0, } } @@ -426,9 +433,16 @@ impl RFBState { if input.is_empty() { return AppLayerResult::ok(); } - - let mut current = input; let mut consumed = 0; + let mut current = input; + if self.to_skip_tc >= input.len() as u32 { + self.to_skip_tc -= input.len() as u32; + return AppLayerResult::ok(); + } else if self.to_skip_tc > 0 { + consumed += self.to_skip_tc as usize; + current = ¤t[self.to_skip_tc as usize..]; + self.to_skip_tc = 0; + } SCLogDebug!( "response_state {}, response_len {}", self.state, @@ -709,9 +723,15 @@ impl RFBState { } } parser::RFBGlobalState::TCFailureReason => { - match parser::parse_failure_reason(current) { - Ok((_rem, request)) => { + match parser::parse_failure_reason(current, unsafe { MAX_STR_LEN }) { + Ok((rem, request)) => { + if request.to_skip >= rem.len() as u32 { + self.to_skip_tc = request.to_skip - rem.len() as u32; + } if let Some(current_transaction) = self.get_current_tx() { + if request.to_skip > 0 { + current_transaction.set_event(RFBEvent::TooLongString); + } current_transaction.tc_failure_reason = Some(request); sc_app_layer_parser_trigger_raw_stream_inspection( flow, @@ -740,23 +760,35 @@ impl RFBState { } } parser::RFBGlobalState::TCServerInit => { - match parser::parse_server_init(current) { + match parser::parse_server_init(current, unsafe { MAX_STR_LEN }) { Ok((rem, request)) => { consumed += current.len() - rem.len(); let _pdu = Frame::new( flow, &stream_slice, current, - consumed as i64, + consumed as i64 + request.to_skip as i64, RFBFrameType::Pdu as u8, None, ); - current = rem; + if request.to_skip >= rem.len() as u32 { + current = &rem[rem.len()..]; + consumed += rem.len(); + self.to_skip_tc = request.to_skip - rem.len() as u32; + } else if request.to_skip > 0 { + current = &rem[request.to_skip as usize..]; + consumed += request.to_skip as usize; + } else { + current = rem; + } self.state = parser::RFBGlobalState::Skip; if let Some(current_transaction) = self.get_current_tx() { + if request.to_skip > 0 { + current_transaction.set_event(RFBEvent::TooLongString); + } current_transaction.tc_server_init = Some(request); sc_app_layer_parser_trigger_raw_stream_inspection( flow, @@ -950,6 +982,17 @@ pub unsafe extern "C" fn SCRfbRegisterParser() { { SCLogDebug!("Failed to register protocol detection pattern for direction TOCLIENT"); } + if let Some(val) = conf_get("app-layer.protocols.rfb.max-string-length") { + if let Ok(v) = get_memval(val) { + if v <= u32::MAX.into() { + MAX_STR_LEN = v as u32; + } else { + SCLogWarning!("rfb.max-string-length max is {}", u32::MAX); + } + } else { + SCLogWarning!("Invalid value for rfb.max-string-length: {}", val); + } + } } else { SCLogDebug!("Protocol detector and parser disabled for RFB."); } diff --git a/src/detect-engine-analyzer.c b/src/detect-engine-analyzer.c index f8d14cbc8de3..b07791697248 100644 --- a/src/detect-engine-analyzer.c +++ b/src/detect-engine-analyzer.c @@ -2146,46 +2146,70 @@ void EngineAnalysisRules(const DetectEngineCtx *de_ctx, #include "app-layer-parser.h" -static void AddPolicy(const DetectEngineCtx *de_ctx, RuleAnalyzer *ctx, const AppProto a, - const uint8_t sub_state, const uint8_t state, const uint8_t direction) +/** + * \brief Render a resolved firewall default policy as ":". + * + * \retval true \p out holds the rendered policy + * \retval false the policy could not be rendered + */ +static bool FirewallPolicyToString( + const struct DetectFirewallPolicy *p, char *out, const size_t out_size) { - char policy_string[64] = ""; - const struct DetectFirewallPolicies *fw_policies = de_ctx->fw_policies; - const struct DetectFirewallAppPolicy lookup = { - .alproto = a, .sub_state = sub_state, .progress = state, .direction = direction - }; - const struct DetectFirewallAppPolicy *ap = - HashTableLookup(fw_policies->app_policies, (void *)&lookup, 0); - if (ap == NULL) - return; - const struct DetectFirewallPolicy *p = &ap->policy; - const char *as = ActionScopeToString(p->action_scope); DEBUG_VALIDATE_BUG_ON(as == NULL); if (as == NULL) - return; + return false; if (p->action & ACTION_REJECT_ANY) { if (p->action & ACTION_REJECT_DST) { - snprintf(policy_string, sizeof(policy_string), "rejectdst:%s", as); + snprintf(out, out_size, "rejectdst:%s", as); } else if (p->action & ACTION_REJECT_BOTH) { - snprintf(policy_string, sizeof(policy_string), "rejectboth:%s", as); + snprintf(out, out_size, "rejectboth:%s", as); } else { - snprintf(policy_string, sizeof(policy_string), "rejectsrc:%s", as); + snprintf(out, out_size, "rejectsrc:%s", as); } } else if (p->action & ACTION_DROP) { - snprintf(policy_string, sizeof(policy_string), "drop:%s", as); + snprintf(out, out_size, "drop:%s", as); } else if (p->action & ACTION_ACCEPT) { - snprintf(policy_string, sizeof(policy_string), "accept:%s", as); + snprintf(out, out_size, "accept:%s", as); } else { DEBUG_VALIDATE_BUG_ON(1); + return false; } if (p->action & ACTION_PASS) { - if (p->action_scope == ACTION_SCOPE_FLOW) { - strlcat(policy_string, ",pass:flow", sizeof(policy_string)); + if (p->action_scope == ACTION_SCOPE_FLOW || p->action_scope == ACTION_SCOPE_PACKET) { + if (strlcat(out, ",pass:", out_size) >= out_size || + strlcat(out, as, out_size) >= out_size) { + DEBUG_VALIDATE_BUG_ON(1); + return false; + } } else { DEBUG_VALIDATE_BUG_ON(1); + return false; } } + if (p->action & ACTION_ALERT) { + if (strlcat(out, ",alert", out_size) >= out_size) { + DEBUG_VALIDATE_BUG_ON(1); + return false; + } + } + return true; +} + +static void AddPolicy(const DetectEngineCtx *de_ctx, RuleAnalyzer *ctx, const AppProto a, + const uint8_t sub_state, const uint8_t state, const uint8_t direction) +{ + char policy_string[64] = ""; + const struct DetectFirewallPolicies *fw_policies = de_ctx->fw_policies; + const struct DetectFirewallAppPolicy lookup = { + .alproto = a, .sub_state = sub_state, .progress = state, .direction = direction + }; + const struct DetectFirewallAppPolicy *ap = + HashTableLookup(fw_policies->app_policies, (void *)&lookup, 0); + if (ap == NULL) + return; + if (!FirewallPolicyToString(&ap->policy, policy_string, sizeof(policy_string))) + return; SCJbSetString(ctx->js, "policy", policy_string); } @@ -2258,7 +2282,11 @@ int FirewallAnalyzer(const DetectEngineCtx *de_ctx) SCJbOpenObject(ctx.js, "tables"); SCJbOpenObject(ctx.js, "packet:filter"); - SCJbSetString(ctx.js, "policy", "drop:packet"); + char pkt_policy[64] = ""; + if (FirewallPolicyToString(&de_ctx->fw_policies->pkt[DETECT_FIREWALL_POLICY_PACKET_FILTER], + pkt_policy, sizeof(pkt_policy))) { + SCJbSetString(ctx.js, "policy", pkt_policy); + } SCJbOpenArray(ctx.js, "rules"); uint32_t accept_rules = 0; uint32_t last_sid = 0; diff --git a/src/detect-parse.c b/src/detect-parse.c index 44d546984c19..6a7c9d768465 100644 --- a/src/detect-parse.c +++ b/src/detect-parse.c @@ -1822,6 +1822,10 @@ static int SigParseActionDo(const char *action_in, const int idx, const bool fw_ "rules"); return -1; } + if (idx > 0 && (flags & ACTION_PASS) && !(*action_out & ACTION_ACCEPT)) { + SCLogError("'pass' is only supported as a secondary action for 'accept'"); + return -1; + } } /* parse scope, if any */ @@ -1873,6 +1877,14 @@ static int SigParseActionDo(const char *action_in, const int idx, const bool fw_ return -1; } *scope_out = scope_flags; + } else if (*scope_out != 0 && (flags & ACTION_PASS)) { + /* No scope given, this action inherits the scope set by the preceding + * actions of a multi-action rule. */ + if (*scope_out != ACTION_SCOPE_PACKET && *scope_out != ACTION_SCOPE_FLOW) { + SCLogError("invalid action scope '%s' in action '%s': only 'packet' and 'flow' allowed", + ActionScopeToString((enum ActionScope) * scope_out), action_in); + return -1; + } } /* require explicit action scope for fw rules */ @@ -4201,6 +4213,11 @@ static int DoParsePolicy(const char *policy_name, struct DetectFirewallPolicy *p return -1; idx++; } + + if (action & ACTION_CONFIG) { + SCLogError("%s: 'config' is not a valid default policy action", policy_name); + return -1; + } pol->action = action; pol->action_scope = action_scope; return 1; diff --git a/src/util-memrchr.c b/src/util-memrchr.c index e531f845c7fd..c490481b57f4 100644 --- a/src/util-memrchr.c +++ b/src/util-memrchr.c @@ -26,37 +26,46 @@ #include "util-unittest.h" #include "util-memrchr.h" -#ifndef HAVE_MEMRCHR -void *memrchr (const void *s, int c, size_t n) +#if !defined(HAVE_MEMRCHR) || defined(UNITTESTS) +static void *SCMemrchrFallback(const void *s, int c, size_t n) { - const char *end = s + n; + const unsigned char *p = (const unsigned char *)s + n; + const unsigned char uc = (unsigned char)c; - while (end > (const char *)s) { - if (*end == (char)c) - return (void *)end; - end--; + while (p > (const unsigned char *)s) { + p--; + if (*p == uc) + return (void *)p; } return NULL; } +#endif + +#ifndef HAVE_MEMRCHR +void *memrchr(const void *s, int c, size_t n) +{ + return SCMemrchrFallback(s, c, n); +} #endif /* HAVE_MEMRCHR */ #ifdef UNITTESTS static int MemrchrTest01 (void) { - const char *haystack = "abcabc"; - char needle = 'b'; - - char *ptr = memrchr(haystack, needle, strlen(haystack)); - if (ptr == NULL) - return 0; - - if (strlen(ptr) != 2) - return 0; + char buf[] = { 'x', 'y', 'z' }; + char one_byte[] = { 'q' }; + char dup[] = { 'a', 'b', 'a' }; + unsigned char high_byte[] = { 0x80, 'x', 0x80 }; - if (strcmp(ptr, "bc") != 0) - return 0; + FAIL_IF(SCMemrchrFallback(buf, 'x', sizeof(buf)) != &buf[0]); + FAIL_IF(SCMemrchrFallback(buf, 'z', sizeof(buf)) != &buf[2]); + FAIL_IF(SCMemrchrFallback(buf, 'y', sizeof(buf)) != &buf[1]); + FAIL_IF(SCMemrchrFallback(buf, 'a', sizeof(buf)) != NULL); + FAIL_IF(SCMemrchrFallback(one_byte, 'q', sizeof(one_byte)) != &one_byte[0]); + FAIL_IF(SCMemrchrFallback(one_byte, 'q', 0) != NULL); + FAIL_IF(SCMemrchrFallback(dup, 'a', sizeof(dup)) != &dup[2]); + FAIL_IF(SCMemrchrFallback(high_byte, 0x80, sizeof(high_byte)) != &high_byte[2]); - return 1; + PASS; } #endif diff --git a/suricata.yaml.in b/suricata.yaml.in index c4a4983bd6ba..055668b1c523 100644 --- a/suricata.yaml.in +++ b/suricata.yaml.in @@ -947,6 +947,7 @@ app-layer: enabled: yes detection-ports: dp: 5900, 5901, 5902, 5903, 5904, 5905, 5906, 5907, 5908, 5909 + # max-string-length: 4 KiB mqtt: enabled: yes # max-msg-length: 1 MiB