db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Knut Anders Hatlen (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (DERBY-5363) Tighten default permissions of DB files with >= JDK6
Date Wed, 21 Sep 2011 12:11:09 GMT

    [ 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

        

Mime
View raw message