OFBiz
  1. OFBiz
  2. OFBIZ-4824

Deprecated use of org.ofbiz.base.crypto.HashCrypt

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: SVN trunk
    • Fix Version/s: SVN trunk
    • Component/s: framework
    • Labels:
      None

      Description

      Hash

      classes:
      [javac16] Compiling 140 source files to /ci/ofbiz/framework/entity/build/classes
      [javac16] /ci/ofbiz/framework/entity/src/org/ofbiz/entity/GenericEntity.java:1299: warning: [deprecation] getDigestHash(java.lang.String) in org.ofbiz.base.crypto.HashCrypt has been deprecated
      [javac16] curValue = HashCrypt.getDigestHash(encryptField);
      [javac16] ^
      [javac16] /ci/ofbiz/framework/entity/src/org/ofbiz/entity/util/EntityCrypto.java:122: warning: [deprecation] getDigestHashOldFunnyHexEncode(java.lang.String,java.lang.String) in org.ofbiz.base.crypto.HashCrypt has been deprecated
      [javac16] String hashedKeyName = useOldFunnyKeyHash? HashCrypt.getDigestHashOldFunnyHexEncode(originalKeyName, null) : HashCrypt.getDigestHash(originalKeyName);
      [javac16] ^
      [javac16] /ci/ofbiz/framework/entity/src/org/ofbiz/entity/util/EntityCrypto.java:122: warning: [deprecation] getDigestHash(java.lang.String) in org.ofbiz.base.crypto.HashCrypt has been deprecated
      [javac16] String hashedKeyName = useOldFunnyKeyHash? HashCrypt.getDigestHashOldFunnyHexEncode(originalKeyName, null) : HashCrypt.getDigestHash(originalKeyName);
      [javac16] ^
      [javac16] 3 warnings

        Activity

        Pierre Smits created issue -
        Adam Heath made changes -
        Field Original Value New Value
        Assignee Adam Heath [ doogie ]
        Hide
        Adam Heath added a comment -

        I have fixed GenericEntity locally. However, it means that every time GenericEntity.toString() is called, you will get a different string, as a random salt is prepended to encrypted fields. But really, no one should be comparing the exact string output anyways.

        And, actually, that call in toString is really truly broken. It should do the same thing as EntityCrypto; to do anything less, would mean that PCI compliance is not being met.

        Fixing EntityCrypto is a bit more difficult. I don't want to just @SuppressWarnings("deprecation") on these methods; they really need to use the new and improved methods. However, I can't just switch completely, 'cuz then all existing crypted fields will no longer load.

        Stay tuned.

        Show
        Adam Heath added a comment - I have fixed GenericEntity locally. However, it means that every time GenericEntity.toString() is called, you will get a different string, as a random salt is prepended to encrypted fields. But really, no one should be comparing the exact string output anyways. And, actually, that call in toString is really truly broken. It should do the same thing as EntityCrypto; to do anything less, would mean that PCI compliance is not being met. Fixing EntityCrypto is a bit more difficult. I don't want to just @SuppressWarnings("deprecation") on these methods; they really need to use the new and improved methods. However, I can't just switch completely, 'cuz then all existing crypted fields will no longer load. Stay tuned.
        Hide
        Adam Heath added a comment -

        Wow, EntityCrypt is full of all sorts of fun stuff.

        First, the constructor creates 20 random keys if none are found in the database. Those 20 keys could never possibly be used. That loop should just be removed.

        Second, getKey(String, boolean) has broken synchronization on the keyMap.get/put pair. We've been lucky that is hasn't entered into a loop inside HashMap. This is problably due to the first item above. 20 keys get stored in the map, which is enough to cause the map to not resize it's internal buckets, when later keys get requested.

        Next, getRandomString() is not secure. That's the point of the SecureRandom class. This is a simple fix, however.

        Also, the transaction suspending that it doesn't doesn't do the right thing if there is an OutOfMemory thrown, or other Error or RuntimeException. It really needs to do that cleanup inside a finally. Fortunately, that's what TransactionUtil.doNewTransaction is for.

        Ideally, decrypt(), when it calls getKey(), shouldn't be creating new keys in the database, nor storing into the keyMap.

        Show
        Adam Heath added a comment - Wow, EntityCrypt is full of all sorts of fun stuff. First, the constructor creates 20 random keys if none are found in the database. Those 20 keys could never possibly be used. That loop should just be removed. Second, getKey(String, boolean) has broken synchronization on the keyMap.get/put pair. We've been lucky that is hasn't entered into a loop inside HashMap. This is problably due to the first item above. 20 keys get stored in the map, which is enough to cause the map to not resize it's internal buckets, when later keys get requested. Next, getRandomString() is not secure. That's the point of the SecureRandom class. This is a simple fix, however. Also, the transaction suspending that it doesn't doesn't do the right thing if there is an OutOfMemory thrown, or other Error or RuntimeException. It really needs to do that cleanup inside a finally. Fortunately, that's what TransactionUtil.doNewTransaction is for. Ideally, decrypt(), when it calls getKey(), shouldn't be creating new keys in the database, nor storing into the keyMap.

          People

          • Assignee:
            Adam Heath
            Reporter:
            Pierre Smits
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:

              Development