Implementing 3DES algorithm in Java: is my code secure? Announcing the arrival of Valued...
What *exactly* is electrical current, voltage, and resistance?
Suing a Police Officer Instead of the Police Department
Does Feeblemind produce an ongoing magical effect that can be dispelled?
How to open locks without disable device?
Check if a string is entirely made of the same substring
Co-worker works way more than he should
std::is_constructible on incomplete types
Are all CP/M-80 implementations binary compatible?
How to count in linear time worst-case?
Has a Nobel Peace laureate ever been accused of war crimes?
What is the term for a person whose job is to place products on shelves in stores?
Why does the Cisco show run command not show the full version, while the show version command does?
SQL Query not selecting all points that it should?
Putting Ant-Man on house arrest
Is Diceware more secure than a long passphrase?
How to find the right literary agent in the USA?
How to get even lighting when using flash for group photos near wall?
Seek and ye shall find
A Paper Record is What I Hamper
Is Electric Central Heating worth it if using Solar Panels?
Are these square matrices always diagonalisable?
Protagonist's race is hidden - should I reveal it?
Retract an already submitted recommendation letter (written for an undergrad student)
How to not starve gigantic beasts
Implementing 3DES algorithm in Java: is my code secure?
Announcing the arrival of Valued Associate #679: Cesar Manara
Unicorn Meta Zoo #1: Why another podcast?Rot -n algorithm in JavaIs my Encryption Module Secure?Secure password hashingWordgenerator algorithm using Java collectionSecure password-hashing in JavaSecure password hashing implementationSecure random number generatorPlacing secure data in Java web applicationSecure Encryption AlgorithmFind two numbers that add up to a given total, from a formatted string
.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 4 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 4 hours ago
This question came from our site for professional and enthusiast programmers.
2
$begingroup$
3DES is effectively broken and retired, so your code is "insecure" in terms of the cipher strength.
$endgroup$
– Benjamin Urquhart
8 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 4 hours ago
Cody Gray
3,490926
3,490926
asked 8 hours ago
adot710
migrated from stackoverflow.com 4 hours ago
This question came from our site for professional and enthusiast programmers.
migrated from stackoverflow.com 4 hours ago
This question came from our site for professional and enthusiast programmers.
2
$begingroup$
3DES is effectively broken and retired, so your code is "insecure" in terms of the cipher strength.
$endgroup$
– Benjamin Urquhart
8 hours ago
add a comment |
2
$begingroup$
3DES is effectively broken and retired, so your code is "insecure" in terms of the cipher strength.
$endgroup$
– Benjamin Urquhart
8 hours ago
2
2
$begingroup$
3DES is effectively broken and retired, so your code is "insecure" in terms of the cipher strength.
$endgroup$
– Benjamin Urquhart
8 hours ago
$begingroup$
3DES is effectively broken and retired, so your code is "insecure" in terms of the cipher strength.
$endgroup$
– Benjamin Urquhart
8 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
3 hours ago
2
$begingroup$
WhyFile
? Why notInputStream
andOutputStream
?
$endgroup$
– jpmc26
2 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
36 mins 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 3 hours ago
answered 4 hours ago
Maarten BodewesMaarten Bodewes
507211
507211
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
3 hours ago
2
$begingroup$
WhyFile
? Why notInputStream
andOutputStream
?
$endgroup$
– jpmc26
2 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
36 mins 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
3 hours ago
2
$begingroup$
WhyFile
? Why notInputStream
andOutputStream
?
$endgroup$
– jpmc26
2 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
36 mins 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 3 hours ago
answered 3 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
3 hours ago
2
$begingroup$
WhyFile
? Why notInputStream
andOutputStream
?
$endgroup$
– jpmc26
2 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
36 mins 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
3 hours ago
2
$begingroup$
WhyFile
? Why notInputStream
andOutputStream
?
$endgroup$
– jpmc26
2 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
36 mins 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
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
3 hours ago
2
2
$begingroup$
Why
File
? Why not InputStream
and OutputStream
?$endgroup$
– jpmc26
2 hours ago
$begingroup$
Why
File
? Why not InputStream
and OutputStream
?$endgroup$
– jpmc26
2 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
36 mins 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
36 mins 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
2
$begingroup$
3DES is effectively broken and retired, so your code is "insecure" in terms of the cipher strength.
$endgroup$
– Benjamin Urquhart
8 hours ago