Derby
  1. Derby
  2. DERBY-5507

Orderly shutdown fails if you are using BUILTIN authentication and turn on derby.database.propertiesOnly

    Details

    • Urgency:
      Normal
    • Bug behavior facts:
      Security

      Description

      The following script raises an assertion on the last line. We are failing during the encryption of the password. The assertion prints out the plaintext of the password. I ran the script with the following command line:

      java \
      -Dderby.connection.requireAuthentication=true \
      -Dderby.authentication.provider=BUILTIN \
      -Dderby.user.test_dbo=test_dbopassword \
      org.apache.derby.tools.ij $SCRIPT

      Here is the script:

      connect 'jdbc:derby:memory:db;create=true;user=test_dbo;password=test_dbopassword';

      call syscs_util.syscs_set_database_property( 'derby.connection.requireAuthentication', 'true' );
      call syscs_util.syscs_set_database_property( 'derby.authentication.provider', 'BUILTIN' );

      – shutdown works correctly if you comment out the following two lines
      call syscs_util.syscs_set_database_property( 'derby.user.test_dbo', 'test_dbopassword' );
      call syscs_util.syscs_set_database_property( 'derby.database.propertiesOnly', 'true' );

      – fails to authenticate correct credentials
      connect 'jdbc:derby:memory:db;shutdown=true;user=test_dbo;password=test_dbopassword';

      Here is the assertion printed on the screen:

      ERROR XJ001: Java exception: 'ASSERT FAILED Unknown authentication scheme for token test_dbopassword: org.apache.derby.shared.common.sanity.AssertFailure'.

      Here is the stack trace in derby.log:

      org.apache.derby.shared.common.sanity.AssertFailure: ASSERT FAILED Unknown authentication scheme for token test_dbopassword
      at org.apache.derby.shared.common.sanity.SanityManager.THROWASSERT(SanityManager.java:162)
      at org.apache.derby.shared.common.sanity.SanityManager.THROWASSERT(SanityManager.java:147)
      at org.apache.derby.impl.jdbc.authentication.BasicAuthenticationServiceImpl.encryptPasswordUsingStoredAlgorithm(BasicAuthenticationServiceImpl.java:282)
      at org.apache.derby.impl.jdbc.authentication.BasicAuthenticationServiceImpl.authenticateUser(BasicAuthenticationServiceImpl.java:199)
      at org.apache.derby.impl.jdbc.authentication.AuthenticationServiceBase.authenticate(AuthenticationServiceBase.java:279)
      at org.apache.derby.impl.jdbc.EmbedConnection.checkUserCredentials(EmbedConnection.java:1220)
      at org.apache.derby.impl.jdbc.EmbedConnection.<init>(EmbedConnection.java:422)
      at org.apache.derby.impl.jdbc.EmbedConnection30.<init>(EmbedConnection30.java:73)
      at org.apache.derby.impl.jdbc.EmbedConnection40.<init>(EmbedConnection40.java:51)
      at org.apache.derby.jdbc.Driver40.getNewEmbedConnection(Driver40.java:70)
      at org.apache.derby.jdbc.InternalDriver.connect(InternalDriver.java:255)
      at org.apache.derby.jdbc.AutoloadedDriver.connect(AutoloadedDriver.java:143)
      at java.sql.DriverManager.getConnection(DriverManager.java:582)
      at java.sql.DriverManager.getConnection(DriverManager.java:154)
      at org.apache.derby.impl.tools.ij.ij.dynamicConnection(ij.java:1528)
      at org.apache.derby.impl.tools.ij.ij.ConnectStatement(ij.java:1358)
      at org.apache.derby.impl.tools.ij.ij.ijStatement(ij.java:1143)
      at org.apache.derby.impl.tools.ij.utilMain.runScriptGuts(utilMain.java:347)
      at org.apache.derby.impl.tools.ij.utilMain.go(utilMain.java:245)
      at org.apache.derby.impl.tools.ij.Main.go(Main.java:229)
      at org.apache.derby.impl.tools.ij.Main.mainCore(Main.java:184)
      at org.apache.derby.impl.tools.ij.Main.main(Main.java:75)
      at org.apache.derby.tools.ij.main(ij.java:59)

      1. d5507-1a.diff
        3 kB
        Knut Anders Hatlen

        Issue Links

          Activity

          Hide
          Knut Anders Hatlen added a comment -

          The failing ASSERT was added in 10.6.1.0 as part of DERBY-4483, but the repro also fails on earlier versions. On 10.5.1.1:

          ij> – fails to authenticate correct credentials
          connect 'jdbc:derby:memory:db;shutdown=true;user=test_dbo;password=test_dbopassword';
          ERROR 08004: Connection authentication failure occurred. Reason: Invalid authentication..

          It looks as if the password is stored as plaintext in the database if it is also defined in a system property.

          Show
          Knut Anders Hatlen added a comment - The failing ASSERT was added in 10.6.1.0 as part of DERBY-4483 , but the repro also fails on earlier versions. On 10.5.1.1: ij> – fails to authenticate correct credentials connect 'jdbc:derby:memory:db;shutdown=true;user=test_dbo;password=test_dbopassword'; ERROR 08004: Connection authentication failure occurred. Reason: Invalid authentication.. It looks as if the password is stored as plaintext in the database if it is also defined in a system property.
          Hide
          Knut Anders Hatlen added a comment - - edited

          Here's a minimal repro that shows the problem:

          $ java -Dderby.user.abc=def -jar derbyrun.jar ij
          ij version 10.9
          ij> connect 'jdbc:derby:memory:db;create=true';
          ij> call syscs_util.syscs_set_database_property('derby.user.abc', 'test');
          0 rows inserted/updated/deleted
          ij> values syscs_util.syscs_get_database_property('derby.user.abc');
          1
          --------------------------------------------------------------------------------------------------------------------------------
          test

          1 row selected

          The expected result is that the password is stored as a hashed token, which is what happens if the derby.user.<NAME> system property isn't set:

          $ java -jar derbyrun.jar ij
          ij version 10.9
          ij> connect 'jdbc:derby:memory:db;create=true';
          ij> call syscs_util.syscs_set_database_property('derby.user.abc', 'test');
          0 rows inserted/updated/deleted
          ij> values syscs_util.syscs_get_database_property('derby.user.abc');
          1
          --------------------------------------------------------------------------------------------------------------------------------
          3b6170e0f1ade11debb6732029c267095e092b5b43ff271d4f8d9158cb004322f38b:SHA-256

          1 row selected

          Show
          Knut Anders Hatlen added a comment - - edited Here's a minimal repro that shows the problem: $ java -Dderby.user.abc=def -jar derbyrun.jar ij ij version 10.9 ij> connect 'jdbc:derby:memory:db;create=true'; ij> call syscs_util.syscs_set_database_property('derby.user.abc', 'test'); 0 rows inserted/updated/deleted ij> values syscs_util.syscs_get_database_property('derby.user.abc'); 1 -------------------------------------------------------------------------------------------------------------------------------- test 1 row selected The expected result is that the password is stored as a hashed token, which is what happens if the derby.user.<NAME> system property isn't set: $ java -jar derbyrun.jar ij ij version 10.9 ij> connect 'jdbc:derby:memory:db;create=true'; ij> call syscs_util.syscs_set_database_property('derby.user.abc', 'test'); 0 rows inserted/updated/deleted ij> values syscs_util.syscs_get_database_property('derby.user.abc'); 1 -------------------------------------------------------------------------------------------------------------------------------- 3b6170e0f1ade11debb6732029c267095e092b5b43ff271d4f8d9158cb004322f38b:SHA-256 1 row selected
          Hide
          Knut Anders Hatlen added a comment -

          Hashing of the password before storing it is skipped because of this code in PropertyValidation.doValidateApplyAndMap():

          // if this property should not be used then
          // don't call apply. This depends on where
          // the old value comes from
          // SET_IN_JVM - property will not be used
          // SET_IN_DATABASE - propery will be used
          // SET_IN_APPLICATION - will become SET_IN_DATABASE
          // NOT_SET - will become SET_IN_DATABASE

          if (!dbOnlyProperty && key.startsWith("derby."))

          { if (PropertyUtil.whereSet(key, d) == PropertyUtil.SET_IN_JVM) continue; }

          The repros pass when this code is commented out, but I suppose that the code is there for a reason so this isn't the proper fix.

          Show
          Knut Anders Hatlen added a comment - Hashing of the password before storing it is skipped because of this code in PropertyValidation.doValidateApplyAndMap(): // if this property should not be used then // don't call apply. This depends on where // the old value comes from // SET_IN_JVM - property will not be used // SET_IN_DATABASE - propery will be used // SET_IN_APPLICATION - will become SET_IN_DATABASE // NOT_SET - will become SET_IN_DATABASE if (!dbOnlyProperty && key.startsWith("derby.")) { if (PropertyUtil.whereSet(key, d) == PropertyUtil.SET_IN_JVM) continue; } The repros pass when this code is commented out, but I suppose that the code is there for a reason so this isn't the proper fix.
          Hide
          Knut Anders Hatlen added a comment -

          FWIW, no regression tests failed when the problematic piece of code in PropertyValidation.doValidateApplyAndMap() was commented out.

          Show
          Knut Anders Hatlen added a comment - FWIW, no regression tests failed when the problematic piece of code in PropertyValidation.doValidateApplyAndMap() was commented out.
          Hide
          Rick Hillegas added a comment -

          Hm, that mysterious piece of code goes back to the first release of Derby so there are no clues available in our code archaeology. Could someone with access to the original Cloudscape repository and bug tracker try to chase this one down? Thanks.

          Show
          Rick Hillegas added a comment - Hm, that mysterious piece of code goes back to the first release of Derby so there are no clues available in our code archaeology. Could someone with access to the original Cloudscape repository and bug tracker try to chase this one down? Thanks.
          Hide
          Myrna van Lunteren added a comment -

          Unfortunately, the original Cloudscape repository comments do not reference bug trackers for this change.

          I did follow this code snippet (minus the 'derby' clause, of course) through the creation of the PropertyValidation class from code in the PropertyConglomerate class to its first inclusion on Aug 27, 1999, by djd, but the only comment on the check-in is: "Lock manager properties" (cloudscape revision 11958, for reference).

          Before that, ValidateAndApplyMap looked like this:

          private Serializable validateApplyAndMap(TransactionController tc,
          String key, Serializable value)
          throws StandardException
          {
          Dictionary d = new Hashtable();
          getProperties(tc,d,false/!stringsOnly/,false/!defaultsOnly/);
          Serializable mappedValue = null;
          if (notifyOnSet != null) {
          synchronized (this) {

          for (int i = 0; i < notifyOnSet.size() ; i++)

          { PropertySetCallback psc = (PropertySetCallback) notifyOnSet.elementAt(i); if (!psc.validate(key, value, d)) continue; Serviceable s; if ((s = psc.apply(key,value,d)) != null) ((TransactionManager) tc).addPostCommitWork(s); if (mappedValue == null) mappedValue = psc.map(key, value, d); }

          }
          }

          //
          // RESOLVE: log device cannot be changed on the fly right now
          if (key.equals(Attribute.LOG_DEVICE))
          throw RawStoreStatementException.cannotChangeLogDevice();

          if (mappedValue == null)
          return value;
          else
          return mappedValue;
          }

          At the time of this change, the method AddPropertySetNotification was also considerably changed, it had code added that also mentioned those SET_IN_* values/comments;
          // set up the initial values by calling the validate and apply methods.
          // the map methods are not called as they will have been called
          // at runtime when the user set the property.
          [...more code added...]
          if (!who.validate(key, value, d))
          continue;

          // SET_IN_JVM - need to get value
          // SET_IN_DATABASE - use the stored value
          // SET_IN_APPLICATION - need to get value
          // NOT_SET - can't happen cos it does exist in d

          if (!dbOnly)

          { int whereSet = PropertyUtil.whereSet(key, d); if (whereSet != PropertyUtil.SET_IN_DATABASE) value = PropertyUtil.getSystemProperty(key); }

          [...]
          but that code was subsequently removed again (in cloudscape revision 12007, on Aug 30, 1999, by djd) with check-in comment:
          "Fix bug with missing read lock configuration parameters."

          At that time, (in addition to adding of a locks.waitTimeout property), it looks like an init method was added to PropertySetCallback, to SinglePool, some dummy init methods were added to some other classes, and PropertyUtil got a new method with signature:
          public static String getPropertyFromSet(boolean dbOnly, Dictionary set, String key)

          Perhaps the code snippet in the method doValidateAndApplyMap should also have been removed at that time?

          Show
          Myrna van Lunteren added a comment - Unfortunately, the original Cloudscape repository comments do not reference bug trackers for this change. I did follow this code snippet (minus the 'derby' clause, of course) through the creation of the PropertyValidation class from code in the PropertyConglomerate class to its first inclusion on Aug 27, 1999, by djd, but the only comment on the check-in is: "Lock manager properties" (cloudscape revision 11958, for reference). Before that, ValidateAndApplyMap looked like this: private Serializable validateApplyAndMap(TransactionController tc, String key, Serializable value) throws StandardException { Dictionary d = new Hashtable(); getProperties(tc,d,false/ !stringsOnly /,false/ !defaultsOnly /); Serializable mappedValue = null; if (notifyOnSet != null) { synchronized (this) { for (int i = 0; i < notifyOnSet.size() ; i++) { PropertySetCallback psc = (PropertySetCallback) notifyOnSet.elementAt(i); if (!psc.validate(key, value, d)) continue; Serviceable s; if ((s = psc.apply(key,value,d)) != null) ((TransactionManager) tc).addPostCommitWork(s); if (mappedValue == null) mappedValue = psc.map(key, value, d); } } } // // RESOLVE: log device cannot be changed on the fly right now if (key.equals(Attribute.LOG_DEVICE)) throw RawStoreStatementException.cannotChangeLogDevice(); if (mappedValue == null) return value; else return mappedValue; } At the time of this change, the method AddPropertySetNotification was also considerably changed, it had code added that also mentioned those SET_IN_* values/comments; // set up the initial values by calling the validate and apply methods. // the map methods are not called as they will have been called // at runtime when the user set the property. [...more code added...] if (!who.validate(key, value, d)) continue; // SET_IN_JVM - need to get value // SET_IN_DATABASE - use the stored value // SET_IN_APPLICATION - need to get value // NOT_SET - can't happen cos it does exist in d if (!dbOnly) { int whereSet = PropertyUtil.whereSet(key, d); if (whereSet != PropertyUtil.SET_IN_DATABASE) value = PropertyUtil.getSystemProperty(key); } [...] but that code was subsequently removed again (in cloudscape revision 12007, on Aug 30, 1999, by djd) with check-in comment: "Fix bug with missing read lock configuration parameters." At that time, (in addition to adding of a locks.waitTimeout property), it looks like an init method was added to PropertySetCallback, to SinglePool, some dummy init methods were added to some other classes, and PropertyUtil got a new method with signature: public static String getPropertyFromSet(boolean dbOnly, Dictionary set, String key) Perhaps the code snippet in the method doValidateAndApplyMap should also have been removed at that time?
          Hide
          Knut Anders Hatlen added a comment -

          Thanks for digging through the old code, Myrna. I think I understand now.

          Our documentation says that system properties take precedence over database properties. This code is in place to ensure that setting a database property does not override the value in the system property. Currently, only the lock manager properties and the derby.database.classpath property have implemented a PropertySetCallback.apply() method, so that's probably what the "Lock manager properties" commit message referred to. If the call to apply() had not been skipped, setting the lock timeout as a database property would have overridden the lock timeout that was already set as a system property.

          It's not the call to PropertySetCallback.apply() that causes problems for us, but rather the call to PropertySetCallback.map(). The comment in the code only mentions that apply() shouldn't be called. No mentioning of map():

          // if this property should not be used then
          // don't call apply.

          I believe calling map(), also in the case where we don't call apply(), would be the right thing to do. Since we're storing the property in the database (although not actually using the value until the database is rebooted without the system property set), we must store the correct value. And to store the correct value, we need to perform the mapping.

          Since AuthenticationServiceBase is the only class that implements a map() method that's not a no-op, making that change should only affect the password properties, so it sounds like a limited and relatively safe change to make.

          Show
          Knut Anders Hatlen added a comment - Thanks for digging through the old code, Myrna. I think I understand now. Our documentation says that system properties take precedence over database properties. This code is in place to ensure that setting a database property does not override the value in the system property. Currently, only the lock manager properties and the derby.database.classpath property have implemented a PropertySetCallback.apply() method, so that's probably what the "Lock manager properties" commit message referred to. If the call to apply() had not been skipped, setting the lock timeout as a database property would have overridden the lock timeout that was already set as a system property. It's not the call to PropertySetCallback.apply() that causes problems for us, but rather the call to PropertySetCallback.map(). The comment in the code only mentions that apply() shouldn't be called. No mentioning of map(): // if this property should not be used then // don't call apply. I believe calling map(), also in the case where we don't call apply(), would be the right thing to do. Since we're storing the property in the database (although not actually using the value until the database is rebooted without the system property set), we must store the correct value. And to store the correct value, we need to perform the mapping. Since AuthenticationServiceBase is the only class that implements a map() method that's not a no-op, making that change should only affect the password properties, so it sounds like a limited and relatively safe change to make.
          Hide
          Knut Anders Hatlen added a comment -

          Here's a patch that makes PropertyValidation.doValidateApplyAndMap() do the mapping also if the property already exists as a system property. It still does not call apply() in those cases.

          The patch includes a test case that verifies that the password is not stored in plaintext in the database. The test case fails without the fix. I've also verified that Rick's repro script passes with the fix.

          Running regression tests.

          Show
          Knut Anders Hatlen added a comment - Here's a patch that makes PropertyValidation.doValidateApplyAndMap() do the mapping also if the property already exists as a system property. It still does not call apply() in those cases. The patch includes a test case that verifies that the password is not stored in plaintext in the database. The test case fails without the fix. I've also verified that Rick's repro script passes with the fix. Running regression tests.
          Hide
          Knut Anders Hatlen added a comment -

          All the regression tests passed.
          Committed revision 1220685.

          Show
          Knut Anders Hatlen added a comment - All the regression tests passed. Committed revision 1220685.

            People

            • Assignee:
              Knut Anders Hatlen
              Reporter:
              Rick Hillegas
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development