do not apply POSIX file attributes through symbolic links - #4229
do not apply POSIX file attributes through symbolic links#4229jmestwa-coder wants to merge 1 commit into
Conversation
c9c9c5a to
21c00ae
Compare
|
any update? |
| final String fileGroup) | ||
| throws IOException { | ||
| final PosixFileAttributeView view = Files.getFileAttributeView(path, PosixFileAttributeView.class); | ||
| final PosixFileAttributeView view = |
There was a problem hiding this comment.
| final PosixFileAttributeView view = | |
| final PosixFileAttributeView view = Files.getFileAttributeView(path, PosixFileAttributeView.class); |
There was a problem hiding this 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.
| * modified and its target is left untouched. | ||
| * </p> | ||
| * | ||
| * @param path Target path |
There was a problem hiding this 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.
| return new SimpleFileVisitor<Path>() { | ||
| @Override | ||
| public FileVisitResult visitFile(final Path file, final BasicFileAttributes attrs) throws IOException { | ||
| if (attrs.isSymbolicLink()) { |
There was a problem hiding this 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.
| if (attrs.isSymbolicLink()) { | |
| if (!isFollowSymbolicLinks() && attrs.isSymbolicLink()) { |
| @@ -87,6 +90,28 @@ void testFileFromUriWithSpacesAndPlusCharactersInName() throws Exception { | |||
| assertTrue(file.exists(), "file exists"); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
@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 |
There was a problem hiding this comment.
Deletion block for the old test:
| @Test |
| 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> |
There was a problem hiding this comment.
Could the entry say which configuration changes? As written, a reader with links in their log directory cannot tell whether this affects them.
| <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> |
PosixViewAttributeapplies permissions and ownership through symbolic links:FileUtils.defineFilePosixAttributeViewlooks the view up withoutNOFOLLOW_LINKS, sosetPermissions/setOwner/setGroupland on the link targetwalkFileTreestill hands symlinks tovisitFile, so thefollowLinks="false"default documented onAbstractPathActionhas no effect herebasePathwhose name matches thePathConditionglob redirects the chmod/chown onto any file the process can reachFileManager.defineAttributeViewgoes through the same helperRequesting the view with
NOFOLLOW_LINKSmakes 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
2.xbranch if you are targeting Log4j 2; usemainotherwise./mvnw verifysucceeds (the build instructions)src/changelog/.2.x.xdirectory