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;
}
$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.");
}
java security cryptography
$endgroup$
migrated from stackoverflow.com 7 hours ago
This question came from our site for professional and enthusiast programmers.
add a comment |
$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.");
}
java security cryptography
$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
add a comment |
$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.");
}
java security cryptography
$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
java security cryptography
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
add a comment |
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
add a comment |
2 Answers
2
active
oldest
votes
$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
?
$endgroup$
add a comment |
$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$
$begingroup$
Hmm, good catch about theRandom
. 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. StillSecureRandom
should definitely be used here.
$endgroup$
– Maarten Bodewes
6 hours ago
2
$begingroup$
WhyFile
? Why notInputStream
andOutputStream
?
$endgroup$
– jpmc26
5 hours ago
$begingroup$
Hmm, for an outward facing interface function I would deemFile
an acceptable parameter - but the actual encryption should always take place on streams or buffers. Note that withFile
you are stuck to the file system, but you could e.g. switch to memory mapped I/O and encrypting / decryptingByteBuffer
.
$endgroup$
– Maarten Bodewes
3 hours ago
add a comment |
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
});
}
});
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
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
$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
?
$endgroup$
add a comment |
$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
?
$endgroup$
add a comment |
$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
?
$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
?
edited 6 hours ago
answered 8 hours ago
Maarten BodewesMaarten Bodewes
537211
537211
add a comment |
add a comment |
$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$
$begingroup$
Hmm, good catch about theRandom
. 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. StillSecureRandom
should definitely be used here.
$endgroup$
– Maarten Bodewes
6 hours ago
2
$begingroup$
WhyFile
? Why notInputStream
andOutputStream
?
$endgroup$
– jpmc26
5 hours ago
$begingroup$
Hmm, for an outward facing interface function I would deemFile
an acceptable parameter - but the actual encryption should always take place on streams or buffers. Note that withFile
you are stuck to the file system, but you could e.g. switch to memory mapped I/O and encrypting / decryptingByteBuffer
.
$endgroup$
– Maarten Bodewes
3 hours ago
add a comment |
$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$
$begingroup$
Hmm, good catch about theRandom
. 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. StillSecureRandom
should definitely be used here.
$endgroup$
– Maarten Bodewes
6 hours ago
2
$begingroup$
WhyFile
? Why notInputStream
andOutputStream
?
$endgroup$
– jpmc26
5 hours ago
$begingroup$
Hmm, for an outward facing interface function I would deemFile
an acceptable parameter - but the actual encryption should always take place on streams or buffers. Note that withFile
you are stuck to the file system, but you could e.g. switch to memory mapped I/O and encrypting / decryptingByteBuffer
.
$endgroup$
– Maarten Bodewes
3 hours ago
add a comment |
$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$
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.
edited 6 hours ago
answered 7 hours ago
Roland IlligRoland Illig
12.1k12049
12.1k12049
$begingroup$
Hmm, good catch about theRandom
. 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. StillSecureRandom
should definitely be used here.
$endgroup$
– Maarten Bodewes
6 hours ago
2
$begingroup$
WhyFile
? Why notInputStream
andOutputStream
?
$endgroup$
– jpmc26
5 hours ago
$begingroup$
Hmm, for an outward facing interface function I would deemFile
an acceptable parameter - but the actual encryption should always take place on streams or buffers. Note that withFile
you are stuck to the file system, but you could e.g. switch to memory mapped I/O and encrypting / decryptingByteBuffer
.
$endgroup$
– Maarten Bodewes
3 hours ago
add a comment |
$begingroup$
Hmm, good catch about theRandom
. 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. StillSecureRandom
should definitely be used here.
$endgroup$
– Maarten Bodewes
6 hours ago
2
$begingroup$
WhyFile
? Why notInputStream
andOutputStream
?
$endgroup$
– jpmc26
5 hours ago
$begingroup$
Hmm, for an outward facing interface function I would deemFile
an acceptable parameter - but the actual encryption should always take place on streams or buffers. Note that withFile
you are stuck to the file system, but you could e.g. switch to memory mapped I/O and encrypting / decryptingByteBuffer
.
$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
add a comment |
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.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
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
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
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
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