From 9b8cb9a4639af3b47b5eeec4f5a04261dcd2a058 Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Wed, 10 Jan 2018 14:24:40 -0800 Subject: [PATCH] Protect against symlink attacks Update embedded launch script to no longer change ownership of files or folders that already exist. Fixes gh-11397 --- .../launchscript/SysVinitLaunchScriptIT.java | 19 ++++++++++++++++++ .../resources/scripts/log-file-ownership.sh | 20 +++++++++++++++++++ .../resources/scripts/pid-file-ownership.sh | 18 +++++++++++++++++ .../resources/scripts/pid-folder-ownership.sh | 17 ++++++++++++++++ .../boot/loader/tools/launch.script | 8 ++++---- 5 files changed, 78 insertions(+), 4 deletions(-) create mode 100755 spring-boot-integration-tests/spring-boot-launch-script-tests/src/test/resources/scripts/log-file-ownership.sh create mode 100755 spring-boot-integration-tests/spring-boot-launch-script-tests/src/test/resources/scripts/pid-file-ownership.sh create mode 100755 spring-boot-integration-tests/spring-boot-launch-script-tests/src/test/resources/scripts/pid-folder-ownership.sh diff --git a/spring-boot-integration-tests/spring-boot-launch-script-tests/src/test/java/org/springframework/boot/launchscript/SysVinitLaunchScriptIT.java b/spring-boot-integration-tests/spring-boot-launch-script-tests/src/test/java/org/springframework/boot/launchscript/SysVinitLaunchScriptIT.java index ad134c23c476..d83091308684 100644 --- a/spring-boot-integration-tests/spring-boot-launch-script-tests/src/test/java/org/springframework/boot/launchscript/SysVinitLaunchScriptIT.java +++ b/spring-boot-integration-tests/spring-boot-launch-script-tests/src/test/java/org/springframework/boot/launchscript/SysVinitLaunchScriptIT.java @@ -220,6 +220,25 @@ public void launchWithRelativePidFolder() throws Exception { coloredString(AnsiColor.GREEN, "Stopped [" + extractPid(output) + "]")); } + @Test + public void pidFolderOwnership() throws Exception { + String output = doTest("pid-folder-ownership.sh"); + System.err.println(output); + assertThat(output).contains("phil root"); + } + + @Test + public void pidFileOwnership() throws Exception { + String output = doTest("pid-file-ownership.sh"); + assertThat(output).contains("phil root"); + } + + @Test + public void logFileOwnership() throws Exception { + String output = doTest("log-file-ownership.sh"); + assertThat(output).contains("phil root"); + } + @Test public void launchWithRelativeLogFolder() throws Exception { String output = doTest("launch-with-relative-log-folder.sh"); diff --git a/spring-boot-integration-tests/spring-boot-launch-script-tests/src/test/resources/scripts/log-file-ownership.sh b/spring-boot-integration-tests/spring-boot-launch-script-tests/src/test/resources/scripts/log-file-ownership.sh new file mode 100755 index 000000000000..919c33e3809f --- /dev/null +++ b/spring-boot-integration-tests/spring-boot-launch-script-tests/src/test/resources/scripts/log-file-ownership.sh @@ -0,0 +1,20 @@ +source ./test-functions.sh +install_service + +chmod o+w /var/log + +useradd phil +mkdir /phil-files +chown phil /phil-files + +useradd andy +chown andy /test-service/spring-boot-app.jar + +start_service +stop_service + +su - andy -c "ln -s -f /phil-files /var/log/spring-boot-app.log" + +start_service + +ls -ld /phil-files diff --git a/spring-boot-integration-tests/spring-boot-launch-script-tests/src/test/resources/scripts/pid-file-ownership.sh b/spring-boot-integration-tests/spring-boot-launch-script-tests/src/test/resources/scripts/pid-file-ownership.sh new file mode 100755 index 000000000000..891bb935fa47 --- /dev/null +++ b/spring-boot-integration-tests/spring-boot-launch-script-tests/src/test/resources/scripts/pid-file-ownership.sh @@ -0,0 +1,18 @@ +source ./test-functions.sh +install_service + +useradd phil +mkdir /phil-files +chown phil /phil-files + +useradd andy +chown andy /test-service/spring-boot-app.jar + +start_service +stop_service + +su - andy -c "ln -s /phil-files /var/run/spring-boot-app/spring-boot-app.pid" + +start_service + +ls -ld /phil-files diff --git a/spring-boot-integration-tests/spring-boot-launch-script-tests/src/test/resources/scripts/pid-folder-ownership.sh b/spring-boot-integration-tests/spring-boot-launch-script-tests/src/test/resources/scripts/pid-folder-ownership.sh new file mode 100755 index 000000000000..c6b7d19c093f --- /dev/null +++ b/spring-boot-integration-tests/spring-boot-launch-script-tests/src/test/resources/scripts/pid-folder-ownership.sh @@ -0,0 +1,17 @@ +source ./test-functions.sh +install_service + +chmod o+w /var/run + +useradd phil +mkdir /phil-files +chown phil /phil-files + +useradd andy +chown andy /test-service/spring-boot-app.jar + +su - andy -c "ln -s -f /phil-files /var/run/spring-boot-app" + +start_service + +ls -ld /phil-files diff --git a/spring-boot-tools/spring-boot-loader-tools/src/main/resources/org/springframework/boot/loader/tools/launch.script b/spring-boot-tools/spring-boot-loader-tools/src/main/resources/org/springframework/boot/loader/tools/launch.script index 25facc1ede11..aac9114eb3a0 100755 --- a/spring-boot-tools/spring-boot-loader-tools/src/main/resources/org/springframework/boot/loader/tools/launch.script +++ b/spring-boot-tools/spring-boot-loader-tools/src/main/resources/org/springframework/boot/loader/tools/launch.script @@ -146,12 +146,12 @@ start() { do_start() { working_dir=$(dirname "$jarfile") pushd "$working_dir" > /dev/null - mkdir -p "$PID_FOLDER" &> /dev/null + if [[ ! -e "$PID_FOLDER" ]]; then + mkdir -p "$PID_FOLDER" &> /dev/null + chown "$run_user" "$PID_FOLDER" + fi if [[ -n "$run_user" ]]; then checkPermissions || return $? - chown "$run_user" "$PID_FOLDER" - chown "$run_user" "$pid_file" - chown "$run_user" "$log_file" if [ $USE_START_STOP_DAEMON = true ] && type start-stop-daemon > /dev/null 2>&1; then start-stop-daemon --start --quiet \ --chuid "$run_user" \