CVE-2015-6644

[agx]

Hi,
it seems the fix for the above CVE is not contained in upstream bouncycastle:

https://source.android.com/security/bulletin/2016-01-01.html#information_disclosure_vulnerability_in_bouncy_castle

https://android.googlesource.com/platform/external/bouncycastle/+/3e128c5fea3a0ca2d372aa09c4fd4bb0eadfbd3f

Isn't it needed there as well?

[peterdettman]

There were a series of commits for GCMBlockCipher in Jan, 2016 that addressed this CVE: https://github.com/bcgit/bc-java/commits/master/core/src/main/java/org/bouncycastle/crypto/modes/GCMBlockCipher.java .

We used a slightly different implementation and did not merge the commit you link to.

[apoleon]

Hi,
I am currently trying to fix this issue for Debian. We ship two older versions of bouncycastle, version 1.44 and 1.49. I don't know enough about the vulnerability to be sure that the backported commits from Jan 2016 will resolve the issue. The affected getNextCounterBlock() function looks very different to me. Does the following patch against 1.49 make sense?
In version 1.44 getNextCounterBlock does not exist at all. Is it naive to believe that 1.44 is not affected by CVE-2016-6644 because the bounds check is not necessary or do we need to backport getNextCounterBlock?

From: Markus Koschany apo@debian.org
Date: Sun, 9 Apr 2017 16:05:34 +0200
Subject: CVE-2015-6644

---

src/org/bouncycastle/crypto/modes/GCMBlockCipher.java | 9 +++++++++
1 file changed, 9 insertions(+)

diff --git a/src/org/bouncycastle/crypto/modes/GCMBlockCipher.java b/src/org/bouncycastle/crypto/modes/GCMBlockCipher.java
index 9e617ec..4f5466e 100644
--- a/src/org/bouncycastle/crypto/modes/GCMBlockCipher.java
+++ b/src/org/bouncycastle/crypto/modes/GCMBlockCipher.java
@@ -41,6 +41,7 @@ public class GCMBlockCipher
private byte[] macBlock;
private byte[] S, S_at, S_atPre;
private byte[] counter;

• private int blocksRemaining;
  private int bufOff;
  private long totalLength;
  private byte[] atBlock;
  @@ -168,6 +169,7 @@ public class GCMBlockCipher
  this.atLength = 0;
  this.atLengthPre = 0;
  this.counter = Arrays.clone(J0);

•    this.blocksRemaining = -2;
     this.bufOff = 0;
     this.totalLength = 0;
  

@@ -428,6 +430,7 @@ public class GCMBlockCipher
atLength = 0;
atLengthPre = 0;
counter = Arrays.clone(J0);

•    blocksRemaining = -2;
     bufOff = 0;
     totalLength = 0;
  

@@ -494,6 +497,12 @@ public class GCMBlockCipher

 private byte[] getNextCounterBlock()
 {

•    if (blocksRemaining == 0)
  

•    {
  

•        throw new IllegalStateException("Attempt to process too many blocks");
  

•    }
  

•    blocksRemaining--;
  

•    for (int i = 15; i >= 12; --i)
     {
         byte b = (byte)((counter[i] + 1) & 0xff);
  

[apoleon]

This is more readable.

https://gist.github.com/apoleon/8289870cacf3e8b107f1082b662da118

[peterdettman]

The linked gist/patch for 1.49 looks correct to me. I'd be inclined to just copy the entirety of the latest getNextCounterBlock back, since the other changes in that function are to perform a constant-time update of the counter block (but it's unrelated to the CVE).

1.44 is affected and should also be patched. I suggest again copying the latest getNextCounterBlock, adding the blocksRemaining declaration, init, reset lines. Then change (lines 326-341):

    private void gCTRBlock(byte[] buf, int bufCount, byte[] out, int outOff)
    {
//        inc(counter);
        for (int i = 15; i >= 12; --i)
        {
            byte b = (byte)((counter[i] + 1) & 0xff);
            counter[i] = b;

            if (b != 0)
            {
                break;
            }
        }

        byte[] tmp = new byte[BLOCK_SIZE];
        cipher.processBlock(counter, 0, tmp, 0);

to

    private void gCTRBlock(byte[] buf, int bufCount, byte[] out, int outOff)
    {
        byte[] tmp = getNextCounterBlock();

I'm happy to check a proposed 1.44 patch also.

[agx]

@apoleon Isn't gCTRBlock in 1.44 the getNextCounterBlock of later versions (so it's also affected)?

[peterdettman]

Nope, in the latest code we see:

    private void gCTRBlock(byte[] block, byte[] out, int outOff)
    {
        byte[] tmp = getNextCounterBlock();

[apoleon]

Thank your for the review and the advice. I followed your recommendation and just used the latest code of the getNextCounterBlock() function. What do you think about the following patches now?

1.44:
https://gist.github.com/apoleon/f38f47fe05ee66efdfbd38ee60a57944

1.49:
https://gist.github.com/apoleon/56c92d38b767d3489e90c5df9304fc8f

[peterdettman]

@apoleon I have reviewed both those patches and both look correct to me.

[apoleon]

@peterdettman Thank you very much for your assistance!