1

I'm working in a Electronic Funds Transfer (EFT) system and need be very careful with sensitive data like credit card numbers.

We need this information do mount the ISO 8583 data before sending to EFT Gateways.

We use char[] and byte[] to keep sensitive data and use Array.fill when We don't need this information anymore.

Thinking in improve security a thought in create a class to wrap this sensitive information and maybe keep data encrypted until needed.

Here's the code I plan to use with a random generated key.

import java.nio.ByteBuffer;
import java.nio.CharBuffer;
import java.nio.charset.Charset;
import java.security.InvalidKeyException;
import java.security.NoSuchAlgorithmException;
import java.security.SecureRandom;
import java.util.Arrays;

import javax.crypto.BadPaddingException;
import javax.crypto.Cipher;
import javax.crypto.IllegalBlockSizeException;
import javax.crypto.KeyGenerator;
import javax.crypto.NoSuchPaddingException;
import javax.crypto.SecretKey;
import javax.security.auth.Destroyable;

public final class SensitiveChars implements CharSequence, Destroyable {

    private static final int KEYSIZE = 56;
    private static final String DES = "DES";
    private static final String DES_ECB_PKCS5_PADDING = DES + "/ECB/PKCS5Padding";

    private byte[] data;
    private Charset charset;
    private int lenght;

    private SecretKey secretKey;

    public SensitiveChars(char[] data) {
        super();
        this.charset = Charset.defaultCharset();
        ByteBuffer byteBuffer = charset.encode(CharBuffer.wrap(data));
        lenght = data.length;

        try {
            KeyGenerator keyGenerator = KeyGenerator.getInstance(DES);

            SecureRandom secureRandom = new SecureRandom();
            int keyBitSize = KEYSIZE;
            keyGenerator.init(keyBitSize, secureRandom);
            secretKey = keyGenerator.generateKey(); 

            setRawData(byteBuffer.array());
        } catch (NoSuchAlgorithmException e) {
            throw new IllegalStateException(e);
        }
    }

    public static SensitiveChars of(byte[] bytes) {
        return of(bytes, Charset.defaultCharset());
    }   

    public static SensitiveChars of(byte[] bytes, Charset charset) {
        return of(bytes, charset, false);
    }

    public static SensitiveChars of(byte[] bytes, Charset charset, boolean cleanBytes) {
        char[] chars = toChar(bytes, charset, cleanBytes);
        return new SensitiveChars(chars);
    }

    @Override
    public int length() {
        return lenght;
    }

    @Override
    public char charAt(int index) {
        char[] tempArray = getChars();
        char c = tempArray[index];
        clearData(tempArray);
        return c;
    }

    @Override
    public CharSequence subSequence(int start, int end) {
        char[] dataTmp = getChars();
        char[] range = Arrays.copyOfRange(dataTmp, start, end);
        clearData(dataTmp);
        return new SensitiveChars(range);
    }

    public char[] getChars() {
        if (lenght == 0) {
            return new char[0];
        } else {
            byte[] rawData = getRawData();
            ByteBuffer byteBuffer = ByteBuffer.wrap(rawData);
            CharBuffer charBuffer = charset.decode(byteBuffer);
            char[] result = Arrays.copyOf(charBuffer.array(), lenght);
            clearData(charBuffer.array());
            clearData(rawData);
            return result;
        }
    }

    public void append(char[] chars) {
        char[] dataTmp = getChars();
        int lengthData = dataTmp.length;
        char[] newData = Arrays.copyOf(dataTmp, lengthData + chars.length);
        clearData(data);

        int posIni = lengthData;
        for (int i = 0; i < chars.length; i++) {
            newData[i + posIni] = chars[i];
        }

        CharBuffer charBuffer = CharBuffer.wrap(newData);
        ByteBuffer byteBuffer = charset.encode(charBuffer);
        byteBuffer.compact();
        setRawData(byteBuffer.array());
        lenght = newData.length;

        clearData(dataTmp);
        clearData(newData);
    }

    private void setRawData(byte[] data) {
        this.data = encrypt(data);
    }

    private byte[] getRawData() {
        return decrypt(data);
    }

    @Override
    public void destroy() {
        clearData(data);
        data = new byte[0];
        lenght = 0;
    }

    private byte[] encrypt(byte[] bytes) {
        try {
            Cipher cipher = Cipher.getInstance(DES_ECB_PKCS5_PADDING);
            cipher.init(Cipher.ENCRYPT_MODE, secretKey);
            return cipher.doFinal(bytes);
        } catch (InvalidKeyException | NoSuchAlgorithmException | NoSuchPaddingException | IllegalBlockSizeException
                | BadPaddingException e) {
            throw new IllegalStateException(e);
        }
    }

    private byte[] decrypt(byte[] bytes) {
        try {
            Cipher cipher = Cipher.getInstance(DES_ECB_PKCS5_PADDING);
            cipher.init(Cipher.DECRYPT_MODE, secretKey);
            return cipher.doFinal(bytes);
        } catch (InvalidKeyException | NoSuchAlgorithmException | NoSuchPaddingException | IllegalBlockSizeException
                | BadPaddingException e) {
            throw new IllegalStateException(e);
        }
    }   

    private static void clearData(byte[] cs) {
        Arrays.fill(cs, (byte) 0); 
    }   

    private static void clearData(char[] cs) {
        CharsUtils.clearData(cs);
    }       

}

It's a good idea or I must use another approach?

Edit 1: Our application need be comply with https://www.pcisecuritystandards.org/

  • 1
    If you have something like this: github.com/OWASP/passfault/blob/master/core/src/main/java/org/… and you finalize your SecureString, after the transaction is done, then you've addressed the primary threat of trying to find the pattern for your sensitive data. The big threat is that normal strings stick around in memory until garbage collection gets rid of them. Going beyond that is going to add a lot of complexity... Perhaps though you might want your SecurityKey to be calculated. That way it's even harder to get at. – Berin Loritsch Feb 27 '18 at 18:26
8

When you are talking about security, it's important to clearly define what you are worried about. This is typically called a 'threat model'. I gather that you are concerned with someone capturing the memory of your application and retrieving the card numbers from it.

If that is correct, I see a fundamental issue in that your key is also being stored in that same memory space as well as the code that decrypts it. You are only making it slightly harder to get the data. I would judge this equal to a basic obfuscation technique.

On a side note, DES is an obsolete cipher. I would suggest that if you are interested in this topic, you may want to do some more research. It's also important to keep up-to-date on such developments in this space. Old articles may suggest using approaches that are not considered secure even when they were authoritative at the time.

  • Our application need be comply with pcisecuritystandards.org and as far as I know they don't require to keep sensitive data encrypted in memory, but they require wipe memory after the data isn't needed anymore. – Rodrigo Menezes Feb 27 '18 at 17:00
  • 1
    About cipher, I'm thinking in use Triple DES – Rodrigo Menezes Feb 27 '18 at 17:05
  • 2
    @RodrigoMenezes Also obsolete. AES 128 or 256 is current. – JimmyJames Feb 27 '18 at 17:17
  • 1
    "don't require to keep sensitive data encrypted in memory" OK, then what is your goal in attempting to do so? – JimmyJames Feb 27 '18 at 17:18
  • 2
    It's not worse than nothing necessarily. If someone isn't very motivated, they might not bother to try to get the data. But you could just do something like rotate the bytes by one bit and get a similar effect. If you are doing this to get around clearing the memory, then maybe it's worse than nothing. – JimmyJames Feb 27 '18 at 18:09
4

OK, what's your threat model?

  • The data gets to the pagefile when the memory is overloaded, and some other process peeks at the pagefile. Your solution improves your odds to not get a decrypted page into the pagefile, but does not eliminate such a chance, since you still have to decrypt the data somewhere in RAM. The right solution would be no paging to disk at all, or an encrypted pagefile / swap partition.

  • A rouge process captures control of the system enough to directly observe the RAM of other processes, e.g. by gaining admin privileges. This is sort of game over for every process on this box; the rouge process now can detect where you store the decryption key, and decrypt the protected info, in RAM or in the DB. You solution becomes irrelevant.

  • You run not entirely trusted code inside your server, e.g. some sort of third-party plugin, and you want to prevent it from peeking information it should not have access to. See above.

The correct solution would be based on clear privilege separation, giving the code the least privilege it needs, and keeping the amount of trusted code as small as possible.

For instance, I'd keep the sensitive data in the main application encrypted at all times, likely even never fetched into RAM. It has nothing to do with it, except maybe checking its presence. This process should not have the access to the decryption key at all. The machine(s) that run the database should not have access to the decryption key either.

I'd run the process of export as a separate process, or maybe a separate VM, or even a separate physical box. That process would be a very small piece of code that takes the IDs of records to export, fetches them from the DB (where they are stored encrypted), decrypts them, and sends to the receiving end, presumably using a differently encrypted channel (e.g. via a TLS connection, or writing an encrypted file).

The exporter should alone be able to access the decryption key, presumably stored elsewhere, and have barely enough privileges to read (not write) the relevant part of the database, and to output to the export channel. Likely you already have some infra for storing, accessing, and rotating secret keys. The exporter should run for as short time as possible, and be shut down when not being used. Certainly it should not be accessible from the Internet, and minimally accessible from your internal network, including the machines running the other parts of the application.

This would reduce the remote attack surface, and add a layer or two for an attacker to pierce if e.g. your main app server, or a database server, is compromised.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.