[ https://issues.apache.org/jira/browse/DERBY-5363?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13109425#comment-13109425 ] Knut Anders Hatlen commented on DERBY-5363: ------------------------------------------- Thanks for the new patch, Dag. I took a look at it (mainly at the tests), and I have some comments below, most of them just minor issues. The tests look extensive and seem to cover most cases. I couldn't think of anything obvious that was missing. Except, perhaps, temporary files under tmp (only the permissions on the tmp directory itself are checked). * StreamFileContainer.java: }catch( PrivilegedActionException pae) { - // method executed under this priveleged block - // does not throw an exception - return false; + throw (SecurityException)pae.getCause(); } Since PrivilegedActionException only wraps checked exceptions, and SecurityException is a subclass of RuntimeException, I think this code will cause a ClassCastException whenever it's executed. * JVMInfo.java: I haven't tested, but I suspect JDK 8 will still be identified as Java 6, unless we also change the catch-all clause: if (Float.parseFloat(javaVersion) > 1.6f) id = J2SE_16; * RestrictiveFilePermissionsTest.java: - I was a little surprised that no security manager magic was needed to read this system property, like we have to do other places in the tests: + final static String pathSep = System.getProperty("file.separator"); Maybe it's because the class is loaded before the security manager is installed? The constants File.separator or File.separatorChar could also be used, without any security manager concerns. - Would it make sense to rename the test cases that require special setup to have another prefix than "test"? Then the majority of test cases could be added in suite() simply by doing ts.addTestSuite(RestrictiveFilePermissionsTest.class), and adding new test cases would also be easier. - Perhaps exclude the two lax cases in suite() if lax testing isn't supported, instead of inside the test cases themselves? Then it's easier to see from the test logs if the test cases actually ran. - In suite(), it would be better to declare it as throws Exception and avoid swallowing the stack trace of the underlying exception, like here: + try { + supportsLaxTesting = + checkAccessToOwner( + null, + f, + false, + UNKNOWN); + } catch (Exception e) { + fail("Error during suite setup: " + e); + } - Similarly, in checkAccessToOwner(): + } catch (NoSuchMethodException e) { + Assert.fail(); + } catch (ClassNotFoundException e) { + Assert.fail(); + } Here, either let the unexpected exceptions propagate up to the JUnit framework, or call BaseTestCase.fail(String, Throwable) to preserve the cause. - There should be a space after "in" in this message: + if (!supportsLaxTesting) { + println("warning: testing of lax file permissions in" + + "RestrictiveFilePermissionsTest can not take place, " + + "use a more liberal runtime default (umask) for the tests"); + } - Many of the test cases contain code similar to this: + Connection c = getConnection(); + Statement s = c.createStatement(); ... + PreparedStatement ps = c.prepareStatement( + "insert into lobtable values (1,?)"); ... + CallableStatement cs = c.prepareCall + ("CALL SYSCS_UTIL.SYSCS_BACKUP_DATABASE(?)"); It's probably better to use the helper methods in BaseJDBCTestCase to create the various statements, so that the statements are closed after use. - testCliServerIsRestrictive(): Just curious... I thought assertDirectoryDeleted() already handled the cases where the file handles weren't closed just yet, so that sleeping shouldn't be necessary? + Thread.sleep(2000); + // ..so hopefully server will have closed files handles before + // we try to delete files: + assertDirectoryDeleted(traceDirF); - exists(): Use existing helper method in PrivilegedFileOpsForTests instead? * NetworkServerTestSetup.java: Spurious white-space diff? > Tighten default permissions of DB files with >= JDK6 > ---------------------------------------------------- > > Key: DERBY-5363 > URL: https://issues.apache.org/jira/browse/DERBY-5363 > Project: Derby > Issue Type: Improvement > Components: Miscellaneous, Services, Store > Reporter: Dag H. Wanvik > Assignee: Dag H. Wanvik > Attachments: derby-5363-basic-1.diff, derby-5363-basic-1.stat, derby-5363-basic-2.diff, derby-5363-basic-2.stat, derby-5363-basic-3.diff, derby-5363-basic-3.stat, derby-5363-full-1.diff, derby-5363-full-1.stat, derby-5363-full-2.diff, derby-5363-full-2.stat, derby-5363-full-3.diff, derby-5363-full-3.stat, derby-5363-server-1.diff, permission-5.diff, permission-5.stat, permission-6.diff, permission-6.stat, property-table.png, releaseNote.html, releaseNote.html, releaseNote.html, releaseNote.html, z.sql > > > Before Java 6, files created by Derby would have the default > permissions of the operating system context. Under Unix, this would > depend on the effective umask of the process that started the Java VM. > In Java 6 and 7, there are methods available that allows tightening up this > (File.setReadable, setWritable), making it less likely that somebody > would accidentally run Derby with a too lenient default. > I suggest we take advantage of this, and let Derby by default (in Java > 6 and higher) limit the visibility to the OS user that starts the VM, > e.g. on Unix this would be equivalent to running with umask 0077. More > secure by default is good, I think. > We could have a flag, e.g. "derby.storage.useDefaultFilePermissions" > that when set to true, would give the old behavior. -- This message is automatically generated by JIRA. For more information on JIRA, see: http://www.atlassian.com/software/jira