LMS: equals/hashCode/concurrency hardening

Author: peterdettman

core/src/main/java/org/bouncycastle/crypto/params/HSSPrivateKeyParameters.java

@@ -29,8 +29,6 @@ public class HSSPrivateKeyParameters
     private final long indexLimit;
     private long index = 0;
 
-    private HSSPublicKeyParameters publicKey;
-
     public HSSPrivateKeyParameters(LMSPrivateKeyParameters key, long index, long indexLimit)
     {
         super(true);
@@ -53,8 +51,8 @@ public HSSPrivateKeyParameters(int l, List<LMSPrivateKeyParameters> keys, List<L
         super(true);
 
         this.l = l;
-        this.keys = Collections.unmodifiableList(keys);
-        this.sig = Collections.unmodifiableList(sig);
+        this.keys = Collections.unmodifiableList(new ArrayList<LMSPrivateKeyParameters>(keys));
+        this.sig = Collections.unmodifiableList(new ArrayList<LMSSignature>(sig));
         this.index = index;
         this.indexLimit = indexLimit;
         this.isShard = false;
@@ -70,6 +68,8 @@ private HSSPrivateKeyParameters(int l, List<LMSPrivateKeyParameters> keys, List<
         super(true);
 
         this.l = l;
+        // No copy here, unlike the public constructor: the callers are extractKeyShard and
+        // getInstance, which build fresh lists they do not retain.
         this.keys = Collections.unmodifiableList(keys);
         this.sig = Collections.unmodifiableList(sig);
         this.index = index;
@@ -98,8 +98,6 @@ public static HSSPrivateKeyParameters getInstance(byte[] privEnc, byte[] pubEnc)
             throw new IOException("HSS private key tree cache does not match the public key");
         }
 
-        pKey.publicKey = pubKey;
-
         return pKey;
     }
 
@@ -308,7 +306,7 @@ private static HSSPrivateKeyParameters makeCopy(HSSPrivateKeyParameters privateK
         }
     }
 
-    protected void updateHierarchy(LMSPrivateKeyParameters[] newKeys, LMSSignature[] newSig)
+    private void updateHierarchy(LMSPrivateKeyParameters[] newKeys, LMSSignature[] newSig)
     {
         synchronized (this)
         {
@@ -341,7 +339,7 @@ public long getUsagesRemaining()
 
     LMSPrivateKeyParameters getRootKey()
     {
-        return keys.get(0);
+        return getKeys().get(0);
     }
 
     /**
@@ -396,11 +394,14 @@ synchronized List<LMSSignature> getSig()
 
     /**
      * Reset to index will ensure that all LMS keys are correct for a given HSS index value.
-     * Normally LMS keys updated in sync with their parent HSS key but in cases of sharding
+     * Normally LMS keys are updated in sync with their parent HSS key but in cases of sharding
      * the normal monotonic updating does not apply and the state of the LMS keys needs to be
      * reset to match the current HSS index.
+     * <p>
+     * Should only be called under the monitor (lock) or during construction before the instance escapes.
+     * </p>
      */
-    void resetKeyToIndex()
+    private void resetKeyToIndex()
     {
         // Extract the original keys
         List<LMSPrivateKeyParameters> originalKeys = getKeys();
@@ -418,8 +419,8 @@ void resetKeyToIndex()
         }
 
         boolean changed = false;
-        LMSPrivateKeyParameters[] keys = originalKeys.toArray(new LMSPrivateKeyParameters[originalKeys.size()]);//  new LMSPrivateKeyParameters[originalKeys.size()];
-        LMSSignature[] sig = this.sig.toArray(new LMSSignature[this.sig.size()]);//   new LMSSignature[originalKeys.size() - 1];
+        LMSPrivateKeyParameters[] keys = originalKeys.toArray(new LMSPrivateKeyParameters[originalKeys.size()]);
+        LMSSignature[] sig = this.sig.toArray(new LMSSignature[this.sig.size()]);
 
         LMSPrivateKeyParameters originalRootKey = this.getRootKey();
 
@@ -626,27 +627,40 @@ public boolean equals(Object o)
 
         HSSPrivateKeyParameters that = (HSSPrivateKeyParameters)o;
 
-        if (l != that.l)
-        {
-            return false;
-        }
-        if (isShard != that.isShard)
-        {
-            return false;
-        }
-        if (indexLimit != that.indexLimit)
+        if (this.l != that.l || this.isShard != that.isShard || this.indexLimit != that.indexLimit)
         {
             return false;
         }
-        if (index != that.index)
+
+        //
+        // index, keys and sig all move as consumed trees are replaced, and they move together -
+        // replaceConsumedKey assigns keys and sig one after the other under this monitor - so read
+        // each key's trio in one synchronized block to get a snapshot no unsynchronized reader
+        // could tear. The lists are unmodifiable and replaced rather than mutated, so a captured
+        // reference stays a coherent view after the lock drops. Neither monitor is held while the
+        // other is taken, so a.equals(b) racing b.equals(a) cannot deadlock.
+        //
+        long thisIndex;
+        List<LMSPrivateKeyParameters> thisKeys;
+        List<LMSSignature> thisSig;
+        synchronized (this)
         {
-            return false;
+            thisIndex = this.index;
+            thisKeys = this.keys;
+            thisSig = this.sig;
         }
-        if (!keys.equals(that.keys))
+
+        long thatIndex;
+        List<LMSPrivateKeyParameters> thatKeys;
+        List<LMSSignature> thatSig;
+        synchronized (that)
         {
-            return false;
+            thatIndex = that.index;
+            thatKeys = that.keys;
+            thatSig = that.sig;
         }
-        return sig.equals(that.sig);
+
+        return thisIndex == thatIndex && thisKeys.equals(thatKeys) && thisSig.equals(thatSig);
     }
 
     @Override
@@ -684,7 +698,20 @@ public synchronized byte[] getEncoded()
     @Override
     public int hashCode()
     {
-        return getPublicKey().hashCode();
+        //
+        // Deliberately not getPublicKey().hashCode(): that reaches the root key's tree, which is
+        // only built if the node cache does not already hold it - 2^h LM-OTS public keys from an
+        // implicit call no caller expects to cost anything. The fields used here are the ones that
+        // do not move as the key signs: the root key material is fixed (resetKeyToIndex only
+        // repositions it, and LMSPrivateKeyParameters.hashCode is itself index-independent),
+        // whereas index, keys and sig all change. Equal keys agree on all of these, so the
+        // equals() contract holds.
+        //
+        int hc = l;
+        hc = 31 * hc + (isShard ? 1 : 0);
+        hc = 31 * hc + (int)(indexLimit ^ (indexLimit >>> 32));
+        hc = 31 * hc + getRootKey().hashCode();
+        return hc;
     }
 
     @Override

core/src/main/java/org/bouncycastle/crypto/params/LMSPrivateKeyParameters.java

@@ -15,6 +15,7 @@
 import org.bouncycastle.crypto.signers.lms.LMSEngine;
 import org.bouncycastle.util.Arrays;
 import org.bouncycastle.util.Exceptions;
+import org.bouncycastle.util.Objects;
 import org.bouncycastle.util.io.Streams;
 
 public class LMSPrivateKeyParameters
@@ -40,7 +41,6 @@ public class LMSPrivateKeyParameters
     private final byte[] masterSecret;
     private final Map<CacheKey, byte[]> tCache;
     private final int maxCacheR;
-    private final Digest tDigest;
 
     private int q;
 
@@ -62,7 +62,6 @@ public LMSPrivateKeyParameters(LMSigParameters lmsParameter, LMOtsParameters ots
         this.masterSecret = Arrays.clone(masterSecret);
         this.maxCacheR = 1 << (parameters.getH() + 1);
         this.tCache = new WeakHashMap<CacheKey, byte[]>();
-        this.tDigest = LMSEngine.createDigest(lmsParameter);
     }
 
     private LMSPrivateKeyParameters(LMSPrivateKeyParameters parent, int q, int maxQ)
@@ -76,7 +75,6 @@ private LMSPrivateKeyParameters(LMSPrivateKeyParameters parent, int q, int maxQ)
         this.masterSecret = parent.masterSecret;
         this.maxCacheR = 1 << parameters.getH();
         this.tCache = parent.tCache;
-        this.tDigest = LMSEngine.createDigest(parameters);
         this.publicKey = parent.publicKey;
     }
 
@@ -85,7 +83,10 @@ public static LMSPrivateKeyParameters getInstance(byte[] privEnc, byte[] pubEnc)
     {
         LMSPrivateKeyParameters pKey = getInstance(privEnc);
 
-        pKey.publicKey = LMSPublicKeyParameters.getInstance(pubEnc);
+        synchronized (pKey)
+        {
+            pKey.publicKey = LMSPublicKeyParameters.getInstance(pubEnc);
+        }
 
         return pKey;
     }
@@ -291,14 +292,16 @@ private static void validateTreeCache(LMSPrivateKeyParameters key, byte[][] cach
      */
     byte[][] deriveChildKey()
     {
+        int q;
         synchronized (this)
         {
+            q = this.q;
             if (q >= maxQ)
             {
                 throw new ExhaustedPrivateKeyException("ots private key exhausted");
             }
-            return LMSEngine.deriveChildKey(otsParameters, I, masterSecret, q);
         }
+        return LMSEngine.deriveChildKey(otsParameters, I, masterSecret, q);
     }
 
     /**
@@ -470,7 +473,11 @@ private byte[] findT(CacheKey key)
 
     private byte[] calcT(int r)
     {
-        int h = this.getSigParameters().getH();
+        LMSigParameters sigParameters = getSigParameters();
+
+        Digest tDigest = LMSEngine.createDigest(sigParameters);
+
+        int h = sigParameters.getH();
 
         int twoToh = 1 << h;
 
@@ -554,43 +561,37 @@ public boolean equals(Object o)
 
         LMSPrivateKeyParameters that = (LMSPrivateKeyParameters)o;
 
-        if (q != that.q)
-        {
-            return false;
-        }
-        if (maxQ != that.maxQ)
-        {
-            return false;
-        }
-        if (!Arrays.areEqual(I, that.I))
-        {
-            return false;
-        }
-        if (parameters != null ? !parameters.equals(that.parameters) : that.parameters != null)
-        {
-            return false;
-        }
-        if (otsParameters != null ? !otsParameters.equals(that.otsParameters) : that.otsParameters != null)
-        {
-            return false;
-        }
-        if (!Arrays.constantTimeAreEqual(masterSecret, that.masterSecret))
-        {
-            return false;
-        }
-
-        return true;
+        return this.getIndex() == that.getIndex()
+            && this.maxQ == that.maxQ
+            && Arrays.areEqual(this.I, that.I)
+            && Objects.areEqual(this.parameters, that.parameters)
+            && Objects.areEqual(this.otsParameters, that.otsParameters)
+            && Arrays.constantTimeAreEqual(this.masterSecret, that.masterSecret);
     }
 
     @Override
     public int hashCode()
     {
-        return getPublicKey().hashCode();
+        //
+        // Deliberately not getPublicKey().hashCode(): the root is only there if the tree cache
+        // holds it, so on a freshly generated or decoded key that builds the whole Merkle tree -
+        // 2^h LM-OTS public keys - from an implicit call no caller expects to cost anything. It is
+        // also independent of q, so a key's hash does not move as it signs, and of the master
+        // secret, so no function of the seed is handed out. Equal keys agree on every field used
+        // here, so the equals() contract holds.
+        //
+        int hc = Objects.hashCode(parameters);
+        hc = 31 * hc + Objects.hashCode(otsParameters);
+        hc = 31 * hc + maxQ;
+        hc = 31 * hc + Arrays.hashCode(I);
+        return hc;
     }
 
     public byte[] getEncoded()
         throws IOException
     {
+        int q = getIndex();
+
         //
         // NB there is no formal specification for the encoding of private keys.
         // It is implementation dependent.