Derby
  1. Derby
  2. DERBY-5363

Tighten permissions of DB files to owner with >= JDK7

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 10.9.1.0
    • Component/s: Miscellaneous, Services, Store
    • Labels:
      None
    • Issue & fix info:
      Release Note Needed
    • Bug behavior facts:
      Security

      Description

      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.

      1. z.sql
        1 kB
        Rick Hillegas
      2. releaseNote.html
        5 kB
        Dag H. Wanvik
      3. releaseNote.html
        5 kB
        Dag H. Wanvik
      4. releaseNote.html
        5 kB
        Rick Hillegas
      5. releaseNote.html
        5 kB
        Dag H. Wanvik
      6. releaseNote.html
        5 kB
        Dag H. Wanvik
      7. releaseNote.html
        6 kB
        Dag H. Wanvik
      8. releaseNote.html
        6 kB
        Rick Hillegas
      9. property-table.png
        53 kB
        Dag H. Wanvik
      10. permission-6.stat
        2 kB
        Dag H. Wanvik
      11. permission-6.diff
        35 kB
        Dag H. Wanvik
      12. permission-5.stat
        1 kB
        Dag H. Wanvik
      13. permission-5.diff
        19 kB
        Dag H. Wanvik
      14. derby-5363-server-1.diff
        4 kB
        Dag H. Wanvik
      15. derby-5363-limit-to-java7b.stat
        0.1 kB
        Dag H. Wanvik
      16. derby-5363-limit-to-java7b.diff
        2 kB
        Dag H. Wanvik
      17. derby-5363-limit-to-java7.stat
        0.1 kB
        Dag H. Wanvik
      18. derby-5363-limit-to-java7.diff
        2 kB
        Dag H. Wanvik
      19. derby-5363-full-5.stat
        3 kB
        Dag H. Wanvik
      20. derby-5363-full-5.diff
        101 kB
        Dag H. Wanvik
      21. derby-5363-full-4.stat
        3 kB
        Dag H. Wanvik
      22. derby-5363-full-4.diff
        102 kB
        Dag H. Wanvik
      23. derby-5363-full-3.stat
        3 kB
        Dag H. Wanvik
      24. derby-5363-full-3.diff
        107 kB
        Dag H. Wanvik
      25. derby-5363-full-2.stat
        3 kB
        Dag H. Wanvik
      26. derby-5363-full-2.diff
        106 kB
        Dag H. Wanvik
      27. derby-5363-full-1.stat
        3 kB
        Dag H. Wanvik
      28. derby-5363-full-1.diff
        106 kB
        Dag H. Wanvik
      29. derby-5363-followup-unix.stat
        1 kB
        Dag H. Wanvik
      30. derby-5363-followup-unix.diff
        17 kB
        Dag H. Wanvik
      31. derby-5363-followup-unix.diff
        17 kB
        Dag H. Wanvik
      32. derby-5363-followup-linux.diff
        18 kB
        Dag H. Wanvik
      33. derby-5363-followup-linux.diff
        25 kB
        Dag H. Wanvik
      34. derby-5363-followup.diff
        1 kB
        Dag H. Wanvik
      35. derby-5363-basic-3.stat
        2 kB
        Dag H. Wanvik
      36. derby-5363-basic-3.diff
        68 kB
        Dag H. Wanvik
      37. derby-5363-basic-2.stat
        2 kB
        Dag H. Wanvik
      38. derby-5363-basic-2.diff
        70 kB
        Dag H. Wanvik
      39. derby-5363-basic-1.stat
        3 kB
        Dag H. Wanvik
      40. derby-5363-basic-1.diff
        78 kB
        Dag H. Wanvik

        Issue Links

        There are no Sub-Tasks for this issue.

          Activity

          Hide
          Rick Hillegas added a comment -

          This sounds reasonable to me. Might be useful to describe how this would affect legacy applications. Thanks.

          Show
          Rick Hillegas added a comment - This sounds reasonable to me. Might be useful to describe how this would affect legacy applications. Thanks.
          Hide
          Dag H. Wanvik added a comment -

          With JDK 7, more is available to control permissions for POSIX-like file systems and ACLs as in NTFS, cf. the tutorial here:

          http://download.oracle.com/javase/tutorial/essential/io/fileAttr.html

          This page:

          http://download.oracle.com/javase/tutorial/essential/io/legacy.html

          shows that the java.io.File#

          {setWritable,setReadable}

          made available in Java 6 are now superceded in Java 7 by the new metadata
          stuff. Cf the package java.nio.file.attribute and java.nio.file.*.
          We may want to consider the opportunities opened by this instead of going with the limited facilities in Java 6.

          Show
          Dag H. Wanvik added a comment - With JDK 7, more is available to control permissions for POSIX-like file systems and ACLs as in NTFS, cf. the tutorial here: http://download.oracle.com/javase/tutorial/essential/io/fileAttr.html This page: http://download.oracle.com/javase/tutorial/essential/io/legacy.html shows that the java.io.File# {setWritable,setReadable} made available in Java 6 are now superceded in Java 7 by the new metadata stuff. Cf the package java.nio.file.attribute and java.nio.file.*. We may want to consider the opportunities opened by this instead of going with the limited facilities in Java 6.
          Hide
          Dag H. Wanvik added a comment -

          It seems on Windows, the Java 6 API can not be used to limit read access:

          http://java.sun.com/developer/technicalArticles/J2SE/Desktop/javase6/enhancements :

          (quote):
          "setReadable(false) returns false - File readability cannot be set to false in Windows"

          Show
          Dag H. Wanvik added a comment - It seems on Windows, the Java 6 API can not be used to limit read access: http://java.sun.com/developer/technicalArticles/J2SE/Desktop/javase6/enhancements : (quote): "setReadable(false) returns false - File readability cannot be set to false in Windows"
          Hide
          Dag H. Wanvik added a comment -

          Here is a proof-of-concept patch that works on my Unix (Solaris 11) box with JDK >=6. Not for commit.

          Show
          Dag H. Wanvik added a comment - Here is a proof-of-concept patch that works on my Unix (Solaris 11) box with JDK >=6. Not for commit.
          Hide
          Rick Hillegas added a comment - - edited

          Thanks for the patch, Dag.

          Attaching z.sql. This is a script which creates a database named db2, exports a table, performs a sort, and exits prematurely, leaving a tmp directory and database lock files hanging around. On my Mac OSC laptop, I have run this script with the trunk (after applying the permission-5 patch) and with 10.8.1.2.

          Here's what the permissions look like for the trunk with the patch applied:

          drwxr-xr-x 8 rh161140 rh161140 272 Aug 9 08:44 db2
          rw------ 1 rh161140 rh161140 611 Aug 9 08:44 derby.log
          total 24
          rw------ 1 rh161140 rh161140 38 Aug 9 08:44 db.lck
          rw------ 1 rh161140 rh161140 4 Aug 9 08:44 dbex.lck
          drwxr-xr-x 14 rh161140 rh161140 476 Aug 9 08:45 log
          drwxr-xr-x 73 rh161140 rh161140 2482 Aug 9 08:44 seg0
          rw------ 1 rh161140 rh161140 851 Aug 9 08:44 service.properties
          drwxr-xr-x 2 rh161140 rh161140 68 Aug 9 08:45 tmp
          rw------ 1 rh161140 rh161140 8192 Aug 9 08:44 db2/seg0/cf0.dat
          rw------ 1 rh161140 rh161140 4 Aug 9 08:44 /Users/rh161140/junk/z.export

          And here's what the permissions look like for 10.8.1.2:

          drwxr-xr-x 8 rh161140 rh161140 272 Aug 9 08:43 db2
          rw-rr- 1 rh161140 rh161140 604 Aug 9 08:43 derby.log
          total 24
          rw-rr- 1 rh161140 rh161140 38 Aug 9 08:43 db.lck
          rw-rr- 1 rh161140 rh161140 4 Aug 9 08:43 dbex.lck
          drwxr-xr-x 14 rh161140 rh161140 476 Aug 9 08:43 log
          drwxr-xr-x 73 rh161140 rh161140 2482 Aug 9 08:43 seg0
          rw-rr- 1 rh161140 rh161140 851 Aug 9 08:43 service.properties
          drwxr-xr-x 2 rh161140 rh161140 68 Aug 9 08:43 tmp
          rw-rr- 1 rh161140 rh161140 8192 Aug 9 08:43 db2/seg0/cf0.dat
          rw-rr- 1 rh161140 rh161140 4 Aug 9 08:43 /Users/rh161140/junk/z.export

          So it looks as though the permissions on the directories could be tightened up.

          A couple comments on the patch:

          o There are several vacuous implementations of limitAccessToOwner(). This may indicate that the original class factoring is not aligned with what you are trying to do. Probably not worth fixing right now, but might be worth filing a JIRA to fix in the future.

          o FileUtil.limitAccessToOwner() - There are several places where you create new Booleans. To avoid the object creation, you could just use Boolean.TRUE and Boolean.FALSE.

          o FileUtil.checkResult() - Not sure that FileNotFoundException is the only possible problem which could fail this method.

          Thanks,
          -Rick

          Show
          Rick Hillegas added a comment - - edited Thanks for the patch, Dag. Attaching z.sql. This is a script which creates a database named db2, exports a table, performs a sort, and exits prematurely, leaving a tmp directory and database lock files hanging around. On my Mac OSC laptop, I have run this script with the trunk (after applying the permission-5 patch) and with 10.8.1.2. Here's what the permissions look like for the trunk with the patch applied: drwxr-xr-x 8 rh161140 rh161140 272 Aug 9 08:44 db2 rw ------ 1 rh161140 rh161140 611 Aug 9 08:44 derby.log total 24 rw ------ 1 rh161140 rh161140 38 Aug 9 08:44 db.lck rw ------ 1 rh161140 rh161140 4 Aug 9 08:44 dbex.lck drwxr-xr-x 14 rh161140 rh161140 476 Aug 9 08:45 log drwxr-xr-x 73 rh161140 rh161140 2482 Aug 9 08:44 seg0 rw ------ 1 rh161140 rh161140 851 Aug 9 08:44 service.properties drwxr-xr-x 2 rh161140 rh161140 68 Aug 9 08:45 tmp rw ------ 1 rh161140 rh161140 8192 Aug 9 08:44 db2/seg0/cf0.dat rw ------ 1 rh161140 rh161140 4 Aug 9 08:44 /Users/rh161140/junk/z.export And here's what the permissions look like for 10.8.1.2: drwxr-xr-x 8 rh161140 rh161140 272 Aug 9 08:43 db2 rw-r r - 1 rh161140 rh161140 604 Aug 9 08:43 derby.log total 24 rw-r r - 1 rh161140 rh161140 38 Aug 9 08:43 db.lck rw-r r - 1 rh161140 rh161140 4 Aug 9 08:43 dbex.lck drwxr-xr-x 14 rh161140 rh161140 476 Aug 9 08:43 log drwxr-xr-x 73 rh161140 rh161140 2482 Aug 9 08:43 seg0 rw-r r - 1 rh161140 rh161140 851 Aug 9 08:43 service.properties drwxr-xr-x 2 rh161140 rh161140 68 Aug 9 08:43 tmp rw-r r - 1 rh161140 rh161140 8192 Aug 9 08:43 db2/seg0/cf0.dat rw-r r - 1 rh161140 rh161140 4 Aug 9 08:43 /Users/rh161140/junk/z.export So it looks as though the permissions on the directories could be tightened up. A couple comments on the patch: o There are several vacuous implementations of limitAccessToOwner(). This may indicate that the original class factoring is not aligned with what you are trying to do. Probably not worth fixing right now, but might be worth filing a JIRA to fix in the future. o FileUtil.limitAccessToOwner() - There are several places where you create new Booleans. To avoid the object creation, you could just use Boolean.TRUE and Boolean.FALSE. o FileUtil.checkResult() - Not sure that FileNotFoundException is the only possible problem which could fail this method. Thanks, -Rick
          Hide
          Dag H. Wanvik added a comment -

          Thanks, Rick. You are right about the directories: so far I have only tightened up permissions for the files - I need to do the directories too. I'll have a look at the code comments, thx.

          Show
          Dag H. Wanvik added a comment - Thanks, Rick. You are right about the directories: so far I have only tightened up permissions for the files - I need to do the directories too. I'll have a look at the code comments, thx.
          Hide
          Dag H. Wanvik added a comment - - edited

          Uploading the patch "permissions-6" which fixes the booleans mentioned by Rick, and added logic for Windows w/ACLs as well. For that platform, I remove all ACL entries that do not belong to the file owner. If understand this correctly, this will effectively limit access to other principals (users and groups). The patch currently requires a Java 7 compiler, but once the code is finalized, we should probably rewrite this to use reflection, so we won't impose another burden on Derby developers, e.g. Java 7 is not available for the Mac yet. I still need to add logic for the directories.

          As for the class factoring, I am open to suggestions, I found this placement the most logical. The code in some places used java.io.File directly, in other places it uses the abstraction StoragetFile. There is a precedence for vacuous implementations here, see releaseExclusiveFileLock for example. I added a forwarding in CorruptFile for the new method similar to the pattern for the other methods in StorageFile interface.

          As for the boolean result checked in FileUtil.checkResult, the Javadoc says "true if and only if the operation succeeded. The operation will fail if the user does not have permission to change the access permissions of this abstract pathname. If readable is false and the underlying file system does not implement a read permission, then the operation will fail." Presumably, since we created the file, we have permissions to change permissions on it. We skip "setReadable" for Windows, so we should not see that either. I chose to use FileNotFoundException to avoid polluting the Derby io code with more checked exceptions since this should not happen for ordinary user operation. Slightly more sneaky is the fact I use FileNotFoundException to wrap Java security access violation: for NTFS/Java 7 the method Files#getOwner needs the RuntimePermission "accessUserInformation", and if the application is running with a security manager that permission needs to be added to the code base. I'll see if I can make this error stand out better. I added this permission to the defautl policy and sample, cf. the patch "permission-6".

          The patch is not ready for commit yet, but feel free to review it. Running regressions now.

          Show
          Dag H. Wanvik added a comment - - edited Uploading the patch "permissions-6" which fixes the booleans mentioned by Rick, and added logic for Windows w/ACLs as well. For that platform, I remove all ACL entries that do not belong to the file owner. If understand this correctly, this will effectively limit access to other principals (users and groups). The patch currently requires a Java 7 compiler, but once the code is finalized, we should probably rewrite this to use reflection, so we won't impose another burden on Derby developers, e.g. Java 7 is not available for the Mac yet. I still need to add logic for the directories. As for the class factoring, I am open to suggestions, I found this placement the most logical. The code in some places used java.io.File directly, in other places it uses the abstraction StoragetFile. There is a precedence for vacuous implementations here, see releaseExclusiveFileLock for example. I added a forwarding in CorruptFile for the new method similar to the pattern for the other methods in StorageFile interface. As for the boolean result checked in FileUtil.checkResult, the Javadoc says "true if and only if the operation succeeded. The operation will fail if the user does not have permission to change the access permissions of this abstract pathname. If readable is false and the underlying file system does not implement a read permission, then the operation will fail." Presumably, since we created the file, we have permissions to change permissions on it. We skip "setReadable" for Windows, so we should not see that either. I chose to use FileNotFoundException to avoid polluting the Derby io code with more checked exceptions since this should not happen for ordinary user operation. Slightly more sneaky is the fact I use FileNotFoundException to wrap Java security access violation: for NTFS/Java 7 the method Files#getOwner needs the RuntimePermission "accessUserInformation", and if the application is running with a security manager that permission needs to be added to the code base. I'll see if I can make this error stand out better. I added this permission to the defautl policy and sample, cf. the patch "permission-6". The patch is not ready for commit yet, but feel free to review it. Running regressions now.
          Hide
          Rick Hillegas added a comment -

          Hi Dag,

          I tried building the patch on Ubuntu using the Oracle jdk1.7.0-143 compiler. The build failed as follows. Not sure if this is worth pursuing since you plan to rework the code to use reflection rather than compiled links to the Java 7 features.

           [echo] Before setting properties: compilerPropsAlreadySet = $

          {compilerPropsAlreadySet}

          [echo] Before setting properties: compilerLevel16 = 1.6
          [echo] Before setting properties: jsr169compile.classpath = /mac-home/sw/phoneME/phoneme_advanced_mr2/lib/btclasses.zip:/mac-home/sw/phoneME/phoneme_advanced_mr2/lib/basis.jar:/mac-home/sw/jsr169Support/jdbc.jar
          [echo] Before setting properties: j14lib = $

          {j14lib}

          [echo] Before setting properties: java14compile.classpath = $

          {java14compile.classpath}

          [echo] Before setting properties: j15lib = $

          {j15lib}

          [echo] Before setting properties: java15compile.classpath = $

          {java15compile.classpath}

          [echo] Before setting properties: j16lib = $

          {j16lib}

          [echo] Before setting properties: java16compile.classpath = $

          {java16compile.classpath}

          [echo] Before setting properties: j17lib = $

          {j17lib}

          [echo] Before setting properties: java17compile.classpath = $

          {java17compile.classpath}

          BUILD FAILED
          /mac-home/derby/mainline/trunk/build.xml:278: Don't know how to set java15compile.classpath, java16compile.classpath using this environment:

          java.vendor = Oracle Corporation
          java.home = /home/rhillegas/sw/java/oracle/jdk1.7.0-143/jre
          java.version = 1.7.0-ea
          os.name = Linux
          j14lib = null
          j15lib = null
          j16lib = null
          jdkSearchPath = /home/rhillegas/sw/java/oracle

          Please consult BUILDING.html for instructions on how to set the compiler-classpath properties.

          Show
          Rick Hillegas added a comment - Hi Dag, I tried building the patch on Ubuntu using the Oracle jdk1.7.0-143 compiler. The build failed as follows. Not sure if this is worth pursuing since you plan to rework the code to use reflection rather than compiled links to the Java 7 features.  [echo] Before setting properties: compilerPropsAlreadySet = $ {compilerPropsAlreadySet} [echo] Before setting properties: compilerLevel16 = 1.6 [echo] Before setting properties: jsr169compile.classpath = /mac-home/sw/phoneME/phoneme_advanced_mr2/lib/btclasses.zip:/mac-home/sw/phoneME/phoneme_advanced_mr2/lib/basis.jar:/mac-home/sw/jsr169Support/jdbc.jar [echo] Before setting properties: j14lib = $ {j14lib} [echo] Before setting properties: java14compile.classpath = $ {java14compile.classpath} [echo] Before setting properties: j15lib = $ {j15lib} [echo] Before setting properties: java15compile.classpath = $ {java15compile.classpath} [echo] Before setting properties: j16lib = $ {j16lib} [echo] Before setting properties: java16compile.classpath = $ {java16compile.classpath} [echo] Before setting properties: j17lib = $ {j17lib} [echo] Before setting properties: java17compile.classpath = $ {java17compile.classpath} BUILD FAILED /mac-home/derby/mainline/trunk/build.xml:278: Don't know how to set java15compile.classpath, java16compile.classpath using this environment: java.vendor = Oracle Corporation java.home = /home/rhillegas/sw/java/oracle/jdk1.7.0-143/jre java.version = 1.7.0-ea os.name = Linux j14lib = null j15lib = null j16lib = null jdkSearchPath = /home/rhillegas/sw/java/oracle Please consult BUILDING.html for instructions on how to set the compiler-classpath properties.
          Hide
          Dag H. Wanvik added a comment -

          Yes, I'll probaby remove the property setter code from the patch if we go with the reflection code, but I'll try this on Linux anyway; would be nice to understand why it fails; we'll need to add support for Java 7 at some point.

          The regressions showed some errors: in at least two cases there were instances for security policies I hadn't updated with the new Runtimepermission required ("accessUserInformation") needed to figure out the file's owner under NTFS. I'll make a new patch soon.

          Show
          Dag H. Wanvik added a comment - Yes, I'll probaby remove the property setter code from the patch if we go with the reflection code, but I'll try this on Linux anyway; would be nice to understand why it fails; we'll need to add support for Java 7 at some point. The regressions showed some errors: in at least two cases there were instances for security policies I hadn't updated with the new Runtimepermission required ("accessUserInformation") needed to figure out the file's owner under NTFS. I'll make a new patch soon.
          Hide
          Kristian Waagan added a comment -

          Rick,

          I looks to me like you used a Java 7 early access build - these builds are ignored by the PropertySetter.isValidVersion().
          If you specify -DprintCompilerProperties[Verbose]=true this should be reported.

          I tested the patch on OpenSUSE 11.4 and it worked when setting JAVA_HOME=jdk1.7. In this case Java 6 was also available.
          If run it with only Java 7 available, the build fails. Is this as intended, or do we want to set the Java 6 compile classpath using Java 7 (as we do for Java 5.0 if only Java 6 is available)?

          Show
          Kristian Waagan added a comment - Rick, I looks to me like you used a Java 7 early access build - these builds are ignored by the PropertySetter.isValidVersion(). If you specify -DprintCompilerProperties [Verbose] =true this should be reported. I tested the patch on OpenSUSE 11.4 and it worked when setting JAVA_HOME=jdk1.7. In this case Java 6 was also available. If run it with only Java 7 available, the build fails. Is this as intended, or do we want to set the Java 6 compile classpath using Java 7 (as we do for Java 5.0 if only Java 6 is available)?
          Hide
          Dag H. Wanvik added a comment -

          Thanks for finding this , Kristian. I would think we could set the Java 6 compile classpath using Java 7. I'll add that unless somebody has misgivings. If we omit this extension of the property setter for this issue (as seems likely), I'll make a new issue and attach the preliminary patch of the Java 7 enabling to that so we can save it for later.

          Show
          Dag H. Wanvik added a comment - Thanks for finding this , Kristian. I would think we could set the Java 6 compile classpath using Java 7. I'll add that unless somebody has misgivings. If we omit this extension of the property setter for this issue (as seems likely), I'll make a new issue and attach the preliminary patch of the Java 7 enabling to that so we can save it for later.
          Hide
          Kathey Marsden added a comment -

          I am just back from vacation and looking at this issue, so my apologies for not speaking up sooner, but I am quite concerned about having a new more restrictive default. I can recall conversations with quite a few different development groups around the requirement that multiple users be able to access the database. I have always told them that as long as the users are in the same group and umask is set appropriately this should work fine. I am concerned about breaking those applications with this change. I think with embedded it is fairly common to have multiple users accessing the database and think the default should be the old behavior and allow the more restrictive file permissions with an option.

          Show
          Kathey Marsden added a comment - I am just back from vacation and looking at this issue, so my apologies for not speaking up sooner, but I am quite concerned about having a new more restrictive default. I can recall conversations with quite a few different development groups around the requirement that multiple users be able to access the database. I have always told them that as long as the users are in the same group and umask is set appropriately this should work fine. I am concerned about breaking those applications with this change. I think with embedded it is fairly common to have multiple users accessing the database and think the default should be the old behavior and allow the more restrictive file permissions with an option.
          Hide
          Dag H. Wanvik added a comment -

          I agree this behavior could be too restrictive for embedded usage. Do you think it is more reasonable in a server context, Kathey? I think it would be good to improve our "secure by default" story a bit these days...

          Show
          Dag H. Wanvik added a comment - I agree this behavior could be too restrictive for embedded usage. Do you think it is more reasonable in a server context, Kathey? I think it would be good to improve our "secure by default" story a bit these days...
          Hide
          Kathey Marsden added a comment -

          I think not with the API which is normally used for embedded server scenarios, but perhaps for the command line start up where we also start a security manager and try to be more secure by default. I can think of at least one product that requires multiple users to be able to start Network Server, but I am pretty sure they use the API. I will check. It might be good to check with the user list too.

          Would it be possible to make the enhanced restrictions only occur on new databases and ones that have been created with the restrictions? The thing that makes me most wary about messing with permissions is that the errors that users get with mixed permissions are pretty ugly, like container cannot be opened or can't read some specific transaction log file during recovery. We sometimes see these errors now with an existing database with liberal permission is accessed by a new user with more restrictive umask and then opened by another user who can't access the new files. What needs to be done is some chmods and adjust the umask of the secondary user to fix it up, but unfortunately often, by the time I see it, somebody has gotten interested in those log and seg0 directories and deleted something corrupting the database. If the new default permissions were to take effect with preexisting databases, we might see this scenario more often.

          Show
          Kathey Marsden added a comment - I think not with the API which is normally used for embedded server scenarios, but perhaps for the command line start up where we also start a security manager and try to be more secure by default. I can think of at least one product that requires multiple users to be able to start Network Server, but I am pretty sure they use the API. I will check. It might be good to check with the user list too. Would it be possible to make the enhanced restrictions only occur on new databases and ones that have been created with the restrictions? The thing that makes me most wary about messing with permissions is that the errors that users get with mixed permissions are pretty ugly, like container cannot be opened or can't read some specific transaction log file during recovery. We sometimes see these errors now with an existing database with liberal permission is accessed by a new user with more restrictive umask and then opened by another user who can't access the new files. What needs to be done is some chmods and adjust the umask of the secondary user to fix it up, but unfortunately often, by the time I see it, somebody has gotten interested in those log and seg0 directories and deleted something corrupting the database. If the new default permissions were to take effect with preexisting databases, we might see this scenario more often.
          Hide
          Dag H. Wanvik added a comment - - edited

          Unfortunately, we don't check whether a database exists (in StorageFactoryService#createServiceRoot before we have already created derby.log. So we would have to chose whether to use default restrictive access permission for derby.log. Then, when we have read system.properties we would know whether this database should use restrictive flags or not for the remainder of the booted time.

          So let's see if this behavior would work:

          We have a property, derby.storage.useDefaultFilePermissions, which would only default to 'false' when we started a database server from the command line (along with Java security).

          This means that in this case, derby.log would always get restrictive permission. (To avoid this one would henceforth need to specify the property to be 'true').

          When connecting, if we have a new database, we would continue to use restrictive permissions for that database. We would store this fact in system.properties.

          If we connect to an existing database, we would check system.properties for the presence of derby.storage.useDefaultFilePermissions. If it is not seen (soft or hard upgrade), we would assume the value to be true, and update system.properties with this fact if hard upgrade. If it is seen, we use that value for it.

          So, as far as compatibility, if the property is not specified when starting a server (as would be the case for oblivious users upgrading), the only change seen would be that derby.log would (usually) have more restrictive permissions than earlier - other (new) db files would be created with the old laxer scheme as before.

          Does this sound acceptable?

          Show
          Dag H. Wanvik added a comment - - edited Unfortunately, we don't check whether a database exists (in StorageFactoryService#createServiceRoot before we have already created derby.log. So we would have to chose whether to use default restrictive access permission for derby.log. Then, when we have read system.properties we would know whether this database should use restrictive flags or not for the remainder of the booted time. So let's see if this behavior would work: We have a property, derby.storage.useDefaultFilePermissions, which would only default to 'false' when we started a database server from the command line (along with Java security). This means that in this case, derby.log would always get restrictive permission. (To avoid this one would henceforth need to specify the property to be 'true'). When connecting, if we have a new database, we would continue to use restrictive permissions for that database. We would store this fact in system.properties. If we connect to an existing database, we would check system.properties for the presence of derby.storage.useDefaultFilePermissions. If it is not seen (soft or hard upgrade), we would assume the value to be true, and update system.properties with this fact if hard upgrade. If it is seen, we use that value for it. So, as far as compatibility, if the property is not specified when starting a server (as would be the case for oblivious users upgrading), the only change seen would be that derby.log would (usually) have more restrictive permissions than earlier - other (new) db files would be created with the old laxer scheme as before. Does this sound acceptable?
          Hide
          Kathey Marsden added a comment -

          Hi Dag,

          I was wondering what would happen in practice in the following upgrade scenario:

          1) Assume the site has been starting network server from the command line with various users, relying on umask settings to control permissions.
          2) They upgrade to 10.9 with your changes and of course didn't read the release notes to set the property.
          3) User A starts network server and a client connects creating the new more restrictive derby.log and stops the server.
          4) User B starts network server and a client connects, but presumably can't access derby.log. What kind of error would they get? What recovery steps do they need to take ?

          Show
          Kathey Marsden added a comment - Hi Dag, I was wondering what would happen in practice in the following upgrade scenario: 1) Assume the site has been starting network server from the command line with various users, relying on umask settings to control permissions. 2) They upgrade to 10.9 with your changes and of course didn't read the release notes to set the property. 3) User A starts network server and a client connects creating the new more restrictive derby.log and stops the server. 4) User B starts network server and a client connects, but presumably can't access derby.log. What kind of error would they get? What recovery steps do they need to take ?
          Hide
          Dag H. Wanvik added a comment -

          Right, if the two users relied on group write access to derby.log (as they would have had to to be able to use the same derby.log file earlier), user B who tried to append to the already created derby.log (created by user A), would experience an error, cf below. I.e. the fact that derby.log is not accessible results in this error message on the console:

          Sat Aug 20 00:46:04 CEST 2011 Thread[main,5,main] java.io.FileNotFoundException: derby.log (Ingen tilgang)

          "Ingen adgang": Norwegian for "no access" :.) Derby then proceeds to use the console as error stream.

          The workaround would be to specify -Dderby.error.derby.stream.error.file=<file>, cf. end of enclosed session trace.

          ----------------------------------------------------------------------------
          dags-lenovo:~/java/sb/sb1$ ls -l derby.log
          rwx----- 1 dag None 1079 Aug 10 00:33 derby.log

          dags-lenovo:~/java/sb/sb1$ chmod 000 derby.log

          dags-lenovo:~/java/sb/sb1$ ls -l derby.log
          ---------- 1 dag None 1079 Aug 10 00:33 derby.log

          dags-lenovo:~/java/sb/sb1$ java org.apache.derby.tools.ij
          ij version 10.9
          ij> connect 'jdbc:derby:wombat';
          Sat Aug 20 00:46:04 CEST 2011 Thread[main,5,main] java.io.FileNotFoundException: derby.log (Ingen tilgang)
          Sat Aug 20 00:46:05 CEST 2011 Thread[main,5,main] Cleanup action starting
          java.sql.SQLException: Database 'wombat' not found.
          at org.apache.derby.impl.jdbc.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:98)
          at org.apache.derby.impl.jdbc.Util.newEmbedSQLException(Util.java:142)
          at org.apache.derby.impl.jdbc.Util.newEmbedSQLException(Util.java:148)
          at org.apache.derby.impl.jdbc.Util.generateCsSQLException(Util.java:227)
          at org.apache.derby.impl.jdbc.EmbedConnection.newSQLException(EmbedConnection.java:3085)
          at org.apache.derby.impl.jdbc.EmbedConnection.handleDBNotFound(EmbedConnection.java:735)
          :
          dags-lenovo:~/java/sb/sb1$ java -Dderby.stream.error.file=error.txt org.apache.derby.tools.ij
          ij version 10.9
          ij> connect 'jdbc:derby:wombat';
          ERROR XJ004: Database 'wombat' not found.
          ij> exit;
          dags-lenovo:~/java/sb/sb1$ Use "exit" to leave the shell.
          dags-lenovo:~/java/sb/sb1$ cat error.txt
          Sat Aug 20 00:54:38 CEST 2011 Thread[main,5,main] Cleanup action starting
          java.sql.SQLException: Database 'wombat' not found.
          at org.apache.derby.impl.jdbc.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:98)
          at org.apache.derby.impl.jdbc.Util.newEmbedSQLException(Util.java:142)
          at org.apache.derby.impl.jdbc.Util.newEmbedSQLException(Util.java:148)
          at org.apache.derby.impl.jdbc.Util.generateCsSQLException(Util.java:227)
          at org.apache.derby.impl.jdbc.EmbedConnection.newSQLException(EmbedConnection.java:3085)

          Show
          Dag H. Wanvik added a comment - Right, if the two users relied on group write access to derby.log (as they would have had to to be able to use the same derby.log file earlier), user B who tried to append to the already created derby.log (created by user A), would experience an error, cf below. I.e. the fact that derby.log is not accessible results in this error message on the console: Sat Aug 20 00:46:04 CEST 2011 Thread [main,5,main] java.io.FileNotFoundException: derby.log (Ingen tilgang) "Ingen adgang": Norwegian for "no access" :.) Derby then proceeds to use the console as error stream. The workaround would be to specify -Dderby.error.derby.stream.error.file=<file>, cf. end of enclosed session trace. ---------------------------------------------------------------------------- dags-lenovo:~/java/sb/sb1$ ls -l derby.log rwx ----- 1 dag None 1079 Aug 10 00:33 derby.log dags-lenovo:~/java/sb/sb1$ chmod 000 derby.log dags-lenovo:~/java/sb/sb1$ ls -l derby.log ---------- 1 dag None 1079 Aug 10 00:33 derby.log dags-lenovo:~/java/sb/sb1$ java org.apache.derby.tools.ij ij version 10.9 ij> connect 'jdbc:derby:wombat'; Sat Aug 20 00:46:04 CEST 2011 Thread [main,5,main] java.io.FileNotFoundException: derby.log (Ingen tilgang) Sat Aug 20 00:46:05 CEST 2011 Thread [main,5,main] Cleanup action starting java.sql.SQLException: Database 'wombat' not found. at org.apache.derby.impl.jdbc.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:98) at org.apache.derby.impl.jdbc.Util.newEmbedSQLException(Util.java:142) at org.apache.derby.impl.jdbc.Util.newEmbedSQLException(Util.java:148) at org.apache.derby.impl.jdbc.Util.generateCsSQLException(Util.java:227) at org.apache.derby.impl.jdbc.EmbedConnection.newSQLException(EmbedConnection.java:3085) at org.apache.derby.impl.jdbc.EmbedConnection.handleDBNotFound(EmbedConnection.java:735) : dags-lenovo:~/java/sb/sb1$ java -Dderby.stream.error.file=error.txt org.apache.derby.tools.ij ij version 10.9 ij> connect 'jdbc:derby:wombat'; ERROR XJ004: Database 'wombat' not found. ij> exit; dags-lenovo:~/java/sb/sb1$ Use "exit" to leave the shell. dags-lenovo:~/java/sb/sb1$ cat error.txt Sat Aug 20 00:54:38 CEST 2011 Thread [main,5,main] Cleanup action starting java.sql.SQLException: Database 'wombat' not found. at org.apache.derby.impl.jdbc.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:98) at org.apache.derby.impl.jdbc.Util.newEmbedSQLException(Util.java:142) at org.apache.derby.impl.jdbc.Util.newEmbedSQLException(Util.java:148) at org.apache.derby.impl.jdbc.Util.generateCsSQLException(Util.java:227) at org.apache.derby.impl.jdbc.EmbedConnection.newSQLException(EmbedConnection.java:3085)
          Hide
          Kathey Marsden added a comment -

          Assuming they want to continue with the old behavior, would a resolution at this point be as simple as take down the server, chmod the derby.log and add derby.storage.useDefaultFilePermissions=true to the derby.properties file?

          Show
          Kathey Marsden added a comment - Assuming they want to continue with the old behavior, would a resolution at this point be as simple as take down the server, chmod the derby.log and add derby.storage.useDefaultFilePermissions=true to the derby.properties file?
          Hide
          Dag H. Wanvik added a comment -

          Yes, in deed.

          Show
          Dag H. Wanvik added a comment - Yes, in deed.
          Hide
          Kathey Marsden added a comment -

          Thanks Dag. Your approach sounds good to me. It might be nice to add some verbosity to the message to give a clue as to the resolution. I am assuming this is for 10.9 not 10.8.2.

          I have a meeting Thursday with the group that I know needs more than one user to be able to start network server. I assume they can use the property, but will report back if they raise any insurmountable concerns.

          Show
          Kathey Marsden added a comment - Thanks Dag. Your approach sounds good to me. It might be nice to add some verbosity to the message to give a clue as to the resolution. I am assuming this is for 10.9 not 10.8.2. I have a meeting Thursday with the group that I know needs more than one user to be able to start network server. I assume they can use the property, but will report back if they raise any insurmountable concerns.
          Hide
          Dag H. Wanvik added a comment -

          Thanks Kathey! If this gets ready for 10.8.2, the default would be the old behavior.

          Show
          Dag H. Wanvik added a comment - Thanks Kathey! If this gets ready for 10.8.2, the default would be the old behavior.
          Hide
          Dag H. Wanvik added a comment -

          Uploading derby-5363-basic-1.

          It passed regressions on Windows and Solaris with classes, sane and insane jars. Manual inspection showed permissions to be restricted as expected on both platform types.

          This patch builds on the previous proof-of-concept patches. It is not for commit yet, I still need to rewrite the Java 7 level code to use reflection, so as not to force use of Java 7. In order to compile the patch you need a Java 7 JDK as the patch stands.

          I have called the patch "basic" since I need to make another patch to include:

          • implement the property persistence needed for old databases to avoid suddenly starting to use the new feature when upgrading, cf discussion with Kathey. See also separate question thread below.

          The patch now also restricts permission for created directories, albeit only for the lowests level if many levels are created with a a single mkdir. This can be improved if you think it is important/necessary, we just need to do more analysis of what parts of the path already exists during creation.

          Tests. I'd like feed-back on this. It's a bit hard to really test this automatically since our tests run within one OS user only. Naturally, we can inspect the file masks (on Posix/Unix), but we'd have to use the same methods in JDK 6 that we presently use to set them, so I am not sure it adds value. Likewise for NTFS ACLs, we would rely on the very same privitimes I use in JDK 7 to manipulate the ACLs. Up to now, I have used manual inspection of the file system permission using tools provided (ls -l on Unix, and The Windows explorer on Windows).

          Reviewers: Please help finding "holes", i.e. place where I have forgotten to limit permissions for for files and dirs created. I have gone through the code, but I can have missed some instances. Bwt, I did not do anything for the tools (ij, dblook).

          The patch only ever restricts permissions of files that are created, never on existing files [1]. This would preclude using the feature to "tighten up" and existing database. Is this the best approach? I think if one wants to secure an existing database, it's better to do it manually using OS level tools, and then start using the feature (lest it happens only gradually, piece-meal over time).

          [1] In the code, in some places I check if there was a file/dir created, in other places where it is statically known to be a new file/dir, I do not check. Verifying the correctness of this could be useful

          Detailed patch comments:

          M java/build/org/apache/derbyPreBuild/PropertySetter.java

          Enables using Java 7 for compilation. This will go away when I move to use reflection, but I'll make a new JIRA and attach this so we don't lose the code. I imagine at some point we will want to start using Java 7/8 going forward.

          M java/engine/org/apache/derby/iapi/services/io/FileUtil.java

          Implements the new static method limitAccessToOwner(File file). This will do the right thing for Unix and Windows if running on >= Java 6 and >= Java 7 respectively. Note: I have presumed that Windows use NTFS with ACLs enabled here. If the fs on Window were, say, an NFS share, it would not work since this would have POSIX permission. I guess we could improve this in a follow-up patch. If running on a Windows FAT system, which doesn't have permissions, the method would do nothing. a noop would also ensue if NTFS had the ACLs turned off.

          A java/engine/org/apache/derby/iapi/services/io/FileUtil7.java

          The Java 7 source level delegate which implements the NTFS ACL part of the above logic. When I move to use reflection, this code will move to FileUtil.java.

          M java/engine/org/apache/derby/iapi/services/io/build.xml

          Enables building of FileUtil7 with Java 7, will go away in next patch.

          M java/engine/org/apache/derby/iapi/reference/Property.java

          The Derby property "derby.storage.useDefaultFilePermissions".

          M java/engine/org/apache/derby/impl/load/ExportWriteData.java

          limit access to files created during export.

          M java/engine/org/apache/derby/impl/services/monitor/FileMonitor.java

          Creates derby.system.home if missing with restricted permissions.

          M java/engine/org/apache/derby/impl/services/monitor/StorageFactoryService.java

          Creation of derby.system.home, db directory ("wombat"), system.properties file with restricted permissions.

          M java/engine/org/apache/derby/impl/services/stream/SingleStream.java

          Creation of derby.log with restricted permissions.

          M java/engine/org/apache/derby/impl/io/DirFile4.java

          Creation of lck file with restricted permissions (NIO).

          M java/engine/org/apache/derby/io/StorageFile.java

          Added limitAccessToOwner to interface. In the code, StorageFile is often used interchangably with File; so StorageFile also implements exists() etc al.

          M java/engine/org/apache/derby/impl/io/InputStreamFile.java

          Implements StorageFile. An empty implementation of limitAccessToOwner since this is the base class for read-only stream implementations of the StorageFile interface. I.e. not file creation happens here.

          M java/engine/org/apache/derby/impl/io/DirFile.java

          Implements StorageFile. The one implemenation of StorageFile that does create files in the "real" file system. Forwards to FileUtil.limitAccessToOwner.

          M java/engine/org/apache/derby/impl/io/vfmem/VirtualFile.java
          M java/engine/org/apache/derby/impl/io/VFMemoryStorageFactory.java

          Implements StorageFile. No disk file system access, so an empty implementation of limitAccessToOwner.

          M java/engine/org/apache/derby/impl/io/BaseStorageFactory.java

          Creation of the tmp directory with restricted permissions.

          M java/engine/org/apache/derby/impl/store/raw/log/LogToFile.java

          Creation of the log directory and its files with restricted permissions.

          M java/engine/org/apache/derby/impl/store/raw/RawStore.java

          Creation of the backup directories and contents with restricted permissions (but see FileUtils#copyDirectory, #copyFile also).

          M java/engine/org/apache/derby/impl/store/raw/data/BaseDataFileFactory.java

          Creation of the db lock file with restricted permissions (but see also NIO getExclusiveFileLock in DirFile4).

          M java/engine/org/apache/derby/impl/store/raw/data/StreamFileContainer.java

          Used for lobs? In any case, tighted up to use restricted permissions.

          M java/engine/org/apache/derby/impl/store/raw/data/RFResource.java

          Jar file logic: restricted permissions.

          M java/engine/org/apache/derby/impl/store/raw/data/RAFContainer.java

          container creation, regular and backup with restricted permissions

          M java/shared/org/apache/derby/shared/common/reference/MessageId.java
          M java/engine/org/apache/derby/loc/messages.xml

          extra int'l for a "caused by string".

          M java/drda/org/apache/derby/impl/drda/DssTrace.java

          Dss trace (server): restricted permissions for trace directory and trace files.

          M java/testing/org/apache/derbyTesting/functionTests/util/corruptio/CorruptFile.java

          Implements StorageFile. Test only.

          M java/drda/org/apache/derby/drda/server.policy
          M java/drda/org/apache/derby/drda/template.policy
          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/SecurityPolicyReloadingTest.modified.policy
          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/SecurityPolicyReloadingTest.initial.policy
          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/noAbortPermission.policy
          M java/testing/org/apache/derbyTesting/functionTests/tests/derbynet/RuntimeInfoTest.policy
          M java/testing/org/apache/derbyTesting/functionTests/tests/derbynet/GetCurrentPropertiesTest.policy
          M java/testing/org/apache/derbyTesting/functionTests/tests/derbynet/NetworkServerControlApiTest.policy
          M java/testing/org/apache/derbyTesting/functionTests/tests/derbynet/ServerPropertiesTest.policy
          M java/testing/org/apache/derbyTesting/functionTests/util/derby_tests.policy

          The new code when running on Java 7 on Windows needs the extra RuntimePermission "accessUserInformation" (to determine the file's owner) when run with the Security Manager. I have added that to the default "server.policy" file, and the "template.policy", as well as were needed to run the tests. The tests also needed some more "read" file permissions.

          M build.xml
          M tools/jar/extraDBMSclasses.properties

          Changes to be able to use Java 7. Will be rolled back when I move to use reflection.

          Show
          Dag H. Wanvik added a comment - Uploading derby-5363-basic-1. It passed regressions on Windows and Solaris with classes, sane and insane jars. Manual inspection showed permissions to be restricted as expected on both platform types. This patch builds on the previous proof-of-concept patches. It is not for commit yet, I still need to rewrite the Java 7 level code to use reflection, so as not to force use of Java 7. In order to compile the patch you need a Java 7 JDK as the patch stands. I have called the patch "basic" since I need to make another patch to include: implement the property persistence needed for old databases to avoid suddenly starting to use the new feature when upgrading, cf discussion with Kathey. See also separate question thread below. The patch now also restricts permission for created directories, albeit only for the lowests level if many levels are created with a a single mkdir. This can be improved if you think it is important/necessary, we just need to do more analysis of what parts of the path already exists during creation. Tests. I'd like feed-back on this. It's a bit hard to really test this automatically since our tests run within one OS user only. Naturally, we can inspect the file masks (on Posix/Unix), but we'd have to use the same methods in JDK 6 that we presently use to set them, so I am not sure it adds value. Likewise for NTFS ACLs, we would rely on the very same privitimes I use in JDK 7 to manipulate the ACLs. Up to now, I have used manual inspection of the file system permission using tools provided (ls -l on Unix, and The Windows explorer on Windows). Reviewers: Please help finding "holes", i.e. place where I have forgotten to limit permissions for for files and dirs created. I have gone through the code, but I can have missed some instances. Bwt, I did not do anything for the tools (ij, dblook). The patch only ever restricts permissions of files that are created, never on existing files [1] . This would preclude using the feature to "tighten up" and existing database. Is this the best approach? I think if one wants to secure an existing database, it's better to do it manually using OS level tools, and then start using the feature (lest it happens only gradually, piece-meal over time). [1] In the code, in some places I check if there was a file/dir created, in other places where it is statically known to be a new file/dir, I do not check. Verifying the correctness of this could be useful Detailed patch comments: M java/build/org/apache/derbyPreBuild/PropertySetter.java Enables using Java 7 for compilation. This will go away when I move to use reflection, but I'll make a new JIRA and attach this so we don't lose the code. I imagine at some point we will want to start using Java 7/8 going forward. M java/engine/org/apache/derby/iapi/services/io/FileUtil.java Implements the new static method limitAccessToOwner(File file). This will do the right thing for Unix and Windows if running on >= Java 6 and >= Java 7 respectively. Note: I have presumed that Windows use NTFS with ACLs enabled here. If the fs on Window were, say, an NFS share, it would not work since this would have POSIX permission. I guess we could improve this in a follow-up patch. If running on a Windows FAT system, which doesn't have permissions, the method would do nothing. a noop would also ensue if NTFS had the ACLs turned off. A java/engine/org/apache/derby/iapi/services/io/FileUtil7.java The Java 7 source level delegate which implements the NTFS ACL part of the above logic. When I move to use reflection, this code will move to FileUtil.java. M java/engine/org/apache/derby/iapi/services/io/build.xml Enables building of FileUtil7 with Java 7, will go away in next patch. M java/engine/org/apache/derby/iapi/reference/Property.java The Derby property "derby.storage.useDefaultFilePermissions". M java/engine/org/apache/derby/impl/load/ExportWriteData.java limit access to files created during export. M java/engine/org/apache/derby/impl/services/monitor/FileMonitor.java Creates derby.system.home if missing with restricted permissions. M java/engine/org/apache/derby/impl/services/monitor/StorageFactoryService.java Creation of derby.system.home, db directory ("wombat"), system.properties file with restricted permissions. M java/engine/org/apache/derby/impl/services/stream/SingleStream.java Creation of derby.log with restricted permissions. M java/engine/org/apache/derby/impl/io/DirFile4.java Creation of lck file with restricted permissions (NIO). M java/engine/org/apache/derby/io/StorageFile.java Added limitAccessToOwner to interface. In the code, StorageFile is often used interchangably with File; so StorageFile also implements exists() etc al. M java/engine/org/apache/derby/impl/io/InputStreamFile.java Implements StorageFile. An empty implementation of limitAccessToOwner since this is the base class for read-only stream implementations of the StorageFile interface. I.e. not file creation happens here. M java/engine/org/apache/derby/impl/io/DirFile.java Implements StorageFile. The one implemenation of StorageFile that does create files in the "real" file system. Forwards to FileUtil.limitAccessToOwner. M java/engine/org/apache/derby/impl/io/vfmem/VirtualFile.java M java/engine/org/apache/derby/impl/io/VFMemoryStorageFactory.java Implements StorageFile. No disk file system access, so an empty implementation of limitAccessToOwner. M java/engine/org/apache/derby/impl/io/BaseStorageFactory.java Creation of the tmp directory with restricted permissions. M java/engine/org/apache/derby/impl/store/raw/log/LogToFile.java Creation of the log directory and its files with restricted permissions. M java/engine/org/apache/derby/impl/store/raw/RawStore.java Creation of the backup directories and contents with restricted permissions (but see FileUtils#copyDirectory, #copyFile also). M java/engine/org/apache/derby/impl/store/raw/data/BaseDataFileFactory.java Creation of the db lock file with restricted permissions (but see also NIO getExclusiveFileLock in DirFile4). M java/engine/org/apache/derby/impl/store/raw/data/StreamFileContainer.java Used for lobs? In any case, tighted up to use restricted permissions. M java/engine/org/apache/derby/impl/store/raw/data/RFResource.java Jar file logic: restricted permissions. M java/engine/org/apache/derby/impl/store/raw/data/RAFContainer.java container creation, regular and backup with restricted permissions M java/shared/org/apache/derby/shared/common/reference/MessageId.java M java/engine/org/apache/derby/loc/messages.xml extra int'l for a "caused by string". M java/drda/org/apache/derby/impl/drda/DssTrace.java Dss trace (server): restricted permissions for trace directory and trace files. M java/testing/org/apache/derbyTesting/functionTests/util/corruptio/CorruptFile.java Implements StorageFile. Test only. M java/drda/org/apache/derby/drda/server.policy M java/drda/org/apache/derby/drda/template.policy M java/testing/org/apache/derbyTesting/functionTests/tests/lang/SecurityPolicyReloadingTest.modified.policy M java/testing/org/apache/derbyTesting/functionTests/tests/lang/SecurityPolicyReloadingTest.initial.policy M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/noAbortPermission.policy M java/testing/org/apache/derbyTesting/functionTests/tests/derbynet/RuntimeInfoTest.policy M java/testing/org/apache/derbyTesting/functionTests/tests/derbynet/GetCurrentPropertiesTest.policy M java/testing/org/apache/derbyTesting/functionTests/tests/derbynet/NetworkServerControlApiTest.policy M java/testing/org/apache/derbyTesting/functionTests/tests/derbynet/ServerPropertiesTest.policy M java/testing/org/apache/derbyTesting/functionTests/util/derby_tests.policy The new code when running on Java 7 on Windows needs the extra RuntimePermission "accessUserInformation" (to determine the file's owner) when run with the Security Manager. I have added that to the default "server.policy" file, and the "template.policy", as well as were needed to run the tests. The tests also needed some more "read" file permissions. M build.xml M tools/jar/extraDBMSclasses.properties Changes to be able to use Java 7. Will be rolled back when I move to use reflection.
          Hide
          Dag H. Wanvik added a comment - - edited

          I am having pains with the property "derby.storage.useDefaultFilePermissions".

          Some half digested ideas follow.

          In the present code the property "derby.storage.useDefaultFilePermissions" is a dynamic, system level property. The discussion showed that we may want to handle this per database (do we really? but we can only persist values per database...) I also think it makes more sense to make this static, at least at the database level.

          But let as assume, we want to persist it per database. This begs the question whether this is really a system level property (e.g. to control derby.log and files created during tracing of DRDA protocol in the server) or a data base level property (all db files). If we persist this per database, we may need and additional property for system level files, or try to use some sensible defaults at that level.

          To illustrate: we need to decide how to handle the following cases, here given with a possible treatment: The matrix elements
          show the resulting value of the property, given the Derby deployment mode, the existing value of the property persisted for a database, and the system level property as specified when the VM started.

          system level files:
          mode: \ prop unspec. F T
          --------------------Unable to render embedded object: File (-----------) not found.------Unable to render embedded object: File (-------) not found.
          ! CLI server ! F ! F ! T !
          ! other modes ! T ! F ! T !
          ---------------------------------------------

          To explain, the three entries for CLI server here compute as follows given values "unspec", "F" (false) and "T" (true) for the property. That is, when we started the server, we did not specify a value for the property (case 1), we specified it to "false" (case 2) or we specified it to "true" (case three). The resulting value for system level files is "false", "false" and "true" respectively.
          That is, when unspecified for CLI server mode, we default to restrictive permissions.

          new db, db level files
          mode: \ prop unspec. F T
          --------------------Unable to render embedded object: File (-----------) not found.------Unable to render embedded object: File (-------) not found.
          ! CLI server ! F ! F ! T !
          ! other modes ! T ! F ! T !
          -------------------------------------------------

          existing db: no prop found persisted
          mode: \ prop unspec. F T
          --------------------Unable to render embedded object: File (------------) not found.------Unable to render embedded object: File (-------) not found.
          ! CLI server ! T ! i/w ! T !
          ! other modes ! T ! i/w ! T !
          --------------------------------------------------

          existing db: found persisted F
          mode: \ prop unspec. F T
          --------------------Unable to render embedded object: File (-----------) not found.------Unable to render embedded object: File (-------) not found.
          ! CLI server ! F ! F ! i/w !
          ! other modes ! F ! F ! i/w !
          -------------------------------------------------

          existing db: found persisted T
          mode: \ prop unspec. F T
          --------------------Unable to render embedded object: File (----------) not found.-------Unable to render embedded object: File (-------) not found.
          ! CLI server ! F ! i/w ! T !
          ! other modes ! T ! i/w ! T !
          -------------------------------------------------

          where "prop" means specified value of derby.storage.useDefaultFilePermissions and "i/w" means "ignore with warning or error".

          Hmm, this is getting a bit messy..

          Show
          Dag H. Wanvik added a comment - - edited I am having pains with the property "derby.storage.useDefaultFilePermissions". Some half digested ideas follow. In the present code the property "derby.storage.useDefaultFilePermissions" is a dynamic, system level property. The discussion showed that we may want to handle this per database (do we really? but we can only persist values per database...) I also think it makes more sense to make this static, at least at the database level. But let as assume, we want to persist it per database. This begs the question whether this is really a system level property (e.g. to control derby.log and files created during tracing of DRDA protocol in the server) or a data base level property (all db files). If we persist this per database, we may need and additional property for system level files, or try to use some sensible defaults at that level. To illustrate: we need to decide how to handle the following cases, here given with a possible treatment: The matrix elements show the resulting value of the property, given the Derby deployment mode, the existing value of the property persisted for a database, and the system level property as specified when the VM started. system level files: mode: \ prop unspec. F T -------------------- Unable to render embedded object: File (-----------) not found. ------ Unable to render embedded object: File (-------) not found. ! CLI server ! F ! F ! T ! ! other modes ! T ! F ! T ! --------------------------------------------- To explain, the three entries for CLI server here compute as follows given values "unspec", "F" (false) and "T" (true) for the property. That is, when we started the server, we did not specify a value for the property (case 1), we specified it to "false" (case 2) or we specified it to "true" (case three). The resulting value for system level files is "false", "false" and "true" respectively. That is, when unspecified for CLI server mode, we default to restrictive permissions. new db, db level files mode: \ prop unspec. F T -------------------- Unable to render embedded object: File (-----------) not found. ------ Unable to render embedded object: File (-------) not found. ! CLI server ! F ! F ! T ! ! other modes ! T ! F ! T ! ------------------------------------------------- existing db: no prop found persisted mode: \ prop unspec. F T -------------------- Unable to render embedded object: File (------------) not found. ------ Unable to render embedded object: File (-------) not found. ! CLI server ! T ! i/w ! T ! ! other modes ! T ! i/w ! T ! -------------------------------------------------- existing db: found persisted F mode: \ prop unspec. F T -------------------- Unable to render embedded object: File (-----------) not found. ------ Unable to render embedded object: File (-------) not found. ! CLI server ! F ! F ! i/w ! ! other modes ! F ! F ! i/w ! ------------------------------------------------- existing db: found persisted T mode: \ prop unspec. F T -------------------- Unable to render embedded object: File (----------) not found. ------- Unable to render embedded object: File (-------) not found. ! CLI server ! F ! i/w ! T ! ! other modes ! T ! i/w ! T ! ------------------------------------------------- where "prop" means specified value of derby.storage.useDefaultFilePermissions and "i/w" means "ignore with warning or error". Hmm, this is getting a bit messy..
          Hide
          Dag H. Wanvik added a comment - - edited

          Not very readable , that.. Wish there were an easy way to make table sin JITA comments...
          See the uploaded ascii art table instead if you can't read the above.."property-table.png".

          Show
          Dag H. Wanvik added a comment - - edited Not very readable , that.. Wish there were an easy way to make table sin JITA comments... See the uploaded ascii art table instead if you can't read the above.."property-table.png".
          Hide
          Kathey Marsden added a comment -

          Yes, I think messy is the operative word. I can't say I've read and understand all you have written but know I wouldn't want to have to try explain it someone else #

          I have been thinking that umask is sort of the standard way to control file permissions on created files. Do other database products try to control this ? I don't know that we are adding a lot of value by trying to control the permissions ourselves. Might it be possible to just print a warning on network server startup if databases will be created readable/writable to others and suggest adjusting the umask to be more restrictive if desired. Are the default permissions something that we can determine at runtime?

          Show
          Kathey Marsden added a comment - Yes, I think messy is the operative word. I can't say I've read and understand all you have written but know I wouldn't want to have to try explain it someone else # I have been thinking that umask is sort of the standard way to control file permissions on created files. Do other database products try to control this ? I don't know that we are adding a lot of value by trying to control the permissions ourselves. Might it be possible to just print a warning on network server startup if databases will be created readable/writable to others and suggest adjusting the umask to be more restrictive if desired. Are the default permissions something that we can determine at runtime?
          Hide
          Dag H. Wanvik added a comment -

          Kathey, traditionally Java hasn't offered much in the way of inspecting file attributes. With 6 and now 7, this has improved, finally. I guess we can "test" create a file and inspect its resulting permission and guess the default permissions in place. But on Windows, defaults can vary per directory, so its not so easy - Derby can create files in many directories. On Unix, I think the umask can be deduced in this way.
          I'll have a look at what other databases do too.

          Show
          Dag H. Wanvik added a comment - Kathey, traditionally Java hasn't offered much in the way of inspecting file attributes. With 6 and now 7, this has improved, finally. I guess we can "test" create a file and inspect its resulting permission and guess the default permissions in place. But on Windows, defaults can vary per directory, so its not so easy - Derby can create files in many directories. On Unix, I think the umask can be deduced in this way. I'll have a look at what other databases do too.
          Hide
          Dag H. Wanvik added a comment -

          I see that PostGresSQL, for example does limit permissions to the db user. The "postmaster" changes its umask to 0077, so no file is
          group-readable.

          http://doxygen.postgresql.org/postmaster_8c_source.html

          cf this code:

          00501 /*
          00502 * for security, no dir or file created can be group or other accessible
          00503 */
          00504 umask(S_IRWXG | S_IRWXO);

          where:
          S_IRWXG
          read, write, execute/search by group
          S_IRWXO
          read, write, execute/search by others

          i.e. effectively 0077.

          Show
          Dag H. Wanvik added a comment - I see that PostGresSQL, for example does limit permissions to the db user. The "postmaster" changes its umask to 0077, so no file is group-readable. http://doxygen.postgresql.org/postmaster_8c_source.html cf this code: 00501 /* 00502 * for security, no dir or file created can be group or other accessible 00503 */ 00504 umask(S_IRWXG | S_IRWXO); where: S_IRWXG read, write, execute/search by group S_IRWXO read, write, execute/search by others i.e. effectively 0077.
          Hide
          Dag H. Wanvik added a comment -

          For MS Server it seem a similar practice is being used, c.f this link:

          http://msdn.microsoft.com/en-us/library/ms189128%28SQL.90%29.aspx

          "Creating a Database or Adding a New File: When a database is created, or modified to add a new file, the MSSQLSERVER service account and members of the local Administrators group are granted Full Control access on the data and log files. File access is removed for all other accounts."

          Show
          Dag H. Wanvik added a comment - For MS Server it seem a similar practice is being used, c.f this link: http://msdn.microsoft.com/en-us/library/ms189128%28SQL.90%29.aspx "Creating a Database or Adding a New File: When a database is created, or modified to add a new file, the MSSQLSERVER service account and members of the local Administrators group are granted Full Control access on the data and log files. File access is removed for all other accounts."
          Hide
          Rick Hillegas added a comment -

          Thanks for continuing to puzzle through these issues, Dag and Kathey. I am afraid that I don't understand the tables posted yesterday.

          I have a couple questions:

          1) Besides derby.log and the network trace files, does Derby write any other system level files?

          2) Is this the problem with these files: append mode won't work if different users boot the server/engine?

          3) Or is there another problem: the files can't even be re-initialized (overwritten) if different users boot the server/engine?

          Thanks,
          -Rick

          Show
          Rick Hillegas added a comment - Thanks for continuing to puzzle through these issues, Dag and Kathey. I am afraid that I don't understand the tables posted yesterday. I have a couple questions: 1) Besides derby.log and the network trace files, does Derby write any other system level files? 2) Is this the problem with these files: append mode won't work if different users boot the server/engine? 3) Or is there another problem: the files can't even be re-initialized (overwritten) if different users boot the server/engine? Thanks, -Rick
          Hide
          Dag H. Wanvik added a comment -

          Thanks, Rick. The tables try to show what should/could be the behavior if we tried to persist the property derby.storage.useDefaultFilePermissions. If we did, we would need to specify what would happen when we later restarted the VM with possibly another value for this property: should the effective value change, or should the the (new) value be ignored.
          In any case, as both Kathey and I observed, it gets messy.

          Your questions: 1) I haven't found any others yet. I believe others pertain to a specific booted database. 2) Plain write or append or not isn't relevant for the permissions, I think. I think we write these files without appending to them, though. The restrictive permissions would preclude any writing to them at all by any user except the one that created them. 3) Correct.

          Show
          Dag H. Wanvik added a comment - Thanks, Rick. The tables try to show what should/could be the behavior if we tried to persist the property derby.storage.useDefaultFilePermissions. If we did, we would need to specify what would happen when we later restarted the VM with possibly another value for this property: should the effective value change, or should the the (new) value be ignored. In any case, as both Kathey and I observed, it gets messy. Your questions: 1) I haven't found any others yet. I believe others pertain to a specific booted database. 2) Plain write or append or not isn't relevant for the permissions, I think. I think we write these files without appending to them, though. The restrictive permissions would preclude any writing to them at all by any user except the one that created them. 3) Correct.
          Hide
          Dag H. Wanvik added a comment -

          I think I would actually prefer to let new Derby versions (>=10.9) of the server CLI startup (or possibly only the startup script, although I don't like the thought that the script should be anything but a convenience) enable the secure permissions by default, and leave it at that. I think any upgrade hassle[1] for users neglecting the read the release notes (to as to keep existing behavior) is worth the added security.

          [1] Should be detected at once when trying to write derby.log. It is easy to fix, just change the permissions back to lax and start using the property when the server is started.

          Show
          Dag H. Wanvik added a comment - I think I would actually prefer to let new Derby versions (>=10.9) of the server CLI startup (or possibly only the startup script, although I don't like the thought that the script should be anything but a convenience) enable the secure permissions by default, and leave it at that. I think any upgrade hassle [1] for users neglecting the read the release notes (to as to keep existing behavior) is worth the added security. [1] Should be detected at once when trying to write derby.log. It is easy to fix, just change the permissions back to lax and start using the property when the server is started.
          Hide
          Rick Hillegas added a comment -

          I think that the default embedded behavior should also be to use the tighter file permissions. From the release notes, embedded users, like client/server users, can figure out how to disable the tighter permissions if necessary.

          We have a significant security hole here: Broad permission is currently given to other users to type out conglomerates which contain sensitive information and to type out derby.log, which can contain SQL text. Backward compatibility is important but I believe it must be trumped by security fixes. I think that the security hole here is big enough to warrant breaking compatibility, particularly since a simple remedy removes the compatibility problem. Thanks.

          Show
          Rick Hillegas added a comment - I think that the default embedded behavior should also be to use the tighter file permissions. From the release notes, embedded users, like client/server users, can figure out how to disable the tighter permissions if necessary. We have a significant security hole here: Broad permission is currently given to other users to type out conglomerates which contain sensitive information and to type out derby.log, which can contain SQL text. Backward compatibility is important but I believe it must be trumped by security fixes. I think that the security hole here is big enough to warrant breaking compatibility, particularly since a simple remedy removes the compatibility problem. Thanks.
          Hide
          Kathey Marsden added a comment -

          I think it would be good to get feedback from the user community. Some response that I got this week when I approached a group about the change was:

          "... that seems backward. Why not keep the current behavior and have a derby.storage.useSecureFilePermissions=true property? I know there are times when a component really needs to change default behavior, but it should only be after a lot of consideration and adopter buy-in"

          Can you ask for more input on the user list?

          Show
          Kathey Marsden added a comment - I think it would be good to get feedback from the user community. Some response that I got this week when I approached a group about the change was: "... that seems backward. Why not keep the current behavior and have a derby.storage.useSecureFilePermissions=true property? I know there are times when a component really needs to change default behavior, but it should only be after a lot of consideration and adopter buy-in" Can you ask for more input on the user list?
          Hide
          Kathey Marsden added a comment -

          I personally don't think we have a security hole in Derby, but rather users might have a security hole if they do not set their umask appropriately to their situation. The proposed changes would help protect users from themselves and that may be the right thing to do, especially for network server, but I don't think it is a inherent hole in Derby.

          Show
          Kathey Marsden added a comment - I personally don't think we have a security hole in Derby, but rather users might have a security hole if they do not set their umask appropriately to their situation. The proposed changes would help protect users from themselves and that may be the right thing to do, especially for network server, but I don't think it is a inherent hole in Derby.
          Hide
          Dag H. Wanvik added a comment -

          Uploading derby-5363-basic-2, which removes the dependency on Java 7 for compiling Derby. The code in FileUtil7.java has been recoded with reflection and moved into FileUtil.java. Rerunning regressions.

          Any treatment of the property default, whichever way we choose to go, is yet pending.

          Apart from that, the patch is complete and ready for review. I do plan to go over the exception handling a bit more: although I have made pains to try to make any exception due to missing security permission propagate all the way up there are still locations in the code where I ignore them due to the fan-out if we introduce checked exception in low level utility classes, the theory being that the missing permissions would be detected elsewhere. I'll have a look to see if we could use unchecked exception and catch those at the top/boot level, cf. iapi.error.PassThroughException, since I don't like ignoring exceptions...

          Show
          Dag H. Wanvik added a comment - Uploading derby-5363-basic-2, which removes the dependency on Java 7 for compiling Derby. The code in FileUtil7.java has been recoded with reflection and moved into FileUtil.java. Rerunning regressions. Any treatment of the property default, whichever way we choose to go, is yet pending. Apart from that, the patch is complete and ready for review. I do plan to go over the exception handling a bit more: although I have made pains to try to make any exception due to missing security permission propagate all the way up there are still locations in the code where I ignore them due to the fan-out if we introduce checked exception in low level utility classes, the theory being that the missing permissions would be detected elsewhere. I'll have a look to see if we could use unchecked exception and catch those at the top/boot level, cf. iapi.error.PassThroughException, since I don't like ignoring exceptions...
          Hide
          Dag H. Wanvik added a comment -

          I am not sure that the property is backward, as suggested by Kathey's users: if people remember to explicitly use "useSecureFilePermissions", they would probably remember to set the correct umask/Windows directory permissions too.
          The whole point of doing this is "to protect users against themselves", i.e. provide a default that is secure rather than insecure.

          The question then becomes, in what use cases, in what deployment modes, is this protection a net benefit, and in which cases/deployment modes is the benefit outweighed by inconvenience (upgrade hassle, need to use a property to get sharing behavior).

          I do think that in the server case, the argument is stronger for a secure default, especially if authentication/authorization is enabled.

          Show
          Dag H. Wanvik added a comment - I am not sure that the property is backward, as suggested by Kathey's users: if people remember to explicitly use "useSecureFilePermissions", they would probably remember to set the correct umask/Windows directory permissions too. The whole point of doing this is "to protect users against themselves", i.e. provide a default that is secure rather than insecure. The question then becomes, in what use cases, in what deployment modes, is this protection a net benefit, and in which cases/deployment modes is the benefit outweighed by inconvenience (upgrade hassle, need to use a property to get sharing behavior). I do think that in the server case, the argument is stronger for a secure default, especially if authentication/authorization is enabled.
          Hide
          Knut Anders Hatlen added a comment -

          I took a brief look at the basic-2 patch. A couple of comments:

          1) This code may not do the intended in all locales (example: Turkish, "I".toLowerCase() returns "ı"):

          + os = PropertyUtil.getSystemProperty("os.name").toLowerCase();
          + onWindows = os.indexOf("windows") >= 0;

          2) Maybe we could avoid checking the os.name property altogether? The limitAccessToOwnerNTFS() method seems to be coded to work on all platforms. If the platform supports ACL, use that to set the permission; otherwise, do nothing. Could we call that method unconditionally and make it return a value indicating whether or not it was successful, and then only fall back to the Java 6-way of doing things if that failed? Using the return value from Files.getFileAttributeView() to decide whether or not ACLs are supported sounds more robust than checking os.name.

          3) FileUtil.limitAccessToOwner() sets many static fields on the first invocation. Is the first invocation guaranteed to be single-threaded, or is some kind of synchronization needed?

          Show
          Knut Anders Hatlen added a comment - I took a brief look at the basic-2 patch. A couple of comments: 1) This code may not do the intended in all locales (example: Turkish, "I".toLowerCase() returns "ı"): + os = PropertyUtil.getSystemProperty("os.name").toLowerCase(); + onWindows = os.indexOf("windows") >= 0; 2) Maybe we could avoid checking the os.name property altogether? The limitAccessToOwnerNTFS() method seems to be coded to work on all platforms. If the platform supports ACL, use that to set the permission; otherwise, do nothing. Could we call that method unconditionally and make it return a value indicating whether or not it was successful, and then only fall back to the Java 6-way of doing things if that failed? Using the return value from Files.getFileAttributeView() to decide whether or not ACLs are supported sounds more robust than checking os.name. 3) FileUtil.limitAccessToOwner() sets many static fields on the first invocation. Is the first invocation guaranteed to be single-threaded, or is some kind of synchronization needed?
          Hide
          Kathey Marsden added a comment -

          Dag Said ...
          "The whole point of doing this is "to protect users against themselves", i.e. provide a default that is secure rather than insecure."

          For embedded the default has always been focused on zero admin and so pretty much wide open by default and not secure. Users have to take specific steps to secure Derby and absorb the necessary administration and work to secure it. I think many embedded applications require multiple user access and that is a perfectly valid use of the product. I don't think we can justify breaking these applications to protect some other users from themselves.

          I am more comfortable with changing the command line startup for Network Server as it is not a zero admin solution. Multiple connecting client users will not be affected by the permission change and we have already made efforts to improve default security. Although they exist, it is harder to think of valid scenarios where multiple users need to start network server and I think we could mitigate it from a support perspective, which I don't think we could for embedded.

          Whether the default changes or it doesn't and in what scenarios I think it would be wise to consult the user list and get feedback. I think the user I talked to was certainly right when he said:
          "there are times when a component really needs to change default behavior, but it should only be after a lot of consideration and adopter buy-in"

          Dag, can you bring this issue up on the user list? I could do it, but I think since you are driving the change it would be most appropriate for you to initiate the user list discussion. I think it will also help raise awareness for current users that they need their umask set appropriately if they want the files protected.

          Show
          Kathey Marsden added a comment - Dag Said ... "The whole point of doing this is "to protect users against themselves", i.e. provide a default that is secure rather than insecure." For embedded the default has always been focused on zero admin and so pretty much wide open by default and not secure. Users have to take specific steps to secure Derby and absorb the necessary administration and work to secure it. I think many embedded applications require multiple user access and that is a perfectly valid use of the product. I don't think we can justify breaking these applications to protect some other users from themselves. I am more comfortable with changing the command line startup for Network Server as it is not a zero admin solution. Multiple connecting client users will not be affected by the permission change and we have already made efforts to improve default security. Although they exist, it is harder to think of valid scenarios where multiple users need to start network server and I think we could mitigate it from a support perspective, which I don't think we could for embedded. Whether the default changes or it doesn't and in what scenarios I think it would be wise to consult the user list and get feedback. I think the user I talked to was certainly right when he said: "there are times when a component really needs to change default behavior, but it should only be after a lot of consideration and adopter buy-in" Dag, can you bring this issue up on the user list? I could do it, but I think since you are driving the change it would be most appropriate for you to initiate the user list discussion. I think it will also help raise awareness for current users that they need their umask set appropriately if they want the files protected.
          Hide
          Kathy Saunders added a comment -

          I just wanted to comment on this issue as I have worked with Derby for a long time and have also worked in a support capacity with customers who use Derby. I think that Kathey said it well--the embedded solution was originally designed to be zero admin, and not secure by default (to keep administration to a mimimum). Many people who use embedded Derby protect it at other levels within their solutions. I have seen many uses of embedded Derby that would break if the permissions of the DB files change. From a support perspective, changing the default behavior generally causes confusion. I would expect that a change like this would generate issues when people upgrade, as they may not have read the documentation that talks about the new behavior. My vote would be to leave the default permissions as is in the embedded case.

          Show
          Kathy Saunders added a comment - I just wanted to comment on this issue as I have worked with Derby for a long time and have also worked in a support capacity with customers who use Derby. I think that Kathey said it well--the embedded solution was originally designed to be zero admin, and not secure by default (to keep administration to a mimimum). Many people who use embedded Derby protect it at other levels within their solutions. I have seen many uses of embedded Derby that would break if the permissions of the DB files change. From a support perspective, changing the default behavior generally causes confusion. I would expect that a change like this would generate issues when people upgrade, as they may not have read the documentation that talks about the new behavior. My vote would be to leave the default permissions as is in the embedded case.
          Hide
          Dag H. Wanvik added a comment -

          Right, let's leave embedded with the present default for now and proceed with the CLI startup of the network server. I'll start a discussion on derby-user and see if we get any feed-back. Thanks for helping us get clarity on what's the best way forward here!

          Show
          Dag H. Wanvik added a comment - Right, let's leave embedded with the present default for now and proceed with the CLI startup of the network server. I'll start a discussion on derby-user and see if we get any feed-back. Thanks for helping us get clarity on what's the best way forward here!
          Hide
          Dag H. Wanvik added a comment -

          Thanks for looking at the patch code, Knut. 1) Agreed, it's brittle. 2) Yes, I did consider that. It is probably more robust, so I like the idea, although it's slightly more code to perform for the Posix case. The upside is probably more important: I read somewhere a NFS share on Windows would have Posix permissions, and your solution might make it work correctly in such a case also. 3) I think it would mostly be single threaded, at least on a new database, but I don't think we have a guarantee, so I'll protect it.

          Show
          Dag H. Wanvik added a comment - Thanks for looking at the patch code, Knut. 1) Agreed, it's brittle. 2) Yes, I did consider that. It is probably more robust, so I like the idea, although it's slightly more code to perform for the Posix case. The upside is probably more important: I read somewhere a NFS share on Windows would have Posix permissions, and your solution might make it work correctly in such a case also. 3) I think it would mostly be single threaded, at least on a new database, but I don't think we have a guarantee, so I'll protect it.
          Hide
          Dag H. Wanvik added a comment -

          Uploading a version #3 of the basic patch, which addresses Knut's comments. It also simplifies the exception handling a bit, I found I could ignore the checked exception at the origin. The new code uses ACL interface on Java 7 also for Posix-like file systems, this should make the code work also on Windows with an NFS share I think. If on Java 6, we try to use the simplier Java 6 lang.File.* API, which admittedly only works on Posix-like systems. Rerunning regressions.

          Show
          Dag H. Wanvik added a comment - Uploading a version #3 of the basic patch, which addresses Knut's comments. It also simplifies the exception handling a bit, I found I could ignore the checked exception at the origin. The new code uses ACL interface on Java 7 also for Posix-like file systems, this should make the code work also on Windows with an NFS share I think. If on Java 6, we try to use the simplier Java 6 lang.File.* API, which admittedly only works on Posix-like systems. Rerunning regressions.
          Hide
          Knut Anders Hatlen added a comment -

          Thanks, Dag. The patch addresses the comments I had. There's still the possible locale issue when checking os.name, but now that code is limited to a debug section, so it shouldn't affect any production use.

          Show
          Knut Anders Hatlen added a comment - Thanks, Dag. The patch addresses the comments I had. There's still the possible locale issue when checking os.name, but now that code is limited to a debug section, so it shouldn't affect any production use.
          Hide
          Dag H. Wanvik added a comment -

          Thanks, Knut. Yeah, I felt for debugging, the chances of Turkish upper case was less of an issue..

          Uploading a patxh to make the "default-restrictive-only-with-cli-network-server" behavior as "derby-5363-server-1". I have tested it manually and it seems to do the right thing.

          Show
          Dag H. Wanvik added a comment - Thanks, Knut. Yeah, I felt for debugging, the chances of Turkish upper case was less of an issue.. Uploading a patxh to make the "default-restrictive-only-with-cli-network-server" behavior as "derby-5363-server-1". I have tested it manually and it seems to do the right thing.
          Hide
          Dag H. Wanvik added a comment - - edited

          Uploading a new version which is a sum of the basic patch + the server patch + a new test (which is the main new contribution of this patch): RestrictiveFilePermissionsTest.

          Some minor omissions were added to the basic patch's code (a couple of places where we had forgotten some created files/dirs).

          Also a small fix for JUnit was necessary to make one constructor of NetworkServerTestSetup used in the test, work.

          The test will only do anything when run under Java 7. Also, if the default umask (or Windows equivalent) is set such that only the owner gets access, the test can't really work and skips the test.

          If you can imagine other userful test cases, that would be appreciated. I have tried to wade through the code and docs to find places where directories and files are created, but it's easy to miss some.

          Running regressions.

          Footnote: the reason why we can restrict but not test permission with Java 6 is that this would require running a test with another OS user; there is not way to inspect the permissions, only to restrict them, essentially. Cf File.setWriteable & friends.

          Show
          Dag H. Wanvik added a comment - - edited Uploading a new version which is a sum of the basic patch + the server patch + a new test (which is the main new contribution of this patch): RestrictiveFilePermissionsTest. Some minor omissions were added to the basic patch's code (a couple of places where we had forgotten some created files/dirs). Also a small fix for JUnit was necessary to make one constructor of NetworkServerTestSetup used in the test, work. The test will only do anything when run under Java 7. Also, if the default umask (or Windows equivalent) is set such that only the owner gets access, the test can't really work and skips the test. If you can imagine other userful test cases, that would be appreciated. I have tried to wade through the code and docs to find places where directories and files are created, but it's easy to miss some. Running regressions. Footnote: the reason why we can restrict but not test permission with Java 6 is that this would require running a test with another OS user; there is not way to inspect the permissions, only to restrict them, essentially. Cf File.setWriteable & friends.
          Hide
          Dag H. Wanvik added a comment -

          As far as asking our users for an opinion on the changed CLI server default, we have only received two replies so far (12 days): One pro and one contra. Not much guidance there.

          Show
          Dag H. Wanvik added a comment - As far as asking our users for an opinion on the changed CLI server default, we have only received two replies so far (12 days): One pro and one contra. Not much guidance there.
          Hide
          Dag H. Wanvik added a comment -

          The latest patch (derby-5363-full-1), broke some tests in the harness. It turns out the mechanism I use for conveying to the engine it was booted from a CLI server is broken: I set a system property in NetworkServerControl which is later consulted by the engine when creating new files. The tests that broke installed their own security policies which did not include the permission to set a system property. There is already a precedent for using internal system properties, cf e.g. NetworkServerControl#installSecurityManager, but in that case, there is no user specified security manager already installed, so setting the properties cannot fail. In my case, we would have to impose a burden on the user to provide the needed permission in the provided policy file, which is awkward, since we are talking about an internal system property here. However, I didn't find any other way to provide the engine with this information since the server uses the generic Class.forName method to boot the engine:
          NetworkServerControlImpl.startNetworkServer, ca line 1009:

          cloudscapeDriver = (Driver) Class.forName(CLOUDSCAPE_DRIVER).newInstance();

          and after that has been done it is too late (derby.log has already been created..) Any clever ideas on how to let the engine know its been booted from the network server? I guess I could try to load the class NetworkServerControlImpl and if available (which it would be in a server context), read a public static member, but so far we have managed to avoid such "backward" coupling from the embedded driver to the server code, so it doesn't sees a good solution either..

          Show
          Dag H. Wanvik added a comment - The latest patch (derby-5363-full-1), broke some tests in the harness. It turns out the mechanism I use for conveying to the engine it was booted from a CLI server is broken: I set a system property in NetworkServerControl which is later consulted by the engine when creating new files. The tests that broke installed their own security policies which did not include the permission to set a system property. There is already a precedent for using internal system properties, cf e.g. NetworkServerControl#installSecurityManager, but in that case, there is no user specified security manager already installed, so setting the properties cannot fail. In my case, we would have to impose a burden on the user to provide the needed permission in the provided policy file, which is awkward, since we are talking about an internal system property here. However, I didn't find any other way to provide the engine with this information since the server uses the generic Class.forName method to boot the engine: NetworkServerControlImpl.startNetworkServer, ca line 1009: cloudscapeDriver = (Driver) Class.forName(CLOUDSCAPE_DRIVER).newInstance(); and after that has been done it is too late (derby.log has already been created..) Any clever ideas on how to let the engine know its been booted from the network server? I guess I could try to load the class NetworkServerControlImpl and if available (which it would be in a server context), read a public static member, but so far we have managed to avoid such "backward" coupling from the embedded driver to the server code, so it doesn't sees a good solution either..
          Hide
          Dag H. Wanvik added a comment -

          Uploading derby-5363-full-2, which fixes this issue by adjusting the relevant test policy file, as well as the template policy file with a suitable comment. Rerunning regressions. I believe this is a cleaner approach after all than referring "upwards" from the engine into the server code..

          Show
          Dag H. Wanvik added a comment - Uploading derby-5363-full-2, which fixes this issue by adjusting the relevant test policy file, as well as the template policy file with a suitable comment. Rerunning regressions. I believe this is a cleaner approach after all than referring "upwards" from the engine into the server code..
          Hide
          Dag H. Wanvik added a comment -

          Regressions passed on Solaris 11 with JDK 1.7. Ill run the full patch through more platform combinations since the patch is platform dependent before I consider committing it. It would be nice to have somebody look at the tests first,though.

          I also need to write a small functional specification to sum up the behavior and attach it to this issue as well as a release note.

          Show
          Dag H. Wanvik added a comment - Regressions passed on Solaris 11 with JDK 1.7. Ill run the full patch through more platform combinations since the patch is platform dependent before I consider committing it. It would be nice to have somebody look at the tests first,though. I also need to write a small functional specification to sum up the behavior and attach it to this issue as well as a release note.
          Hide
          Dag H. Wanvik added a comment -

          Attaching a first rev of the release notes.

          Show
          Dag H. Wanvik added a comment - Attaching a first rev of the release notes.
          Hide
          Rick Hillegas added a comment -

          Thanks, Dag. I could not resist wordsmithing the release note. I've attached a new version. Feel free to discard any or all of this if I have garbled your meaning. Thanks.

          Show
          Rick Hillegas added a comment - Thanks, Dag. I could not resist wordsmithing the release note. I've attached a new version. Feel free to discard any or all of this if I have garbled your meaning. Thanks.
          Hide
          Dag H. Wanvik added a comment -

          Thanks, Rick. Much more readable, at the cost of a slight loss of precision in a few places. I'll try to add that back without making it (much) harder to read.

          Show
          Dag H. Wanvik added a comment - Thanks, Rick. Much more readable, at the cost of a slight loss of precision in a few places. I'll try to add that back without making it (much) harder to read.
          Hide
          Dag H. Wanvik added a comment -

          Uploading a new version of the release notes. Added back some precision, fixed a typo, and changed doc links to point to 10.9, which is the likely release vehicle.

          Show
          Dag H. Wanvik added a comment - Uploading a new version of the release notes. Added back some precision, fixed a typo, and changed doc links to point to 10.9, which is the likely release vehicle.
          Hide
          Dag H. Wanvik added a comment -

          Uploading version derby-5363-full-3 of the patch. Relative to version -2, this makes a couple of small changes to make the test work on Windows: we tried to delete files that were not closed. See also DERBY-5418; that patch is needed for this patch to run correctly on Windows.

          Regressions passed on Solaris, Windows with JDK 1.6 and 1.7.

          Show
          Dag H. Wanvik added a comment - Uploading version derby-5363-full-3 of the patch. Relative to version -2, this makes a couple of small changes to make the test work on Windows: we tried to delete files that were not closed. See also DERBY-5418 ; that patch is needed for this patch to run correctly on Windows. Regressions passed on Solaris, Windows with JDK 1.6 and 1.7.
          Hide
          Knut Anders Hatlen added a comment -

          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?

          Show
          Knut Anders Hatlen added a comment - 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?
          Hide
          Kathey Marsden added a comment -

          I have not been following this issue closely, but seem to recall a comment about the patch working only on JDK 1.7 and not on 1.6. I don't see that now in skimming the comments. Is that the case? If so we should probably change the summary.

          Show
          Kathey Marsden added a comment - I have not been following this issue closely, but seem to recall a comment about the patch working only on JDK 1.7 and not on 1.6. I don't see that now in skimming the comments. Is that the case? If so we should probably change the summary.
          Hide
          Dag H. Wanvik added a comment -

          Kathey, the patch will do the right thing for Java 6 on Unix/Linux only (Posix fs, not NTFS). On 7, it works on NTFS, too. The tests only work with Java 7 because there is no way to find out programmatically if it worked under Java 6. I have verified it by inspection of the created files only in that case.

          Show
          Dag H. Wanvik added a comment - Kathey, the patch will do the right thing for Java 6 on Unix/Linux only (Posix fs, not NTFS). On 7, it works on NTFS, too. The tests only work with Java 7 because there is no way to find out programmatically if it worked under Java 6. I have verified it by inspection of the created files only in that case.
          Hide
          Dag H. Wanvik added a comment -

          Thanks for your diligent review, Knut! Uploading patch *-full-4 in which we:

          • fixed the misguided StreamFileContainer exception, early draft cruft
          • updated the JVMInfo to default to 7 if >= 8
          • added creating a temporary table to testTmpDirectory test case
          • changed naming to avoid having to list all tests that are part of the majority group as suggested
          • excluded the two lax cases from running if lax testing isn't supported
          • in suite(), declared it as throws Exception and avoid swallowing the stack trace of the underlying exception when calling checkAccessToOwner, ditto in checkAccessToOwner
          • added missing blank in warning string
          • converted missing use of helper methods from BaseJDBCTestCase
          • switched to using PrivilegedFileOpsForTests#exists
          • removed spurious white space diff in NetworkServerTestSetup
          • changed to using "/" since it's a cheaper pattern than reading the property and works fine on Windows, too.
          • removed instances of now superfluous exception handling code similar to this:
            ioe instanceof FileNotFoundException &&
            ((FileNotFoundException)ioe).getCause() instanceof
            AccessControlException) {

          since limitAccessToOwner no longer wraps AccessControlException in a IOException

          Answers to your questions:

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

          Not sure what's going on here. I tried to move it inside a test, but I still got no security exception. I thought I had seen somewhere that this property was safe to read, but I can't say where. I found this, though:

          Cf. http://www.iam.ubc.ca/guides/javatut99/applet/practical/properties.html

          which lists this as a legal property for an applet to read. I guess if a property is considered safe for an applet to read, it's safe for an app running under Java security, too..

          > - 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);

          That's true, but I didn't like seeing the warning in my log.. This way, mostly the method doesn't have to retry..

          Rerunning regressions.

          Show
          Dag H. Wanvik added a comment - Thanks for your diligent review, Knut! Uploading patch *-full-4 in which we: fixed the misguided StreamFileContainer exception, early draft cruft updated the JVMInfo to default to 7 if >= 8 added creating a temporary table to testTmpDirectory test case changed naming to avoid having to list all tests that are part of the majority group as suggested excluded the two lax cases from running if lax testing isn't supported in suite(), declared it as throws Exception and avoid swallowing the stack trace of the underlying exception when calling checkAccessToOwner, ditto in checkAccessToOwner added missing blank in warning string converted missing use of helper methods from BaseJDBCTestCase switched to using PrivilegedFileOpsForTests#exists removed spurious white space diff in NetworkServerTestSetup changed to using "/" since it's a cheaper pattern than reading the property and works fine on Windows, too. removed instances of now superfluous exception handling code similar to this: ioe instanceof FileNotFoundException && ((FileNotFoundException)ioe).getCause() instanceof AccessControlException) { since limitAccessToOwner no longer wraps AccessControlException in a IOException Answers to your questions: > - 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. Not sure what's going on here. I tried to move it inside a test, but I still got no security exception. I thought I had seen somewhere that this property was safe to read, but I can't say where. I found this, though: Cf. http://www.iam.ubc.ca/guides/javatut99/applet/practical/properties.html which lists this as a legal property for an applet to read. I guess if a property is considered safe for an applet to read, it's safe for an app running under Java security, too.. > - 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); That's true, but I didn't like seeing the warning in my log.. This way, mostly the method doesn't have to retry.. Rerunning regressions.
          Hide
          Knut Anders Hatlen added a comment -

          Thanks, Dag, for the new patch and for answering my questions. The full-4 patch addresses all my comments (well, except for the spurious white space diff in NetworkServerTestSetup, which seems to have crept back in). +1

          Show
          Knut Anders Hatlen added a comment - Thanks, Dag, for the new patch and for answering my questions. The full-4 patch addresses all my comments (well, except for the spurious white space diff in NetworkServerTestSetup, which seems to have crept back in). +1
          Hide
          Dag H. Wanvik added a comment -

          Thanks, Knut. yeah, pilot error: it had crept back in.. too many workspaces in parallel got me confused I won't respin the patch for that, but fix it when committing.

          Show
          Dag H. Wanvik added a comment - Thanks, Knut. yeah, pilot error: it had crept back in.. too many workspaces in parallel got me confused I won't respin the patch for that, but fix it when committing.
          Hide
          Dag H. Wanvik added a comment -

          I will commit this patch over the week-end, then. Notwithstanding the ongoing discussions about security, we can always change the new default for CLI server back to old (possibly lax) default permissions if that turns out to be the conclusion.

          Show
          Dag H. Wanvik added a comment - I will commit this patch over the week-end, then. Notwithstanding the ongoing discussions about security, we can always change the new default for CLI server back to old (possibly lax) default permissions if that turns out to be the conclusion.
          Hide
          Kathey Marsden added a comment -

          Dag, I have lost track. Can you summarize the default behavior and the behavior if the property is set for the various configurations, jvms and for existing vs new databases?

          Thanks

          Kathey

          Show
          Kathey Marsden added a comment - Dag, I have lost track. Can you summarize the default behavior and the behavior if the property is set for the various configurations, jvms and for existing vs new databases? Thanks Kathey
          Hide
          Dag H. Wanvik added a comment - - edited

          Kathey, the present releasenotes.html summarizes the behavior, I think. Since this is a system level property, we make no difference between existing and new databases. If one starts the (new) CLI server, it will treat existing and new databases the same, as well as system level files (e.g. server trace files): all files and directories created will be restricted to the owner. The permissions of existing files are not touched. To get the old behavior for the CLI server the property must be used.

          Show
          Dag H. Wanvik added a comment - - edited Kathey, the present releasenotes.html summarizes the behavior, I think. Since this is a system level property, we make no difference between existing and new databases. If one starts the (new) CLI server, it will treat existing and new databases the same, as well as system level files (e.g. server trace files): all files and directories created will be restricted to the owner. The permissions of existing files are not touched. To get the old behavior for the CLI server the property must be used.
          Hide
          Dag H. Wanvik added a comment - - edited

          And, just to be clear, embedded Derby and server started via API are not affected by default.

          Show
          Dag H. Wanvik added a comment - - edited And, just to be clear, embedded Derby and server started via API are not affected by default.
          Hide
          Dag H. Wanvik added a comment - - edited

          I had a new regression error. It turned out that after I changed to implicit fixure addition, the indeterministic test fixture ordering resulting under Java 7 intermittently exposed a bug: If the test containing the jar file install were executed prior to the backup test, the database would contain a jar file which is, of course, then also backed up. There was code to to handle this, but it wasn't part of the test (except by this "accident") and it had a bug. Fixed now in version *-full-5, which I upload. Rerunning regressions.

          Show
          Dag H. Wanvik added a comment - - edited I had a new regression error. It turned out that after I changed to implicit fixure addition, the indeterministic test fixture ordering resulting under Java 7 intermittently exposed a bug: If the test containing the jar file install were executed prior to the backup test, the database would contain a jar file which is, of course, then also backed up. There was code to to handle this, but it wasn't part of the test (except by this "accident") and it had a bug. Fixed now in version *-full-5, which I upload. Rerunning regressions.
          Hide
          Dag H. Wanvik added a comment -

          Committed patch derby-5363-full-5 as svn 1176591, resolving.

          Show
          Dag H. Wanvik added a comment - Committed patch derby-5363-full-5 as svn 1176591, resolving.
          Hide
          Knut Anders Hatlen added a comment -

          One small issue that I didn't notice until now. sysinfo outputs this on Java 7 after the commit:

          --------- Derby Information --------
          JRE - JDBC: ?-?

          I think JVMInfo.derbyVMLevel() needs to be updated now that Java 7 isn't handled as Java 6 anymore.

          Show
          Knut Anders Hatlen added a comment - One small issue that I didn't notice until now. sysinfo outputs this on Java 7 after the commit: --------- Derby Information -------- JRE - JDBC: ?-? I think JVMInfo.derbyVMLevel() needs to be updated now that Java 7 isn't handled as Java 6 anymore.
          Hide
          Kathey Marsden added a comment -

          For trunk we are going to remove this line as part of DERBY-1046, so that will take care of the sysinfo issue.

          Show
          Kathey Marsden added a comment - For trunk we are going to remove this line as part of DERBY-1046 , so that will take care of the sysinfo issue.
          Hide
          Kathey Marsden added a comment -

          The release note does not mention the difference in behavior for different JVM's but is my understanding correct?

          • java 5 and lower there is no change in behavior.
          • java 6 some operating systems change and some don't. Also we cannot test on any platform for java 6
          • java 7 All platforms change.

          If this summary is correct, I think perhaps we should only have the new behavior with java 7. If we can't test it on java 6 we won't know if there are jvm bugs or derby code changes that break it. Also it is confusing that some platforms are affected and some aren't. Lastly I think it is very clear and more appropriate to introduce a new behavior with a new JVM. Applications will test more rigorously on a JVM upgrade and hopefully any associated problems will be caught early.

          Show
          Kathey Marsden added a comment - The release note does not mention the difference in behavior for different JVM's but is my understanding correct? java 5 and lower there is no change in behavior. java 6 some operating systems change and some don't. Also we cannot test on any platform for java 6 java 7 All platforms change. If this summary is correct, I think perhaps we should only have the new behavior with java 7. If we can't test it on java 6 we won't know if there are jvm bugs or derby code changes that break it. Also it is confusing that some platforms are affected and some aren't. Lastly I think it is very clear and more appropriate to introduce a new behavior with a new JVM. Applications will test more rigorously on a JVM upgrade and hopefully any associated problems will be caught early.
          Hide
          Dag H. Wanvik added a comment - - edited

          Uploading a follow-up patch: "derby-5363-followup", which adds a missing accessController block around setting the system property SERVER_STARTED_FROM_CMD_LINE. If missing, this would fail if running with a security manager specified on the command line. Regressions passed.
          If the property permission is missing, the error is printed unconditionally and exit from main taken. Cf. DERBY-5413 which tried another (aborted) approach to make sure it got printed.

          Show
          Dag H. Wanvik added a comment - - edited Uploading a follow-up patch: "derby-5363-followup", which adds a missing accessController block around setting the system property SERVER_STARTED_FROM_CMD_LINE. If missing, this would fail if running with a security manager specified on the command line. Regressions passed. If the property permission is missing, the error is printed unconditionally and exit from main taken. Cf. DERBY-5413 which tried another (aborted) approach to make sure it got printed.
          Hide
          Dag H. Wanvik added a comment -

          Kathey, your understanding is correct. As for only introducing the new behavior on Java 7, that would make it easier to explain indeed. The missing testing is of course also worrying, especially since it takes another code path. On the other hand, it does work on Unix (nad Unix derived) platforms on Java 6 and with some work I could probably add some (non-portable) testing for it by forking a shell and doing ls(1).

          Show
          Dag H. Wanvik added a comment - Kathey, your understanding is correct. As for only introducing the new behavior on Java 7, that would make it easier to explain indeed. The missing testing is of course also worrying, especially since it takes another code path. On the other hand, it does work on Unix (nad Unix derived) platforms on Java 6 and with some work I could probably add some (non-portable) testing for it by forking a shell and doing ls(1).
          Hide
          Dag H. Wanvik added a comment -

          Committed derby-5363-followup as svn 1177718.

          Show
          Dag H. Wanvik added a comment - Committed derby-5363-followup as svn 1177718.
          Hide
          Kathey Marsden added a comment -

          I think my first preference would be to make this only a java 7 and up feature.
          The change is very clear and we can use our normal testing methods. Meanwhile users can set their umask appropriately.

          My second preference would be only to only change permissions by default with java 7 and higher.
          I think we really need to have tests for the default configuration and non-portable tests might have to be customized to different environments, which seems like a lot of effort for something that won't be used moving forward.

          I am sorry I didn't chime in earlier. Time has been quite tight recently.

          Show
          Kathey Marsden added a comment - I think my first preference would be to make this only a java 7 and up feature. The change is very clear and we can use our normal testing methods. Meanwhile users can set their umask appropriately. My second preference would be only to only change permissions by default with java 7 and higher. I think we really need to have tests for the default configuration and non-portable tests might have to be customized to different environments, which seems like a lot of effort for something that won't be used moving forward. I am sorry I didn't chime in earlier. Time has been quite tight recently.
          Hide
          Mike Matrigali added a comment -

          It is already strange that one will get different behavior depending on which JVM one uses. There is a precedent when we added JVM locking support to prevent dual booting the behavior was different depending on JVM version, so documentation/explanation is key to let users
          know what to expect.

          It seems more straight forward to only support the change with jdk17 and up JVM's. It is easily documented, and I assume implemented. Users upgrading to a new JVM should be looking for consequences to their applications and hopefully at that point will
          run tests.

          It also seems like testing of the feature is more straight forward with this restriction.

          Show
          Mike Matrigali added a comment - It is already strange that one will get different behavior depending on which JVM one uses. There is a precedent when we added JVM locking support to prevent dual booting the behavior was different depending on JVM version, so documentation/explanation is key to let users know what to expect. It seems more straight forward to only support the change with jdk17 and up JVM's. It is easily documented, and I assume implemented. Users upgrading to a new JVM should be looking for consequences to their applications and hopefully at that point will run tests. It also seems like testing of the feature is more straight forward with this restriction.
          Hide
          Dag H. Wanvik added a comment - - edited

          Ok, folks, let's make this a Java 7 and upwards feature. I'll change the implementation accordingly.

          Show
          Dag H. Wanvik added a comment - - edited Ok, folks, let's make this a Java 7 and upwards feature. I'll change the implementation accordingly.
          Hide
          Dag H. Wanvik added a comment - - edited

          Uploading a patch which limits default setting of restrictive permissions (for network server started from the command line) to Java 7. The documentation will describe the whole feature as a Java 7 and higher feature.

          The present patch does not actually stop anyone from trying to use it with Java 6 on Unix, though (by switching it on explicitly with the property), so that would be an undocumented, unsupported feature. If you think I should actively prohibit using it with that platform, speak out, please.

          Re-running regressions.

          Show
          Dag H. Wanvik added a comment - - edited Uploading a patch which limits default setting of restrictive permissions (for network server started from the command line) to Java 7. The documentation will describe the whole feature as a Java 7 and higher feature. The present patch does not actually stop anyone from trying to use it with Java 6 on Unix, though (by switching it on explicitly with the property), so that would be an undocumented, unsupported feature. If you think I should actively prohibit using it with that platform, speak out, please. Re-running regressions.
          Hide
          Dag H. Wanvik added a comment -

          Uploading a revised releaseNotes.html to reflect that this is a >= Java 7 feature only.

          Show
          Dag H. Wanvik added a comment - Uploading a revised releaseNotes.html to reflect that this is a >= Java 7 feature only.
          Hide
          Dag H. Wanvik added a comment -

          It seems RestrictiveFilePermissionsTest for this features breaks on some platforms (thanks to Kathey for noticing). Apparently, the ACL view of Posix file system permissions is not available for all Unix versions in JDK 1.7 as I had thought from testing on Solaris 11. I have changed the test now so it runs on Linux as well. Hopefully this solves the issue. Perhaps this will make the test work on IBM's JDK 1.7 also. I'd appreciate it if somebody tried to enable the test again for IBM JVMs to check (RestrictiveFilePermissionsTest is skipped in engine._Suite now).

          Uploading patch derby-5363-followup-linux.

          Show
          Dag H. Wanvik added a comment - It seems RestrictiveFilePermissionsTest for this features breaks on some platforms (thanks to Kathey for noticing). Apparently, the ACL view of Posix file system permissions is not available for all Unix versions in JDK 1.7 as I had thought from testing on Solaris 11. I have changed the test now so it runs on Linux as well. Hopefully this solves the issue. Perhaps this will make the test work on IBM's JDK 1.7 also. I'd appreciate it if somebody tried to enable the test again for IBM JVMs to check (RestrictiveFilePermissionsTest is skipped in engine._Suite now). Uploading patch derby-5363-followup-linux.
          Hide
          Dag H. Wanvik added a comment -

          Committed derby-5363-followup-linux at svn 1179042.

          Show
          Dag H. Wanvik added a comment - Committed derby-5363-followup-linux at svn 1179042.
          Hide
          Dag H. Wanvik added a comment -

          Uploading a slightly improved version: derby-5363-limit-to-java7b.

          Show
          Dag H. Wanvik added a comment - Uploading a slightly improved version: derby-5363-limit-to-java7b.
          Hide
          Dag H. Wanvik added a comment -

          Committed derby-5363-limit-to-java7b at svn 1179320.

          Show
          Dag H. Wanvik added a comment - Committed derby-5363-limit-to-java7b at svn 1179320.
          Hide
          Dag H. Wanvik added a comment -

          Uploaded a new slightly improved version of the release notes.

          Show
          Dag H. Wanvik added a comment - Uploaded a new slightly improved version of the release notes.
          Hide
          Dag H. Wanvik added a comment -

          Uploading a patch derby-5363-followup-unix to fix the problem seen on Solaris not running under ZFS, but it should apply in general.

          It turns out there is no guarantee the the underlying file system supports ACLs even though Files#getFileAttributeView called with aclFileAttributeViewClz.class as an argument returns an object. We also need to call the method:

          FileStore#supportsFileAttributeView(AclFileAttributeView.class)

          to ascertain whether we have support for ACLs. To get at the current FileStore, we need to inquire about that given a path:

          Files.getFileStore(<path>)

          which requires the RuntimePermission "getFileStoreAttributes", hence the current patch's changes to the policy files.

          With the patch, RestrictiveFilePermissionsTest run OK on Solaris/UFS. Rerunning regressions on all platforms.

          Show
          Dag H. Wanvik added a comment - Uploading a patch derby-5363-followup-unix to fix the problem seen on Solaris not running under ZFS, but it should apply in general. It turns out there is no guarantee the the underlying file system supports ACLs even though Files#getFileAttributeView called with aclFileAttributeViewClz.class as an argument returns an object. We also need to call the method: FileStore#supportsFileAttributeView(AclFileAttributeView.class) to ascertain whether we have support for ACLs. To get at the current FileStore, we need to inquire about that given a path: Files.getFileStore(<path>) which requires the RuntimePermission "getFileStoreAttributes", hence the current patch's changes to the policy files. With the patch, RestrictiveFilePermissionsTest run OK on Solaris/UFS. Rerunning regressions on all platforms.
          Hide
          Dag H. Wanvik added a comment -

          Regressions OK so far: Solaris 11/JDK7, Solaris 10/JDK6, Solaris 10/JDK7, Windows Vista/JDK6, Windows Vista/JDK 7. I'll run on Linux, then commit.

          Show
          Dag H. Wanvik added a comment - Regressions OK so far: Solaris 11/JDK7, Solaris 10/JDK6, Solaris 10/JDK7, Windows Vista/JDK6, Windows Vista/JDK 7. I'll run on Linux, then commit.
          Hide
          Dag H. Wanvik added a comment -

          Regressions passed on Linux as well , committed as svn 1180713, resolving.

          Show
          Dag H. Wanvik added a comment - Regressions passed on Linux as well , committed as svn 1180713, resolving.
          Hide
          Knut Anders Hatlen added a comment -

          I assume this part was unintended?

          @@ -747,6 +752,7 @@ nextFile: for (int i = 0; i < list.lengt
          allow = aclEntryTypeClz.getField("ALLOW");

          } catch (NoSuchMethodException e)

          { + e.printStackTrace(); // not Java 7 or higher }

          catch (ClassNotFoundException e) {
          // not Java 7 or higher

          Show
          Knut Anders Hatlen added a comment - I assume this part was unintended? @@ -747,6 +752,7 @@ nextFile: for (int i = 0; i < list.lengt allow = aclEntryTypeClz.getField("ALLOW"); } catch (NoSuchMethodException e) { + e.printStackTrace(); // not Java 7 or higher } catch (ClassNotFoundException e) { // not Java 7 or higher
          Hide
          Dag H. Wanvik added a comment - - edited

          Good catch, yes, remaining test cruft. Fix committed as 1182266.

          Dag

          Show
          Dag H. Wanvik added a comment - - edited Good catch, yes, remaining test cruft. Fix committed as 1182266. Dag
          Hide
          Rick Hillegas added a comment -

          Attaching a modified version of the release note. Correcting a bad href which choked the release notes generator.

          Show
          Rick Hillegas added a comment - Attaching a modified version of the release note. Correcting a bad href which choked the release notes generator.
          Hide
          Kathey Marsden added a comment - - edited

          Are the security manager permissions added for this issue, required for all uses of Derby including embedded?

          "The new code when running on Java 7 on Windows needs the extra RuntimePermission "accessUserInformation" (to determine the file's owner) when run with the Security Manager. I have added that to the default "server.policy" file, and the "template.policy", as well as were needed to run the tests. The tests also needed some more "read" file permissions."

          I also don't understand why the tests needed read permission for the server trace file. Shouldn't t that be derbynet.jar that would need that?

          Sorry for the ancient history question.

          Show
          Kathey Marsden added a comment - - edited Are the security manager permissions added for this issue, required for all uses of Derby including embedded? "The new code when running on Java 7 on Windows needs the extra RuntimePermission "accessUserInformation" (to determine the file's owner) when run with the Security Manager. I have added that to the default "server.policy" file, and the "template.policy", as well as were needed to run the tests. The tests also needed some more "read" file permissions." I also don't understand why the tests needed read permission for the server trace file. Shouldn't t that be derbynet.jar that would need that? Sorry for the ancient history question.
          Hide
          Dag H. Wanvik added a comment - - edited

          Q1) Only iff derby.storage.useDefaultFilePermissions is set to false and you are running with a security manager and VM >= JDK7. So if the property is never set, the permissions shouldn't be necessary on embedded (the default value for derby.storage.useDefaultFilePermissions there is true).
          Q2) I don't remember off the top of my head. I'll try to investigate it.

          Show
          Dag H. Wanvik added a comment - - edited Q1) Only iff derby.storage.useDefaultFilePermissions is set to false and you are running with a security manager and VM >= JDK7. So if the property is never set, the permissions shouldn't be necessary on embedded (the default value for derby.storage.useDefaultFilePermissions there is true). Q2) I don't remember off the top of my head. I'll try to investigate it.
          Hide
          Dag H. Wanvik added a comment - - edited

          Q2: In test NetworkServerControlApiTest, the test performs this operation, ca line 172:

          assertTrue(fileExists(derbysystemhome+"/trace/Server1.trace"));

          now, the Javadoc of File.exists has this stanza:

          throws SecurityException ... If a security manager exists and its SecurityManager.checkRead(java.lang.String) method denies read access to the file

          In the Javadoc for that checkRead overload, we see this:

          This method calls checkPermission with the FilePermission(file,"read") permission.

          so we do need the filePermission "read" permission here granted to the test.

          Show
          Dag H. Wanvik added a comment - - edited Q2: In test NetworkServerControlApiTest, the test performs this operation, ca line 172: assertTrue(fileExists(derbysystemhome+"/trace/Server1.trace")); now, the Javadoc of File.exists has this stanza: throws SecurityException ... If a security manager exists and its SecurityManager.checkRead(java.lang.String) method denies read access to the file In the Javadoc for that checkRead overload, we see this: This method calls checkPermission with the FilePermission(file,"read") permission. so we do need the filePermission "read" permission here granted to the test.

            People

            • Assignee:
              Dag H. Wanvik
              Reporter:
              Dag H. Wanvik
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development