Skip to content

do not apply POSIX file attributes through symbolic links - #4229

Open
jmestwa-coder wants to merge 1 commit into
apache:2.xfrom
jmestwa-coder:posix-view-symlink
Open

do not apply POSIX file attributes through symbolic links#4229
jmestwa-coder wants to merge 1 commit into
apache:2.xfrom
jmestwa-coder:posix-view-symlink

Conversation

@jmestwa-coder

Copy link
Copy Markdown

PosixViewAttribute applies permissions and ownership through symbolic links:

  • FileUtils.defineFilePosixAttributeView looks the view up without NOFOLLOW_LINKS, so setPermissions/setOwner/setGroup land on the link target
  • walkFileTree still hands symlinks to visitFile, so the followLinks="false" default documented on AbstractPathAction has no effect here
  • a link planted in basePath whose name matches the PathCondition glob redirects the chmod/chown onto any file the process can reach
  • FileManager.defineAttributeView goes through the same helper

Requesting the view with NOFOLLOW_LINKS makes the change fail on a link rather than hit its target, and the visitor now skips symlinks so a planted one does not abort the rollover action.

Checklist

  • Base your changes on 2.x branch if you are targeting Log4j 2; use main otherwise
  • ./mvnw verify succeeds (the build instructions)
  • Non-trivial changes contain an entry file in the src/changelog/.2.x.x directory
  • Tests are provided

@jmestwa-coder

Copy link
Copy Markdown
Author

any update?

final String fileGroup)
throws IOException {
final PosixFileAttributeView view = Files.getFileAttributeView(path, PosixFileAttributeView.class);
final PosixFileAttributeView view =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
final PosixFileAttributeView view =
final PosixFileAttributeView view = Files.getFileAttributeView(path, PosixFileAttributeView.class);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This helper is also called by FileManager:289 and, through defineAttributeView, by RollingFileManager:409 and RollingRandomAccessFileManager:270,333 — on the appender's own configured fileName, not on anything an attacker planted. If that name is a symlink I get FileSystemException: Too many levels of symbolic links, which FileManager swallows into LOGGER.error("Could not define attribute view …"), so permissions are silently not applied. Could this line stay as it was, and let the visitor guard do the work? I tried it locally and both followLinks modes then behave, with your PosixViewAttributeActionTest still passing. The import java.nio.file.LinkOption; added at the top can go with it.

* modified and its target is left untouched.
* </p>
*
* @param path Target path

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Permissions never land on the link — setPermissions throws, which is why the new FileUtilsTest has to swallow an IOException. Only owner and group do, via lchown. If the lookup goes back to following, this paragraph can go entirely.

return new SimpleFileVisitor<Path>() {
@Override
public FileVisitResult visitFile(final Path file, final BasicFileAttributes attrs) throws IOException {
if (attrs.isSymbolicLink()) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

followLinks="true" is documented as supported — manual/appenders/rolling-file.adoc:1033, with its own security warning — but under FOLLOW_LINKS the attributes come from stat, so attrs.isSymbolicLink() is false and this guard never fires. Gating the skip on the action's own setting makes both modes work. isFollowSymbolicLinks() is already public on AbstractPathAction:143.

Suggested change
if (attrs.isSymbolicLink()) {
if (!isFollowSymbolicLinks() && attrs.isSymbolicLink()) {

@@ -87,6 +90,28 @@ void testFileFromUriWithSpacesAndPlusCharactersInName() throws Exception {
assertTrue(file.exists(), "file exists");
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Test
void testSymbolicLinksAreFollowedWhenConfigured(@TempDir final Path tempDir) throws Exception {
    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"));
    Files.createSymbolicLink(baseDir.resolve("app-2.log"), outsider);

    final Configuration config = new BasicConfigurationFactory().new BasicConfiguration();
    final PosixViewAttributeAction action = PosixViewAttributeAction.newBuilder()
            .setBasePath(baseDir.toString())
            .setFollowLinks(true)
            .setMaxDepth(1)
            .setPathConditions(PathCondition.EMPTY_ARRAY)
            .setConfiguration(config)
            .setFilePermissionsString("rw-rw-rw-")
            .build();

    action.execute();

    assertEquals(
            "rw-rw-rw-",
            PosixFilePermissions.toString(Files.getPosixFilePermissions(outsider)),
            "followLinks=\"true\" should still follow the link");
}

assertTrue(file.exists(), "file exists");
}

@Test

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deletion block for the old test:

Suggested change
@Test

@github-project-automation github-project-automation Bot moved this to Changes requested in Log4j pull request tracker Aug 24, 2026
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>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could the entry say which configuration changes? As written, a reader with links in their log directory cannot tell whether this affects them.

Suggested change
<description format="asciidoc">Stop `PosixViewAttribute` from applying permissions and ownership through symbolic links</description>
<description format="asciidoc">Stop `PosixViewAttribute` from applying permissions and ownership through symbolic links found in `basePath` unless `followLinks` is set to `true`.</description>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Changes requested

Development

Successfully merging this pull request may close these issues.

2 participants