[ https://issues.apache.org/jira/browse/HADOOP-18235?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17615947#comment-17615947 ]
ASF GitHub Bot commented on HADOOP-18235: ----------------------------------------- cbaenziger commented on code in PR #4998: URL: https://github.com/apache/hadoop/pull/4998#discussion_r992538793 ########## hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/alias/LocalKeyStoreProvider.java: ########## @@ -142,21 +142,27 @@ protected void initFileSystem(URI uri) @Override public void flush() throws IOException { - super.flush(); - if (LOG.isDebugEnabled()) { - LOG.debug("Resetting permissions to '" + permissions + "'"); - } - if (!Shell.WINDOWS) { - Files.setPosixFilePermissions(Paths.get(file.getCanonicalPath()), - permissions); - } else { - // FsPermission expects a 10-character string because of the leading - // directory indicator, i.e. "drwx------". The JDK toString method returns - // a 9-character string, so prepend a leading character. - FsPermission fsPermission = FsPermission.valueOf( - "-" + PosixFilePermissions.toString(permissions)); - FileUtil.setPermission(file, fsPermission); + try { + super.getWriteLock().lock(); + file.createNewFile(); + if (LOG.isDebugEnabled()) { + LOG.debug("Resetting permissions to '" + permissions + "'"); + } + if (!Shell.WINDOWS) { + Files.setPosixFilePermissions(Paths.get(file.getCanonicalPath()), + permissions); + } else { + // FsPermission expects a 10-character string because of the leading + // directory indicator, i.e. "drwx------". The JDK toString method returns + // a 9-character string, so prepend a leading character. + FsPermission fsPermission = FsPermission.valueOf( + "-" + PosixFilePermissions.toString(permissions)); + FileUtil.setPermission(file, fsPermission); + } + } finally { + super.getWriteLock().unlock(); } + super.flush(); Review Comment: ## My initial assumptions were: If we fail to set permissions, I would expect an `IOError` or the like to interrupt execution and for us to not get to the flush call. I am a bit novice to Java ReadWriteLocks, and was thinking we could deadlock if we do not release the write lock as flush in the [super class](https://github.com/apache/hadoop/blob/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/alias/AbstractJavaKeyStoreProvider.java#L285) also attempts to acquire the writeLock. Lastly, I was thinking nothing in Hadoop which would be setting more permissive permissions on this file. ## My updated understanding thanks to your question: I think the locks are handled per-thread though and not per-scope reading the [JavaDocs](https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/locks/ReentrantReadWriteLock.html) for ReentrantReadWriteLock. And it appears one can acquire a write lock multiple times, so I think flush can move inside the initial lock. Further, in reading the JavaDocs and other uses in Hadoop, it looks like the write lock acquisition should be outside the try/finally block. Code updated as such. > vulnerability: we may leak sensitive information in LocalKeyStoreProvider > -------------------------------------------------------------------------- > > Key: HADOOP-18235 > URL: https://issues.apache.org/jira/browse/HADOOP-18235 > Project: Hadoop Common > Issue Type: Bug > Reporter: lujie > Assignee: Clay B. > Priority: Critical > Labels: pull-request-available > > Currently, we implement flush like: > {code:java} > // public void flush() throws IOException { > super.flush(); > if (LOG.isDebugEnabled()) { > LOG.debug("Resetting permissions to '" + permissions + "'"); > } > if (!Shell.WINDOWS) { > Files.setPosixFilePermissions(Paths.get(file.getCanonicalPath()), > permissions); > } else { > // FsPermission expects a 10-character string because of the leading > // directory indicator, i.e. "drwx------". The JDK toString method > returns > // a 9-character string, so prepend a leading character. > FsPermission fsPermission = FsPermission.valueOf( > "-" + PosixFilePermissions.toString(permissions)); > FileUtil.setPermission(file, fsPermission); > } > } {code} > we wirite the Credential first, then set permission. > The correct order is setPermission first, then write Credential . > Otherswise, we may leak Credential . For example, the origin perms of file is > 755(default on linux), when the Credential is flushed, Credential can be > leaked when > > 1)between flush and setPermission, others have a chance to access the file. > 2) CredentialShell(or the machine node ) crash between flush and > setPermission, the file permission is 755 for ever before we run the > CredentialShell again. > -- This message was sent by Atlassian Jira (v8.20.10#820010) --------------------------------------------------------------------- To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org