LLM reports

[hakavlad]

A professional security audit carried out by experts is expensive. But a code audit performed by an LLM is free. So let's not miss the chance to look at the issues found by the LLM. At minimum, it's an additional powerful linter. At maximum, it can point out real, previously unnoticed problems that need fixing.

1. Code Security Audit of Kryptor v4.1.1 by Kimi K2 Thinking

https://drive.google.com/file/d/1QPTKHKpLB8xAqgaJ66LZHC9NSqURuyi6/view

Executive Summary

This audit identified 4 critical, 10 high-severity, 9 medium-severity, and 14 low-severity security
issues in the Kryptor codebase. The most severe vulnerabilities include unauthenticated key
wrapping enabling key manipulation attacks, a destructive file overwrite bug, integer overflow risks
with extremely large files, and incorrect private key version validation. The codebase shows
generally good cryptographic hygiene but has several protocol-level weaknesses and
implementation flaws that require immediate attention.

@samuel-lucas6 Please review the report. Most of the issues found are likely false positives, but some may be real and require fixing.

Edited: link to the report

[samuel-lucas6]

Code Security Audit of Kryptor v4.1.1 by Kimi K2 Thinking.pdf

Thank you for doing this. I agree that an LLM can help catch certain things.

I've read most of the report (not the verification part). The sections are a bit confusing and repetitive, so I'll focus on pages 25-37.

As it's getting late, I haven't written a summary of which ones are worth pursuing, but a lot of these are false positives. I'm not really surprised by these types of findings. I could do with a better background in secure development.

Critical Vulnerabilities

1. Unauthenticated Key Wrapping (Recipient Key Encryption)

The key wrap header is authenticated via the associated data when encrypting the file metadata header. This was done to avoid having a tag for each wrapped key, saving space.

Therefore, this is a false positive.

2. Catastrophic File Overwrite Logic Bug

The encrypted file gets copied to overwrite the plaintext file, and the overwritten file is deleted.

Therefore, this is a false positive. However, it would be better to avoid copying and instead open the plaintext file for writing to overwrite it with zeros, for example. File.Replace() creates a backup file so doesn't make sense.

3. Integer Overflow in Padding Calculation

The file would need to be larger than the biggest disks you can buy for this to be an issue.

Therefore, this is not a vulnerability, but the check or a checked statement could be added for best practice.

4. Incorrect Private Key Version Extraction

Constants.Curve25519KeyHeader and Constants.Ed25519KeyHeader have the same length, which is fixed. I think I wanted to avoid defining another constant that wouldn't be used much.

Therefore, this is a code readability issue. Another constant could be used, or this length could be selected based on the algorithm.

High Vulnerabilities

5. Unvalidated Base64 String Format

The exceptions are caught for Encodings.FromBase64().

Therefore, this is a best practice recommendation. The validation could be improved.

6. Predictable Temporary File Paths

The file name/path is predictable. I'd have to read into this further to understand the impact.

This could be changed to random file names.

7. Time-of-Check-Time-of-Use (TOCTOU) Race Conditions

If the file is read-only, this is required before you can overwrite. There will be an exception if the file is inaccessible.

I don't think this is a problem.

8. Fragile Nonce Management Pattern

Whilst an all-zero nonce is used, keys are unique each time.

Therefore, this isn't an issue. However, if I was starting from scratch, I would use random nonces more.

9. Incomplete Memory Zeroing on Exception Paths

The placement of the try ... catch statements means the zeroing will occur for the examples I looked at.

Therefore, this is a false positive. However, I wouldn't be surprised if there's room for improvement with zeroing somewhere.

10. Case-Sensitive Path Traversal on Windows

newPath.StartsWith() within FileHandling.ReplaceFileName() does seem to be case sensitive. I think this code was based on the Zip Slip Vulnerability advice by Synk.

This can be changed to case insensitive.

11. Race Condition in Public Key Export

If the file exists, nothing else happens.

This doesn't appear to be a problem.

12. Inadequate Key Length Validation

I believe there's length validation for public key strings but perhaps not when reading from a file.

This needs further investigation.

13. Inconsistent Prehashing Threshold

I don't consider this a problem because modern machines have 8+ GiB of memory. Signing without prehashing is also preferable from a security perspective, so a default of 100 MiB is rather low. If the user doesn't have enough memory, they can manually specify prehashing.

This can be left alone.

14. Floating-Point Arithmetic in Cryptographic Code

This rounding and casting seems to follow the original implementation of Covert Padding.

This can be left alone.

Medium Vulnerabilities

15. Signature Comment Byte Length Validation

The comment length is only validated based on the string rather than the bytes count.

This could be changed, but the comment is allowed to be a variable size.

16. Timing Side-Channel in Recipient Key Selection

I don't think this provides any useful information. The identity of the recipient isn't stored in the file.

This can be left alone.

17. Unsanitized Comment Injection in Key Files

This shouldn't corrupt the file format. The idea was to allow the user to write whatever they want as a comment.

There could be character validation for generating key files and when changing the private key passphrase.

18. Stackalloc in Unbounded Loops

unencryptedHeaders isn't allocated inside the loop.

This is a false positive.

19. Insecure Default Passphrase Generation Trigger

A space is not a valid Base64 character, and there would be a length difference here if more than one character was provided.

This isn't an issue.

20. Simultaneous Boolean and Exception Error Signaling

The return value is checked.

This type of return null pattern could potentially be improved but can make sense.

21. Magic Number in Key Algorithm Detection

The private key lengths are different for these algorithms. The algorithm header is being validated elsewhere, so I think I was trying to avoid doing that twice.

This may be worth reviewing as a code improvement.

22. Insufficient Validation of Private Key File Structure

This was deliberate to allow the user to add any sort of comment. However, this doesn't seem like the best file format.

This can be left alone for now.

23. Non-Atomic File Attribute Setting

It's not possible to write to read-only files.

This is a false positive.

Low Vulnerabilities

24. Global Mutable State

The program isn't trying to be thread safe.

However, this could potentially be improved.

25. Hardcoded Cryptographic Constants

These constants aren't in the Constants class because they're not used elsewhere.

This could be reviewed for readability.

26. Inconsistent Exception Wrapping

The original exception is being preserved.

This is a false positive.

27. Duplicate Code in Validation

This is because of the IEnumerable<string> approach to validation where multiple errors can be returned to the user at once, which is horrible to implement.

This could be investigated.

28. Lack of Domain Separation in Prehashing

This would be good practice, but the global signature signs whether or not the file was prehashed, which mitigates the issue with Minisign.

This would be a breaking change so should be left alone if possible. Ideally, prehashing and pure variants shouldn't both be supported.

29. Unnecessary Console Output in Cryptographic Operations

This shouldn't be a security problem.

The message could be displayed in another method, but there was probably a reason why this was put here, although the UI logic should ideally be separate.

30. Insufficient Input Sanitization in Command Line

This could be looked into, but there is validation for file paths, like whether they exist.

31. Missing Bounds Check in Chunk Encryption (continued)

The value of bytesRead probably should be being checked. However, the file shouldn't be possible to modify once opened, with the number of chunks calculated earlier after opening.

32. Inconsistent Use of pinned: true Memory Allocation

This could be reviewed, but the idea is to use stackalloc when possible, with GC.AllocateArray<byte>() if the data is too large for stackalloc. Both are suitable for zeroing.

33. Unvalidated File Name Encoding During Rename

This could be validated. There's some validation during encryption, so causing an error would likely need another tool.

34. Lack of Constant-Time Memory Comparison in Some Locations

This shouldn't need to be constant time. It was done this way for a simpler implementation due to them being strings rather than bytes.

35. Inconsistent Buffer Size Calculations

These are constants.

This is a false positive.

36. Unbounded Recursion in Directory Signing

With a lot of files, this may be a memory problem.

It would be worth looking at the Directory.GetFiles() implementation in .NET.

37. Missing Culture Invariance in String Operations

The version number isn't affected by culture.

This is a false positive.

Architectural Security Concerns

38. Mixing of Cryptographic Layers

Some logic in methods is intertwined. It would be better if things were written more like a library. That would require a massive overhaul though.

39. Key Derivation Personalization String Reuse

This is meant to be an application-specific personalisation string.

It would be better to have a longer context string that separates the type of key being derived. Unfortunately, BLAKE2 has a length limitation here and isn't really a proper KDF.

40. Missing Post-Quantum Cryptography Roadmap

This isn't a problem if you use symmetric cryptography, and I'm reliant on libsodium supporting post-quantum algorithms unless I add more dependencies or switch to another cryptographic library. Doing signatures shouldn't be difficult, but the KEM situation will be a nightmare. The key format would also need to change.

[samuel-lucas6]

As none of these seem particularly major, I'm going to focus on my API/code review of Geralt, which is a dependency here. It would make sense to use the next version of Geralt in the next version of Kryptor.