0

I have a server which let's sockets connect to it to send data over inputstreams, this data is being encrypted with AES/GCM/NoPadding in a class called Cryptographer. The server has threads that hold functionalities for the connected clients, and each thread is being represented in a ConnectionThread class, this class holds a reference to the cryptograhper class which is being initialized in the server class.

Problem:

When I send my first command it works just fine, no problems at all. But somehow when I send my second command if gives the following stacktrace:

javax.crypto.AEADBadTagException: Tag mismatch!
    at java.base/com.sun.crypto.provider.GaloisCounterMode.decryptFinal(GaloisCounterMode.java:595)
    at java.base/com.sun.crypto.provider.CipherCore.finalNoPadding(CipherCore.java:1116)
    at java.base/com.sun.crypto.provider.CipherCore.fillOutputBuffer(CipherCore.java:1053)
    at java.base/com.sun.crypto.provider.CipherCore.doFinal(CipherCore.java:853)
    at java.base/com.sun.crypto.provider.AESCipher.engineDoFinal(AESCipher.java:446)
    at java.base/javax.crypto.Cipher.doFinal(Cipher.java:2208)
    at com.company.security.Cryptographer.decrypt(Cryptographer.java:53)
    at com.company.client.Reader.run(Reader.java:45)
    at java.base/java.lang.Thread.run(Thread.java:835)
Exception in thread "Thread-3" java.lang.NullPointerException
    at java.base/java.lang.String.<init>(String.java:623)
    at com.company.client.Reader.run(Reader.java:47)
    at java.base/java.lang.Thread.run(Thread.java:835)

These are the classes mentioned in the stacktrace

Cryptographer

package com.company.security;

import javax.crypto.*;
import javax.crypto.spec.GCMParameterSpec;
import javax.crypto.spec.SecretKeySpec;
import java.security.InvalidAlgorithmParameterException;
import java.security.InvalidKeyException;
import java.security.Key;
import java.security.NoSuchAlgorithmException;
import java.util.Base64;

public class Cryptographer {
    private Key secretKey;
    private GCMParameterSpec gcmParameterSpec;

    public Cryptographer() {
        byte[] secret = new byte[16]; // 128 bit is 16 bytes, and AES accepts 16 bytes, and a few others.
        byte[] secretBytes = "secret".getBytes();
        byte[] IV = new byte[12];
        gcmParameterSpec = new GCMParameterSpec(16 * 8, IV);
        System.arraycopy(secretBytes, 0, secret, 0, secretBytes.length);
        secretKey = new SecretKeySpec(secret, "AES");
    }

    /**
     * Encrypt data.
     * @param data to encrypt
     * @return encrypted data
     */
    public byte[] encrypt(byte[] data) {
        try {
            Cipher cipher = Cipher.getInstance("AES/GCM/NoPadding");
            cipher.init(Cipher.ENCRYPT_MODE, secretKey, gcmParameterSpec);
            byte[] encrypted = cipher.doFinal(data);
            return encrypted;
        } catch (InvalidKeyException | BadPaddingException
                | IllegalBlockSizeException | NoSuchPaddingException
                | NoSuchAlgorithmException | InvalidAlgorithmParameterException e) {
            e.printStackTrace();
            return null;
        }
    }

    /**
     * Decrypt data.
     * @param data to decrypt
     * @return decrypted data
     */
    public byte[] decrypt(byte[] data) {
        try {
            Cipher cipher = Cipher.getInstance("AES/GCM/NoPadding");
            cipher.init(Cipher.DECRYPT_MODE, secretKey, gcmParameterSpec);
            return cipher.doFinal(data);
        } catch (InvalidKeyException | BadPaddingException
                | IllegalBlockSizeException | NoSuchPaddingException
                | NoSuchAlgorithmException | InvalidAlgorithmParameterException e) {
            e.printStackTrace();
            return null;
        }
    }
}

Reader

package com.company.client;

import com.company.FileLoader;
import com.company.client.helpers.ClientFileHelper;
import com.company.client.workers.MessageSender;
import com.company.security.Cryptographer;

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStream;

public class Reader implements Runnable {
    private InputStream inputStream;
    private ClientFileHelper fileHelper;
    private Cryptographer cryptographer;
    private FileLoader fileLoader;
    private BufferedReader bufferedReader;
    private MessageSender messageSender;
    private boolean isActive = true;
    private boolean isReceivingFile = false;


    public Reader(BufferedReader bufferedReader, MessageSender messageSender, InputStream inputStream) {
        this.bufferedReader = bufferedReader;
        this.messageSender = messageSender;
        this.inputStream = inputStream;
        cryptographer = new Cryptographer();
    }

    @Override
    public void run() {
        while (isActive) {
            try {
                int count;
                byte[] buffer;

                if(!isReceivingFile) {
                    buffer = new byte[inputStream.available()];
                } else {
                    buffer = new byte[inputStream.available()];
                }

                while ((count = inputStream.read(buffer)) > 0)
                {
                    byte[] decrypted = cryptographer.decrypt(buffer);
                    if(!isReceivingFile) {
                        handleInput(new String(decrypted));
                    } else {
                        if(fileHelper.getFileBytes().length == 0) {
                            fileHelper.setFileBytes(decrypted);
                        } else {
                            fileHelper.saveFile();
                            isReceivingFile = false;
                        }
                    }
                }
            } catch (IOException e) {
                e.printStackTrace();
                break;
            }
        }

    }

    /**
     * Handle the user input form the console.
     * @param input user input from console
     */
    private void handleInput(String input) {
        try {
            if (input.equals("PING")) { // If we get a PING message we send back a PONG message.
                messageSender.send("PONG");
            } else if (input.contains("FILE")) {
                setupFileAccept(input);
                isReceivingFile = true;
            } else {
                System.out.println(input);
            }
        } catch (Exception ex) {
            isActive = false;
        }
    }

    /**
     * Setup the file helper for the client that's going to receive a file.
     * @param line command
     */
    private void setupFileAccept(String line) {
        String[] args = line.split(" ");
        if(args[0].equals("FILE")) {
            fileHelper = new ClientFileHelper(args[1], Integer.valueOf(args[2]));
        }
    }
}

The ConnectionThread also has a similar read functionality which looks like this:

while (isActive) {
        try {
            int count;
            byte[] buffer;

            if(!isReceivingFile) {
                buffer = new byte[inputStream.available()];
            } else {
                buffer = fileHelper.getFileBytes();
            }

            while ((count = inputStream.read(buffer)) > 0)
            {
                byte[] decrypted = server.cryptographer.decrypt(buffer);
                if(!isReceivingFile) {
                    getInput(new String(decrypted));
                } else {
                    fileHelper.setFileBytes(decrypted);
                    // bytes received, now we can send the file!
                    if(fileHelper.sendToReceiver()) {
                        writeToClient(fileHelper.getReceiverName()
                                + " received " + fileHelper.getFilename());
                        fileHelper = null;
                    }
                }
            }
        } catch (IOException e) {
            e.printStackTrace();
            break;
        }
    }

In this case just assume that the Server class has the cryptographer property properly initialized, which is always the case.

My guess is that somewhere a value is doing something wrong but I am not sure. I am rather clueless to what I should do to fix this problem. Can somebody help me point out the mistakes and come up with possible solutions to fix this problem? My java version is 12.0.1

kelalaka
  • 5,064
  • 5
  • 27
  • 44
Berend Hulshof
  • 1,039
  • 1
  • 16
  • 36

2 Answers2

1

I would encourage to cosider to use SSL/TLS or DTLS instead of trying to reimplement parts of it.

Whether it causes your error I'm not sure but if my interpretation of the Java documentation is correct than you should change the GCMParameterSpec for each message:

after each encryption operation using GCM mode, callers should re-initialize the cipher objects with GCM parameters which has a different IV value

and:

GCM mode has a uniqueness requirement on IVs used in encryption with a given key. When IVs are repeated for GCM encryption, such usages are subject to forgery attacks.

Also you are not using the updateAAD (Additional Authentication Data), although that is optional according to https://www.rfc-editor.org/rfc/rfc5084 from the error message it sounds like it is causing errors here, but it may just be a misleading error message.

Update:

I wrote lots of unit tests for the Cryptographer class and only if I start to make random changes to the ciphertext before I decrypt it again I often get the same error. Because we can trust TCP/IP to reproduce exactly the same bytes on the other side of the connection we're left with these kind of problems may be:

  1. Concurrency
  2. Converting ciphertext bytes into Strings, Chars, Readers/Writers
  3. Not reading the entire message from the socket (did you check how many bytes you sent and compared it to how many you received?

And no, I have not yet written and tested my own implementation yet, but there are examples out there, like this example, nicely explained by the author here from the code was found by this search

Community
  • 1
  • 1
JohannesB
  • 2,214
  • 1
  • 11
  • 18
  • So should I reset the IV and GCM array after encryption and keep it the same with decryption? – Berend Hulshof Mar 31 '20 at 14:59
  • Consider existing frameworks like Netty, or at least start refactoring your code into separate documented methods and write unit tests for them instead of multiple nested while loops. – JohannesB Mar 31 '20 at 22:18
  • I will refuse to use frameworks, I am not going to take the easy way out and I have to do this myself – Berend Hulshof Apr 01 '20 at 11:02
  • Sometimes you can learn a lot from studying other approaches, but changing architecture when you are almost? finished isn't cheap – JohannesB Apr 01 '20 at 14:49
1

THanks to JohannesB for pointing me into the right direction!

I now have solved my problems. It first started by the reading method which I had to change to this:

byte[] buffer;

while (inputStream.available() > 0)
{
    int read = inputStream.read(buffer);
    if(read == 0)
        break;
}
// An if statement checking if the buffer has been filled and based on this
// It will execute methods

And my Cryptographer class now looks like this:

public class Cryptographer {
    private SecretKey secretKey;
    private byte[] aad;
    private SecureRandom secureRandom;
    private byte[] IV;

    public Cryptographer(SecretKey secretKey) {
        this.secretKey = secretKey;
        secureRandom = new SecureRandom();
        IV = new byte[12];
        secureRandom.nextBytes(IV);
        aad = "association".getBytes();
    }

    /**
     * Encrypt data.
     * @param data to encrypt
     * @return encrypted data
     */
    public byte[] encrypt(byte[] data) {
        try {
            Cipher cipher = Cipher.getInstance("AES/GCM/NoPadding");
            SecretKeySpec keySpec = new SecretKeySpec(secretKey.getEncoded(), "AES");
            secureRandom.nextBytes(IV);
            GCMParameterSpec gcmParameterSpec = new GCMParameterSpec(128, IV);
            cipher.init(Cipher.ENCRYPT_MODE, keySpec, gcmParameterSpec);
            cipher.updateAAD(aad);
            return toByteBuffer(cipher.doFinal(data));
        } catch (InvalidKeyException | BadPaddingException
                | IllegalBlockSizeException | NoSuchPaddingException
                | NoSuchAlgorithmException | InvalidAlgorithmParameterException e) {
            e.printStackTrace();
            return null;
        }
    }

    /**
     * Decrypt data.
     * @param data to decrypt
     * @return decrypted data
     */
    public byte[] decrypt(byte[] data) {
        try {
            Cipher cipher = Cipher.getInstance("AES/GCM/NoPadding");
            SecretKeySpec keySpec = new SecretKeySpec(secretKey.getEncoded(), "AES");
            // get the data from the byte buffer
            data = fromByteBuffer(data);
            // create the gcm parameter with the received IV.
            GCMParameterSpec gcmParameterSpec = new GCMParameterSpec(128, IV);

            cipher.init(Cipher.DECRYPT_MODE, keySpec, gcmParameterSpec);
            cipher.updateAAD(aad);
            return cipher.doFinal(data);
        } catch (InvalidKeyException | BadPaddingException
                | IllegalBlockSizeException | NoSuchPaddingException
                | NoSuchAlgorithmException | InvalidAlgorithmParameterException e) {
            e.printStackTrace();
            return null;
        }
    }

    /**
     * Put the encrypted data through a byte buffer.
     * This buffer will contain information about the IV array.
     * @param data encrypted data
     * @return the ByteBuffer result as byte array
     */
    private byte[] toByteBuffer(byte[] data) {
        ByteBuffer byteBuffer = ByteBuffer.allocate(4 + IV.length + data.length);
        byteBuffer.putInt(IV.length);
        byteBuffer.put(IV);
        byteBuffer.put(data);
        return byteBuffer.array();
    }

    /**
     * Gets data from a ByteBuffer and sets up data needed for decryption.
     * @param data ByteBuffer data as byte array
     * @return ByteBuffer encrypted data
     */
    private byte[] fromByteBuffer(byte[] data) {
        ByteBuffer byteBuffer = ByteBuffer.wrap(data);

        int ivLength = byteBuffer.getInt();
        if(ivLength < 12 || ivLength >= 16) {
            throw new IllegalArgumentException("invalid iv length");
        }

        IV = new byte[ivLength];
        byteBuffer.get(IV);

        byte[] remaining = new byte[byteBuffer.remaining()];
        byteBuffer.get(remaining);

        return remaining;
    }
}

As for the reasons why I did this you can see JohannesB's suggestions and check out these articles:

https://proandroiddev.com/security-best-practices-symmetric-encryption-with-aes-in-java-7616beaaade9

How to read all of Inputstream in Server Socket JAVA

Berend Hulshof
  • 1,039
  • 1
  • 16
  • 36