Skip to content

Commit d088fcf

Browse files
openshift-merge-bot[bot]openshift-merge-bot[bot]
authored
Merge pull request #8978 from sohankunkerkar/release-1.30
[release-1.30] CVE: Fix path traversal in CRI-O log handling
2 parents 9e82c3f + 76b7919 commit d088fcf

File tree

2 files changed

+113
-8
lines changed

2 files changed

+113
-8
lines changed

‎internal/linklogs/link_logs.go‎

Lines changed: 39 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,10 @@ import (
55
"errors"
66
"fmt"
77
"os"
8-
"path/filepath"
8+
"strings"
99

1010
"github.com/cri-o/cri-o/internal/log"
11+
securejoin "github.com/cyphar/filepath-securejoin"
1112
"github.com/opencontainers/selinux/go-selinux/label"
1213
"golang.org/x/sys/unix"
1314
"k8s.io/apimachinery/pkg/util/validation"
@@ -28,12 +29,22 @@ func MountPodLogs(ctx context.Context, kubePodUID, emptyDirVolName, namespace, k
2829
if errs := validation.IsDNS1123Label(emptyDirVolName); len(errs) != 0 {
2930
return errors.New("empty dir vol name is invalid")
3031
}
31-
emptyDirLoggingVolumePath := podEmptyDirPath(kubePodUID, emptyDirVolName)
32+
33+
emptyDirLoggingVolumePath, err := podEmptyDirPath(kubePodUID, emptyDirVolName)
34+
if err != nil {
35+
return fmt.Errorf("failed to get empty dir path: %w", err)
36+
}
37+
3238
if _, err := os.Stat(emptyDirLoggingVolumePath); err != nil {
3339
return fmt.Errorf("failed to find %v: %w", emptyDirLoggingVolumePath, err)
3440
}
3541
podLogsDirectory := namespace + "_" + kubeName + "_" + kubePodUID
36-
podLogsPath := filepath.Join(kubeletPodLogsRootDir, podLogsDirectory)
42+
43+
podLogsPath, err := securejoin.SecureJoin(kubeletPodLogsRootDir, podLogsDirectory)
44+
if err != nil {
45+
return fmt.Errorf("failed to join %v and %v: %w", kubeletPodLogsRootDir, podLogsDirectory, err)
46+
}
47+
3748
log.Infof(ctx, "Mounting from %s to %s for linked logs", podLogsPath, emptyDirLoggingVolumePath)
3849
if err := unix.Mount(podLogsPath, emptyDirLoggingVolumePath, "bind", unix.MS_BIND|unix.MS_RDONLY, ""); err != nil {
3950
return fmt.Errorf("failed to mount %v to %v: %w", podLogsPath, emptyDirLoggingVolumePath, err)
@@ -46,7 +57,11 @@ func MountPodLogs(ctx context.Context, kubePodUID, emptyDirVolName, namespace, k
4657

4758
// UnmountPodLogs unmounts the pod log directory from the specified empty dir volume
4859
func UnmountPodLogs(ctx context.Context, kubePodUID, emptyDirVolName string) error {
49-
emptyDirLoggingVolumePath := podEmptyDirPath(kubePodUID, emptyDirVolName)
60+
emptyDirLoggingVolumePath, err := podEmptyDirPath(kubePodUID, emptyDirVolName)
61+
if err != nil {
62+
return fmt.Errorf("failed to get empty dir path: %w", err)
63+
}
64+
5065
log.Infof(ctx, "Unmounting %s for linked logs", emptyDirLoggingVolumePath)
5166
if _, err := os.Stat(emptyDirLoggingVolumePath); !os.IsNotExist(err) {
5267
if err := unix.Unmount(emptyDirLoggingVolumePath, unix.MNT_DETACH); err != nil {
@@ -57,14 +72,30 @@ func UnmountPodLogs(ctx context.Context, kubePodUID, emptyDirVolName string) err
5772
}
5873

5974
func LinkContainerLogs(ctx context.Context, kubePodUID, emptyDirVolName, id string, metadata *types.ContainerMetadata) error {
60-
emptyDirLoggingVolumePath := podEmptyDirPath(kubePodUID, emptyDirVolName)
75+
emptyDirLoggingVolumePath, err := podEmptyDirPath(kubePodUID, emptyDirVolName)
76+
if err != nil {
77+
return fmt.Errorf("failed to get empty dir path: %w", err)
78+
}
79+
6180
// Symlink a relative path so the location is legitimate inside and outside the container.
6281
from := fmt.Sprintf("%s/%d.log", metadata.Name, metadata.Attempt)
63-
to := filepath.Join(emptyDirLoggingVolumePath, id+".log")
82+
83+
to, err := securejoin.SecureJoin(emptyDirLoggingVolumePath, id+".log")
84+
if err != nil {
85+
return fmt.Errorf("failed to join %v and %v: %w", emptyDirLoggingVolumePath, id+".log", err)
86+
}
87+
6488
log.Infof(ctx, "Symlinking from %s to %s for linked logs", from, to)
6589
return os.Symlink(from, to)
6690
}
6791

68-
func podEmptyDirPath(podUID, emptyDirVolName string) string {
69-
return filepath.Join(kubeletPodsRootDir, podUID, "volumes", kubeletEmptyDirLogDir, emptyDirVolName)
92+
func podEmptyDirPath(podUID, emptyDirVolName string) (string, error) {
93+
relativePath := strings.Join([]string{podUID, "volumes", kubeletEmptyDirLogDir, emptyDirVolName}, "/")
94+
95+
dirPath, err := securejoin.SecureJoin(kubeletPodsRootDir, relativePath)
96+
if err != nil {
97+
return "", fmt.Errorf("failed to join %v and %v: %w", kubeletPodsRootDir, relativePath, err)
98+
}
99+
100+
return dirPath, err
70101
}

‎test/ctr.bats‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,54 @@ function create_test_rro_mounts() {
6262
echo "$directory"
6363
}
6464

65+
function setup_log_linking_test() {
66+
local pod_uid=$1
67+
local pod_name pod_namespace pod_log_dir pod_empty_dir_volume_path pod_id ctr_name ctr_attempt ctr_id
68+
69+
pod_name=$(jq -r '.metadata.name' "$TESTDATA/sandbox_config.json")
70+
pod_namespace=$(jq -r '.metadata.namespace' "$TESTDATA/sandbox_config.json")
71+
pod_log_dir="/var/log/pods/${pod_namespace}_${pod_name}_${pod_uid}"
72+
pod_empty_dir_volume_path="/var/lib/kubelet/pods/$pod_uid/volumes/kubernetes.io~empty-dir/logging-volume"
73+
74+
# Create directories and set up pod/container.
75+
mkdir -p "$pod_log_dir" "$pod_empty_dir_volume_path"
76+
jq --arg pod_log_dir "$pod_log_dir" --arg pod_uid "$pod_uid" '.annotations["io.kubernetes.cri-o.LinkLogs"] = "logging-volume"
77+
| .log_directory = $pod_log_dir | .metadata.uid = $pod_uid' \
78+
"$TESTDATA/sandbox_config.json" > "$TESTDIR/sandbox_config.json"
79+
pod_id=$(crictl runp "$TESTDIR/sandbox_config.json")
80+
81+
# Touch the log file.
82+
ctr_name=$(jq -r '.metadata.name' "$TESTDATA/container_config.json")
83+
ctr_attempt=$(jq -r '.metadata.attempt' "$TESTDATA/container_config.json")
84+
mkdir -p "$pod_log_dir/$ctr_name"
85+
touch "$pod_log_dir/$ctr_name/$ctr_attempt.log"
86+
87+
jq --arg host_path "$pod_empty_dir_volume_path" --arg ctr_path "/mnt/logging-volume" --arg log_path "$ctr_name/$ctr_attempt.log" \
88+
'.command = ["sh", "-c", "echo Hello log linking && sleep 1000"]
89+
| .log_path = $log_path
90+
| .mounts = [ { host_path: $host_path, container_path: $ctr_path } ]' \
91+
"$TESTDATA"/container_config.json > "$TESTDIR/container_config.json"
92+
ctr_id=$(crictl create "$pod_id" "$TESTDIR/container_config.json" "$TESTDIR/sandbox_config.json")
93+
}
94+
95+
function assert_log_linking() {
96+
local pod_empty_dir_volume_path=$1
97+
local ctr_name=$2
98+
local ctr_attempt=$3
99+
local ctr_id=$4
100+
local should_succeed=$5
101+
102+
if $should_succeed; then
103+
[ -f "$pod_empty_dir_volume_path/$ctr_name/$ctr_attempt.log" ]
104+
[ -f "$pod_empty_dir_volume_path/$ctr_id.log" ]
105+
grep -E "Hello log linking" "$pod_empty_dir_volume_path/$ctr_name/$ctr_attempt.log"
106+
grep -E "Hello log linking" "$pod_empty_dir_volume_path/$ctr_id.log"
107+
else
108+
[ ! -f "$pod_empty_dir_volume_path/$ctr_name/$ctr_attempt.log" ]
109+
[ ! -f "$pod_empty_dir_volume_path/$ctr_id.log" ]
110+
fi
111+
}
112+
65113
@test "ctr not found correct error message" {
66114
start_crio
67115
run ! crictl inspect "container_not_exist"
@@ -1356,6 +1404,32 @@ function create_test_rro_mounts() {
13561404
[ ! -f "$linked_log_path" ]
13571405
}
13581406

1407+
@test "ctr log linking with malicious paths" {
1408+
if [[ $RUNTIME_TYPE == vm ]]; then
1409+
skip "not applicable to vm runtime type"
1410+
fi
1411+
setup_crio
1412+
create_runtime_with_allowed_annotation logs io.kubernetes.cri-o.LinkLogs
1413+
start_crio_no_setup
1414+
1415+
read -r pod_empty_dir_volume_path ctr_name ctr_attempt ctr_id <<< "$(setup_log_linking_test "../../../malicious")"
1416+
assert_log_linking "$pod_empty_dir_volume_path" "$ctr_name" "$ctr_attempt" "$ctr_id" false
1417+
crictl rmp -fa
1418+
}
1419+
1420+
@test "ctr log linking with invalid paths" {
1421+
if [[ $RUNTIME_TYPE == vm ]]; then
1422+
skip "not applicable to vm runtime type"
1423+
fi
1424+
setup_crio
1425+
create_runtime_with_allowed_annotation logs io.kubernetes.cri-o.LinkLogs
1426+
start_crio_no_setup
1427+
1428+
read -r pod_empty_dir_volume_path ctr_name ctr_attempt ctr_id <<< "$(setup_log_linking_test "invalid path")"
1429+
assert_log_linking "$pod_empty_dir_volume_path" "$ctr_name" "$ctr_attempt" "$ctr_id" false
1430+
crictl rmp -fa
1431+
}
1432+
13591433
@test "ctr stop loop kill retry attempts" {
13601434
FAKE_RUNTIME_BINARY_PATH="$TESTDIR"/fake
13611435
FAKE_RUNTIME_ATTEMPTS_LOG="$TESTDIR"/fake.log

0 commit comments

Comments
 (0)