[
https://issues.apache.org/jira/browse/HADOOP-18235?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17721613#comment-17721613
]
ASF GitHub Bot commented on HADOOP-18235:
-----------------------------------------
saxenapranav commented on code in PR #4998:
URL: https://github.com/apache/hadoop/pull/4998#discussion_r1190612252
##########
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:
I mean to say is what if some other process writes into the file between
`file.createNewFile()` and `FileUtil.setPermission(file, fsPermission);`. In
that case, the file would be having corrupted data. Kindly correct me if it
looks wrong. Thanks.
@arp7
> 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: [email protected]
For additional commands, e-mail: [email protected]
