Implementing 3DES algorithm in Java: is my code secure?





.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty,.everyoneloves__bot-mid-leaderboard:empty{ margin-bottom:0;
}







3












$begingroup$


I am making a project just for fun that implements 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 FileInputStream("C:\Documents\Encryptor\" + filename + ".txt");

// encrypted file
FileOutputStream outputFile = new FileOutputStream("C:\Documents\Encryptor\encryptedfile.des");

// password to encrypt the file
String passKey = "tkfhkggovubm";
byte salt = new byte[8];
Random r = new Random();
r.nextBytes(salt);
PBEKeySpec pbeKeySpec = new PBEKeySpec(passKey.toCharArray());
SecretKeyFactory secretKeyFactory = SecretKeyFactory
.getInstance("PBEWithSHA1AndDESede");
SecretKey secretKey = secretKeyFactory.generateSecret(pbeKeySpec);


PBEParameterSpec pbeParameterSpec = new PBEParameterSpec(salt, 99999);
Cipher cipher = Cipher.getInstance("PBEWithSHA1AndDESede");
cipher.init(Cipher.ENCRYPT_MODE, secretKey, pbeParameterSpec);
outputFile.write(salt);

byte input = new byte[64];
int bytesRead;
while ((bytesRead = inputFile.read(input)) != -1) {
byte output = cipher.update(input, 0, bytesRead);
if (output != null)
outputFile.write(output);
}

byte output = cipher.doFinal();
if (output != null)
outputFile.write(output);

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









share|improve this question











$endgroup$



migrated from stackoverflow.com 7 hours ago


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














  • 4




    $begingroup$
    3DES is effectively broken and retired, so your code is "insecure" in terms of the cipher strength.
    $endgroup$
    – Benjamin Urquhart
    11 hours ago


















3












$begingroup$


I am making a project just for fun that implements 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 FileInputStream("C:\Documents\Encryptor\" + filename + ".txt");

// encrypted file
FileOutputStream outputFile = new FileOutputStream("C:\Documents\Encryptor\encryptedfile.des");

// password to encrypt the file
String passKey = "tkfhkggovubm";
byte salt = new byte[8];
Random r = new Random();
r.nextBytes(salt);
PBEKeySpec pbeKeySpec = new PBEKeySpec(passKey.toCharArray());
SecretKeyFactory secretKeyFactory = SecretKeyFactory
.getInstance("PBEWithSHA1AndDESede");
SecretKey secretKey = secretKeyFactory.generateSecret(pbeKeySpec);


PBEParameterSpec pbeParameterSpec = new PBEParameterSpec(salt, 99999);
Cipher cipher = Cipher.getInstance("PBEWithSHA1AndDESede");
cipher.init(Cipher.ENCRYPT_MODE, secretKey, pbeParameterSpec);
outputFile.write(salt);

byte input = new byte[64];
int bytesRead;
while ((bytesRead = inputFile.read(input)) != -1) {
byte output = cipher.update(input, 0, bytesRead);
if (output != null)
outputFile.write(output);
}

byte output = cipher.doFinal();
if (output != null)
outputFile.write(output);

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









share|improve this question











$endgroup$



migrated from stackoverflow.com 7 hours ago


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














  • 4




    $begingroup$
    3DES is effectively broken and retired, so your code is "insecure" in terms of the cipher strength.
    $endgroup$
    – Benjamin Urquhart
    11 hours ago














3












3








3





$begingroup$


I am making a project just for fun that implements 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 FileInputStream("C:\Documents\Encryptor\" + filename + ".txt");

// encrypted file
FileOutputStream outputFile = new FileOutputStream("C:\Documents\Encryptor\encryptedfile.des");

// password to encrypt the file
String passKey = "tkfhkggovubm";
byte salt = new byte[8];
Random r = new Random();
r.nextBytes(salt);
PBEKeySpec pbeKeySpec = new PBEKeySpec(passKey.toCharArray());
SecretKeyFactory secretKeyFactory = SecretKeyFactory
.getInstance("PBEWithSHA1AndDESede");
SecretKey secretKey = secretKeyFactory.generateSecret(pbeKeySpec);


PBEParameterSpec pbeParameterSpec = new PBEParameterSpec(salt, 99999);
Cipher cipher = Cipher.getInstance("PBEWithSHA1AndDESede");
cipher.init(Cipher.ENCRYPT_MODE, secretKey, pbeParameterSpec);
outputFile.write(salt);

byte input = new byte[64];
int bytesRead;
while ((bytesRead = inputFile.read(input)) != -1) {
byte output = cipher.update(input, 0, bytesRead);
if (output != null)
outputFile.write(output);
}

byte output = cipher.doFinal();
if (output != null)
outputFile.write(output);

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









share|improve this question











$endgroup$




I am making a project just for fun that implements 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 FileInputStream("C:\Documents\Encryptor\" + filename + ".txt");

// encrypted file
FileOutputStream outputFile = new FileOutputStream("C:\Documents\Encryptor\encryptedfile.des");

// password to encrypt the file
String passKey = "tkfhkggovubm";
byte salt = new byte[8];
Random r = new Random();
r.nextBytes(salt);
PBEKeySpec pbeKeySpec = new PBEKeySpec(passKey.toCharArray());
SecretKeyFactory secretKeyFactory = SecretKeyFactory
.getInstance("PBEWithSHA1AndDESede");
SecretKey secretKey = secretKeyFactory.generateSecret(pbeKeySpec);


PBEParameterSpec pbeParameterSpec = new PBEParameterSpec(salt, 99999);
Cipher cipher = Cipher.getInstance("PBEWithSHA1AndDESede");
cipher.init(Cipher.ENCRYPT_MODE, secretKey, pbeParameterSpec);
outputFile.write(salt);

byte input = new byte[64];
int bytesRead;
while ((bytesRead = inputFile.read(input)) != -1) {
byte output = cipher.update(input, 0, bytesRead);
if (output != null)
outputFile.write(output);
}

byte output = cipher.doFinal();
if (output != null)
outputFile.write(output);

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






java security cryptography






share|improve this question















share|improve this question













share|improve this question




share|improve this question








edited 7 hours ago









Cody Gray

3,490926




3,490926










asked 11 hours ago







adot710











migrated from stackoverflow.com 7 hours ago


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









migrated from stackoverflow.com 7 hours ago


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










  • 4




    $begingroup$
    3DES is effectively broken and retired, so your code is "insecure" in terms of the cipher strength.
    $endgroup$
    – Benjamin Urquhart
    11 hours ago














  • 4




    $begingroup$
    3DES is effectively broken and retired, so your code is "insecure" in terms of the cipher strength.
    $endgroup$
    – Benjamin Urquhart
    11 hours ago








4




4




$begingroup$
3DES is effectively broken and retired, so your code is "insecure" in terms of the cipher strength.
$endgroup$
– Benjamin Urquhart
11 hours ago




$begingroup$
3DES is effectively broken and retired, so your code is "insecure" in terms of the cipher strength.
$endgroup$
– Benjamin Urquhart
11 hours ago










2 Answers
2






active

oldest

votes


















9












$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, reencrypt it with the newer protocol or keys and finally securely erase the old ciphertext.



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





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?






share|improve this answer











$endgroup$





















    7












    $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.






    share|improve this answer











    $endgroup$













    • $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
      6 hours ago






    • 2




      $begingroup$
      Why File? Why not InputStream and OutputStream?
      $endgroup$
      – jpmc26
      5 hours ago










    • $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
      3 hours ago














    Your Answer






    StackExchange.ifUsing("editor", function () {
    StackExchange.using("externalEditor", function () {
    StackExchange.using("snippets", function () {
    StackExchange.snippets.init();
    });
    });
    }, "code-snippets");

    StackExchange.ready(function() {
    var channelOptions = {
    tags: "".split(" "),
    id: "196"
    };
    initTagRenderer("".split(" "), "".split(" "), channelOptions);

    StackExchange.using("externalEditor", function() {
    // Have to fire editor after snippets, if snippets enabled
    if (StackExchange.settings.snippets.snippetsEnabled) {
    StackExchange.using("snippets", function() {
    createEditor();
    });
    }
    else {
    createEditor();
    }
    });

    function createEditor() {
    StackExchange.prepareEditor({
    heartbeatType: 'answer',
    autoActivateHeartbeat: false,
    convertImagesToLinks: false,
    noModals: true,
    showLowRepImageUploadWarning: true,
    reputationToPostImages: null,
    bindNavPrevention: true,
    postfix: "",
    imageUploader: {
    brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
    contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
    allowUrls: true
    },
    onDemand: true,
    discardSelector: ".discard-answer"
    ,immediatelyShowMarkdownHelp:true
    });


    }
    });














    draft saved

    draft discarded


















    StackExchange.ready(
    function () {
    StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f219039%2fimplementing-3des-algorithm-in-java-is-my-code-secure%23new-answer', 'question_page');
    }
    );

    Post as a guest















    Required, but never shown
























    2 Answers
    2






    active

    oldest

    votes








    2 Answers
    2






    active

    oldest

    votes









    active

    oldest

    votes






    active

    oldest

    votes









    9












    $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, reencrypt it with the newer protocol or keys and finally securely erase the old ciphertext.



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





    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?






    share|improve this answer











    $endgroup$


















      9












      $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, reencrypt it with the newer protocol or keys and finally securely erase the old ciphertext.



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





      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?






      share|improve this answer











      $endgroup$
















        9












        9








        9





        $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, reencrypt it with the newer protocol or keys and finally securely erase the old ciphertext.



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





        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?






        share|improve this answer











        $endgroup$



        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, reencrypt it with the newer protocol or keys and finally securely erase the old ciphertext.



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





        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?







        share|improve this answer














        share|improve this answer



        share|improve this answer








        edited 6 hours ago

























        answered 8 hours ago









        Maarten BodewesMaarten Bodewes

        537211




        537211

























            7












            $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.






            share|improve this answer











            $endgroup$













            • $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
              6 hours ago






            • 2




              $begingroup$
              Why File? Why not InputStream and OutputStream?
              $endgroup$
              – jpmc26
              5 hours ago










            • $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
              3 hours ago


















            7












            $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.






            share|improve this answer











            $endgroup$













            • $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
              6 hours ago






            • 2




              $begingroup$
              Why File? Why not InputStream and OutputStream?
              $endgroup$
              – jpmc26
              5 hours ago










            • $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
              3 hours ago
















            7












            7








            7





            $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.






            share|improve this answer











            $endgroup$



            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.







            share|improve this answer














            share|improve this answer



            share|improve this answer








            edited 6 hours ago

























            answered 7 hours ago









            Roland IlligRoland Illig

            12.1k12049




            12.1k12049












            • $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
              6 hours ago






            • 2




              $begingroup$
              Why File? Why not InputStream and OutputStream?
              $endgroup$
              – jpmc26
              5 hours ago










            • $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
              3 hours ago




















            • $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
              6 hours ago






            • 2




              $begingroup$
              Why File? Why not InputStream and OutputStream?
              $endgroup$
              – jpmc26
              5 hours ago










            • $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
              3 hours ago


















            $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
            6 hours ago




            $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
            6 hours ago




            2




            2




            $begingroup$
            Why File? Why not InputStream and OutputStream?
            $endgroup$
            – jpmc26
            5 hours ago




            $begingroup$
            Why File? Why not InputStream and OutputStream?
            $endgroup$
            – jpmc26
            5 hours ago












            $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
            3 hours ago






            $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
            3 hours ago




















            draft saved

            draft discarded




















































            Thanks for contributing an answer to Code Review Stack Exchange!


            • Please be sure to answer the question. Provide details and share your research!

            But avoid



            • Asking for help, clarification, or responding to other answers.

            • Making statements based on opinion; back them up with references or personal experience.


            Use MathJax to format equations. MathJax reference.


            To learn more, see our tips on writing great answers.




            draft saved


            draft discarded














            StackExchange.ready(
            function () {
            StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f219039%2fimplementing-3des-algorithm-in-java-is-my-code-secure%23new-answer', 'question_page');
            }
            );

            Post as a guest















            Required, but never shown





















































            Required, but never shown














            Required, but never shown












            Required, but never shown







            Required, but never shown

































            Required, but never shown














            Required, but never shown












            Required, but never shown







            Required, but never shown







            Popular posts from this blog

            How did Captain America manage to do this?

            迪纳利

            南乌拉尔铁路局