dromara / dromara/Sa-Token

Crypto Issues: ECB Mode, Predictable Key Derivation, and Broken Hashes

Open
#830 0 comments 0 reactions 0 assignees View on GitHub
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.