[icedtea-web] RFC: reduce permissions on created files
Deepak Bhole
dbhole at redhat.com
Mon Nov 22 12:31:22 PST 2010
* Omair Majid <omajid at redhat.com> [2010-11-12 09:46]:
> On 11/12/2010 06:54 AM, Dr Andrew John Hughes wrote:
> >On 20:14 Thu 11 Nov , Omair Majid wrote:
> >>Hi,
> >>
> >>The attached patch tries to make files created by netx/plugin more
> >>secure by removing unnecessary permissions. IcedTea6 used to carry a
> >>patch to change the umask used by the javaws process which IcedTea-Web
> >>does not. This patch tries to make netx/plugin behave like they would in
> >>the presence of such a patch.
> >>
> >>This patch does not change the file permissions on files that are cached
> >>(mostly because I dont see why they should be protected), but does
> >>change permissions on KeyStores, native directories created under /tmp/,
> >>lock files, files created through the JNLP api and log files.
> >>
> >
> >I think this is a better approach, though we should be aware that the permissions
> >are now changed after the file has been created, whereas before (I presume) they
> >were created with the correct permissions to begin with.
> >
>
> Hm... Does creating a new (blank) file, setting appropriate
> permissions on it and then writing content to it ensure that the
> content can not be seen by others? This was the original intent with
> this patch (I have since noticed one mistake in the patch). If
> creating a blank file, setting permissions on it and then writing
> actual content does not ensure the confidentiality of the data, then
> I would like to find another way to accomplish this.
>
> Thanks for looking over the patch.
>
Good point Andrew! This patch needs to be modified. If a file is created
with og+[r|w|x] and then restricted, anything that opened the handle
before the change will have the permissions it did when the handle was
opened. We need to create the file under a different (temp) name, change
permissions, and then move it. AFAIK that shouldn't allow access.
Access bypass can be verified as follows:
1. Create a file as one user (A), allow a+r
2. tail -f that file as another user (B)
3. As A, og-rwx the file
4. As A, write to the file
The tail -f command from #2 will show the output, thus confirming that
user B has access when they shouldn't.
tail -f no longer gets the info if the file is renamed to something else
between step 3 and 4.
Cheers,
Deepak
> Cheers,
> Omair
>
> >>ChangeLog:
> >>2010-11-11 Omair Majid<omajid at redhat.com>
> >>
> >> * netx/net/sourceforge/jnlp/util/FileUtils.java
> >> (restrictFile): New method. Removes extra permissions on a file.
> >> * netx/net/sourceforge/jnlp/Launcher.java
> >> (markNetxRunning): Do not grant unnecessary file permissions.
> >> * netx/net/sourceforge/jnlp/security/viewer/CertificatePane.java
> >> (ImportButtonListener.actionPerformed): Likewise.
> >> (RemoveButtonListener.actionPerformed): Likewise.
> >> * netx/net/sourceforge/jnlp/services/SingleInstanceLock.java
> >> (createWithPort): Likewise.
> >> (getLockFile): Likewise.
> >> * netx/net/sourceforge/jnlp/services/XExtendedService.java
> >> (openFile): Likewise.
> >> * netx/net/sourceforge/jnlp/services/XFileSaveService.java
> >> (writeToFile): Likewise.
> >> * netx/net/sourceforge/jnlp/services/XPersistenceService.java
> >> (create): Likewise.
> >> * netx/net/sourceforge/jnlp/util/XDesktopEntry.java
> >> (installDesktopLauncher): Likewise.
> >> * plugin/icedteanp/java/sun/applet/PluginMain.java
> >> (PluginMain): Likewise.
> >>
> >>Any thoughts or comments?
> >>
> >>Cheers,
> >>Omair
> >
> >>diff -r e82455c47f08 netx/net/sourceforge/jnlp/Launcher.java
> >>--- a/netx/net/sourceforge/jnlp/Launcher.java Thu Nov 11 11:43:13 2010 -0500
> >>+++ b/netx/net/sourceforge/jnlp/Launcher.java Thu Nov 11 19:56:02 2010 -0500
> >>@@ -48,6 +48,7 @@
> >> import net.sourceforge.jnlp.runtime.JNLPRuntime;
> >> import net.sourceforge.jnlp.services.InstanceExistsException;
> >> import net.sourceforge.jnlp.services.ServiceUtil;
> >>+import net.sourceforge.jnlp.util.FileUtils;
> >> import net.sourceforge.jnlp.util.Reflect;
> >>
> >> import javax.swing.SwingUtilities;
> >>@@ -729,6 +730,7 @@
> >> .getProperty(DeploymentConfiguration.KEY_USER_NETX_RUNNING_FILE));
> >> netxRunningFile.getParentFile().mkdirs();
> >> if (netxRunningFile.createNewFile()) {
> >>+ FileUtils.restrictFile(netxRunningFile, true);
> >> FileOutputStream fos = new FileOutputStream(netxRunningFile);
> >> try {
> >> fos.write(message.getBytes());
> >>diff -r e82455c47f08 netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
> >>--- a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java Thu Nov 11 11:43:13 2010 -0500
> >>+++ b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java Thu Nov 11 19:56:02 2010 -0500
> >>@@ -815,7 +815,10 @@
> >> nativeDir = getNativeDir();
> >>
> >> File outFile = new File(nativeDir, name);
> >>-
> >>+ if (!outFile.isFile()&& !outFile.createNewFile()) {
> >>+ throw new IOException("unable to create " + outFile);
> >>+ }
> >>+ FileUtils.restrictFile(outFile, true);
> >> CacheUtil.streamCopy(jarFile.getInputStream(e),
> >> new FileOutputStream(outFile));
> >>
> >>@@ -842,6 +845,7 @@
> >> else {
> >> // add this new native directory to the search path
> >> addNativeDirectory(nativeDir);
> >>+ FileUtils.restrictFile(nativeDir, true);
> >> return nativeDir;
> >> }
> >> }
> >>diff -r e82455c47f08 netx/net/sourceforge/jnlp/security/CertWarningPane.java
> >>--- a/netx/net/sourceforge/jnlp/security/CertWarningPane.java Thu Nov 11 11:43:13 2010 -0500
> >>+++ b/netx/net/sourceforge/jnlp/security/CertWarningPane.java Thu Nov 11 19:56:02 2010 -0500
> >>@@ -47,6 +47,7 @@
> >> import java.awt.GridLayout;
> >> import java.awt.event.ActionEvent;
> >> import java.awt.event.ActionListener;
> >>+import java.io.File;
> >> import java.io.FileOutputStream;
> >> import java.io.OutputStream;
> >> import java.security.KeyStore;
> >>@@ -68,6 +69,7 @@
> >> import net.sourceforge.jnlp.security.KeyStores.Level;
> >> import net.sourceforge.jnlp.security.KeyStores.Type;
> >> import net.sourceforge.jnlp.security.SecurityWarning.AccessType;
> >>+import net.sourceforge.jnlp.util.FileUtils;
> >>
> >> /**
> >> * Provides the panel for using inside a SecurityWarningDialog. These dialogs are
> >>@@ -246,7 +248,13 @@
> >> KeyStore ks = KeyStores.getKeyStore(Level.USER, Type.CERTS);
> >> X509Certificate c = (X509Certificate) parent.getJarSigner().getPublisher();
> >> CertificateUtils.addToKeyStore(c, ks);
> >>- OutputStream os = new FileOutputStream(KeyStores.getKeyStoreLocation(Level.USER, Type.CERTS));
> >>+ File keyStoreFile = new File(KeyStores.getKeyStoreLocation(Level.USER, Type.CERTS));
> >>+ if (!keyStoreFile.isFile()) {
> >>+ keyStoreFile.createNewFile();
> >>+ }
> >>+ FileUtils.restrictFile(keyStoreFile, true);
> >>+
> >>+ OutputStream os = new FileOutputStream(keyStoreFile);
> >> ks.store(os, KeyStores.getPassword());
> >> if (JNLPRuntime.isDebug()) {
> >> System.out.println("certificate is now permanently trusted");
> >>diff -r e82455c47f08 netx/net/sourceforge/jnlp/security/KeyStores.java
> >>--- a/netx/net/sourceforge/jnlp/security/KeyStores.java Thu Nov 11 11:43:13 2010 -0500
> >>+++ b/netx/net/sourceforge/jnlp/security/KeyStores.java Thu Nov 11 19:56:02 2010 -0500
> >>@@ -53,6 +53,7 @@
> >> import net.sourceforge.jnlp.runtime.DeploymentConfiguration;
> >> import net.sourceforge.jnlp.runtime.JNLPRuntime;
> >> import net.sourceforge.jnlp.runtime.Translator;
> >>+import net.sourceforge.jnlp.util.FileUtils;
> >>
> >> /**
> >> * The<code>KeyStores</code> class allows easily accessing the various KeyStores
> >>@@ -344,6 +345,8 @@
> >> FileOutputStream fos = new FileOutputStream(file);
> >> ks.store(fos, password.toCharArray());
> >> fos.close();
> >>+
> >>+ FileUtils.restrictFile(file, true);
> >> }
> >>
> >> // TODO catch exception when password is incorrect and prompt user
> >>diff -r e82455c47f08 netx/net/sourceforge/jnlp/security/viewer/CertificatePane.java
> >>--- a/netx/net/sourceforge/jnlp/security/viewer/CertificatePane.java Thu Nov 11 11:43:13 2010 -0500
> >>+++ b/netx/net/sourceforge/jnlp/security/viewer/CertificatePane.java Thu Nov 11 19:56:02 2010 -0500
> >>@@ -45,6 +45,7 @@
> >> import java.awt.event.ActionEvent;
> >> import java.awt.event.ActionListener;
> >> import java.awt.event.KeyEvent;
> >>+import java.io.File;
> >> import java.io.FileOutputStream;
> >> import java.io.OutputStream;
> >> import java.io.PrintStream;
> >>@@ -76,6 +77,7 @@
> >> import net.sourceforge.jnlp.security.SecurityUtil;
> >> import net.sourceforge.jnlp.security.SecurityWarningDialog;
> >> import net.sourceforge.jnlp.security.KeyStores.Level;
> >>+import net.sourceforge.jnlp.util.FileUtils;
> >>
> >> public class CertificatePane extends JPanel {
> >>
> >>@@ -361,8 +363,13 @@
> >> try {
> >> KeyStore ks = keyStore;
> >> CertificateUtils.addToKeyStore(chooser.getSelectedFile(), ks);
> >>- OutputStream os = new FileOutputStream(
> >>- KeyStores.getKeyStoreLocation(currentKeyStoreLevel, currentKeyStoreType));
> >>+ File keyStoreFile = new File(KeyStores
> >>+ .getKeyStoreLocation(currentKeyStoreLevel, currentKeyStoreType));
> >>+ if (!keyStoreFile.isFile()) {
> >>+ keyStoreFile.createNewFile();
> >>+ }
> >>+ FileUtils.restrictFile(keyStoreFile, true);
> >>+ OutputStream os = new FileOutputStream(keyStoreFile);
> >> ks.store(os, KeyStores.getPassword());
> >> repopulateTables();
> >> } catch (Exception ex) {
> >>@@ -436,8 +443,14 @@
> >> JOptionPane.YES_NO_OPTION);
> >> if (i == 0) {
> >> keyStore.deleteEntry(alias);
> >>- FileOutputStream fos = new FileOutputStream(
> >>- KeyStores.getKeyStoreLocation(currentKeyStoreLevel, currentKeyStoreType));
> >>+ File keyStoreFile = new File(KeyStores
> >>+ .getKeyStoreLocation(currentKeyStoreLevel, currentKeyStoreType));
> >>+ if (!keyStoreFile.isFile()) {
> >>+ keyStoreFile.createNewFile();
> >>+ }
> >>+ FileUtils.restrictFile(keyStoreFile, true);
> >>+
> >>+ FileOutputStream fos = new FileOutputStream(keyStoreFile);
> >> keyStore.store(fos, KeyStores.getPassword());
> >> fos.close();
> >> }
> >>diff -r e82455c47f08 netx/net/sourceforge/jnlp/services/SingleInstanceLock.java
> >>--- a/netx/net/sourceforge/jnlp/services/SingleInstanceLock.java Thu Nov 11 11:43:13 2010 -0500
> >>+++ b/netx/net/sourceforge/jnlp/services/SingleInstanceLock.java Thu Nov 11 19:56:02 2010 -0500
> >>@@ -67,6 +67,8 @@
> >> */
> >> public void createWithPort(int localPort) throws IOException {
> >>
> >>+ lockFile.createNewFile();
> >>+ FileUtils.restrictFile(lockFile, true);
> >> BufferedWriter lockFileWriter = new BufferedWriter(new FileWriter(lockFile, false));
> >> lockFileWriter.write(String.valueOf(localPort));
> >> lockFileWriter.newLine();
> >>@@ -135,6 +137,9 @@
> >> if (!baseDir.isDirectory()&& !baseDir.mkdirs()) {
> >> throw new RuntimeException(R("RNoLockDir", baseDir));
> >> }
> >>+
> >>+ FileUtils.restrictFile(baseDir, true);
> >>+
> >> String lockFileName = getLockFileName();
> >> File applicationLockFile = new File(baseDir, lockFileName);
> >> return applicationLockFile;
> >>diff -r e82455c47f08 netx/net/sourceforge/jnlp/services/XExtendedService.java
> >>--- a/netx/net/sourceforge/jnlp/services/XExtendedService.java Thu Nov 11 11:43:13 2010 -0500
> >>+++ b/netx/net/sourceforge/jnlp/services/XExtendedService.java Thu Nov 11 19:56:02 2010 -0500
> >>@@ -23,6 +23,7 @@
> >> import javax.jnlp.FileContents;
> >>
> >> import net.sourceforge.jnlp.security.SecurityWarning.AccessType;
> >>+import net.sourceforge.jnlp.util.FileUtils;
> >>
> >> /**
> >> * Implementation of ExtendedService
> >>@@ -36,6 +37,7 @@
> >>
> >> /* FIXME: this opens a file with read/write mode, not just read or write */
> >> if (ServiceUtil.checkAccess(AccessType.READ_FILE, new Object[]{ file.getAbsolutePath() })) {
> >>+ FileUtils.restrictFile(file, true);
> >> return (FileContents) ServiceUtil.createPrivilegedProxy(FileContents.class,
> >> new XFileContents(file));
> >> } else {
> >>diff -r e82455c47f08 netx/net/sourceforge/jnlp/services/XFileSaveService.java
> >>--- a/netx/net/sourceforge/jnlp/services/XFileSaveService.java Thu Nov 11 11:43:13 2010 -0500
> >>+++ b/netx/net/sourceforge/jnlp/services/XFileSaveService.java Thu Nov 11 19:56:02 2010 -0500
> >>@@ -44,6 +44,7 @@
> >> import javax.jnlp.*;
> >>
> >> import net.sourceforge.jnlp.security.SecurityWarning.AccessType;
> >>+import net.sourceforge.jnlp.util.FileUtils;
> >>
> >> import javax.swing.JFileChooser;
> >> import javax.swing.JOptionPane;
> >>@@ -123,6 +124,7 @@
> >> } else {
> >> file.createNewFile();
> >> }
> >>+ FileUtils.restrictFile(file, true);
> >>
> >> if (file.canWrite()) {
> >> FileOutputStream out = new FileOutputStream(file);
> >>diff -r e82455c47f08 netx/net/sourceforge/jnlp/services/XPersistenceService.java
> >>--- a/netx/net/sourceforge/jnlp/services/XPersistenceService.java Thu Nov 11 11:43:13 2010 -0500
> >>+++ b/netx/net/sourceforge/jnlp/services/XPersistenceService.java Thu Nov 11 19:56:02 2010 -0500
> >>@@ -26,6 +26,7 @@
> >> import net.sourceforge.jnlp.*;
> >> import net.sourceforge.jnlp.cache.*;
> >> import net.sourceforge.jnlp.runtime.*;
> >>+import net.sourceforge.jnlp.util.FileUtils;
> >>
> >> /**
> >> * The BasicService JNLP service.
> >>@@ -99,6 +100,8 @@
> >> if (!file.createNewFile())
> >> throw new IOException("File already exists.");
> >>
> >>+ FileUtils.restrictFile(file, true);
> >>+
> >> return maxsize;
> >> }
> >>
> >>diff -r e82455c47f08 netx/net/sourceforge/jnlp/util/FileUtils.java
> >>--- a/netx/net/sourceforge/jnlp/util/FileUtils.java Thu Nov 11 11:43:13 2010 -0500
> >>+++ b/netx/net/sourceforge/jnlp/util/FileUtils.java Thu Nov 11 19:56:02 2010 -0500
> >>@@ -72,6 +72,30 @@
> >> }
> >>
> >> /**
> >>+ * Sets the file permissions on a file such that it is not readable or
> >>+ * writable by anyone other than the owner. If writeableByOnwer is false,
> >>+ * even the owner can not write to it. The file must exist before this
> >>+ * method is called.
> >>+ */
> >>+ public static void restrictFile(File file, boolean writableByOwner) {
> >>+
> >>+ file.setExecutable(false, false);
> >>+ file.setReadable(false, false);
> >>+ file.setWritable(false, false);
> >>+
> >>+ file.setReadable(true, true);
> >>+
> >>+ if (writableByOwner) {
> >>+ file.setWritable(true, true);
> >>+ }
> >>+
> >>+ if (file.isDirectory()) {
> >>+ file.setExecutable(true, true);
> >>+ }
> >>+
> >>+ }
> >>+
> >>+ /**
> >> * Returns a String that is suitable for using in GUI elements for
> >> * displaying (long) paths to users.
> >> *
> >>diff -r e82455c47f08 netx/net/sourceforge/jnlp/util/XDesktopEntry.java
> >>--- a/netx/net/sourceforge/jnlp/util/XDesktopEntry.java Thu Nov 11 11:43:13 2010 -0500
> >>+++ b/netx/net/sourceforge/jnlp/util/XDesktopEntry.java Thu Nov 11 19:56:02 2010 -0500
> >>@@ -142,6 +142,10 @@
> >> if (!shortcutFile.getParentFile().isDirectory()&& !shortcutFile.getParentFile().mkdirs()) {
> >> throw new IOException(shortcutFile.getParentFile().toString());
> >> }
> >>+
> >>+ shortcutFile.createNewFile();
> >>+ FileUtils.restrictFile(shortcutFile, true);
> >>+
> >> /*
> >> * Write out a Java String (UTF-16) as a UTF-8 file
> >> */
> >>diff -r e82455c47f08 plugin/icedteanp/java/sun/applet/PluginMain.java
> >>--- a/plugin/icedteanp/java/sun/applet/PluginMain.java Thu Nov 11 11:43:13 2010 -0500
> >>+++ b/plugin/icedteanp/java/sun/applet/PluginMain.java Thu Nov 11 19:56:02 2010 -0500
> >>@@ -77,6 +77,7 @@
> >>
> >> import net.sourceforge.jnlp.runtime.DeploymentConfiguration;
> >> import net.sourceforge.jnlp.runtime.JNLPRuntime;
> >>+import net.sourceforge.jnlp.util.FileUtils;
> >>
> >> /**
> >> * The main entry point into PluginAppletViewer.
> >>@@ -130,8 +131,12 @@
> >> try {
> >> File errFile = new File(logDir, PLUGIN_STDERR_FILE);
> >> errFile.getParentFile().mkdirs();
> >>+ errFile.createNewFile();
> >>+ FileUtils.restrictFile(errFile, true);
> >> File outFile = new File(logDir, PLUGIN_STDOUT_FILE);
> >> outFile.getParentFile().mkdirs();
> >>+ outFile.createNewFile();
> >>+ FileUtils.restrictFile(outFile, true);
> >>
> >> System.setErr(new TeeOutputStream(new FileOutputStream(errFile), System.err));
> >> System.setOut(new TeeOutputStream(new FileOutputStream(outFile), System.out));
> >
> >
>
More information about the distro-pkg-dev
mailing list