This code is very bad. If you're copying it from elsewhere, you need to stop doing so. If you're writing it yourself, well, you shouldn't use it in any system that's actually security critical—you should seek expert assistance if that's the goal. The only justifiable purpose to code like this is as an educational exercise.
Let's dive in to the problems.
Use of obsolete and insecure DES cipher
DES is a 1970s cipher with 56-bit keys. That is much too small a keyspace for security; brute force search of DES keys was already demonstrated over 20 years ago.
Hardcoded cryptographic key?
I have found a string called key
= ucnTRFY<jhvbu,fwy8\u00a3329+3yrwiefDY#FGVoD@IOHa?widNCD(ZH3*82YU9%Fjpo
If this is a string constant in a real-world program, then that's really terrible. Programs should never hardcode cryptographic keys; they should read them at runtime from secure storage. The only scenario where it's acceptable to have a hardcoded key like that is in a trivial demo program.
Even if the string comes from a file instead of being hardcoded, the fact that you're able to read it already should make you wonder whether it was adequately protected. If it's a secret key, how come I'm reading it on the interwebs?
Obfuscated/convoluted key deserialization
DES takes its key as a byte[8]
. The program computes that byte[8]
from the key
string in a really convoluted manner:
private SecretKey getKey() {
byte[] desKeyData = new byte[8];
for (int i = 0; i < 7; ++i) {
int j = i * 3;
desKeyData[i] = (byte)(key.charAt(j) ^ key.charAt(j + 2));
}
return new SecretKeySpec(desKeyData, "DES");
}
This doesn't do anything other than make the code needlessly complicated. A well-written real-world program would obtain the desKeyData
by reading it directly from a binary file or by deserializing it from a string representation of a binary array (e.g. hexadecimal digits, Base64).
Passwords should not be encrypted
The code is encrypting a password. This is an insecure method of password storage. See the top answer to of our top passwords questions, "How to securely hash passwords?". That should answer your question about bcrypt as well.
Use of insecure cipher mode
This line of code:
Cipher cipher = Cipher.getInstance("DES");
...initializes the Cipher
object in ECB ("electronic codebook") mode. This is almost without an exception a bad idea.
Unauthenticated encryption
As a general rule, you should only use ciphers that protect both the authenticity and confidentiality of the messages. Modern ciphers protect not only against an attacker reading the content of the messages, but also against forgeries.
ECB mode doesn't do that. And you'll find lots of example code using modes like CBC that don't do so either.
Use of numeric literal instead of mode constant
This line is needlessly hard to read:
cipher.init(1, key);
It should be this:
cipher.init(Cipher.ENCRYPT_MODE, key);
Bad exception handling
Lots of lines like these:
catch (UnsupportedEncodingException ex) {
// empty catch block
}
Or these:
catch (BadPaddingException e) {
logger.error((Object)e);
}
return password;
Security-critical code needs to be exceptionally clean. If anything fails you should generally assume the worst and cause the whole operation to fail, not swallow an exception and keep going like nothing wrong happened.
(Also, that upcast to Object
in the second example is trivial and can be removed.)
Code discipline (method contracts, state avoidance, type safety)
All that logic with the doEncryption
and encrypted
boolean fields makes me nervous. It sounds like this code is inside a class whose objects can be in four states:
- Contains encrypted data, does encryption/decryption when requested to
- Contains unencrypted data, does encryption/decryption when requested to
- Contains encrypted data, ignores encryption/decryption requests
- Contains unecrypted data, ignores encryption/decryption requests
...and the class's methods and external clients then have to be mindful about its state so as not to expose an unencrypted password by mistake. The following bit of code in the encrypt()
method is the most symptomatic example:
if (!doEncryption) {
return encryptedBytes;
}
So if I'm using this class and I call encrypt(mySuperSecretData)
method, it might actually not encrypt it at all? What is the contract of the class and the methods? Is that contract really confusing or allow for the class's clients to easily misuse it?
Why have such a complicated class with these many self-defeating options? Maybe instead you should have separate types PlaintextPassword
and ScrambledPassword
so that when you try to pass a PlaintextPassword
to a piece of code that expects a ScrambledPassword
, it become an error at compilation time and not a security failure. With careful design and discipline you can gain much greater confidence that the program will not expose sensitive data by mistake.
This is part of the same theme as the previous point: security-critical code needs to be exceptionally clean. If I have to pause and read carefully the if
statements and a || b
conditions across those two methods to figure out whether there's a risk that they were incorrectly written and will reveal secret information in some scenarios, then I have to ask whether all that logic can just be removed.
Conclusion
This is the wrong level from which to approach cryptography. If you're interested in practical uses of cryptography, you should seek out a good, high-level cryptographic library that offers an easy interface, with good, safe default choices made for you an really good documentation that you can count on to tell you exactly how to use it.
The default Java cryptography API isn't that. Java has lots of cryptography support, but it all tends to be terribly hard and dangerous to use. It requires a lot of knowledge to use it safely, and that knowledge cannot be acquired by tinkering around with random code samples that you find somewhere—because if they're not just flat-out wrong, they're at least often outdated and no longer reflect best practices. Learning to use Java cryptography APIs generally requires reading third party materials that allow you to understand sound cryptography fundamentals.