do not apply POSIX file attributes through symbolic links

Author: jmestwa-coder

log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/rolling/action/PosixViewAttributeActionTest.java

@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.logging.log4j.core.appender.rolling.action;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assumptions.assumeTrue;
+
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.attribute.PosixFilePermissions;
+import org.apache.logging.log4j.core.config.Configuration;
+import org.apache.logging.log4j.core.test.BasicConfigurationFactory;
+import org.apache.logging.log4j.core.util.FileUtils;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+/**
+ * Tests the {@code PosixViewAttributeAction} class.
+ */
+class PosixViewAttributeActionTest {
+
+    @BeforeAll
+    static void beforeClass() {
+        assumeTrue(FileUtils.isFilePosixAttributeViewSupported());
+    }
+
+    @Test
+    void testSymbolicLinksAreNotFollowed(@TempDir final Path tempDir) throws Exception {
+        // A file outside the scanned directory, that the action must not touch.
+        final Path outsider = tempDir.resolve("outsider.txt");
+        Files.write(outsider, "secret".getBytes(StandardCharsets.UTF_8));
+        Files.setPosixFilePermissions(outsider, PosixFilePermissions.fromString("rw-------"));
+
+        final Path baseDir = Files.createDirectory(tempDir.resolve("logs"));
+        final Path regularFile = baseDir.resolve("app-1.log");
+        Files.write(regularFile, "log".getBytes(StandardCharsets.UTF_8));
+        Files.setPosixFilePermissions(regularFile, PosixFilePermissions.fromString("rw-------"));
+        Files.createSymbolicLink(baseDir.resolve("app-2.log"), outsider);
+
+        final Configuration config = new BasicConfigurationFactory().new BasicConfiguration();
+        final PosixViewAttributeAction action = PosixViewAttributeAction.newBuilder()
+                .setBasePath(baseDir.toString())
+                .setFollowLinks(false)
+                .setMaxDepth(1)
+                .setPathConditions(PathCondition.EMPTY_ARRAY)
+                .setConfiguration(config)
+                .setFilePermissionsString("rw-rw-rw-")
+                .build();
+
+        action.execute();
+
+        assertEquals(
+                "rw-rw-rw-",
+                PosixFilePermissions.toString(Files.getPosixFilePermissions(regularFile)),
+                "regular file should have been updated");
+        assertEquals(
+                "rw-------",
+                PosixFilePermissions.toString(Files.getPosixFilePermissions(outsider)),
+                "symbolic link target should have been left alone");
+    }
+}

log4j-core-test/src/test/java/org/apache/logging/log4j/core/util/FileUtilsTest.java

@@ -20,11 +20,14 @@
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assumptions.assumeTrue;
 
 import java.io.File;
 import java.io.IOException;
 import java.net.URI;
 import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.attribute.PosixFilePermissions;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.concurrent.atomic.AtomicBoolean;
@@ -87,6 +90,28 @@ void testFileFromUriWithSpacesAndPlusCharactersInName() throws Exception {
         assertTrue(file.exists(), "file exists");
     }
 
+    @Test
+    void testDefineFilePosixAttributeViewDoesNotFollowSymbolicLinks(@TempDir final Path tempDir) throws Exception {
+        assumeTrue(FileUtils.isFilePosixAttributeViewSupported());
+
+        final Path target = tempDir.resolve("target.txt");
+        Files.createFile(target);
+        Files.setPosixFilePermissions(target, PosixFilePermissions.fromString("rw-------"));
+        final Path link = tempDir.resolve("link.txt");
+        Files.createSymbolicLink(link, target);
+
+        try {
+            FileUtils.defineFilePosixAttributeView(link, PosixFilePermissions.fromString("rw-rw-rw-"), null, null);
+        } catch (final IOException expected) {
+            // POSIX has no way to change the permissions of the link itself
+        }
+
+        assertEquals(
+                "rw-------",
+                PosixFilePermissions.toString(Files.getPosixFilePermissions(target)),
+                "link target should have been left alone");
+    }
+
     @Nested
     class TestMkdir {
         @TempDir

log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/action/PosixViewAttributeAction.java

@@ -369,6 +369,10 @@ protected FileVisitor<Path> createFileVisitor(final Path basePath, final List<Pa
         return new SimpleFileVisitor<Path>() {
             @Override
             public FileVisitResult visitFile(final Path file, final BasicFileAttributes attrs) throws IOException {
+                if (attrs.isSymbolicLink()) {
+                    LOGGER.trace("Not defining POSIX attribute on symbolic link {}", file);
+                    return FileVisitResult.CONTINUE;
+                }
                 for (final PathCondition pathFilter : conditions) {
                     final Path relative = basePath.relativize(file);
                     if (!pathFilter.accept(basePath, relative, attrs)) {

log4j-core/src/main/java/org/apache/logging/log4j/core/util/FileUtils.java

@@ -24,6 +24,7 @@
 import java.nio.file.FileAlreadyExistsException;
 import java.nio.file.FileSystems;
 import java.nio.file.Files;
+import java.nio.file.LinkOption;
 import java.nio.file.Path;
 import java.nio.file.attribute.GroupPrincipal;
 import java.nio.file.attribute.PosixFileAttributeView;
@@ -146,6 +147,10 @@ public static void makeParentDirs(final File file) throws IOException {
 
     /**
      * Define file POSIX attribute view on a path/file.
+     * <p>
+     * Symbolic links are never followed: if {@code path} is a link, the attributes of the link itself are
+     * modified and its target is left untouched.
+     * </p>
      *
      * @param path Target path
      * @param filePermissions Permissions to apply
@@ -159,7 +164,8 @@ public static void defineFilePosixAttributeView(
             final String fileOwner,
             final String fileGroup)
             throws IOException {
-        final PosixFileAttributeView view = Files.getFileAttributeView(path, PosixFileAttributeView.class);
+        final PosixFileAttributeView view =
+                Files.getFileAttributeView(path, PosixFileAttributeView.class, LinkOption.NOFOLLOW_LINKS);
         if (view != null) {
             final UserPrincipalLookupService lookupService =
                     FileSystems.getDefault().getUserPrincipalLookupService();

src/changelog/.2.x.x/fix_posix_view_attribute_symlink.xml

@@ -0,0 +1,8 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+       xmlns="https://logging.apache.org/xml/ns"
+       xsi:schemaLocation="https://logging.apache.org/xml/ns https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"
+       type="fixed">
+  <issue id="4229" link="https://github.com/apache/logging-log4j2/pull/4229"/>
+  <description format="asciidoc">Stop `PosixViewAttribute` from applying permissions and ownership through symbolic links</description>
+</entry>