security(logging): fix aes implementation in audit logging

[joshua-goldstein]

Background

Audit logging is an enterprise feature with an option for encryption using AES. An AES cipher has three ingredients: a private key, an iv (or nonce), and plaintext. The nonce is public but it must be unique.

Problem

Currently we use a [16]byte nonce. The first 12 bytes come from a baseIv which is initialized when an audit log is created. The last 4 bytes come from the length of the log line being encrypted. This is problematic because two log lines will often have the same length, so due to these collisions we are reusing the same nonce many times. AES requires a unique nonce and without a unique nonce it is trivial to break. Many thanks to @HakuPiku for pointing this out to us.

Solution

Use crypto/rand to generate a unique nonce every time we encrypt a log line. Previously we used what was essentially a 16 byte header (12 byte baseIv and 4 byte length of line). We now use a [20] byte header, consisting of a unique nonce (16 bytes) and length of line (4 bytes).

Remarks

• For audit logging, x/log_writer.go controls all the audit logging (i.e., writing) while ee/audit/run_ee.go does all the decrypting. There are other encryption/decryption tools (in testutil, e.g.) that are used for other purposes (e.g., exports).
• Customers may have old audit logs that will have to be decrypted using the deprecated method. Now the decrypt method will determine whether the audit log is old or new and use the appropriate scheme.
• This PR also adds several integration tests to check if we can appropriately encrypt and decrypt audit logs (previously we had no integration tests for this, only a small unit test)
• We also add tests to check if an audit log in the old format can be decrypted correctly. This is the reason there is an .enc file checked into the PR.
• Finally we add an extra unit test to verify that encrypted audit logs can be truncated and still decrypted successfully (up to the line that was truncated)

References

• https://en.wikipedia.org/wiki/Block_cipher_mode_of_operation
• https://pkg.go.dev/crypto/cipher#example-NewCTR

[coveralls]

Coverage decreased (-0.05%) to 37.046% when pulling 8c42c19 on joshua/fix-logger into b69b8e5 on main.

[HakuPiku]

I can see that a unique nonce is generated for every log line and the fix seems correct.

[HakuPiku]

Any updates on when this will be merged and a security advisory created?

[mangalaman93]

I think we should consider writing more robust functions and test. This needs more work.

[mangalaman93]

I didn't realize that byte array/slice can't be declared as a constant. This article has a few ideas https://blog.boot.dev/golang/golang-constant-maps-slices/. I don't mind if we declare a function that returns the constant value. I think it is useful to avoid a allocation that converts byte slice to string.

[joshua-goldstein]

I think we should consider writing more robust functions and test. This needs more work.

I have already added 4 integration tests, and we have a unit test to check basic parsing. What additional tests are necessary?

[joshua-goldstein]

I didn't realize that byte array/slice can't be declared as a constant. This article has a few ideas https://blog.boot.dev/golang/golang-constant-maps-slices/. I don't mind if we declare a function that returns the constant value. I think it is useful to avoid a allocation that converts byte slice to string.

What is the purpose of this? We are doing exactly one allocation per log file (default ~200 mb), so this is not a memory issue?

[joshua-goldstein]

The scope of this PR is now much larger than originally intended. This PR now introduces:

• Four new integration tests for encrypted audit logging in systest/audit_encrypted/audit_test.go, two of which verify that encrypted audit logs can be encrypted, and two that verify that deprecated audit logs can be decrypted
• One new unit test in ee/audit/run_ee_test.go to verify that truncating an encrypted audit log does not cause the decrypt process to panic
• A modified unit test to decrypt using the new audit log format

This PR also does some minor refactoring to the code.

[mangalaman93]

Did a quick review for now. I will review it again tomorrow.