You are viewing a plain text version of this content. The canonical link for it is here.
Posted to common-issues@hadoop.apache.org by "ASF GitHub Bot (Jira)" <ji...@apache.org> on 2023/05/10 19:11:00 UTC

[jira] [Commented] (HADOOP-18235) vulnerability: we may leak sensitive information in LocalKeyStoreProvider

    [ https://issues.apache.org/jira/browse/HADOOP-18235?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17721517#comment-17721517 ] 

ASF GitHub Bot commented on HADOOP-18235:
-----------------------------------------

arp7 commented on code in PR #4998:
URL: https://github.com/apache/hadoop/pull/4998#discussion_r1190300509


##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/alias/LocalKeyStoreProvider.java:
##########
@@ -142,20 +142,26 @@ 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);
+    super.getWriteLock().lock();
+    try {
+      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);
+      }

Review Comment:
   @saxenapranav I don't believe this is an issue. If this process has successfully got a write handle then it is assumed no one else is actively writing to the file.





> 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-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org