10
\$\begingroup\$

I am making a project just for fun that employs the 3DES algorithm in Java. I was wondering if my algorithm looks secure. Is there any advice or feedback you could give me? Do I need to include an IV? Am I transferring the salt successfully?

My code can encrypt a file using the Triple DES algorithm.

public static void main(String[] args) throws Exception {

    // file to be encrypted
    Scanner inputScanner = new Scanner(System.in);  
    System.out.println("Enter filename");
    String filename = inputScanner.nextLine();

    FileInputStream inputFile = new 

    // encrypted file
    FileOutputStream outputFile = new 

    // password to encrypt the file
    String passKey = "secretpasskey";
    byte[] salt = new byte[8];
    Random r = new Random();    
    r.nextBytes(salt);



    PBEParameterSpec pbeParameterSpec = new PBEParameterSpec(salt, 45432543);

    inputFile.close();
    outputFile.flush();
    outputFile.close();
    inputScanner.close();
    System.out.println("File has been Encrypted.");
}
\$\endgroup\$

migrated from stackoverflow.com Apr 24 at 18:41

This question came from our site for professional and enthusiast programmers.

  • 7
    \$\begingroup\$ 3DES is effectively broken and retired, so your code is "insecure" in terms of the cipher strength. \$\endgroup\$ – Benjamin Urquhart Apr 24 at 14:29
  • 1
    \$\begingroup\$ @BenjaminUrquhart It's not optimal and it is being officially retired, but it's not "effectively broken". The main issue is the small block size (limiting the amount you can encrypt with any given key, depending on block mode), and vulnerability to linear cryptanalysis (which is only an issue after on the order of a terabyte of known-plaintext is encrypted). \$\endgroup\$ – forest Apr 25 at 7:14
  • \$\begingroup\$ @forest fair enough, I did make that assumption. \$\endgroup\$ – Benjamin Urquhart Apr 25 at 14:59
18
\$\begingroup\$

It's kind of secure, but it uses older algorithms.

Although Benjamin correctly identifies 3DES, I would not call 3 key triple DES "broken". It still delivers a security of about 112 bits which nobody sane will try and break.

There is a chance that somebody would try and break your password though, and the shown password is clearly not random enough as it only contains 12 lowercase characters from a 26 character alphabet, which translates in 4.7 * 12 = 56 bits of entropy (each fully random letter delivers about 4.7 bits of entropy, 5.7 if upper and lowercase are randomly mixed). It may be that the relatively high number of iterations (99,999 iterations) will save you, but you're only supplying the 3DES key with half the entropy it requires to obtain the 112 bit security, so that's not enough.

The derivation method is probably secure, but it likely also performs too many operations which may just benefit an adversary. You are much better off with a more modern key derivation method such as Argon2. Likewise, we generally try and use authenticated encryption nowadays instead of the underlying CBC mode encryption. Problem is that there is no such prebuild solution directly available from the Java API, so you'd have to implement a copy of a protocol yourself or use a good library. Fernet would e.g. give you a more modern format.

You may want to include a version number to your encrypted messages so you can upgrade your algorithms or iteration count / salt size (etc.) at a later date. That way you can recognize older ciphertext, decrypt it, re-encrypt it with the newer protocol or keys and finally securely erase the old ciphertext. Or you could add additional encryption by encrypting it again using a different key derived from the password (a bit harder to do and to decrypt, but certainly possible).

SHA-1 has been broken, but not enough for it to become a problem for PBE. Of course you should still try and avoid old algorithms such as 3DES and SHA-1 and replace them with new ones such as AES and SHA-256.

Oh, almost forgot. Triple DES has a block size of 64 bits / 8 bytes, which is deemed pretty small nowadays. You could lose some confidentiality when encrypting large files with it in CBC mode because blocks may start to repeat (you might lose much more confidentiality with other modes of operation).


The idea of the password consisting of characters is that you can clear a char array, while you cannot do the same thing for a String. If you supply the password as a string then you lose this ability.

Do you know that there is a CipherInputStream and CipherOutputStream that can be put in front of a FileInputStream or FileOutputStream?

\$\endgroup\$
  • \$\begingroup\$ Great advice with the password I did not realise that thanks. I have also looked further into SHA-1 but I believe it will be okay for now, it's more secure than MD5 at least. I am having trouble trying to implement CipherInputStream and CipherOutputStream though is there any chance you could help me please? \$\endgroup\$ – adot710 Apr 25 at 12:02
  • 1
    \$\begingroup\$ Probably, but please ask on StackOverflow (check for dupes first). I've got some things to take care of today, so I might not be as responsive. There are other good programmers there though (kelalaka is of great help, jbtule etc.). \$\endgroup\$ – Maarten Bodewes Apr 25 at 13:33
  • \$\begingroup\$ sorry just had a thought about what block mode this code uses. Could you please let me know how you know it's ECB mode when it is not mentioned in the code? @Maarten Bodewes \$\endgroup\$ – adot710 May 6 at 21:09
  • 1
    \$\begingroup\$ I mentioned CBC, not ECB. Actually, my search on the internet page only showed one "ECB" mentioned and that's in your comment (probably now 3 :P). I mentioned CBC mode, and that's described e.g. here and, probably more authoritative, here. \$\endgroup\$ – Maarten Bodewes May 6 at 21:14
  • 1
    \$\begingroup\$ Well, yes, but as long as your salt is random for each encryption (preferably reencryption of the same file) then your key is unique. In that case your key / IV combination is of course also unique even if the IV is static. What you need to do in your code is to store the salt with the ciphertext, but you need to do that anyway to be able to decrypt. OpenSSL uses a similar format and doesn't store the IV either. \$\endgroup\$ – Maarten Bodewes May 8 at 15:19
13
\$\begingroup\$

No, it's not secure.

Your code is using Random instead of SecureRandom, which limits the entropy of the salt to 48 bits.

In addition, as an auditor I would immediately reject any "security code" that is implemented directly in the main method. To demonstrate that you understand the building blocks of a cipher, your code has to be structured into manageable methods that make the relation between the basic ingredients as clear as possible. The code should explain how the encryption works, without overwhelming the reader with needless technical details. Keeping track of 5 variables in your head is already difficult.

The outermost method should be encrypt(File in, File out, Key key, Random rnd). Only if you provide this kind of API can you write useful unit tests to demonstrate that the encryption code works for at least a few select examples.

\$\endgroup\$
  • 1
    \$\begingroup\$ Hmm, good catch about the Random. Although random required for a salt is kind of in between; you just don't want salt values never to repeat, but otherwise they don't need much security. Still SecureRandom should definitely be used here. \$\endgroup\$ – Maarten Bodewes Apr 24 at 19:23
  • 5
    \$\begingroup\$ Why File? Why not InputStream and OutputStream? \$\endgroup\$ – jpmc26 Apr 24 at 20:20
  • \$\begingroup\$ Hmm, for an outward facing interface function I would deem File an acceptable parameter - but the actual encryption should always take place on streams or buffers. Note that with File you are stuck to the file system, but you could e.g. switch to memory mapped I/O and encrypting / decrypting ByteBuffer. \$\endgroup\$ – Maarten Bodewes Apr 24 at 22:27
  • 1
    \$\begingroup\$ I chose to use File because it's closest to the current code and makes testing possible. It's not the most flexible or most elegant choice. I just wanted something a little more expressive than main(String...). It's correct that the encryption code should not force the plaintext to be written to disk. \$\endgroup\$ – Roland Illig Apr 25 at 3:55
  • \$\begingroup\$ Thank you Roland, I changed mine to SecureRandom now. I am also attempting to implement my code to the 5 methods you mentioned. \$\endgroup\$ – adot710 Apr 25 at 12:25

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.