From 56498e7e58e4ee0da94783bf6ec18425ce250ea4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Rafa=C5=82=20Sumis=C5=82awski?=
 <rafal.sumislawski@gmail.com>
Date: Tue, 28 Aug 2018 09:34:47 +0200
Subject: [PATCH] Fix vulnerability in AESCounterBuiltinRNG
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* The problem is that that internal counter isn’t incremented before cipher.doFinal() is used
* AES/CTR has a counter internally and cipher.update() should be used
* Another issue is that AES128CounterSecureRNG is initially seeded with a 16 bytes seed/key, but once it
  hits ReseedingThreshold it reseeds itself with a 32 bytes seed, effectively becoming AES256. This will
  crash if strong encryption is disabled.
---
 .../security/provider/AESCounterBuiltinRNG.scala  | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/akka-remote/src/main/scala/akka/remote/security/provider/AESCounterBuiltinRNG.scala b/akka-remote/src/main/scala/akka/remote/security/provider/AESCounterBuiltinRNG.scala
index a446d5444f..8a802edc41 100644
--- a/akka-remote/src/main/scala/akka/remote/security/provider/AESCounterBuiltinRNG.scala
+++ b/akka-remote/src/main/scala/akka/remote/security/provider/AESCounterBuiltinRNG.scala
@@ -41,7 +41,6 @@ private[akka] class AESCounterBuiltinRNG(val seed: Array[Byte], implicit val exe
   private val entropySource = new SecureRandom
 
   // mutable state below, concurrent accesses need synchronized or lock
-  private val counter: Array[Byte] = Array.fill[Byte](CounterSizeBytes)(0)
   private var index: Int = 0
   private var currentBlock: Array[Byte] = null
   private var reseedFuture: Future[Array[Byte]] = null
@@ -52,29 +51,31 @@ private[akka] class AESCounterBuiltinRNG(val seed: Array[Byte], implicit val exe
   // this algorithm can be further improved by better selection of the iv
   // here and at re-seeding time further below
   private val ivArr = Array.fill[Byte](CounterSizeBytes)(0)
-  ivArr(0) = (ivArr(0) + 1.toByte).toByte
+  ivArr(0) = (ivArr(0) + 1).toByte
   private val ivSpec = new IvParameterSpec(ivArr)
   cipher.init(Cipher.ENCRYPT_MODE, new this.AESKey(seed), ivSpec)
 
+  private val zeros: Array[Byte] = Array.fill[Byte](CounterSizeBytes)(0)
+
   @Override
   override protected def next(bits: Int): Int = synchronized {
-    // random result generation phase - if there is not enough bits in counter variable
+    // random result generation phase - if there is not enough bits in the currentBlock
     // we generate some more with AES/CTR
     bitsSinceSeeding += bits
     if (currentBlock == null || currentBlock.length - index < 4) {
       try {
-        currentBlock = cipher.doFinal(counter)
+        currentBlock = cipher.update(zeros)
         index = 0
       } catch {
         case ex: Exception ⇒
-          // Generally Cipher.doFinal() from nextBlock may throw various exceptions.
+          // Generally Cipher.update() from nextBlock may throw various exceptions.
           // However this should never happen.  If initialisation succeeds without exceptions
           // we should be able to proceed indefinitely without exceptions.
           throw new IllegalStateException("Failed creating next random block.", ex)
       }
     }
 
-    // now, enough bits in counter, generate pseudo-random result
+    // now, enough bits in currentBlock, generate pseudo-random result
     val result = (BitwiseByteToInt & currentBlock(index + 3)) |
       ((BitwiseByteToInt & currentBlock(index + 2)) << 8) |
       ((BitwiseByteToInt & currentBlock(index + 1)) << 16) |
@@ -86,7 +87,7 @@ private[akka] class AESCounterBuiltinRNG(val seed: Array[Byte], implicit val exe
     if (bitsSinceSeeding > reseedingThreshold) {
       if (reseedFuture == null) {
         // ask for a seed and process async on a separate thread using AESCounterBuiltinRNGReSeeder threadpool
-        reseedFuture = Future { entropySource.generateSeed(32) }
+        reseedFuture = Future { entropySource.generateSeed(seed.length) }
       }
       // check if reseedingDeadline is exceeded - in that case we cannot proceed, as that would be insecure
       // we need to block on the future to wait for entropy