From 981c3cfbde518715ab278b89e2b2b6fb19f04a01 Mon Sep 17 00:00:00 2001 From: Daniel Xu Date: Thu, 9 May 2019 19:16:08 -0700 Subject: [PATCH] Add unsafe-mode and make default execution mode safe-mode bpftrace as an tracing language should try to remain as safe as possible. That is to say, anything that can cause side effects on a system should be used with care. Similar to the concept of unsafe code in rust, we introduce an unsafe (`-u`, `--unsafe`) mode to bpftrace. Currently, `system()` is the only builtin function marked as unsafe. If/when more unsafe builtins get added, we can adjust the list accordingly. This patch also moves bpftrace to safe by default. Note: this is an API breaking change. Test Plan: ``` $ sudo ./build/src/bpftrace -e 'interval:s:5 { system("ls"); }' system() is an unsafe function being used in safe mode $ sudo ./build/src/bpftrace -e 'interval:s:1 { system("ls"); }' --unsafe Attaching 1 probe... build build-debug build-debug.sh build-docker-image.sh build-release build-release.sh build.sh CHANGELOG.md cmake CMakeLists.txt CONTRIBUTING-TOOLS.md docker docs images INSTALL.md LICENSE man README.md resources scripts src tests tools build build-debug build-debug.sh build-docker-image.sh build-release build-release.sh build.sh CHANGELOG.md cmake CMakeLists.txt CONTRIBUTING-TOOLS.md docker docs images INSTALL.md LICENSE man README.md resources scripts src tests tools ^C $ sudo ./build/src/bpftrace -e 'interval:s:5 { printf("ls\n"); }' Attaching 1 probe... ls ^C ``` --- docs/reference_guide.md | 4 +++- man/man8/bpftrace.8 | 7 +++++- src/ast/semantic_analyser.cpp | 7 ++++++ src/bpftrace.h | 1 + src/main.cpp | 8 +++++++ src/utils.cpp | 10 +++++++++ src/utils.h | 6 ++++++ tests/codegen/call_system.cpp | 2 +- tests/codegen/common.h | 6 +++++- tests/semantic_analyser.cpp | 40 +++++++++++++++++++++++------------ 10 files changed, 74 insertions(+), 17 deletions(-) diff --git a/docs/reference_guide.md b/docs/reference_guide.md index 3f3390a29b0..1b3ab0ef117 100644 --- a/docs/reference_guide.md +++ b/docs/reference_guide.md @@ -1625,7 +1625,7 @@ Syntax: `system(fmt)` This runs the provided command at the shell. For example: ``` -# bpftrace -e 'kprobe:do_nanosleep { system("ps -p %d\n", pid); }' +# bpftrace --unsafe -e 'kprobe:do_nanosleep { system("ps -p %d\n", pid); }' Attaching 1 probe... PID TTY TIME CMD 1339 ? 00:00:15 iscsid @@ -1640,6 +1640,8 @@ Attaching 1 probe... This can be useful to execute commands or a shell script when an instrumented event happens. +Note this is an unsafe function. To use it, bpftrace must be run with `--unsafe`. + ## 12. `exit()`: Exit Syntax: `exit()` diff --git a/man/man8/bpftrace.8 b/man/man8/bpftrace.8 index 9b7c96fe7b1..7ef96cfab36 100644 --- a/man/man8/bpftrace.8 +++ b/man/man8/bpftrace.8 @@ -68,6 +68,11 @@ Helper to run CMD. Equivalent to manually running CMD and then giving passing th you've traced at least the duration CMD's execution. . .TP +\fB\--unsafe\fR +Enable unsafe builtin functions. By default, bpftrace runs in safe mode. Safe mode ensure programs cannot modify system state. +Unsafe builtin functions are marked as such in \fBBUILTINS (functions)\fR. +. +.TP \fB\-v\fR Verbose messages. . @@ -384,7 +389,7 @@ Print file content Convert IP address data to text . .TP -\fBsystem(char *fmt)\fR +\fBsystem(char *fmt)\fR (unsafe) Execute shell command . .TP diff --git a/src/ast/semantic_analyser.cpp b/src/ast/semantic_analyser.cpp index 83800924f3c..ea69a59d0cb 100644 --- a/src/ast/semantic_analyser.cpp +++ b/src/ast/semantic_analyser.cpp @@ -190,6 +190,13 @@ void SemanticAnalyser::visit(Builtin &builtin) void SemanticAnalyser::visit(Call &call) { + // Check for unsafe-ness first. It is likely the most pertinent issue + // (and should be at the top) for any function call. + if (bpftrace_.safe_mode && is_unsafe_func(call.func)) { + err_ << call.func << "() is an unsafe function being used in safe mode" + << std::endl; + } + // needed for positional parameters context: call_ = &call; diff --git a/src/bpftrace.h b/src/bpftrace.h index 53c15a0b64b..5f40bc3050e 100644 --- a/src/bpftrace.h +++ b/src/bpftrace.h @@ -99,6 +99,7 @@ class BPFtrace uint64_t strlen_ = 64; uint64_t mapmax_ = 4096; bool demangle_cpp_symbols = true; + bool safe_mode = true; static void sort_by_key(std::vector key_args, std::vector, std::vector>> &values_by_key); diff --git a/src/main.cpp b/src/main.cpp index 8f5478bbe95..31600be6c30 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -34,6 +34,7 @@ void usage() std::cerr << " -l [search] list probes" << std::endl; std::cerr << " -p PID enable USDT probes on PID" << std::endl; std::cerr << " -c 'CMD' run CMD and enable USDT probes on resulting process" << std::endl; + std::cerr << " --unsafe allow unsafe builtin functions" << std::endl; std::cerr << " -v verbose messages" << std::endl; std::cerr << " -V, --version bpftrace version" << std::endl << std::endl; std::cerr << "ENVIRONMENT:" << std::endl; @@ -89,6 +90,7 @@ int main(int argc, char *argv[]) char *pid_str = nullptr; char *cmd_str = nullptr; bool listing = false; + bool safe_mode = true; std::string script, search, file_name; int c; @@ -96,6 +98,7 @@ int main(int argc, char *argv[]) option long_options[] = { option{"help", no_argument, nullptr, 'h'}, option{"version", no_argument, nullptr, 'V'}, + option{"unsafe", no_argument, nullptr, 'u'}, option{nullptr, 0, nullptr, 0}, // Must be last }; while ((c = getopt_long( @@ -137,6 +140,9 @@ int main(int argc, char *argv[]) case 'c': cmd_str = optarg; break; + case 'u': + safe_mode = false; + break; case 'h': usage(); return 0; @@ -171,6 +177,8 @@ int main(int argc, char *argv[]) BPFtrace bpftrace; Driver driver(bpftrace); + bpftrace.safe_mode = safe_mode; + // PID is currently only used for USDT probes that need enabling. Future work: // - make PID a filter for all probe types: pass to perf_event_open(), etc. // - provide PID in USDT probe specification as a way to override -p. diff --git a/src/utils.cpp b/src/utils.cpp index da9967386fd..f7260fc8154 100644 --- a/src/utils.cpp +++ b/src/utils.cpp @@ -362,6 +362,16 @@ std::string is_deprecated(std::string &str) return str; } +bool is_unsafe_func(const std::string &func_name) +{ + return std::any_of( + UNSAFE_BUILTIN_FUNCS.begin(), + UNSAFE_BUILTIN_FUNCS.end(), + [&](const auto& cand) { + return func_name == cand; + }); +} + std::string exec_system(const char* cmd) { std::array buffer; diff --git a/src/utils.h b/src/utils.h index f88c8a24dc4..9be19546591 100644 --- a/src/utils.h +++ b/src/utils.h @@ -53,6 +53,11 @@ static std::vector DEPRECATED_LIST = { "sym", "ksym"}, }; +static std::vector UNSAFE_BUILTIN_FUNCS = +{ + "system", +}; + bool has_wildcard(const std::string &str); std::vector split_string(const std::string &str, char delimiter); @@ -66,6 +71,7 @@ std::vector get_kernel_cflags( const std::string& ksrc, const std::string& kobj); std::string is_deprecated(std::string &str); +bool is_unsafe_func(const std::string &func_name); std::string exec_system(const char* cmd); std::string resolve_binary_path(const std::string& cmd); void cat_file(const char *filename); diff --git a/tests/codegen/call_system.cpp b/tests/codegen/call_system.cpp index a17c2649e9a..b6d862eb3a5 100644 --- a/tests/codegen/call_system.cpp +++ b/tests/codegen/call_system.cpp @@ -36,7 +36,7 @@ declare void @llvm.lifetime.end.p0i8(i64, i8* nocapture) #1 attributes #0 = { nounwind } attributes #1 = { argmemonly nounwind } -)EXPECTED"); +)EXPECTED", false); } } // namespace codegen diff --git a/tests/codegen/common.h b/tests/codegen/common.h index 7e8ebc23f0c..d82c21693aa 100644 --- a/tests/codegen/common.h +++ b/tests/codegen/common.h @@ -21,9 +21,13 @@ target triple = "bpf-pc-linux" )HEAD"; -static void test(const std::string &input, const std::string expected_output) +static void test( + const std::string &input, + const std::string expected_output, + bool safe_mode = true) { BPFtrace bpftrace; + bpftrace.safe_mode = safe_mode; Driver driver(bpftrace); FakeMap::next_mapfd_ = 1; diff --git a/tests/semantic_analyser.cpp b/tests/semantic_analyser.cpp index c803f77ea14..2f582b4906e 100644 --- a/tests/semantic_analyser.cpp +++ b/tests/semantic_analyser.cpp @@ -11,8 +11,14 @@ namespace semantic_analyser { using ::testing::_; -void test(BPFtrace &bpftrace, Driver &driver, const std::string &input, int expected_result=0) -{ +void test( + BPFtrace &bpftrace, + Driver &driver, + const std::string &input, + int expected_result=0, + bool safe_mode = true) +{ + bpftrace.safe_mode = safe_mode; ASSERT_EQ(driver.parse_str(input), 0); ClangParser clang; @@ -26,23 +32,31 @@ void test(BPFtrace &bpftrace, Driver &driver, const std::string &input, int expe EXPECT_EQ(expected_result, semantics.analyse()) << msg.str() + out.str(); } -void test(BPFtrace &bpftrace, const std::string &input, int expected_result=0) +void test(BPFtrace &bpftrace, + const std::string &input, + int expected_result=0, + bool safe_mode = true) { Driver driver(bpftrace); - test(bpftrace, driver, input, expected_result); + test(bpftrace, driver, input, expected_result, safe_mode); } -void test(Driver &driver, const std::string &input, int expected_result=0) +void test(Driver &driver, + const std::string &input, + int expected_result=0, + bool safe_mode = true) { BPFtrace bpftrace; - test(bpftrace, driver, input, expected_result); + test(bpftrace, driver, input, expected_result, safe_mode); } -void test(const std::string &input, int expected_result=0) +void test(const std::string &input, + int expected_result=0, + bool safe_mode = true) { BPFtrace bpftrace; Driver driver(bpftrace); - test(bpftrace, driver, input, expected_result); + test(bpftrace, driver, input, expected_result, safe_mode); } TEST(semantic_analyser, builtin_variables) @@ -94,7 +108,7 @@ TEST(semantic_analyser, builtin_functions) test("kprobe:f { exit() }", 0); test("kprobe:f { str(0xffff) }", 0); test("kprobe:f { printf(\"hello\\n\") }", 0); - test("kprobe:f { system(\"ls\\n\") }", 0); + test("kprobe:f { system(\"ls\\n\") }", 0, false /* safe_node */); test("kprobe:f { join(0) }", 0); test("kprobe:f { sym(0xffff) }", 0); test("kprobe:f { ksym(0xffff) }", 0); @@ -456,10 +470,10 @@ TEST(semantic_analyser, printf) TEST(semantic_analyser, system) { - test("kprobe:f { system(\"ls\") }", 0); - test("kprobe:f { system(1234) }", 1); - test("kprobe:f { system() }", 1); - test("kprobe:f { $fmt = \"mystring\"; system($fmt) }", 1); + test("kprobe:f { system(\"ls\") }", 0, false /* safe_mode */); + test("kprobe:f { system(1234) }", 1, false /* safe_mode */); + test("kprobe:f { system() }", 1, false /* safe_mode */); + test("kprobe:f { $fmt = \"mystring\"; system($fmt) }", 1, false /* safe_mode */); } TEST(semantic_analyser, printf_format_int)