Crypto Issues: ECB Mode, Predictable Key Derivation, and Broken Hashes
- Dominant language
- Java
- Stars
- 19k
- Forks
- 2.9k
- PR merge metrics
- No merged PRs in 30d
Description
We are security researchers at Digit Institute in Germany. During code review of [sa-token-core/src/main/java/cn/dev33/satoken/secure/SaSecureUtil.java], we identified several critical cryptography issues:
**1. AES in ECB Mode (Line 199)**
```
private static final String DEFAULT_CIPHER_ALGORITHM = "AES/ECB/PKCS5Padding";
Cipher cipher = Cipher.getInstance(DEFAULT_CIPHER_ALGORITHM);
```
Misuse: Use of AES in ECB mode for general encryption.
Risk: ECB mode leaks plaintext patterns and is insecure for almost any data.
Fix: Use "AES/GCM/NoPadding" with a random IV for each encryption.
**2. Predictable and Non-Standard Key Derivation from Password (Line 242)**
```
KeyGenerator kg = KeyGenerator.getInstance("AES");
SecureRandom random = SecureRandom.getInstance("SHA1PRNG");
random.setSeed(password.getBytes());
kg.init(128, random);
SecretKey secretKey = kg.generateKey();
```
Misuse: Predictable and non-standard key derivation from password.
Risk: No use of salt or iterations; resulting keys are easily guessable and vulnerable to brute-force and dictionary attacks. This is not a recognized secure Key Derivation Function (KDF).
Fix: Use a standard KDF (e.g., PBKDF2, scrypt, or Argon2) with a random salt and sufficient iterations.
```
SecretKeyFactory factory = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA256");
byte[] salt = new byte[16];
new SecureRandom().nextBytes(salt);
PBEKeySpec spec = new PBEKeySpec(password.toCharArray(), salt, 65536, 128);
SecretKey tmp = factory.generateSecret(spec);
SecretKeySpec secretKey = new SecretKeySpec(tmp.getEncoded(), "AES");
```
**Question:** _We are genuinely interested to know why, given that standard, robust key derivation functions are available (such as PBKDF2, scrypt, or Argon2), this non-standard approach was chosen. Was there a particular requirement, constraint, or legacy compatibility consideration driving this decision? Any insight you can provide would be greatly appreciated._
**3. MD5/SHA-1 Hashing (Lines 69, 94)**
```
MessageDigest md = MessageDigest.getInstance("MD5");
// or
MessageDigest sha = MessageDigest.getInstance("SHA1");
```
Misuse: Use of cryptographically broken hash functions for passwords or security-sensitive data.
Risk: MD5 and SHA-1 are vulnerable to collision and preimage attacks and should not be used in any security context.
Fix: For general-purpose hashing, use SHA-256/512. For password hashing, use bcrypt, PBKDF2, or Argon2.
Summary:
We recommend promptly addressing these issues to meet modern cryptographic standards and ensure application security.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start by reading sa-token-core/src/main/java/cn/dev33/satoken/secure/SaSecureUtil.java at the cited lines for AES encryption, password-based key generation, and MD5/SHA-1 hashing. Trace how these methods are used and determine compatibility requirements before changing the cryptographic approaches; done means all three reported weaknesses are addressed without breaking existing callers.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- cryptography, security
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Clearly specified
- Newbie friendliness
- 30/100