ramanathan1504 commented on code in PR #4229:
URL: https://github.com/apache/logging-log4j2/pull/4229#discussion_r3843510490
##########
log4j-core-test/src/test/java/org/apache/logging/log4j/core/util/FileUtilsTest.java:
##########
@@ -87,6 +90,28 @@ void testFileFromUriWithSpacesAndPlusCharactersInName()
throws Exception {
assertTrue(file.exists(), "file exists");
}
Review Comment:
```java
@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");
}
```
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/FileUtils.java:
##########
@@ -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
Review Comment:
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.
##########
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()) {
Review Comment:
`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`.
```suggestion
if (!isFollowSymbolicLinks() && attrs.isSymbolicLink()) {
```
##########
log4j-core-test/src/test/java/org/apache/logging/log4j/core/util/FileUtilsTest.java:
##########
@@ -87,6 +90,28 @@ void testFileFromUriWithSpacesAndPlusCharactersInName()
throws Exception {
assertTrue(file.exists(), "file exists");
}
+ @Test
Review Comment:
Deletion block for the old test:
```suggestion
```
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/FileUtils.java:
##########
@@ -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 =
Review Comment:
```suggestion
final PosixFileAttributeView view = Files.getFileAttributeView(path,
PosixFileAttributeView.class);
```
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/FileUtils.java:
##########
@@ -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 =
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
