[
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: [email protected]
For additional commands, e-mail: [email protected]
