Mifare AES-128 symmetric key diversification

I have implemented a symmetric (AES 128-bit) key diversification algorithm for Java following the NXP Notes. It works as expected but am not the best Java programmer around, I guess I have done quite a mess so I need help on how can improve the code and make it better?

```
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.security.InvalidAlgorithmParameterException;
import java.security.InvalidKeyException;
import javax.crypto.Cipher;
import javax.crypto.spec.IvParameterSpec;
import java.security.NoSuchAlgorithmException;
import java.util.Arrays;
import javax.crypto.BadPaddingException;
import javax.crypto.IllegalBlockSizeException;
import javax.crypto.NoSuchPaddingException;
import javax.crypto.spec.SecretKeySpec;

/**
*
* @author Chris Skinner
*/
public class AES {

static SecretKeySpec skeySpec = null;
static public byte[] K = new byte[16]; // 128 bit key
static public byte[] K1 = new byte[16]; // 128 bit sub key
static public byte[] K2 = new byte[16]; // 128 bit sub key
static final public byte[] Z16 = new byte[16]; // 128 bit zero
static Cipher cipher = null;
private static final IvParameterSpec ZERO_IV = new IvParameterSpec(new byte[16]);

public static byte[] getDiversifiedKey() throws IOException, NoSuchAlgorithmException, NoSuchPaddingException, InvalidKeyException, InvalidAlgorithmParameterException, IllegalBlockSizeException, BadPaddingException // make K1 K2 from key
{
byte bRb = (byte) 0x87; // Rb for AES128
// key must be 16 bytes

//Step 1

byte[] key = hexStringToByteArray("00112233445566778899AABBCCDDEEFF");
try {
skeySpec = new SecretKeySpec(key, "AES");
if (cipher == null) {
cipher = Cipher.getInstance("AES/ECB/NoPadding");
}
cipher.init(Cipher.ENCRYPT_MODE, skeySpec);
K1 = cipher.doFinal(Z16);
//Step 2
System.out.println("KO " + bytesToHex(K1

static SecretKeySpec skeySpec = null;

Assigning null is never a good idea. Besides that, for fields (static or not) Java will automatically assign null anyway.

static public byte[] K1 = new byte[16]; // 128 bit sub key

static final public byte[] Z16 = new byte[16]; // 128 bit zero

Now for the keys you had an excuse, but is this not called ZERO_VECTOR? Z16 is not in the specification.

public static byte[] getDiversifiedKey() throws IOException, NoSuchAlgorithmException, NoSuchPaddingException, InvalidKeyException, InvalidAlgorithmParameterException, IllegalBlockSizeException, BadPaddingException // make K1 K2 from key

A getter should never throw an IOException: handle the exception internally or name the method differently.

byte bRb = (byte) 0x87; // Rb for AES128

Completely unclear naming, and seemingly using Hungarian notation, which Java programmers will never use.

hexStringToByteArray

Not sure why you had to program that yourself, but at the naming is spot on at least.

K1 = cipher.doFinal(Z16);

This doesn't make sense, you don't set static fields that way. Just keep the variable local. Assigning the new array to a field just tosses away the previous array reference.

System.out.println("KO " + bytesToHex(K1));

Use a logger, or use a field set to System.out or something similar, but never print to console from anything other than a UI class.

System.out.println("\n error 400 AES   " + ex.getMessage());

After catching never print and continue. I default not to printStackTrace (like crappy IDE's do) but to

// TODO better exception
throw new IllegalStateException("Exception not handled", e);

These runtime exceptions don't need to be handled and will at least exit the method without running on with invalid state.

ByteArrayOutputStream outputStream = new ByteArrayOutputStream();

Consider ByteBuffer.allocate() initialized to the correct size instead, then put the byte arrays into that, and retrieve the backing array using array(). Saves you the IOExceptions. In this case an IOException will never happen, so convert it to IllegalStateException again.

final protected static char[] hexArray = "0123456789ABCDEF".toCharArray();

Always keep to the right order: protected static final .... However, in this case private static final ... makes a lot more sense.

public static String bytesToHex(byte[] bytes) {

Why is this method named rather differently than hexStringToByteArray?

static byte[] xor16(byte[] ba, byte[] bb) {

What's the 16 for?

static byte[] shl(byte[] bin) // << 16 byte array

Ah, another 16 this time. But in this case it is required. Please document things like the input size in the JavaDoc, and not in an end of line comment. Methods like these should be private (without any access specifier, methods and fields are visible from within the package).

General remarks:

-
If you use names from a specification (which is good) then point out the spec in the documentation of the class - use clear names otherwise;

-
Generally we don't use static anything, especially not for values that will change. None of the static values are necessary (bar the one for the hex decoding, possibly).

-
If you have static bytes then don't decode them each time, either decode them once and assign them to a private static final constant / class field or assign them directly to the class field (using { (byte) 0x3B, (byte) 0x17, (byte) 0x35 }).

-
Please group the various steps in logical groups and implement them using separate methods.

StackExchange Code Review Q#99287, answer score: 2