From 69afc4a88269e497af4a9890f1a403e55694f934 Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Thu, 24 Jan 2013 09:48:23 -0600 Subject: [PATCH] Add an error for record coercions that would orphan a field. These cases should be avoidable by fixing scripts where they occur and they can also help catch typos that would lead to unintentional runtime behavior. Adding this already revealed several scripts where a field in an inlined record was never removed after a code refactor. --- scripts/base/frameworks/signatures/main.bro | 8 +++---- scripts/base/protocols/http/file-hash.bro | 2 +- scripts/base/protocols/http/file-ident.bro | 4 +--- scripts/base/protocols/socks/main.bro | 2 +- scripts/policy/protocols/http/detect-MHR.bro | 2 +- src/Expr.cc | 7 ++++-- .../language.record-ceorce-orphan/out | 2 ++ .../btest/language/record-ceorce-orphan.bro | 22 +++++++++++++++++++ 8 files changed, 36 insertions(+), 13 deletions(-) create mode 100644 testing/btest/Baseline/language.record-ceorce-orphan/out create mode 100644 testing/btest/language/record-ceorce-orphan.bro diff --git a/scripts/base/frameworks/signatures/main.bro b/scripts/base/frameworks/signatures/main.bro index 26f78a68d1..4102214075 100644 --- a/scripts/base/frameworks/signatures/main.bro +++ b/scripts/base/frameworks/signatures/main.bro @@ -148,7 +148,7 @@ function has_signature_matched(id: string, orig: addr, resp: addr): bool event sig_summary(orig: addr, id: string, msg: string) { NOTICE([$note=Signature_Summary, $src=orig, - $filename=id, $msg=fmt("%s: %s", orig, msg), + $msg=fmt("%s: %s", orig, msg), $n=count_per_orig[orig,id] ]); } @@ -209,7 +209,6 @@ event signature_match(state: signature_state, msg: string, data: string) { NOTICE([$note=Count_Signature, $conn=state$conn, $msg=msg, - $filename=sig_id, $n=count_per_resp[dst,sig_id], $sub=fmt("%d matches of signature %s on host %s", count_per_resp[dst,sig_id], @@ -240,7 +239,7 @@ event signature_match(state: signature_state, msg: string, data: string) if ( notice ) NOTICE([$note=Sensitive_Signature, $conn=state$conn, $src=src_addr, - $dst=dst_addr, $filename=sig_id, $msg=fmt("%s: %s", src_addr, msg), + $dst=dst_addr, $msg=fmt("%s: %s", src_addr, msg), $sub=data]); if ( action == SIG_FILE_BUT_NO_SCAN || action == SIG_SUMMARY ) @@ -274,7 +273,7 @@ event signature_match(state: signature_state, msg: string, data: string) $src_addr=orig, $sig_id=sig_id, $event_msg=msg, $host_count=hcount, $sub_msg=horz_scan_msg]); - NOTICE([$note=Multiple_Sig_Responders, $src=orig, $filename=sig_id, + NOTICE([$note=Multiple_Sig_Responders, $src=orig, $msg=msg, $n=hcount, $sub=horz_scan_msg]); last_hthresh[orig] = hcount; @@ -295,7 +294,6 @@ event signature_match(state: signature_state, msg: string, data: string) $sub_msg=vert_scan_msg]); NOTICE([$note=Multiple_Signatures, $src=orig, $dst=resp, - $filename=sig_id, $msg=fmt("%s different signatures triggered", vcount), $n=vcount, $sub=vert_scan_msg]); diff --git a/scripts/base/protocols/http/file-hash.bro b/scripts/base/protocols/http/file-hash.bro index bc7547e51a..2545cbf817 100644 --- a/scripts/base/protocols/http/file-hash.bro +++ b/scripts/base/protocols/http/file-hash.bro @@ -73,7 +73,7 @@ event http_message_done(c: connection, is_orig: bool, stat: http_message_stat) & delete c$http$md5_handle; NOTICE([$note=MD5, $msg=fmt("%s %s %s", c$id$orig_h, c$http$md5, url), - $sub=c$http$md5, $conn=c, $URL=url]); + $sub=c$http$md5, $conn=c]); } } diff --git a/scripts/base/protocols/http/file-ident.bro b/scripts/base/protocols/http/file-ident.bro index b493f02bf0..706ea58558 100644 --- a/scripts/base/protocols/http/file-ident.bro +++ b/scripts/base/protocols/http/file-ident.bro @@ -68,9 +68,7 @@ event signature_match(state: signature_state, msg: string, data: string) &priori local message = fmt("%s %s %s", msg, c$http$method, url); NOTICE([$note=Incorrect_File_Type, $msg=message, - $conn=c, - $method=c$http$method, - $URL=url]); + $conn=c]); } } diff --git a/scripts/base/protocols/socks/main.bro b/scripts/base/protocols/socks/main.bro index 79ae4baa19..df5ee69f16 100644 --- a/scripts/base/protocols/socks/main.bro +++ b/scripts/base/protocols/socks/main.bro @@ -67,7 +67,7 @@ event socks_request(c: connection, version: count, request_type: count, # proxied connection. We treat this as a singular "tunnel". local cid = copy(c$id); cid$orig_p = 0/tcp; - Tunnel::register([$cid=cid, $tunnel_type=Tunnel::SOCKS, $payload_proxy=T]); + Tunnel::register([$cid=cid, $tunnel_type=Tunnel::SOCKS]); } event socks_reply(c: connection, version: count, reply: count, sa: SOCKS::Address, p: port) &priority=5 diff --git a/scripts/policy/protocols/http/detect-MHR.bro b/scripts/policy/protocols/http/detect-MHR.bro index 5abb98d39c..1898022978 100644 --- a/scripts/policy/protocols/http/detect-MHR.bro +++ b/scripts/policy/protocols/http/detect-MHR.bro @@ -37,7 +37,7 @@ event log_http(rec: HTTP::Info) local url = HTTP::build_url_http(rec); local message = fmt("%s %s %s", rec$id$orig_h, rec$md5, url); NOTICE([$note=Malware_Hash_Registry_Match, - $msg=message, $id=rec$id, $URL=url]); + $msg=message, $id=rec$id]); } } } diff --git a/src/Expr.cc b/src/Expr.cc index 9e71f27897..7b28d6f4df 100644 --- a/src/Expr.cc +++ b/src/Expr.cc @@ -3921,8 +3921,11 @@ RecordCoerceExpr::RecordCoerceExpr(Expr* op, RecordType* r) { int t_i = t_r->FieldOffset(sub_r->FieldName(i)); if ( t_i < 0 ) - // Orphane field in rhs, that's ok. - continue; + { + ExprError(fmt("orphaned field \"%s\" in record coercion", + sub_r->FieldName(i))); + break; + } BroType* sub_t_i = sub_r->FieldType(i); BroType* sup_t_i = t_r->FieldType(t_i); diff --git a/testing/btest/Baseline/language.record-ceorce-orphan/out b/testing/btest/Baseline/language.record-ceorce-orphan/out new file mode 100644 index 0000000000..aa42d13892 --- /dev/null +++ b/testing/btest/Baseline/language.record-ceorce-orphan/out @@ -0,0 +1,2 @@ +error in /Users/jsiwek/Projects/bro/bro/testing/btest/.tmp/language.record-ceorce-orphan/record-ceorce-orphan.bro, line 19: orphaned field "wtf" in record coercion ((coerce [$a=test, $b=42, $wtf=1.0 sec] to record { a:string; b:count; c:interval; })) +error in /Users/jsiwek/Projects/bro/bro/testing/btest/.tmp/language.record-ceorce-orphan/record-ceorce-orphan.bro, line 21: orphaned field "wtf" in record coercion ((coerce [$a=test, $b=42, $wtf=1.0 sec] to record { a:string; b:count; c:interval; })) diff --git a/testing/btest/language/record-ceorce-orphan.bro b/testing/btest/language/record-ceorce-orphan.bro new file mode 100644 index 0000000000..126b99d5ff --- /dev/null +++ b/testing/btest/language/record-ceorce-orphan.bro @@ -0,0 +1,22 @@ +# @TEST-EXEC-FAIL: bro -b %INPUT >out 2>&1 +# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff out + +type myrec: record { + a: string; + b: count; + c: interval &optional; +}; + +function myfunc(rec: myrec) + { + print rec; + } + +event bro_init() + { + # Orhpaned fields in a record coercion reflect a programming error, like a typo, so should + # be reported at parse-time to prevent unexpected run-time behavior. + local rec: myrec = [$a="test", $b=42, $wtf=1sec]; + print rec; + myfunc([$a="test", $b=42, $wtf=1sec]); + }