Derby
  1. Derby
  2. DERBY-623

Derby monitor accesses two system properties without using a privileged block when built sane=true

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 10.2.1.6
    • Fix Version/s: 10.2.1.6
    • Component/s: Services
    • Labels:
      None

      Description

      When built with sane=true and testing with the jars these permissions are required to be granted all the way up the stack, currently this means for the tests granting them to the network server jar.

      permission java.util.PropertyPermission "derby.monitor.verbose", "read";
      permission java.util.PropertyPermission "derby.debug.*", "read";

      The engine contains code to read system properties using privileged blocks, this should be used by the monitor.

      1. DERBY-623-2b-javadoc.diff
        3 kB
        Kristian Waagan
      2. DERBY-623-2b-javadoc.stat
        0.2 kB
        Kristian Waagan
      3. DERBY-623-1a.diff
        3 kB
        Kristian Waagan
      4. DERBY-623-1a.stat
        0.2 kB
        Kristian Waagan
      5. DERBY-623-1b.diff
        1 kB
        Kristian Waagan
      6. DERBY-623-1b.stat
        0.2 kB
        Kristian Waagan

        Activity

        Gavin made changes -
        Workflow jira [ 12331109 ] Default workflow, editable Closed status [ 12797708 ]
        Kristian Waagan made changes -
        Status Open [ 1 ] Closed [ 6 ]
        Fix Version/s 10.2.0.0 [ 11187 ]
        Resolution Fixed [ 1 ]
        Hide
        Kristian Waagan added a comment -

        Closing the issue. No problems reported regarding the fix.

        Show
        Kristian Waagan added a comment - Closing the issue. No problems reported regarding the fix.
        Kristian Waagan made changes -
        Status In Progress [ 3 ] Open [ 1 ]
        Hide
        Daniel John Debrunner added a comment -

        Patch derby-623-1b.diff Committed revision 385859

        Show
        Daniel John Debrunner added a comment - Patch derby-623-1b.diff Committed revision 385859
        Kristian Waagan made changes -
        Attachment DERBY-623-1b.diff [ 12324014 ]
        Attachment DERBY-623-1b.stat [ 12324015 ]
        Hide
        Kristian Waagan added a comment -

        Dan, I see your point now! I do still mean we need another Jira though, to track cleaning up BaseMonitor (primarily method 'runWithState').

        'DERBY-623-1b.diff' is a reduced patch, only addressing reading two specific system properties in privileged blocks and updating the test policy file accordingly.
        Patch 'DERBY-623-1a.diff' is now deprecated.
        Tests run on Solaris10, Sun 1.5:
        derbyall - insane - jars: derbynet/runtimeinfo.java, jdbcapi/Stream.java and jdbcapi/SURTest.junit failed.
        derbyall - sane - jars: OK! (stress.multi failed)
        derbyall - sane - classes: NSinSameJVM failed.

        I don't think the errors are caused by the changes, but if anyone else thinks so, speak up!

        Show
        Kristian Waagan added a comment - Dan, I see your point now! I do still mean we need another Jira though, to track cleaning up BaseMonitor (primarily method 'runWithState'). ' DERBY-623 -1b.diff' is a reduced patch, only addressing reading two specific system properties in privileged blocks and updating the test policy file accordingly. Patch ' DERBY-623 -1a.diff' is now deprecated. Tests run on Solaris10, Sun 1.5: derbyall - insane - jars: derbynet/runtimeinfo.java, jdbcapi/Stream.java and jdbcapi/SURTest.junit failed. derbyall - sane - jars: OK! (stress.multi failed) derbyall - sane - classes: NSinSameJVM failed. I don't think the errors are caused by the changes, but if anyone else thinks so, speak up!
        Hide
        Daniel John Debrunner added a comment -

        It seems out of scope to me because the summary talks about reading two specific properties, listed in the description, and the description talks about using existing code with existing privilege blocks.

        Your code addresses reading all system properties and adds a new privilege block, doesn't seem to overlap in my mind.

        I think the patch is fine and can go into the trunk, but say I wanted to merge this up to 10.1 branch, I would be more comfortable merging just the specific fix rather than this additional change that can change the behaviour.

        Show
        Daniel John Debrunner added a comment - It seems out of scope to me because the summary talks about reading two specific properties, listed in the description, and the description talks about using existing code with existing privilege blocks. Your code addresses reading all system properties and adds a new privilege block, doesn't seem to overlap in my mind. I think the patch is fine and can go into the trunk, but say I wanted to merge this up to 10.1 branch, I would be more comfortable merging just the specific fix rather than this additional change that can change the behaviour.
        Hide
        Kristian Waagan added a comment -

        By looking at the description of the bug, I really can't see that the changes I implemented are outside the scope of the bug:
        "The engine contains code to read system properties using privileged blocks, this should be used by the monitor."

        About the only thing changed by patch 'DERBY-623-1a.diff' is that the properties (including 'System.getProperties()') that was previously read outside a privileged block is now read inside one.
        I take your previous comment was related to the parts of the code that use the result of 'System.getProperties()', and removing this should be easy enough for someone that knows the code a little. Based on the comments in the method 'runWithState', I think I'll step down since I don't know the story of the unit tests and the overall function of BaseMonitor.

        In my opinion, your previous comment points in the direction of a new Jira issue.
        That said, discarding the current patch and implementing the changes you talk about as part of this issue is fine with me - but for the reason mentioned above this is not my itch.

        Show
        Kristian Waagan added a comment - By looking at the description of the bug, I really can't see that the changes I implemented are outside the scope of the bug: "The engine contains code to read system properties using privileged blocks, this should be used by the monitor." About the only thing changed by patch ' DERBY-623 -1a.diff' is that the properties (including 'System.getProperties()') that was previously read outside a privileged block is now read inside one. I take your previous comment was related to the parts of the code that use the result of 'System.getProperties()', and removing this should be easy enough for someone that knows the code a little. Based on the comments in the method 'runWithState', I think I'll step down since I don't know the story of the unit tests and the overall function of BaseMonitor. In my opinion, your previous comment points in the direction of a new Jira issue. That said, discarding the current patch and implementing the changes you talk about as part of this issue is fine with me - but for the reason mentioned above this is not my itch.
        Hide
        Daniel John Debrunner added a comment -

        The changes to reading system properties are really outside the scope of this bug. I actually think that the original code should be removed,
        ideally the debug (sane) server should not behave differently to the non-debug (insane) server. There is already a mechanism that works
        in both sane and insane for adding additional modules, debug or otherwise. That is having an additional modules.properties in the class path.

        I believe the code of having system properties set modules should be removed because it can be a security risk, that is enabled
        by just allowing the derby engine to read system properties. Then malicious code has the possibility to change runtime modules,
        by setting system properties, such as changing the authentication module to allow any user. That's why the code is not security
        manager enabled at the moment, though that is not a complete solution, removing the code would be better.

        Show
        Daniel John Debrunner added a comment - The changes to reading system properties are really outside the scope of this bug. I actually think that the original code should be removed, ideally the debug (sane) server should not behave differently to the non-debug (insane) server. There is already a mechanism that works in both sane and insane for adding additional modules, debug or otherwise. That is having an additional modules.properties in the class path. I believe the code of having system properties set modules should be removed because it can be a security risk, that is enabled by just allowing the derby engine to read system properties. Then malicious code has the possibility to change runtime modules, by setting system properties, such as changing the authentication module to allow any user. That's why the code is not security manager enabled at the moment, though that is not a complete solution, removing the code would be better.
        Hide
        Daniel John Debrunner added a comment -

        which tests were tests run? with classes and with jars?

        Show
        Daniel John Debrunner added a comment - which tests were tests run? with classes and with jars?
        Kristian Waagan made changes -
        Other Info [Patch available]
        Hide
        Kristian Waagan added a comment -

        Patch ready for review

        Show
        Kristian Waagan added a comment - Patch ready for review
        Kristian Waagan made changes -
        Attachment DERBY-623-1a.stat [ 12323850 ]
        Attachment DERBY-623-1a.diff [ 12323849 ]
        Hide
        Kristian Waagan added a comment -

        'DERBY-623-1a.diff' is a patch changing BaseMonitor to use privileged blocks for reading system properties and also removing the workaround permissions granted in 'derby_tests.policy'.

        Reading of specific properties are done with 'PropertyUtil.getSystemProperty()'.
        Besides general feedback, I would like feedback on the following issues:

        1) I have not separated reading all system properties (System.getProperties) into its own method, because this is done only here (and one place in testing code). It is also something we should in general not do. Further, keeping it inside the sanity block removes it in insane builds.

        2) Should the swallowed exception be logged somewhere?

        3) Is the order of the 'startServices' calls important? If not, I would move the call inside the sanity block to the end. I also removed the call for insane builds, since the variable passed in is only set for sane builds.

        The patch is a mix of my own work and the patch I got by Rick Hillegas on derby-dev (subject 'Security manager problems with Class.forName()').

        Show
        Kristian Waagan added a comment - ' DERBY-623 -1a.diff' is a patch changing BaseMonitor to use privileged blocks for reading system properties and also removing the workaround permissions granted in 'derby_tests.policy'. Reading of specific properties are done with 'PropertyUtil.getSystemProperty()'. Besides general feedback, I would like feedback on the following issues: 1) I have not separated reading all system properties (System.getProperties) into its own method, because this is done only here (and one place in testing code). It is also something we should in general not do. Further, keeping it inside the sanity block removes it in insane builds. 2) Should the swallowed exception be logged somewhere? 3) Is the order of the 'startServices' calls important? If not, I would move the call inside the sanity block to the end. I also removed the call for insane builds, since the variable passed in is only set for sane builds. The patch is a mix of my own work and the patch I got by Rick Hillegas on derby-dev (subject 'Security manager problems with Class.forName()').
        Kristian Waagan made changes -
        Attachment DERBY-623-2a-javadoc.stat [ 12323766 ]
        Kristian Waagan made changes -
        Attachment DERBY-623-2a-javadoc.diff [ 12323767 ]
        Kristian Waagan made changes -
        Attachment DERBY-623-2b-javadoc.diff [ 12323771 ]
        Attachment DERBY-623-2b-javadoc.stat [ 12323772 ]
        Hide
        Kristian Waagan added a comment -

        'DERBY-623-2b-javadoc.diff' is a patch fixing spelling errors for a patch fixing spelling errors
        See description for rev 2a for patch description.

        Must have had a blackout or something. Sorry for the noise...

        Show
        Kristian Waagan added a comment - ' DERBY-623 -2b-javadoc.diff' is a patch fixing spelling errors for a patch fixing spelling errors See description for rev 2a for patch description. Must have had a blackout or something. Sorry for the noise...
        Kristian Waagan made changes -
        Attachment DERBY-623-2a-javadoc.stat [ 12323766 ]
        Attachment DERBY-623-2a-javadoc.diff [ 12323767 ]
        Hide
        Kristian Waagan added a comment -

        A separate patch ('DERBY-623-2a-javadoc.diff') for a few Javadoc fixes. First, a spelling error is corrected several places ('privledged' -> 'priviledged') and the prefix for properties have been changed from 'db2j' to 'derby.' (only documentation is changed in this patch, implementation is untouched).

        A committer should be able to both review and commit this small patch (8 lines changed).

        DERBY-623-2a-javadoc.diff does not resolve this issue, it is only an additional documentation fix.

        Show
        Kristian Waagan added a comment - A separate patch (' DERBY-623 -2a-javadoc.diff') for a few Javadoc fixes. First, a spelling error is corrected several places ('privledged' -> 'priviledged') and the prefix for properties have been changed from 'db2j' to 'derby.' (only documentation is changed in this patch, implementation is untouched). A committer should be able to both review and commit this small patch (8 lines changed). DERBY-623 -2a-javadoc.diff does not resolve this issue, it is only an additional documentation fix.
        Kristian Waagan made changes -
        Status Open [ 1 ] In Progress [ 3 ]
        Kristian Waagan made changes -
        Assignee Kristian Waagan [ kristwaa ]
        Hide
        Daniel John Debrunner added a comment -

        To reproduce this bug remove these permissions from derby_test.policy

        // BUG DERBY-623 - sane=true
        permission java.util.PropertyPermission "derby.monitor.verbose", "read";
        permission java.util.PropertyPermission "derby.debug.*", "read";

        Build with sane=true and then run derbyall with the sane jars. Probably will see the issue by only running derbynetclientmats and derbynetmats suites.

        Show
        Daniel John Debrunner added a comment - To reproduce this bug remove these permissions from derby_test.policy // BUG DERBY-623 - sane=true permission java.util.PropertyPermission "derby.monitor.verbose", "read"; permission java.util.PropertyPermission "derby.debug.*", "read"; Build with sane=true and then run derbyall with the sane jars. Probably will see the issue by only running derbynetclientmats and derbynetmats suites.
        Hide
        Daniel John Debrunner added a comment -

        The engine method to read a system property is:

        org.apache.derby.iapi.services.property.PropertyUtil.getSystemProperty

        Show
        Daniel John Debrunner added a comment - The engine method to read a system property is: org.apache.derby.iapi.services.property.PropertyUtil.getSystemProperty
        Mike Matrigali made changes -
        Field Original Value New Value
        Component/s Services [ 11415 ]
        Daniel John Debrunner created issue -

          People

          • Assignee:
            Kristian Waagan
            Reporter:
            Daniel John Debrunner
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development