From 48e2d8fea92a65206902fefd9b1903fca2007441 Mon Sep 17 00:00:00 2001 From: Klemens Nanni Date: Wed, 31 Jan 2024 11:44:03 +0100 Subject: [PATCH 1/8] Use pledge(2) on OpenBSD to restrict system calls As per sio_open(3) "Use with pledge(2)" and "cpath wpath dpath" - metadata pipe, coverart cache "inet dns" - metadata socket, RTSP, NQPTP, MQTT "unix" - Avahi, D-Bus, MPRIS "proc exec" - external hooks/scripts Filesystem access is limited to a handful of paths, but accounting for them takes careful work, thus full "[rwcd]path" is not dropped. More importantly, prevent fork(2) and execve(2) if no hooks have been defined (default configuration). (As `config.cmd_*` are split into individual argv[] strings, they cannot be unveil(2)ed to limit "x" permissions to those commands.) --- shairport.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/shairport.c b/shairport.c index bded41652..5c3700d59 100644 --- a/shairport.c +++ b/shairport.c @@ -1929,6 +1929,12 @@ void _display_config(const char *filename, const int linenumber, __attribute__(( #define display_config(argc, argv) _display_config(__FILE__, __LINE__, argc, argv) int main(int argc, char **argv) { +#ifdef COMPILE_FOR_OPENBSD + /* Start with the superset of all possible promises. */ + if (pledge("stdio rpath wpath cpath dpath inet unix dns proc exec audio", NULL) == -1) + die("pledge"); +#endif + memset(&config, 0, sizeof(config)); // also clears all strings, BTW /* Check if we are called with -V or --version parameter */ if (argc >= 2 && ((strcmp(argv[1], "-V") == 0) || (strcmp(argv[1], "--version") == 0))) { @@ -2102,6 +2108,18 @@ int main(int argc, char **argv) { // parse arguments into config -- needed to locate pid_dir int audio_arg = parse_options(argc, argv); +#ifdef COMPILE_FOR_OPENBSD + /* Drop "proc exec" unless external commands are to be run. */ + if ((config.cmd_active_start == NULL) && + (config.cmd_active_stop == NULL) && + (config.cmd_start == NULL) && + (config.cmd_stop == NULL) && + (config.cmd_set_volume == NULL)) { + if (pledge("stdio rpath wpath cpath dpath inet unix dns audio", NULL) == -1) + die("pledge"); + } +#endif + // mDNS supports maximum of 63-character names (we append 13). if (strlen(config.service_name) > 50) { warn("The service name \"%s\" is too long (max 50 characters) and has been truncated.", From d12203ec5db1c398fc907d2acf90f1b6a2f57e20 Mon Sep 17 00:00:00 2001 From: Klemens Nanni Date: Thu, 1 Feb 2024 00:28:21 +0100 Subject: [PATCH 2/8] Drop "proc exec" after daemon(3)ising Account for double-fork(2) when built `--with-libdaemon`. --- shairport.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/shairport.c b/shairport.c index 5c3700d59..ce038159a 100644 --- a/shairport.c +++ b/shairport.c @@ -2108,18 +2108,6 @@ int main(int argc, char **argv) { // parse arguments into config -- needed to locate pid_dir int audio_arg = parse_options(argc, argv); -#ifdef COMPILE_FOR_OPENBSD - /* Drop "proc exec" unless external commands are to be run. */ - if ((config.cmd_active_start == NULL) && - (config.cmd_active_stop == NULL) && - (config.cmd_start == NULL) && - (config.cmd_stop == NULL) && - (config.cmd_set_volume == NULL)) { - if (pledge("stdio rpath wpath cpath dpath inet unix dns audio", NULL) == -1) - die("pledge"); - } -#endif - // mDNS supports maximum of 63-character names (we append 13). if (strlen(config.service_name) > 50) { warn("The service name \"%s\" is too long (max 50 characters) and has been truncated.", @@ -2255,6 +2243,18 @@ int main(int argc, char **argv) { #endif +#ifdef COMPILE_FOR_OPENBSD + /* Drop "proc exec" unless external commands are to be run. */ + if ((config.cmd_active_start == NULL) && + (config.cmd_active_stop == NULL) && + (config.cmd_start == NULL) && + (config.cmd_stop == NULL) && + (config.cmd_set_volume == NULL)) { + if (pledge("stdio rpath wpath cpath dpath inet unix dns audio", NULL) == -1) + die("pledge"); + } +#endif + #ifdef CONFIG_AIRPLAY_2 if (has_fltp_capable_aac_decoder() == 0) { From 46538e5b5ab57940102de50ef7096238df988cb5 Mon Sep 17 00:00:00 2001 From: Klemens Nanni Date: Thu, 1 Feb 2024 05:00:54 +0100 Subject: [PATCH 3/8] Honour CONFIG_LIBDAEMON, drop "proc exec" as early as possible Worst case is pleding the same unchanged promises a second time, but this draws a better frame around the code that runs with "proc exec" merely because it is between main() start and daemon_fork(). --- shairport.c | 35 ++++++++++++++++++++++++----------- 1 file changed, 24 insertions(+), 11 deletions(-) diff --git a/shairport.c b/shairport.c index ce038159a..d157fabfe 100644 --- a/shairport.c +++ b/shairport.c @@ -1930,7 +1930,7 @@ void _display_config(const char *filename, const int linenumber, __attribute__(( int main(int argc, char **argv) { #ifdef COMPILE_FOR_OPENBSD - /* Start with the superset of all possible promises. */ + /* Start with the superset of all potentially required promises. */ if (pledge("stdio rpath wpath cpath dpath inet unix dns proc exec audio", NULL) == -1) die("pledge"); #endif @@ -2108,6 +2108,24 @@ int main(int argc, char **argv) { // parse arguments into config -- needed to locate pid_dir int audio_arg = parse_options(argc, argv); +#ifdef COMPILE_FOR_OPENBSD + int run_cmds = + config.cmd_active_start != NULL || + config.cmd_active_stop != NULL || + config.cmd_set_volume != NULL || + config.cmd_start != NULL || + config.cmd_stop != NULL; + + /* Drop "proc exec" immediately, if possible. */ +# if CONFIG_LIBDAEMON + if (!run_cmds && !config.daemonise) +# else + if (!run_cmds) +#endif + if (pledge("stdio rpath wpath cpath dpath inet unix dns audio", NULL) == -1) + die("pledge"); +#endif + // mDNS supports maximum of 63-character names (we append 13). if (strlen(config.service_name) > 50) { warn("The service name \"%s\" is too long (max 50 characters) and has been truncated.", @@ -2241,18 +2259,13 @@ int main(int argc, char **argv) { /* end libdaemon stuff */ } -#endif - -#ifdef COMPILE_FOR_OPENBSD - /* Drop "proc exec" unless external commands are to be run. */ - if ((config.cmd_active_start == NULL) && - (config.cmd_active_stop == NULL) && - (config.cmd_start == NULL) && - (config.cmd_stop == NULL) && - (config.cmd_set_volume == NULL)) { +# ifdef COMPILE_FOR_OPENBSD + /* Drop "proc exec", if possible. */ + if (!run_cmds) if (pledge("stdio rpath wpath cpath dpath inet unix dns audio", NULL) == -1) die("pledge"); - } +# endif + #endif #ifdef CONFIG_AIRPLAY_2 From 24b30fa34d36478a6fc3540ec43fd783e854951f Mon Sep 17 00:00:00 2001 From: Klemens Nanni Date: Thu, 1 Feb 2024 06:09:54 +0100 Subject: [PATCH 4/8] Drop "cpath dpath" unless metadata is enabled --- shairport.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/shairport.c b/shairport.c index d157fabfe..fe17ef1eb 100644 --- a/shairport.c +++ b/shairport.c @@ -2383,6 +2383,31 @@ int main(int argc, char **argv) { } config.output->init(argc - audio_arg, argv + audio_arg); +#ifdef COMPILE_FOR_OPENBSD + /* + * At this point, the first and last sio_open(3) call was made, i.e. + * the sndio(7) cookie is dealt with and only "audio" is needed. + */ + + if (run_cmds) { + /* Do not bother with "*path" as long as "proc exec" can do everything. */ + } else { + /* + * Only coverart cache is created/written. + * Only metadata pipe is special. + */ + int need_cpath_dpath = 0; +# ifdef CONFIG_METADATA + if (config.metadata_enabled) + need_cpath_dpath = 1; +# endif + /* Drop "cpath dpath". */ + if (!need_cpath_dpath) + if (pledge("stdio rpath wpath inet unix dns audio", NULL) == -1) + die("pledge"); + } +#endif + // pthread_cleanup_push(main_cleanup_handler, NULL); // daemon_log(LOG_NOTICE, "startup"); From 0372801edff935cf9deb3e5ef2da71366f33e605 Mon Sep 17 00:00:00 2001 From: Klemens Nanni Date: Thu, 1 Feb 2024 06:19:07 +0100 Subject: [PATCH 5/8] unveil(2) the few paths unless commands are run In case nothing is to be executed at runtime, lock down filesystem access down to - write/create metadata pipe/cache - read/rwite D-Bus socket - read glib2 locales If D-Bus setup were to happen earlier, we'd be able to ignore that entirely (socket already opened) at this point and, iff metadata is off, drop "wpath" as well, i.e. run with nothing but read-only locales. --- shairport.c | 43 +++++++++++++++++++++++++++++++++++++------ 1 file changed, 37 insertions(+), 6 deletions(-) diff --git a/shairport.c b/shairport.c index fe17ef1eb..ace3af1ed 100644 --- a/shairport.c +++ b/shairport.c @@ -1931,7 +1931,7 @@ void _display_config(const char *filename, const int linenumber, __attribute__(( int main(int argc, char **argv) { #ifdef COMPILE_FOR_OPENBSD /* Start with the superset of all potentially required promises. */ - if (pledge("stdio rpath wpath cpath dpath inet unix dns proc exec audio", NULL) == -1) + if (pledge("stdio rpath wpath cpath dpath inet unix dns proc exec unveil audio", NULL) == -1) die("pledge"); #endif @@ -2122,7 +2122,7 @@ int main(int argc, char **argv) { # else if (!run_cmds) #endif - if (pledge("stdio rpath wpath cpath dpath inet unix dns audio", NULL) == -1) + if (pledge("stdio rpath wpath cpath dpath inet unix dns unveil audio", NULL) == -1) die("pledge"); #endif @@ -2262,7 +2262,7 @@ int main(int argc, char **argv) { # ifdef COMPILE_FOR_OPENBSD /* Drop "proc exec", if possible. */ if (!run_cmds) - if (pledge("stdio rpath wpath cpath dpath inet unix dns audio", NULL) == -1) + if (pledge("stdio rpath wpath cpath dpath inet unix dns unveil audio", NULL) == -1) die("pledge"); # endif @@ -2392,19 +2392,50 @@ int main(int argc, char **argv) { if (run_cmds) { /* Do not bother with "*path" as long as "proc exec" can do everything. */ } else { + /* + * unveil(2) TODO: + * - assume system D-Bus, hoist setup/defer unveil + * - glib2 locale (not critical!) + * - MQTT? + */ +# if defined(CONFIG_DBUS_INTERFACE) || defined(CONFIG_MPRIS_INTERFACE) + if (unveil("/var/run/dbus/system_bus_socket", "rw") == -1) + die("unveil D-Bus"); +# endif + if (unveil("/usr/local/share/locale", "r") == -1) + die("unveil locale"); + /* * Only coverart cache is created/written. * Only metadata pipe is special. */ int need_cpath_dpath = 0; # ifdef CONFIG_METADATA - if (config.metadata_enabled) + if (config.metadata_enabled) { need_cpath_dpath = 1; +# ifdef CONFIG_METADATA_HUB + int do_cache = + config.cover_art_cache_dir != NULL && + config.cover_art_cache_dir[0] != '\0'; + + if (do_cache) + if (unveil(config.cover_art_cache_dir, "wc") == -1) + die("unveil %s", config.cover_art_cache_dir); +# endif + if (unveil(config.metadata_pipename, "wc") == -1) + die("unveil %s", config.metadata_pipename); + } # endif - /* Drop "cpath dpath". */ - if (!need_cpath_dpath) + + /* Drop "unveil". */ + if (need_cpath_dpath) { + if (pledge("stdio rpath wpath cpath dpath inet unix dns audio", NULL) == -1) + die("pledge"); + } else { + /* Drop "cpath dpath". */ if (pledge("stdio rpath wpath inet unix dns audio", NULL) == -1) die("pledge"); + } } #endif From 5fa38214235b8727f9d9372cda27630610d06985 Mon Sep 17 00:00:00 2001 From: Klemens Nanni Date: Thu, 1 Feb 2024 06:45:26 +0100 Subject: [PATCH 6/8] Comment on covertart cache unveil 'mkdir -p /tmp/shairport-sync/.cache/' before helps, but ideally, this would be done earlier during startup, or the whole unveil block gets deferred to whenever... --- shairport.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/shairport.c b/shairport.c index ace3af1ed..fb18cc15c 100644 --- a/shairport.c +++ b/shairport.c @@ -2414,6 +2414,10 @@ int main(int argc, char **argv) { if (config.metadata_enabled) { need_cpath_dpath = 1; # ifdef CONFIG_METADATA_HUB + /* + * XXX unveiling default /tmp/shairport-sync/.cache/coverart may fail + * as parent directories do not exist. + */ int do_cache = config.cover_art_cache_dir != NULL && config.cover_art_cache_dir[0] != '\0'; From ae0428f10173994e94f5536f8a5ba1f8a45e32dc Mon Sep 17 00:00:00 2001 From: Klemens Nanni Date: Thu, 1 Feb 2024 18:45:29 +0100 Subject: [PATCH 7/8] reflect that glib2 is pulled in via D-Bus/MPRIS --- shairport.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/shairport.c b/shairport.c index fb18cc15c..f31f4f534 100644 --- a/shairport.c +++ b/shairport.c @@ -2395,15 +2395,15 @@ int main(int argc, char **argv) { /* * unveil(2) TODO: * - assume system D-Bus, hoist setup/defer unveil - * - glib2 locale (not critical!) + * - glib2 locale (not critical!) * - MQTT? */ # if defined(CONFIG_DBUS_INTERFACE) || defined(CONFIG_MPRIS_INTERFACE) if (unveil("/var/run/dbus/system_bus_socket", "rw") == -1) die("unveil D-Bus"); -# endif if (unveil("/usr/local/share/locale", "r") == -1) die("unveil locale"); +# endif /* * Only coverart cache is created/written. From 2e8f90be4400a0255f83bab87b4fa5911b7c1dd4 Mon Sep 17 00:00:00 2001 From: Klemens Nanni Date: Thu, 1 Feb 2024 23:30:00 +0100 Subject: [PATCH 8/8] pass pledge/unveil errno strings to die() --- shairport.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/shairport.c b/shairport.c index f31f4f534..556140234 100644 --- a/shairport.c +++ b/shairport.c @@ -114,6 +114,10 @@ #include #endif +#ifdef CONFIG_OPENBSD +#include // strerror +#endif + pid_t pid; #ifdef CONFIG_LIBDAEMON int this_is_the_daemon_process = 0; @@ -1932,7 +1936,7 @@ int main(int argc, char **argv) { #ifdef COMPILE_FOR_OPENBSD /* Start with the superset of all potentially required promises. */ if (pledge("stdio rpath wpath cpath dpath inet unix dns proc exec unveil audio", NULL) == -1) - die("pledge"); + die("pledge: %s", strerror(errno)); #endif memset(&config, 0, sizeof(config)); // also clears all strings, BTW @@ -2121,9 +2125,9 @@ int main(int argc, char **argv) { if (!run_cmds && !config.daemonise) # else if (!run_cmds) -#endif +# endif if (pledge("stdio rpath wpath cpath dpath inet unix dns unveil audio", NULL) == -1) - die("pledge"); + die("pledge: %s", strerror(errno)); #endif // mDNS supports maximum of 63-character names (we append 13). @@ -2263,7 +2267,7 @@ int main(int argc, char **argv) { /* Drop "proc exec", if possible. */ if (!run_cmds) if (pledge("stdio rpath wpath cpath dpath inet unix dns unveil audio", NULL) == -1) - die("pledge"); + die("pledge: %s", strerror(errno)); # endif #endif @@ -2400,9 +2404,9 @@ int main(int argc, char **argv) { */ # if defined(CONFIG_DBUS_INTERFACE) || defined(CONFIG_MPRIS_INTERFACE) if (unveil("/var/run/dbus/system_bus_socket", "rw") == -1) - die("unveil D-Bus"); + die("unveil D-Bus: %s", strerror(errno)); if (unveil("/usr/local/share/locale", "r") == -1) - die("unveil locale"); + die("unveil locale: %s", strerror(errno)); # endif /* @@ -2424,21 +2428,21 @@ int main(int argc, char **argv) { if (do_cache) if (unveil(config.cover_art_cache_dir, "wc") == -1) - die("unveil %s", config.cover_art_cache_dir); + die("unveil %s: %s", config.cover_art_cache_dir, strerror(errno)); # endif if (unveil(config.metadata_pipename, "wc") == -1) - die("unveil %s", config.metadata_pipename); + die("unveil %s: %s", config.metadata_pipename, strerror(errno)); } # endif /* Drop "unveil". */ if (need_cpath_dpath) { if (pledge("stdio rpath wpath cpath dpath inet unix dns audio", NULL) == -1) - die("pledge"); + die("pledge: %s", strerror(errno)); } else { /* Drop "cpath dpath". */ if (pledge("stdio rpath wpath inet unix dns audio", NULL) == -1) - die("pledge"); + die("pledge: %s", strerror(errno)); } } #endif