Skip to content

HADOOP-18235. vulnerability: we may leak sensitive information in LocalKeyStoreProvider #4998

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
cbaenziger wants to merge 2 commits into from
Closed

HADOOP-18235. vulnerability: we may leak sensitive information in LocalKeyStoreProvider #4998

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b408f76
HADOOP-18235. vulnerability: we may leak sensitive information in Loc…
cbaenziger Oct 9, 2022
5d04590
Correct lock and flush scoping
cbaenziger Oct 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 20 additions & 14 deletions ...t/hadoop-common/src/main/java/org/apache/hadoop/security/alias/LocalKeyStoreProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Contributor

@steveloughran steveloughran Jan 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for the super. prefix here

but: we do now require lock() to be reentrant

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);
}
Copy link
Contributor

@saxenapranav saxenapranav Feb 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the method getOutputStreamForKeystore(), before sending outputStream, should it be checked that the file is empty. Reason being, between creatingFile and setting permissions, there could be that some other process puts something in the file.

Copy link
Contributor

@arp7 arp7 May 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

@saxenapranav saxenapranav May 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

super.flush();
} finally {
super.getWriteLock().unlock();
}
}

Expand Down