From 9f72353a411dae0cc71c132354a419359afd15fa Mon Sep 17 00:00:00 2001 From: Johanna Amann Date: Tue, 7 Jan 2025 17:46:27 +0000 Subject: [PATCH 1/3] Raise warnings when for DNS events that are not raised due to dns_skip_all_addl By default, dns_skip_all_addl is set to false. This causes several events to not be raised. This change emits warnings when a user defines event handlers for events that will not be raised. Furthermore, it adds notes about this behavior to the documentation. We also introduce a new BIF, `is_event_handled`, which checks if an event is handled. Fixes GH-4061 --- scripts/base/protocols/dns/__load__.zeek | 1 + .../protocols/dns/check-event-handlers.zeek | 19 ++++++++++++ src/analyzer/protocol/dns/events.bif | 30 ++++++++++++++++++ src/zeek.bif | 16 ++++++++++ .../.stderr | 6 ++++ .../base/protocols/dns/dns-edns-cookie.zeek | 6 ++-- .../protocols/dns/event-handler-warning.zeek | 31 +++++++++++++++++++ 7 files changed, 106 insertions(+), 3 deletions(-) create mode 100644 scripts/base/protocols/dns/check-event-handlers.zeek create mode 100644 testing/btest/Baseline/scripts.base.protocols.dns.event-handler-warning/.stderr create mode 100644 testing/btest/scripts/base/protocols/dns/event-handler-warning.zeek diff --git a/scripts/base/protocols/dns/__load__.zeek b/scripts/base/protocols/dns/__load__.zeek index 1d47f6e0cd..0c7d19a67f 100644 --- a/scripts/base/protocols/dns/__load__.zeek +++ b/scripts/base/protocols/dns/__load__.zeek @@ -1,2 +1,3 @@ @load ./consts @load ./main +@load ./check-event-handlers diff --git a/scripts/base/protocols/dns/check-event-handlers.zeek b/scripts/base/protocols/dns/check-event-handlers.zeek new file mode 100644 index 0000000000..af9f33b0cb --- /dev/null +++ b/scripts/base/protocols/dns/check-event-handlers.zeek @@ -0,0 +1,19 @@ +##! This script checks if DNS event handlers that will not be raised +##! are used and raises a warning in those cases. + +module DNS; + +event zeek_init() &priority=20 + { + if ( ! dns_skip_all_addl ) + return; + + local addl_functions = ["dns_TSIG_addl", "dns_EDNS_addl", "dns_EDNS_ecs", "dns_EDNS_tcp_keepalive", "dns_EDNS_cookie"]; + + for ( event_name in addl_functions ) + if ( is_event_handled(event_name) ) + Reporter::warning(fmt("Used event '%s' will not be raised because 'dns_skip_all_addl' is true", event_name)); + + if ( is_event_handled("dns_TKEY") ) + Reporter::warning("Used event 'dns_TKEY' will not contain any data in 'ans' because 'dns_skip_all_addl' is true"); + } diff --git a/src/analyzer/protocol/dns/events.bif b/src/analyzer/protocol/dns/events.bif index df7140e56e..46d3822235 100644 --- a/src/analyzer/protocol/dns/events.bif +++ b/src/analyzer/protocol/dns/events.bif @@ -496,6 +496,11 @@ event dns_unknown_reply%(c: connection, msg: dns_msg, ans: dns_answer%); ## ## ans: The parsed EDNS reply. ## +## .. note:: +## +## Note that this event will only be raised if ``dns_skip_all_addl`` +## is set to false. +## ## .. zeek:see:: dns_AAAA_reply dns_A_reply dns_CNAME_reply dns_HINFO_reply dns_MX_reply ## dns_NS_reply dns_PTR_reply dns_SOA_reply dns_SRV_reply dns_TSIG_addl ## dns_TXT_reply dns_SPF_reply dns_WKS_reply dns_end dns_mapping_altered @@ -519,6 +524,11 @@ event dns_EDNS_addl%(c: connection, msg: dns_msg, ans: dns_edns_additional%); ## ## opt: The parsed EDNS option. ## +## .. note:: +## +## Note that this event will only be raised if ``dns_skip_all_addl`` +## is set to false. +## ## .. zeek:see:: dns_AAAA_reply dns_A_reply dns_CNAME_reply dns_HINFO_reply dns_MX_reply ## dns_NS_reply dns_PTR_reply dns_SOA_reply dns_SRV_reply dns_TSIG_addl ## dns_TXT_reply dns_SPF_reply dns_WKS_reply dns_end dns_mapping_altered @@ -544,6 +554,11 @@ event dns_EDNS_ecs%(c: connection, msg: dns_msg, opt: dns_edns_ecs%); ## ## opt: The parsed EDNS Keepalive option. ## +## .. note:: +## +## Note that this event will only be raised if ``dns_skip_all_addl`` +## is set to false. +## ## .. zeek:see:: dns_AAAA_reply dns_A_reply dns_CNAME_reply dns_HINFO_reply dns_MX_reply ## dns_NS_reply dns_PTR_reply dns_SOA_reply dns_SRV_reply dns_TSIG_addl ## dns_TXT_reply dns_SPF_reply dns_WKS_reply dns_end dns_mapping_altered @@ -569,6 +584,11 @@ event dns_EDNS_tcp_keepalive%(c: connection, msg: dns_msg, opt: dns_edns_tcp_kee ## ## opt: The parsed EDNS Cookie option. ## +## .. note:: +## +## Note that this event will only be raised if ``dns_skip_all_addl`` +## is set to false. +## ## .. zeek:see:: dns_AAAA_reply dns_A_reply dns_CNAME_reply dns_HINFO_reply dns_MX_reply ## dns_NS_reply dns_PTR_reply dns_SOA_reply dns_SRV_reply dns_TSIG_addl ## dns_TXT_reply dns_SPF_reply dns_WKS_reply dns_end dns_mapping_altered @@ -592,6 +612,11 @@ event dns_EDNS_cookie%(c: connection, msg: dns_msg, opt: dns_edns_cookie%); ## ## ans: The parsed TKEY reply. ## +## .. note:: +## +## Note that ``ans`` will only be populated if ``dns_skip_all_addl`` +## is set to false. +## ## .. zeek:see:: dns_TSIG_addl event dns_TKEY%(c: connection, msg: dns_msg, ans: dns_tkey%); @@ -608,6 +633,11 @@ event dns_TKEY%(c: connection, msg: dns_msg, ans: dns_tkey%); ## msg: The parsed DNS message header. ## ## ans: The parsed TSIG reply. +# +## .. note:: +## +## Note that this event will only be raised if ``dns_skip_all_addl`` +## is set to false. ## ## .. zeek:see:: dns_AAAA_reply dns_A_reply dns_CNAME_reply dns_EDNS_addl ## dns_HINFO_reply dns_MX_reply dns_NS_reply dns_PTR_reply dns_SOA_reply diff --git a/src/zeek.bif b/src/zeek.bif index 1a42f00344..09b497e8c1 100644 --- a/src/zeek.bif +++ b/src/zeek.bif @@ -5015,6 +5015,22 @@ function generate_all_events%(%) : bool return zeek::val_mgr->True(); %} +## Check if an event is handled. Typically this means that a script defines an event. +## This currently is mainly used to warn when events are defined that will not be used +## in certain conditions. +## +## event_name: event name to check +## +## returns: true if the named event is handled. +function is_event_handled%(event_name: string%) : bool + %{ + auto *h = event_registry->Lookup(event_name->ToStdStringView()); + if ( h && *h ) + return zeek::val_mgr->True(); + + return zeek::val_mgr->False(); + %} + %%{ // Autogenerated from CMake bif_target() #include "__all__.bif.cc" diff --git a/testing/btest/Baseline/scripts.base.protocols.dns.event-handler-warning/.stderr b/testing/btest/Baseline/scripts.base.protocols.dns.event-handler-warning/.stderr new file mode 100644 index 0000000000..bdf7f1bc81 --- /dev/null +++ b/testing/btest/Baseline/scripts.base.protocols.dns.event-handler-warning/.stderr @@ -0,0 +1,6 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +warning in <...>/check-event-handlers.zeek, line 15: Used event 'dns_EDNS_tcp_keepalive' will not be raised because 'dns_skip_all_addl' is true +warning in <...>/check-event-handlers.zeek, line 15: Used event 'dns_EDNS_cookie' will not be raised because 'dns_skip_all_addl' is true +warning in <...>/check-event-handlers.zeek, line 15: Used event 'dns_EDNS_ecs' will not be raised because 'dns_skip_all_addl' is true +warning in <...>/check-event-handlers.zeek, line 15: Used event 'dns_EDNS_addl' will not be raised because 'dns_skip_all_addl' is true +warning in <...>/check-event-handlers.zeek, line 18: Used event 'dns_TKEY' will not contain any data in 'ans' because 'dns_skip_all_addl' is true diff --git a/testing/btest/scripts/base/protocols/dns/dns-edns-cookie.zeek b/testing/btest/scripts/base/protocols/dns/dns-edns-cookie.zeek index 6875099ec9..abf69cc49e 100644 --- a/testing/btest/scripts/base/protocols/dns/dns-edns-cookie.zeek +++ b/testing/btest/scripts/base/protocols/dns/dns-edns-cookie.zeek @@ -3,6 +3,6 @@ @load policy/protocols/dns/auth-addl event dns_EDNS_cookie(c: connection, msg: dns_msg, opt: dns_edns_cookie) - { - print opt; - } \ No newline at end of file + { + print opt; + } diff --git a/testing/btest/scripts/base/protocols/dns/event-handler-warning.zeek b/testing/btest/scripts/base/protocols/dns/event-handler-warning.zeek new file mode 100644 index 0000000000..f5507ea8b4 --- /dev/null +++ b/testing/btest/scripts/base/protocols/dns/event-handler-warning.zeek @@ -0,0 +1,31 @@ +# Check that warnings are for events that will not be raised + +# @TEST-EXEC: zeek -b %INPUT +# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff .stderr + +@load base/protocols/dns + +event dns_EDNS_addl(c: connection, msg: dns_msg, ans: dns_edns_additional) + { + print ""; + } + +event dns_EDNS_ecs(c: connection, msg: dns_msg, opt: dns_edns_ecs) + { + print ""; + } + +event dns_EDNS_tcp_keepalive(c: connection, msg: dns_msg, opt: dns_edns_tcp_keepalive) + { + print ""; + } + +event dns_EDNS_cookie(c: connection, msg: dns_msg, opt: dns_edns_cookie) + { + print ""; + } + +event dns_TKEY(c: connection, msg: dns_msg, ans: dns_tkey) + { + print ""; + } From 13f042cc270e967918ad8d76a2b53c67ea7f69d0 Mon Sep 17 00:00:00 2001 From: Johanna Amann Date: Wed, 8 Jan 2025 15:01:30 +0000 Subject: [PATCH 2/3] Address review comments and small updates for DNS warnings This commit addresses review feedback for DH-4155. Furthermore it fixes test failures, and adds a new test for the is_event_handled bif. --- src/analyzer/protocol/dns/events.bif | 14 +++++++------- src/zeek.bif | 10 +++++++++- testing/btest/Baseline/bifs.is_event_handled/err | 3 +++ testing/btest/Baseline/bifs.is_event_handled/out | 5 +++++ .../canonified_loaded_scripts.log | 1 + testing/btest/bifs/is_event_handled.zeek | 12 ++++++++++++ 6 files changed, 37 insertions(+), 8 deletions(-) create mode 100644 testing/btest/Baseline/bifs.is_event_handled/err create mode 100644 testing/btest/Baseline/bifs.is_event_handled/out create mode 100644 testing/btest/bifs/is_event_handled.zeek diff --git a/src/analyzer/protocol/dns/events.bif b/src/analyzer/protocol/dns/events.bif index 46d3822235..d271065642 100644 --- a/src/analyzer/protocol/dns/events.bif +++ b/src/analyzer/protocol/dns/events.bif @@ -498,7 +498,7 @@ event dns_unknown_reply%(c: connection, msg: dns_msg, ans: dns_answer%); ## ## .. note:: ## -## Note that this event will only be raised if ``dns_skip_all_addl`` +## Note that this event will only be raised if :zeek:see:`dns_skip_all_addl` ## is set to false. ## ## .. zeek:see:: dns_AAAA_reply dns_A_reply dns_CNAME_reply dns_HINFO_reply dns_MX_reply @@ -526,7 +526,7 @@ event dns_EDNS_addl%(c: connection, msg: dns_msg, ans: dns_edns_additional%); ## ## .. note:: ## -## Note that this event will only be raised if ``dns_skip_all_addl`` +## Note that this event will only be raised if :zeek:see:`dns_skip_all_addl` ## is set to false. ## ## .. zeek:see:: dns_AAAA_reply dns_A_reply dns_CNAME_reply dns_HINFO_reply dns_MX_reply @@ -556,7 +556,7 @@ event dns_EDNS_ecs%(c: connection, msg: dns_msg, opt: dns_edns_ecs%); ## ## .. note:: ## -## Note that this event will only be raised if ``dns_skip_all_addl`` +## Note that this event will only be raised if :zeek:see:`dns_skip_all_addl` ## is set to false. ## ## .. zeek:see:: dns_AAAA_reply dns_A_reply dns_CNAME_reply dns_HINFO_reply dns_MX_reply @@ -586,7 +586,7 @@ event dns_EDNS_tcp_keepalive%(c: connection, msg: dns_msg, opt: dns_edns_tcp_kee ## ## .. note:: ## -## Note that this event will only be raised if ``dns_skip_all_addl`` +## Note that this event will only be raised if :zeek:see:`dns_skip_all_addl` ## is set to false. ## ## .. zeek:see:: dns_AAAA_reply dns_A_reply dns_CNAME_reply dns_HINFO_reply dns_MX_reply @@ -614,7 +614,7 @@ event dns_EDNS_cookie%(c: connection, msg: dns_msg, opt: dns_edns_cookie%); ## ## .. note:: ## -## Note that ``ans`` will only be populated if ``dns_skip_all_addl`` +## Note that ``ans`` will only be populated if :zeek:see:`dns_skip_all_addl` ## is set to false. ## ## .. zeek:see:: dns_TSIG_addl @@ -633,10 +633,10 @@ event dns_TKEY%(c: connection, msg: dns_msg, ans: dns_tkey%); ## msg: The parsed DNS message header. ## ## ans: The parsed TSIG reply. -# +## ## .. note:: ## -## Note that this event will only be raised if ``dns_skip_all_addl`` +## Note that this event will only be raised if :zeek:see:`dns_skip_all_addl` ## is set to false. ## ## .. zeek:see:: dns_AAAA_reply dns_A_reply dns_CNAME_reply dns_EDNS_addl diff --git a/src/zeek.bif b/src/zeek.bif index 09b497e8c1..e15e7dbe39 100644 --- a/src/zeek.bif +++ b/src/zeek.bif @@ -5019,13 +5019,21 @@ function generate_all_events%(%) : bool ## This currently is mainly used to warn when events are defined that will not be used ## in certain conditions. ## +## Raises an error if the named event does not exist. +## ## event_name: event name to check ## ## returns: true if the named event is handled. function is_event_handled%(event_name: string%) : bool %{ auto *h = event_registry->Lookup(event_name->ToStdStringView()); - if ( h && *h ) + if ( ! h ) + { + zeek::emit_builtin_error(zeek::util::fmt("is_event_handled: '%s' is not an event", event_name->CheckString())); + return zeek::val_mgr->False(); + } + + if ( *h ) return zeek::val_mgr->True(); return zeek::val_mgr->False(); diff --git a/testing/btest/Baseline/bifs.is_event_handled/err b/testing/btest/Baseline/bifs.is_event_handled/err new file mode 100644 index 0000000000..b0c916b9a1 --- /dev/null +++ b/testing/btest/Baseline/bifs.is_event_handled/err @@ -0,0 +1,3 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +error in <...>/is_event_handled.zeek, line 11: is_event_handled: 'myfunc1' is not an event (is_event_handled(myfunc1)) +error in <...>/is_event_handled.zeek, line 12: is_event_handled: 'conn_id' is not an event (is_event_handled(conn_id)) diff --git a/testing/btest/Baseline/bifs.is_event_handled/out b/testing/btest/Baseline/bifs.is_event_handled/out new file mode 100644 index 0000000000..311b22d902 --- /dev/null +++ b/testing/btest/Baseline/bifs.is_event_handled/out @@ -0,0 +1,5 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +T +F +F +F diff --git a/testing/btest/Baseline/coverage.default-load-baseline/canonified_loaded_scripts.log b/testing/btest/Baseline/coverage.default-load-baseline/canonified_loaded_scripts.log index 353e066690..a7ed97effa 100644 --- a/testing/btest/Baseline/coverage.default-load-baseline/canonified_loaded_scripts.log +++ b/testing/btest/Baseline/coverage.default-load-baseline/canonified_loaded_scripts.log @@ -385,6 +385,7 @@ scripts/base/init-default.zeek scripts/base/protocols/dns/__load__.zeek scripts/base/protocols/dns/consts.zeek scripts/base/protocols/dns/main.zeek + scripts/base/protocols/dns/check-event-handlers.zeek scripts/base/protocols/finger/__load__.zeek scripts/base/protocols/finger/spicy-events.zeek scripts/base/protocols/finger/main.zeek diff --git a/testing/btest/bifs/is_event_handled.zeek b/testing/btest/bifs/is_event_handled.zeek new file mode 100644 index 0000000000..e3f8c1f6ed --- /dev/null +++ b/testing/btest/bifs/is_event_handled.zeek @@ -0,0 +1,12 @@ +# @TEST-EXEC: zeek -b %INPUT >out 2>err +# @TEST-EXEC: btest-diff out +# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff err + +function myfunc1(a: addr, b: addr): int + { + } + +print is_event_handled("zeek_init"); # T +print is_event_handled("dns_EDNS_cookie"); # F +print is_event_handled("myfunc1"); # builtin error +print is_event_handled("conn_id"); # builtin error From 6bfa55904cec7f8ce88cc37500571825ed201ee8 Mon Sep 17 00:00:00 2001 From: Johanna Amann Date: Wed, 8 Jan 2025 16:15:03 +0000 Subject: [PATCH 3/3] Update BiF-tracking, add is_event_handled --- src/script_opt/FuncInfo.cc | 1 + testing/btest/Baseline/opt.ZAM-bif-tracking/output | 2 +- testing/btest/opt/ZAM-bif-tracking.zeek | 1 + 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/script_opt/FuncInfo.cc b/src/script_opt/FuncInfo.cc index b244a7753c..2a962e7dc7 100644 --- a/src/script_opt/FuncInfo.cc +++ b/src/script_opt/FuncInfo.cc @@ -324,6 +324,7 @@ static std::unordered_map func_attrs = { {"is_alnum", ATTR_FOLDABLE}, {"is_alpha", ATTR_FOLDABLE}, {"is_ascii", ATTR_FOLDABLE}, + {"is_event_handled", ATTR_IDEMPOTENT}, // can error {"is_file_analyzer", ATTR_NO_ZEEK_SIDE_EFFECTS}, {"is_icmp_port", ATTR_FOLDABLE}, {"is_local_interface", ATTR_IDEMPOTENT}, diff --git a/testing/btest/Baseline/opt.ZAM-bif-tracking/output b/testing/btest/Baseline/opt.ZAM-bif-tracking/output index fc17f81c25..aeea02bae2 100644 --- a/testing/btest/Baseline/opt.ZAM-bif-tracking/output +++ b/testing/btest/Baseline/opt.ZAM-bif-tracking/output @@ -1,2 +1,2 @@ ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. -542 seen BiFs, 0 unseen BiFs (), 0 new BiFs () +543 seen BiFs, 0 unseen BiFs (), 0 new BiFs () diff --git a/testing/btest/opt/ZAM-bif-tracking.zeek b/testing/btest/opt/ZAM-bif-tracking.zeek index 1e46d837f1..bf36ebc46f 100644 --- a/testing/btest/opt/ZAM-bif-tracking.zeek +++ b/testing/btest/opt/ZAM-bif-tracking.zeek @@ -357,6 +357,7 @@ global known_BiFs = set( "is_alnum", "is_alpha", "is_ascii", + "is_event_handled", "is_file_analyzer", "is_icmp_port", "is_local_interface",