Take the 2-minute tour ×
Information Security Stack Exchange is a question and answer site for Information security professionals. It's 100% free, no registration required.

I am currently working on a file encryption program using Java Cryptography Architecture.

My plan is to have a number of encrypted files where each file has a different AES key. To keep track of the encrypted files I also include a metadata file which stores an entry for each encrypted file with the following values: plain text file name, cipher file name and AES Key.

The metadata file is then encrypted using an AES key that is derived from the user's password.

My question is should I implement a mode? and do you think keeping all the passwords in the encrypted metadata is acceptable practice?

Here is the code I have been referring to:

public class PasswordBasedEncryption {

PBEKeySpec pbeKeySpec;
PBEParameterSpec pbeParamSpec;
SecretKeyFactory keyFac;

// Salt
byte[] salt = {
        (byte)0xc7, (byte)0x73, (byte)0x21, (byte)0x8c,
        (byte)0x7e, (byte)0xc8, (byte)0xee, (byte)0x99
};
// Iteration count
int count = 65536;
int keySize = 128;

Cipher pbeCipher;
SecretKey pbeKey;
FileInputStream fis;
FileOutputStream fos;

/**
 * constructor given a master password
 * Use password based derivation function II to make AES key.
 * used only for the metadata file encryption
 * @param password
 */
public PasswordBasedEncryption(char[] password){
    try{
        keyFac = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA1");
        pbeKeySpec = new PBEKeySpec(password, salt, count, keySize);
        SecretKey tempKey = keyFac.generateSecret(pbeKeySpec);
        pbeKey = new SecretKeySpec(tempKey.getEncoded(), "AES");
        pbeCipher = Cipher.getInstance("AES");
    }
    catch (Exception e){e.printStackTrace();}
}
/**
 * constructor given a generated AES key
 * each file has its own AES key to avoid known text attacks
 * @param key
 */
public PasswordBasedEncryption(SecretKey key){
    try{
        pbeKey = key;
    }
    catch (Exception e){e.printStackTrace();}
}

public void encrypt(String filePath, String cipherName){
    try{
        File clearFile = new File(filePath);
        fis = new FileInputStream(clearFile);

        pbeCipher = Cipher.getInstance("AES");
        pbeCipher.init(Cipher.ENCRYPT_MODE, pbeKey);

        CipherInputStream cis = new CipherInputStream(fis, pbeCipher);          
        File cipherFile = new File(Path.TEMP + cipherName);
        fos = new FileOutputStream(cipherFile);
        int read;
        while((read = cis.read())!=-1)
        {
            fos.write((char)read);
            fos.flush();
        } 
        cis.close();
        fos.close();
        fis.close();
    }
    catch(Exception e ){e.printStackTrace();}
}

public void decrypt(String cipherName, String filePath){
    try{

        fis = new FileInputStream(Path.TEMP + cipherName);          
        File clearFile = new File(filePath);
        fos = new FileOutputStream(clearFile);

        pbeCipher = Cipher.getInstance("AES");
        pbeCipher.init(Cipher.DECRYPT_MODE, pbeKey);
        CipherOutputStream cos = new CipherOutputStream(fos, pbeCipher);            
        int read;
        while((read = fis.read())!=-1)
        {
            cos.write(read);
            cos.flush();
        } 
        cos.close();
        fis.close();
        fos.close();
    }
    catch(Exception e ){e.printStackTrace();}
}


/**
 * Generate secret password used each time a new file needs encrypting
 * @return
 */
public static SecretKey genPass(){
    KeyGenerator keyGen;
    try {
        keyGen = KeyGenerator.getInstance("AES");
        keyGen.init(128);
        return keyGen.generateKey();
    } catch (NoSuchAlgorithmException e) {
        e.printStackTrace();
        return null;
    }
}
}
share|improve this question

3 Answers 3

AES is a block cipher: it takes as input a key, and one block of exactly 16 bytes, and outputs another block of 16 bytes. That's the extent of what AES itself does.

So, if you want to encrypt a sequence of bytes which is not a single 16-byte block, then you must use a "mode of operation". And that is what you are already doing in your code. You just did not actually specify it in your code, and you let Java select one for you. Unfortunately, the default may be ECB, which is atrocious and weak; see this document which includes the following:

For example, the SunJCE provider uses ECB as the default mode, and PKCS5Padding as the default padding scheme for DES, DES-EDE and Blowfish ciphers.

(No word about AES here.)

So, in any case:

  • You MUST understand what you are doing. Security cannot be tested, so the only way to make secure code, especially with cryptography, is to have a thorough and in-depth knowledge of what happens internally.

  • You MUST specify exactly what you are doing. Don't let your language select ill-defined default values.

  • You MUST NOT do it yourself. Writing secure crypto code is one of the hardest tasks in development, precisely because there is no way to know if you did it correctly (although experience shows that the answer "no, it's been done poorly, it is weak" almost always fits).

share|improve this answer
    
Yes you are right Java 7 defaults to ECB. docs.oracle.com/javase/7/docs/technotes/guides/security/crypto/… –  John May 1 at 14:46

I see some issues with your code in general; it is not thrad-safe at all and discards exceptions, imho that is not suitable for any security-relevant application.

I assume that it is not for some kind of commercial project, but a private project you do to (and only to) learn something. That is totally fine. I would, however, recommend against shipping this to any customers (this includes friends), until you are more familiar with writing secure code.


You should specify padding and mode of your cipher, like "AES/CBC/PKCS5Padding". Not specifying a mode and padding may lead to implementation-dependent issues (such as a different JVM using another mode and thus unable to decrypt your file). Also note that modes like ECB do not protect against some attacks, refer to this encrypted image:

ECB-Encrypted Tux

Image Copyright: All uses are permitted provided that Larry Ewing, the owner of the original image, who requires that you mention him, his email address, lewing@isc.tamu.edu, and The GIMP, according to http://www.isc.tamu.edu/~lewing/linux/.

For more details on that, see Wikipedia's article about block cipher modes. So, I'd go CBC for your application.

share|improve this answer
    
Thanks for the quick response. I'm only calling these methods in a sequential manner but how could I make this thread safe? adding synchronized declarations? This work is for a uni project so no customers should be getting it ! –  John May 1 at 14:32
    
In general, I'd recommend writing all security-relevant bits of your code in a thread-safe way (even if they only are guaranteed to fail) with proper exception handling, no matter how you use it. You currently have many instance variables within the class, but you only use them locally. You assign these variables, so the JVM could for example change the fos while your method is running to a new fos from another thread. Note that it is not guaranteed that changes to fos are distributed among threads (without volatile). –  dst May 1 at 15:18

Well, no, I suppose you don't, but then there would be no way to encrypt anything beyond the first block with a given key. This is a bit like asking if you can encrypt something without input or without an encryption algorithm. The mode is how the key is derived for each block and how the encryption actually occurs. It is a critical part of the encryption, you can't "not have a mode" and have a working algorithm.

As for storing the AES file keys in the encrypted meta-data. It won't give you much extra security over using the same key for everything, but it isn't wrong. Your metadata file is basically a keyring then, thus if one file gets lost and cracked, the rest won't be compromised, though realistically, if you've done things correctly, an AES file isn't currently expected to be cracked before the heat death of the universe, so not really gaining much...

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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