Code paths for db restore do not use doPrivileged-calls, causing SecurityException

• Key: DERBY-2556
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: Kristian Waagan
• Reporter: Kristian Waagan
• Component/s: Services
• Affects Version/s: 10.2.2.0, 10.3.1.4
• Fix Version/s: 10.3.1.4

Description

When using 'createFrom' or 'restoreFrom' in the JDBC url to restore a database from a backup image, a SecurityException is thrown even though the policyfile for codebase derby.jar is correctly configured (giving Derby access to the backup image).

A few comments on this issue can be found here (and in subsequent comments): https://issues.apache.org/jira/browse/DERBY-1001#action_12439811

A workaround is wrapping the connection call in doPrivileged at the "application-level code", or granting the required permissions to the application codebase as well.

Comments

[Kristian Waagan]

See DERBY-1001 for a stack trace and some additional comments/info.

[Kathey Marsden]

I applied the patch for DERBY-1001 and found that testRestoreFrom seems to work just fine. Is it possible that this issue has already been fixed or am I missing something?

[Kristian Waagan]

The test includes a workaround, where the getConnection-call is wrapped in a doPrivileged-call (see getPrivilegedConnection). If you remove these, I suppose you should still see the error. DERBY-2555 is the issue tracking this task. If you don't beat me to it, I can create a patch for it tomorrow, which you can apply and see if you see the problem reappear.

I would recommend reading the comments in DERBY-1001, as it was not clear to me at first what the correct solution would be. Dan gave his opinion, and when I thought about the issue again, his suggestion seemed right.

[Kristian Waagan]

I have uploaded a patch for DERBY-2555. If it is applied, the EncryptionKeyXXXTests will fail. I believe it is with the error I created this issue for.

[Kathey Marsden]

Here is preliminary patch for this issue which also includes the changes for DERBY-2555.
I'll commit tomorrow if all tests pass tonight and I hear no objections.

Thanks

Kathey

[Kristian Waagan]

I reviewed the patch, and have some questions/comments:

 1) Then run method is declared to throw a number of exceptions, but only SecurityException can be thrown.
 2) The patch for DERBY-2555 seems to be included. This doesn't really matter, but the two patches can be committed separately (though only in one order).
 3) The newly added method mixes tabs and spaces for indentation (also on the same line).
 4) Missing JavaDoc (not required?)


thanks,

[Kathey Marsden]

Thanks Kristian for the review. I made the changes you suggested and committed the change.
Date: Wed May 9 15:01:34 2007
New Revision: 536677

[Kristian Waagan]

Reopening the issue due to failures in the tinderbox test.

Stack trace with debug/sane information below, and the traces look the same for all failures.

============= begin nested exception, level (1) ===========
java.sql.SQLException: Java exception: 'access denied (java.io.FilePermission extinout/backups/encryptionKeyDBToCreateFrom read): java.security.AccessControlException'.
    at java.security.AccessControlContext.checkPermission(AccessControlContext.java:264)
    at java.security.AccessController.checkPermission(AccessController.java:427)
    at java.lang.SecurityManager.checkPermission(SecurityManager.java:532)
    at java.lang.SecurityManager.checkRead(SecurityManager.java:871)
    at java.io.File.list(File.java:935)
    at org.apache.derby.impl.store.raw.data.BaseDataFileFactory.restoreDataDirectory(BaseDataFileFactory.java:2516)
    at org.apache.derby.impl.store.raw.data.BaseDataFileFactory.boot(BaseDataFileFactory.java:349)
    at org.apache.derby.impl.services.monitor.BaseMonitor.boot(BaseMonitor.java:1994)
    at org.apache.derby.impl.services.monitor.TopService.bootModule(TopService.java:291)
    at org.apache.derby.impl.services.monitor.BaseMonitor.startModule(BaseMonitor.java:546)
    at org.apache.derby.iapi.services.monitor.Monitor.bootServiceModule(Monitor.java:419)
    at org.apache.derby.impl.store.raw.RawStore.boot(RawStore.java:183)
    at org.apache.derby.impl.services.monitor.BaseMonitor.boot(BaseMonitor.java:1994)
    at org.apache.derby.impl.services.monitor.TopService.bootModule(TopService.java:291)
    at org.apache.derby.impl.services.monitor.BaseMonitor.startModule(BaseMonitor.java:546)
    at org.apache.derby.iapi.services.monitor.Monitor.bootServiceModule(Monitor.java:419)
    at org.apache.derby.impl.store.access.RAMAccessManager.boot(RAMAccessManager.java:985)
    at org.apache.derby.impl.services.monitor.BaseMonitor.boot(BaseMonitor.java:1994)
    at org.apache.derby.impl.services.monitor.TopService.bootModule(TopService.java:291)
    at org.apache.derby.impl.services.monitor.BaseMonitor.startModule(BaseMonitor.java:546)
    at org.apache.derby.iapi.services.monitor.Monitor.bootServiceModule(Monitor.java:419)
    at org.apache.derby.impl.db.BasicDatabase.bootStore(BasicDatabase.java:767)
    at org.apache.derby.impl.db.BasicDatabase.boot(BasicDatabase.java:196)
    at org.apache.derby.impl.services.monitor.BaseMonitor.boot(BaseMonitor.java:1994)
    at org.apache.derby.impl.services.monitor.TopService.bootModule(TopService.java:291)
    at org.apache.derby.impl.services.monitor.BaseMonitor.bootService(BaseMonitor.java:1829)
    at org.apache.derby.impl.services.monitor.BaseMonitor.startProviderService(BaseMonitor.java:1695)
    at org.apache.derby.impl.services.monitor.BaseMonitor.findProviderAndStartService(BaseMonitor.java:1575)
    at org.apache.derby.impl.services.monitor.BaseMonitor.startPersistentService(BaseMonitor.java:994)
    at org.apache.derby.iapi.services.monitor.Monitor.startPersistentService(Monitor.java:542)
    at org.apache.derby.impl.jdbc.EmbedConnection.bootDatabase(EmbedConnection.java:1796)
    at org.apache.derby.impl.jdbc.EmbedConnection.<init>(EmbedConnection.java:253)
    at org.apache.derby.impl.jdbc.EmbedConnection30.<init>(EmbedConnection30.java:73)
    at org.apache.derby.jdbc.Driver30.getNewEmbedConnection(Driver30.java:80)
    at org.apache.derby.jdbc.InternalDriver.connect(InternalDriver.java:209)
    at org.apache.derby.jdbc.EmbeddedDataSource.getConnection(EmbeddedDataSource.java:479)
    at org.apache.derby.jdbc.EmbeddedDataSource.getConnection(EmbeddedDataSource.java:423)
    at org.apache.derbyTesting.functionTests.tests.store.EncryptionKeyTest.getConnection(EncryptionKeyTest.java:580)
    at org.apache.derbyTesting.functionTests.tests.store.EncryptionKeyTest.testCreateDbFromBackup(EncryptionKeyTest.java:279)

[Kristian Waagan]

There is at least a problem in the following code in BaseDataFileFactory. Not sure how to interpret the comment, but I think adding more doPrivileged-blocks is the right approach. Any comments on this?

private void restoreDataDirectory(String backupPath)
        throws StandardException
{
        File bsegdir; //segment directory in the backup
        File backupRoot = new java.io.File(backupPath); //root dir of backup db

        /* To be safe we first check if the backup directory exist and it has
         * atleast one seg* directory before removing the current data directory.
         *
         * This will fail with a security exception unless the database engine
         * and all its callers have permission to read the backup directory.
         */
        String[] bfilelist = backupRoot.list();
        if(bfilelist !=null)
        {
            boolean segmentexist = false;
            for (int i = 0; i < bfilelist.length; i++)
            {
                //check if it is a seg* directory
                if(bfilelist[i].startsWith("seg"))
                {
                    bsegdir = new File(backupRoot , bfilelist[i]);
                    if(bsegdir.exists() && bsegdir.isDirectory())
                    {
                        segmentexist = true;
                        break;
                    }
                }
            }

[Kristian Waagan]

'derby-2556-2a_whitespace-javadoc.diff' replaces a few tabs with spaces in the JavaDoc for the newly added privExists-methods. Also updated the JavaDoc.

Committed to trunk with revision 536856.

[Kathey Marsden]

> There is at least a problem in the following code in BaseDataFileFactory. Not sure how to interpret the comment, >but I think adding more doPrivileged-blocks is the right approach. Any comments on this?

Yes, I do think the exists() need privilege blocks for this code.

 Thanks Kristian for looking at this. I am not sure how my tests passed while the tinderbox failed but I will take a closer look.

Kathey

[Kathey Marsden]

I am still a bit baffled and am seeing these tests pass. If I can't figure out what is going on this morning, I will back out the test change.
java junit.textui.TestRunner org.apache.derbyTesting.functionTests.tests.store.EncryptionKeyAESTest
........
Time: 23.781

OK (8 tests)

 java junit.textui.TestRunner org.apache.derbyTesting.functionTests.tests.store.EncryptionKeyBlowfishTest
........
Time: 25.094

OK (8 tests)

[Kristian Waagan]

The test failed for me when I ran it individually, but I don't think I saw the previous errors when I ran it outside suites.All. Maybe I'll look at it later.

I made an alternative patch, which seems to fix the problem that caused 9 failures. It is only half-baked, and the previous patch for this issue is also required. A final patch will have to be made anyway, in one or the other direction... (can be done in two steps though)

The patch 'derby-2556-3a_alternative-patch.diff' adds a utility class for running file operations in a privileged block. It is kept very simple, which does duplicate some code, but reduces complexity.
Any comments on this issue?

As far as I can tell, there are more places where the "machinery" to run in a privileged block can be simplified by reuse. I will create a separate Jira for this if the approach is acceptable.

To finish this patch, the added privExists method must be removed, and the appropriate calls to PrivilegedFileOps must be inserted. If you test the patch, don't forget to apply the patch for DERBY-2555 as well!

[Kathey Marsden]

Thanks Kristian for the patch.

I did not notice your alternative patch until after I had made another patch myself. I preferred yours so made the changes to StorageFactoryService and corrected a small compile error in the patch.
I seem to be having inconsistant results running these tests. Now I am seeing errors starting the database with the trace below. This is what I was getting when I first tried your patch and may be related to my environment. Anyway could you give it a try and commit if it works ok for you.

Thanks

Kathey

....XSDG8 : Unable to copy directory 'verifyKey.dat' to 'C:\test\extinout\restor
erifyKey.dat' during restore. Please make sure that there is enough space and pe
java.sql.SQLException: Unable to copy directory 'verifyKey.dat' to 'C:\test\exti
mRestored\verifyKey.dat' during restore. Please make sure that there is enough s
        at org.apache.derby.iapi.error.StandardException.newException(StandardEx
        at org.apache.derby.impl.store.raw.data.BaseDataFileFactory.privRestoreD
02)
        at org.apache.derby.impl.store.raw.data.BaseDataFileFactory.run(BaseData
        at java.security.AccessController.doPrivileged1(Native Method)
        at java.security.AccessController.doPrivileged(AccessController.java(Com
        at org.apache.derby.impl.store.raw.data.BaseDataFileFactory.restoreDataD
        at org.apache.derby.impl.store.raw.data.BaseDataFileFactory.boot(BaseDat
        at org.apache.derby.impl.services.monitor.BaseMonitor.boot(BaseMonitor.j
        at org.apache.derby.impl.services.monitor.TopService.bootModule(TopServi
        at org.apache.derby.impl.services.monitor.BaseMonitor.startModule(BaseMo
        at org.apache.derby.iapi.services.monitor.Monitor.bootServiceModule(Moni
        at org.apache.derby.impl.store.raw.RawStore.boot(RawStore.java:183)
        at org.apache.derby.impl.services.monitor.BaseMonitor.boot(BaseMonitor.j
        at org.apache.derby.impl.services.monitor.TopService.bootModule(TopServi
        at org.apache.derby.impl.services.monitor.BaseMonitor.startModule(BaseMo
        at org.apache.derby.iapi.services.monitor.Monitor.bootServiceModule(Moni
        at org.apache.derby.impl.store.access.RAMAccessManager.boot(RAMAccessMan
        at org.apache.derby.impl.services.monitor.BaseMonitor.boot(BaseMonitor.j
        at org.apache.derby.impl.services.monitor.TopService.bootModule(TopServi
        at org.apache.derby.impl.services.monitor.BaseMonitor.startModule(BaseMo
        at org.apache.derby.iapi.services.monitor.Monitor.bootServiceModule(Moni
        at org.apache.derby.impl.db.BasicDatabase.bootStore(BasicDatabase.java:7
        at org.apache.derby.impl.db.BasicDatabase.boot(BasicDatabase.java:196)
        at org.apache.derby.impl.services.monitor.BaseMonitor.boot(BaseMonitor.j
        at org.apache.derby.impl.services.monitor.TopService.bootModule(TopServi
        at org.apache.derby.impl.services.monitor.BaseMonitor.bootService(BaseMo
        at org.apache.derby.impl.services.monitor.BaseMonitor.startProviderServi
        at org.apache.derby.impl.services.monitor.BaseMonitor.findProviderAndSta
        at org.apache.derby.impl.services.monitor.BaseMonitor.startPersistentSer
        at org.apache.derby.iapi.services.monitor.Monitor.startPersistentService
        at org.apache.derby.impl.jdbc.EmbedConnection.bootDatabase(EmbedConnecti
        at org.apache.derby.impl.jdbc.EmbedConnection.<init>(EmbedConnection.jav
        at org.apache.derby.impl.jdbc.EmbedConnection30.<init>(EmbedConnection30
        at org.apache.derby.jdbc.Driver30.getNewEmbedConnection(Driver30.java:80
        at org.apache.derby.jdbc.InternalDriver.connect(InternalDriver.java:209)
        at org.apache.derby.jdbc.EmbeddedDataSource.getConnection(EmbeddedDataSo
        at org.apache.derby.jdbc.EmbeddedDataSource.getConnection(EmbeddedDataSo
        at org.apache.derbyTesting.functionTests.tests.store.EncryptionKeyTest.g

        at org.apache.derbyTesting.functionTests.tests.store.EncryptionKeyTest.a
        at org.apache.derbyTesting.functionTests.tests.store.EncryptionKeyTest$2
        at java.security.AccessController.doPrivileged1(Native Method)
        at java.security.AccessController.doPrivileged(AccessController.java(Com
        at org.apache.derbyTesting.functionTests.tests.store.EncryptionKeyTest.g
.java:552)
        at org.apache.derbyTesting.functionTests.tests.store.EncryptionKeyTest.t
java:280)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAcces
        at java.lang.reflect.Method.invoke(Method.java(Compiled Code))
        at junit.framework.TestCase.runTest(TestCase.java:154)
        at junit.framework.TestCase.runBare(TestCase.java:127)
        at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:
        at junit.framework.TestResult$1.protect(TestResult.java:106)
        at junit.framework.TestResult.runProtected(TestResult.java:124)
        at junit.framework.TestResult.run(TestResult.java:109)
        at junit.framework.TestCase.run(TestCase.java:118)
        at junit.framework.TestSuite.runTest(TestSuite.java:208)
        at junit.framework.TestSuite.run(TestSuite.java:203)
        at junit.extensions.TestDecorator.basicRun(TestDecorator.java:22)
        at junit.extensions.TestSetup$1.protect(TestSetup.java:19)
        at junit.framework.TestResult.runProtected(TestResult.java:124)
        at junit.extensions.TestSetup.run(TestSetup.java:23)
        at junit.textui.TestRunner.doRun(TestRunner.java:116)
        at junit.textui.TestRunner.start(TestRunner.java:172)
        at junit.textui.TestRunner.main(TestRunner.java:138)

[Kristian Waagan]

Committed 'derby-2556-4a_alternative-patch.diff' to trunk with revision 537735.

I ran derbyall/suites.All four times (once without DERBY-2555, the rest with). No failures seen. I plan to commit DERBY-2555 tomorrow, after a tinderbox test has confirmed that this patch is okay by itself. The tests were run with Sun JVMs 1.4, 1.5 and 1.6, all on Solaris 10.
If the tinderbox fails again, there must be something funny going on with my environment.

Thanks for the additional work on the patch Kathey.

[Kristian Waagan]

Problems seem to be fixed. Closing issue.

[Daniel John Debrunner]

PrivilegedFileOps is a utility class with privileged blocks which is strongly discouraged by Java docs (see comment in DERBY-3745).

This fix needs to be re-worked to not open uch security holes.

[Daniel John Debrunner]

Assigning to original coder

[Kristian Waagan]

I'll have a look at this tomorrow and try to get it done before the 10.4 release.

[Kristian Waagan]

Patch 5a removes the utility class and duplicates the required code in a few places.
I decided not to write separate classes for the various Privileged[Exception]Actions, because they are all so simple.
It would be nice if someone could look at the permission changes I did.

Running suites.All/derbyall.
Patch ready for review.

[Kristian Waagan]

I'm still getting three errors in deryball, which I guess is related to missing file properties.
I'll upload a new revision tomorrow, but the other changes in the patch can still be reviewed.

[Kristian Waagan]

Revision 5b runs without failures (derbyall and suites.All).

I plan to commit it tomorrow.

[Dag H. Wanvik]

Looks good,

 +1

Minor note, why not factor out the privileged ops as private methods in
BaseDataFileFactory as well (e.g. as in Export). Would improve readabilty I think.

[Kristian Waagan]

Committed patch 5b to trunk with revision 691576.

Thanks for looking at the patch, Dag.
I decided to not factor out the privileged ops into private methods in BaseDataFileFactory because each File method is called only once, and there is a run method calling the same File methods that doesn't require a separate AccessController.doPrivileged call. The reason is because the run method itself is executed in a privileged block. I thought it would be confusing to add methods and not use them consistently.

I agree on the readability aspect though, and factoring out the methods is very easy.

Regarding backporting, I think it will require some manual changes due to a test change. The GetCurrentProperties test was recently converted to JUnit, and a different policy file would have to be modified. I also got a conflict in DssTrace, maybe because the functionality to create the trace directory has been added?

Does anyone have thoughts on the factoring and backporting issues?

[Kristian Waagan]

Created subtask DERBY-3864 to track the PrivilegedFileOps issue (i.e. affects and fix versions).

[Kathey Marsden]

I wonder if this issue can be resolved.

[Kristian Waagan]

Thanks for bringing this up again, Kathey.

I'm closing the issue, and I'm not planning to port it to 10.4.
There are conflicts in the merge, and in my view the potential security threat is limited (exists, isDirectory, list, mkdirs).

In case anyone feel differently, porting the fix is doable with some manual changes.