Change compression prior to encryption for best results #351
Conversation
WalkthroughThe changes in this pull request modify the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
utils/crypto.py (1)
Line range hint
1-38: Consider alternative approaches and migration strategyWhile compressing before encryption is more efficient, the current implementation introduces several concerns:
Breaking Changes:
- Existing encrypted configurations will fail to decrypt
- Need for a migration strategy or backward compatibility
Security Considerations:
- Compression before encryption needs careful analysis of potential oracle attacks
- Consider if the compression gain justifies the security complexity
Alternative Approaches:
- Instead of modifying the encryption process, consider:
- Using shorter field names in the JSON structure
- Implementing a more efficient encoding scheme
- Using a binary serialization format like Protocol Buffers
Recommendations:
- Add a version identifier to support both old and new formats
- Document the breaking changes and provide migration scripts
- Add comprehensive security tests for compression-related attacks
- Consider implementing the suggested alternative approaches first
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
utils/crypto.py(1 hunks)
🔇 Additional comments (1)
utils/crypto.py (1)
22-26:
Security: Evaluate compression oracle attacks and use standard padding
The implementation has several security considerations:
- Compressing data before encryption could be vulnerable to compression oracle attacks (like CRIME/BREACH) if the data contains both secrets and attacker-controlled input.
- Using custom null byte padding instead of standard PKCS7 padding (from Crypto.Util.Padding) is risky.
Let's verify if there's any attacker-controlled input being encrypted:
Suggested fixes:
- encoded_text = zlib.compress(text.encode("utf-8"))
- encrypted_data = cipher.encrypt(
- encoded_text + b"\0" * (16 - len(encoded_text) % 16)
- )
+ encoded_text = zlib.compress(text.encode("utf-8"))
+ padded_data = pad(encoded_text, AES.block_size)
+ encrypted_data = cipher.encrypt(padded_data)| decrypted_data = decrypted_data.rstrip(b"\0") | ||
| return decrypted_data.decode("utf-8") | ||
| return zlib.decompress(decrypted_data).decode("utf-8") |
There was a problem hiding this comment.
Add error handling and backward compatibility
The decompression step lacks error handling and breaks backward compatibility with existing encrypted data.
- Add error handling for decompression:
decrypted_data = decrypted_data.rstrip(b"\0")
- return zlib.decompress(decrypted_data).decode("utf-8")
+ try:
+ return zlib.decompress(decrypted_data).decode("utf-8")
+ except zlib.error:
+ # Attempt backward compatibility with uncompressed data
+ try:
+ return decrypted_data.decode("utf-8")
+ except UnicodeDecodeError:
+ raise ValueError("Failed to decrypt: invalid data format")- Consider adding a version byte to the encrypted data to properly handle both compressed and uncompressed formats:
# In encrypt_text:
version = b'\x01' # Version 1: compressed
encrypted_str = urlsafe_b64encode(version + iv + encrypted_data).decode("utf-8")
# In decrypt_text:
version = decoded_data[0:1]
iv = decoded_data[1:17]
if version == b'\x01':
# Handle compressed data
else:
# Handle uncompressed data| iv = decoded_data[:16] | ||
| if isinstance(secret_key, str): | ||
| secret_key = secret_key.encode("utf-8") | ||
| cipher = AES.new(secret_key.ljust(32)[:32], AES.MODE_CBC, iv) | ||
| decrypted_data = cipher.decrypt(encrypted_data[16:]) | ||
| decrypted_data = cipher.decrypt(decoded_data[16:]) |
There was a problem hiding this comment.
Add input validation for IV and ciphertext
The function lacks crucial input validation which could lead to security issues:
- No validation of IV length (should be 16 bytes)
- No validation of decoded data length (should be at least IV length + block size)
Apply this fix:
decoded_data = urlsafe_b64decode(secret_str)
+ if len(decoded_data) < 32: # minimum length: 16 (IV) + 16 (one AES block)
+ raise ValueError("Invalid encrypted data length")
iv = decoded_data[:16]
if isinstance(secret_key, str):
secret_key = secret_key.encode("utf-8")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| iv = decoded_data[:16] | |
| if isinstance(secret_key, str): | |
| secret_key = secret_key.encode("utf-8") | |
| cipher = AES.new(secret_key.ljust(32)[:32], AES.MODE_CBC, iv) | |
| decrypted_data = cipher.decrypt(encrypted_data[16:]) | |
| decrypted_data = cipher.decrypt(decoded_data[16:]) | |
| decoded_data = urlsafe_b64decode(secret_str) | |
| if len(decoded_data) < 32: # minimum length: 16 (IV) + 16 (one AES block) | |
| raise ValueError("Invalid encrypted data length") | |
| iv = decoded_data[:16] | |
| if isinstance(secret_key, str): | |
| secret_key = secret_key.encode("utf-8") | |
| cipher = AES.new(secret_key.ljust(32)[:32], AES.MODE_CBC, iv) | |
| decrypted_data = cipher.decrypt(decoded_data[16:]) |
|
This is a good finding, I remember that I changed this method with my test cases for good compression and speed. So even with this, we're always going to hit the URL max size (according to pydantic validation its 2083 chars). I think it's better to go for storing the encrypted text in REDIS with 1 month of expire & when getting the data from Redis, we can use The Encrypt logicuser data -> encrypt with server secret -> find the md5 hash (32 chars) + random chars with dynamic length (5-18 chars to harden the prediction) so always this will be a lesser than 50 chars -> store in Redis as key as The Decrypt logicThis will be in the reverse. We know the secret_str then find it from Redis or raise an error (don't use default config, this might be issue where a user configured RD for safety then if he doesn't use MF for 1 month and try to use it then it accidentally open up the torrent). |
Config string length : 1297 characters
Compression after encryption : 1789 characters.
Compression before encryption : 833 characters.
This is much efficient in general cause base text has better compression than encrypted text. As you can see, compression after encryption leads to more characters than the original config string length itself without compression.
This change will break existing installation - this PR is to get comments first before figuring out backwards compatibility required to make it work with existing configs too if needed.
Another improvement that can be achieved is to also convert fields like tamil_hdrip etc to more efficient strings like "thd" etc. That should keep the URL length very short.
Summary by CodeRabbit
New Features
Bug Fixes