From 12f97ef51f7921008707bc69647195bc16e45469 Mon Sep 17 00:00:00 2001 From: Gusted Date: Tue, 20 Aug 2024 23:13:04 +0200 Subject: [PATCH] [SEC] Add `keying` module The keying modules tries to solve two problems, the lack of key separation and the lack of AEAD being used for encryption. The currently used `secrets` doesn't provide this and is hard to adjust to provide this functionality. For encryption, the additional data is now a parameter that can be used, as the underlying primitive is an AEAD constructions. This allows for context binding to happen and can be seen as defense-in-depth; it ensures that if a value X is encrypted for context Y (e.g. ID=3, Column="private_key") it will only decrypt if that context Y is also given in the Decrypt function. This makes confused deputy attack harder to exploit.[^1] For key separation, HKDF is used to derives subkeys from some IKM, which is the value of the `[service].SECRET_KEY` config setting. The context for subkeys are hardcoded, any variable should be shuffled into the the additional data parameter when encrypting. [^1]: This is still possible, because the used AEAD construction is not key-comitting. For Forgejo's current use-case this risk is negligible, because the subkeys aren't known to a malicious user (which is required for such attack), unless they also have access to the IKM (at which point you can assume the whole system is compromised). See https://scottarc.blog/2022/10/17/lucid-multi-key-deputies-require-commitment/ --- .deadcode-out | 5 ++ modules/keying/keying.go | 111 ++++++++++++++++++++++++++++++++++ modules/keying/keying_test.go | 96 +++++++++++++++++++++++++++++ modules/setting/security.go | 2 + 4 files changed, 214 insertions(+) create mode 100644 modules/keying/keying.go create mode 100644 modules/keying/keying_test.go diff --git a/.deadcode-out b/.deadcode-out index 72d5df86dc..539056a429 100644 --- a/.deadcode-out +++ b/.deadcode-out @@ -170,6 +170,11 @@ code.gitea.io/gitea/modules/json StdJSON.NewDecoder StdJSON.Indent +code.gitea.io/gitea/modules/keying + DeriveKey + Key.Encrypt + Key.Decrypt + code.gitea.io/gitea/modules/markup GetRendererByType RenderString diff --git a/modules/keying/keying.go b/modules/keying/keying.go new file mode 100644 index 0000000000..7cf5f28a44 --- /dev/null +++ b/modules/keying/keying.go @@ -0,0 +1,111 @@ +// Copyright 2024 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +// Keying is a module that allows for subkeys to be determistically generated +// from the same master key. It allows for domain seperation to take place by +// using new keys for new subsystems/domains. These subkeys are provided with +// an API to encrypt and decrypt data. The module panics if a bad interaction +// happened, the panic should be seen as an non-recoverable error. +// +// HKDF (per RFC 5869) is used to derive new subkeys in a safe manner. It +// provides a KDF security property, which is required for Forgejo, as the +// secret key would be an ASCII string and isn't a random uniform bit string. +// XChaCha-Poly1305 (per draft-irtf-cfrg-xchacha-01) is used as AEAD to encrypt +// and decrypt messages. A new fresh random nonce is generated for every +// encryption. The nonce gets prepended to the ciphertext. +package keying + +import ( + "crypto/rand" + "crypto/sha256" + + "golang.org/x/crypto/chacha20poly1305" + "golang.org/x/crypto/hkdf" +) + +var ( + // The hash used for HKDF. + hash = sha256.New + // The AEAD used for encryption/decryption. + aead = chacha20poly1305.NewX + aeadKeySize = chacha20poly1305.KeySize + aeadNonceSize = chacha20poly1305.NonceSizeX + // The pseudorandom key generated by HKDF-Extract. + prk []byte +) + +// Set the main IKM for this module. +func Init(ikm []byte) { + // Salt is intentionally left empty, it's not useful to Forgejo's use case. + prk = hkdf.Extract(hash, ikm, nil) +} + +// Specifies the context for which a subkey should be derived for. +// This must be a hardcoded string and must not be arbitrarily constructed. +type Context string + +// Derive *the* key for a given context, this is a determistic function. The +// same key will be provided for the same context. +func DeriveKey(context Context) *Key { + if len(prk) == 0 { + panic("keying: not initialized") + } + + r := hkdf.Expand(hash, prk, []byte(context)) + + key := make([]byte, aeadKeySize) + // This should never return an error, but if it does, panic. + if _, err := r.Read(key); err != nil { + panic(err) + } + + return &Key{key} +} + +type Key struct { + key []byte +} + +// Encrypts the specified plaintext with some additional data that is tied to +// this plaintext. The additional data can be seen as the context in which the +// data is being encrypted for, this is different than the context for which the +// key was derrived this allows for more granuality without deriving new keys. +// Avoid any user-generated data to be passed into the additional data. The most +// common usage of this would be to encrypt a database field, in that case use +// the ID and database column name as additional data. The additional data isn't +// appended to the ciphertext and may be publicly known, it must be available +// when decryping the ciphertext. +func (k *Key) Encrypt(plaintext, additionalData []byte) []byte { + // Construct a new AEAD with the key. + e, err := aead(k.key) + if err != nil { + panic(err) + } + + // Generate a random nonce. + nonce := make([]byte, aeadNonceSize) + if _, err := rand.Read(nonce); err != nil { + panic(err) + } + + // Returns the ciphertext of this plaintext. + return e.Seal(nonce, nonce, plaintext, additionalData) +} + +// Decrypts the ciphertext and authenticates it against the given additional +// data that was given when it was encrypted. It returns an error if the +// authentication failed. +func (k *Key) Decrypt(ciphertext, additionalData []byte) ([]byte, error) { + if len(ciphertext) <= aeadNonceSize { + panic("keying: ciphertext is too short") + } + + e, err := aead(k.key) + if err != nil { + panic(err) + } + + nonce, ciphertext := ciphertext[:aeadNonceSize], ciphertext[aeadNonceSize:] + + return e.Open(nil, nonce, ciphertext, additionalData) +} diff --git a/modules/keying/keying_test.go b/modules/keying/keying_test.go new file mode 100644 index 0000000000..16a6781af8 --- /dev/null +++ b/modules/keying/keying_test.go @@ -0,0 +1,96 @@ +// Copyright 2024 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package keying_test + +import ( + "testing" + + "code.gitea.io/gitea/modules/keying" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/crypto/chacha20poly1305" +) + +func TestKeying(t *testing.T) { + t.Run("Not initalized", func(t *testing.T) { + assert.Panics(t, func() { + keying.DeriveKey(keying.Context("TESTING")) + }) + }) + + t.Run("Initialization", func(t *testing.T) { + keying.Init([]byte{0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07}) + }) + + t.Run("Context seperation", func(t *testing.T) { + key1 := keying.DeriveKey(keying.Context("TESTING")) + key2 := keying.DeriveKey(keying.Context("TESTING2")) + + ciphertext := key1.Encrypt([]byte("This is for context TESTING"), nil) + + plaintext, err := key2.Decrypt(ciphertext, nil) + require.Error(t, err) + assert.Empty(t, plaintext) + + plaintext, err = key1.Decrypt(ciphertext, nil) + require.NoError(t, err) + assert.EqualValues(t, "This is for context TESTING", plaintext) + }) + + context := keying.Context("TESTING PURPOSES") + plainText := []byte("Forgejo is run by [Redacted]") + var cipherText []byte + t.Run("Encrypt", func(t *testing.T) { + key := keying.DeriveKey(context) + + cipherText = key.Encrypt(plainText, []byte{0x05, 0x06}) + cipherText2 := key.Encrypt(plainText, []byte{0x05, 0x06}) + + // Ensure ciphertexts don't have an determistic output. + assert.NotEqualValues(t, cipherText, cipherText2) + }) + + t.Run("Decrypt", func(t *testing.T) { + key := keying.DeriveKey(context) + + t.Run("Succesful", func(t *testing.T) { + convertedPlainText, err := key.Decrypt(cipherText, []byte{0x05, 0x06}) + require.NoError(t, err) + assert.EqualValues(t, plainText, convertedPlainText) + }) + + t.Run("Not enougn additional data", func(t *testing.T) { + plainText, err := key.Decrypt(cipherText, []byte{0x05}) + require.Error(t, err) + assert.Empty(t, plainText) + }) + + t.Run("Too much additional data", func(t *testing.T) { + plainText, err := key.Decrypt(cipherText, []byte{0x05, 0x06, 0x07}) + require.Error(t, err) + assert.Empty(t, plainText) + }) + + t.Run("Incorrect nonce", func(t *testing.T) { + // Flip the first byte of the nonce. + cipherText[0] = ^cipherText[0] + + plainText, err := key.Decrypt(cipherText, []byte{0x05, 0x06}) + require.Error(t, err) + assert.Empty(t, plainText) + }) + + t.Run("Incorrect ciphertext", func(t *testing.T) { + assert.Panics(t, func() { + key.Decrypt(nil, nil) + }) + + assert.Panics(t, func() { + cipherText := make([]byte, chacha20poly1305.NonceSizeX) + key.Decrypt(cipherText, nil) + }) + }) + }) +} diff --git a/modules/setting/security.go b/modules/setting/security.go index 3d7b1f9ce7..678a57cb30 100644 --- a/modules/setting/security.go +++ b/modules/setting/security.go @@ -10,6 +10,7 @@ import ( "code.gitea.io/gitea/modules/auth/password/hash" "code.gitea.io/gitea/modules/generate" + "code.gitea.io/gitea/modules/keying" "code.gitea.io/gitea/modules/log" ) @@ -110,6 +111,7 @@ func loadSecurityFrom(rootCfg ConfigProvider) { // Until it supports rotating an existing secret key, we shouldn't move users off of the widely used default value SecretKey = "!#@FDEWREWR&*(" //nolint:gosec } + keying.Init([]byte(SecretKey)) CookieRememberName = sec.Key("COOKIE_REMEMBER_NAME").MustString("gitea_incredible")