Skip to content

Commit

Permalink
Add unsafe-mode and make default execution mode safe-mode
Browse files Browse the repository at this point in the history
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

```
  • Loading branch information
danobi committed May 13, 2019
1 parent bdc9f16 commit 981c3cf
Show file tree
Hide file tree
Showing 10 changed files with 74 additions and 17 deletions.
4 changes: 3 additions & 1 deletion docs/reference_guide.md
Expand Up @@ -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
Expand All @@ -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()`
Expand Down
7 changes: 6 additions & 1 deletion man/man8/bpftrace.8
Expand Up @@ -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.
.
Expand Down Expand Up @@ -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
Expand Down
7 changes: 7 additions & 0 deletions src/ast/semantic_analyser.cpp
Expand Up @@ -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;

Expand Down
1 change: 1 addition & 0 deletions src/bpftrace.h
Expand Up @@ -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<SizedType> key_args,
std::vector<std::pair<std::vector<uint8_t>, std::vector<uint8_t>>> &values_by_key);
Expand Down
8 changes: 8 additions & 0 deletions src/main.cpp
Expand Up @@ -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;
Expand Down Expand Up @@ -89,13 +90,15 @@ 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;

const char* const short_options = "dB:e:hlp:vc:V";
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(
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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.
Expand Down
10 changes: 10 additions & 0 deletions src/utils.cpp
Expand Up @@ -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<char, 128> buffer;
Expand Down
6 changes: 6 additions & 0 deletions src/utils.h
Expand Up @@ -53,6 +53,11 @@ static std::vector<DeprecatedName> DEPRECATED_LIST =
{ "sym", "ksym"},
};

static std::vector<std::string> UNSAFE_BUILTIN_FUNCS =
{
"system",
};


bool has_wildcard(const std::string &str);
std::vector<std::string> split_string(const std::string &str, char delimiter);
Expand All @@ -66,6 +71,7 @@ std::vector<std::string> 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);
Expand Down
2 changes: 1 addition & 1 deletion tests/codegen/call_system.cpp
Expand Up @@ -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
Expand Down
6 changes: 5 additions & 1 deletion tests/codegen/common.h
Expand Up @@ -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;

Expand Down
40 changes: 27 additions & 13 deletions tests/semantic_analyser.cpp
Expand Up @@ -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;
Expand All @@ -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)
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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)
Expand Down

0 comments on commit 981c3cf

Please sign in to comment.