[GitHub] [hadoop] cbaenziger commented on a diff in pull request #4998: HADOOP-18235. vulnerability: we may leak sensitive information in LocalKeyStoreProvider

[GitBox <gi...@apache.org>]

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.



-- 
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: common-issues-unsubscribe@hadoop.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org