Panic if DES encryption or decryption fails (#169)

* panic if DES encryption or decryption fails

* fix 3DES CBC mode support detection

* test that DES conforms to the cipher.Block interface

* move TestBlock to internal/cryptotest

Author: qmuntal

aes_test.go

@@ -3,35 +3,21 @@ package openssl_test
 import (
 	"bytes"
 	"crypto/cipher"
+	"fmt"
 	"math"
 	"testing"
 
 	"github.com/golang-fips/openssl/v2"
+	"github.com/golang-fips/openssl/v2/internal/cryptotest"
 )
 
-func TestAESShortBlocks(t *testing.T) {
-	bytes := func(n int) []byte { return make([]byte, n) }
-
-	c, _ := openssl.NewAESCipher(bytes(16))
-
-	mustPanic(t, "crypto/aes: input not full block", func() { c.Encrypt(bytes(1), bytes(1)) })
-	mustPanic(t, "crypto/aes: input not full block", func() { c.Decrypt(bytes(1), bytes(1)) })
-	mustPanic(t, "crypto/aes: input not full block", func() { c.Encrypt(bytes(100), bytes(1)) })
-	mustPanic(t, "crypto/aes: input not full block", func() { c.Decrypt(bytes(100), bytes(1)) })
-	mustPanic(t, "crypto/aes: output not full block", func() { c.Encrypt(bytes(1), bytes(100)) })
-	mustPanic(t, "crypto/aes: output not full block", func() { c.Decrypt(bytes(1), bytes(100)) })
-}
-
-func mustPanic(t *testing.T, msg string, f func()) {
-	defer func() {
-		err := recover()
-		if err == nil {
-			t.Errorf("function did not panic, wanted %q", msg)
-		} else if err != msg {
-			t.Errorf("got panic %v, wanted %q", err, msg)
-		}
-	}()
-	f()
+// Test AES against the general cipher.Block interface tester.
+func TestAESBlock(t *testing.T) {
+	for _, keylen := range []int{128, 192, 256} {
+		t.Run(fmt.Sprintf("AES-%d", keylen), func(t *testing.T) {
+			cryptotest.TestBlock(t, keylen/8, openssl.NewAESCipher)
+		})
+	}
 }
 
 func TestNewGCMNonce(t *testing.T) {

des.go

@@ -33,27 +33,22 @@ func NewDESCipher(key []byte) (cipher.Block, error) {
 	if len(key) != 8 {
 		return nil, errors.New("crypto/des: invalid key size")
 	}
-	c, err := newEVPCipher(key, cipherDES)
-	if err != nil {
-		return nil, err
-	}
-	// Should always be true for stock OpenSSL.
-	if loadCipher(cipherDES, cipherModeCBC) == nil {
-		return &desCipherWithoutCBC{c}, nil
-	}
-	return &desCipher{c}, nil
+	return newDESCipher(key, cipherDES)
 }
 
 func NewTripleDESCipher(key []byte) (cipher.Block, error) {
 	if len(key) != 24 {
 		return nil, errors.New("crypto/des: invalid key size")
 	}
-	c, err := newEVPCipher(key, cipherDES3)
+	return newDESCipher(key, cipherDES3)
+}
+
+func newDESCipher(key []byte, kind cipherKind) (cipher.Block, error) {
+	c, err := newEVPCipher(key, kind)
 	if err != nil {
 		return nil, err
 	}
-	// Should always be true for stock OpenSSL.
-	if loadCipher(cipherDES, cipherModeCBC) != nil {
+	if loadCipher(kind, cipherModeCBC) == nil {
 		return &desCipherWithoutCBC{c}, nil
 	}
 	return &desCipher{c}, nil
@@ -105,9 +100,15 @@ func (c *desCipherWithoutCBC) BlockSize() int {
 }
 
 func (c *desCipherWithoutCBC) Encrypt(dst, src []byte) {
-	c.encrypt(dst, src)
+	if err := c.encrypt(dst, src); err != nil {
+		// crypto/des expects that the panic message starts with "crypto/des: ".
+		panic("crypto/des: " + err.Error())
+	}
 }
 
 func (c *desCipherWithoutCBC) Decrypt(dst, src []byte) {
-	c.decrypt(dst, src)
+	if err := c.decrypt(dst, src); err != nil {
+		// crypto/des expects that the panic message starts with "crypto/des: ".
+		panic("crypto/des: " + err.Error())
+	}
 }

des_test.go

@@ -5,6 +5,7 @@ import (
 	"crypto/cipher"
 	"testing"
 
+	"github.com/golang-fips/openssl/v2/internal/cryptotest"
 	"github.com/golang-fips/openssl/v2"
 )
 
@@ -1665,6 +1666,23 @@ func TestDESCBCDecryptSimple(t *testing.T) {
 	}
 }
 
+// Test DES against the general cipher.Block interface tester
+func TestDESBlock(t *testing.T) {
+	t.Run("DES", func(t *testing.T) {
+		if !openssl.SupportsDESCipher() {
+			t.Skip("DES is not supported")
+		}
+		cryptotest.TestBlock(t, 8, openssl.NewDESCipher)
+	})
+
+	t.Run("TripleDES", func(t *testing.T) {
+		if !openssl.SupportsTripleDESCipher() {
+			t.Skip("3DES is not supported")
+		}
+		cryptotest.TestBlock(t, 24, openssl.NewTripleDESCipher)
+	})
+}
+
 func BenchmarkEncrypt(b *testing.B) {
 	if !openssl.SupportsDESCipher() {
 		b.Skip("DES is not supported")

internal/cryptotest/README.md

@@ -0,0 +1,6 @@
+The `internal/cryptotest` package provides a set of tests for cryptographic primitives.
+
+`internal/cryptotest` has been copied from the Go standard library's [crypo/internal/cryptotest](https://github.com/golang/go/tree/807e01db4840e25e4d98911b28a8fa54244b8dfa/src/crypto/internal/cryptotest)
+package and trimmed down to only include the tests that are relevant for the `openssl` package.
+
+[1]: https://github.com/golang/go/tree/807e01db4840e25e4d98911b28a8fa54244b8dfa/src/crypto/internal/cryptotest

internal/cryptotest/block.go

@@ -0,0 +1,256 @@
+// Copyright 2024 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package cryptotest
+
+import (
+	"bytes"
+	"crypto/cipher"
+	"testing"
+)
+
+// This file is a copy of https://github.com/golang/go/blob/9e9b1f57c26a6d13fdaebef67136718b8042cdba/src/crypto/internal/cryptotest/block.go.
+
+type MakeBlock func(key []byte) (cipher.Block, error)
+
+// TestBlock performs a set of tests on cipher.Block implementations, checking
+// the documented requirements of BlockSize, Encrypt, and Decrypt.
+func TestBlock(t *testing.T, keySize int, mb MakeBlock) {
+	// Generate random key
+	key := make([]byte, keySize)
+	newRandReader(t).Read(key)
+	t.Logf("Cipher key: 0x%x", key)
+
+	block, err := mb(key)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	blockSize := block.BlockSize()
+
+	t.Run("Encryption", func(t *testing.T) {
+		testCipher(t, block.Encrypt, blockSize)
+	})
+
+	t.Run("Decryption", func(t *testing.T) {
+		testCipher(t, block.Decrypt, blockSize)
+	})
+
+	// Checks baseline Encrypt/Decrypt functionality.  More thorough
+	// implementation-specific characterization/golden tests should be done
+	// for each block cipher implementation.
+	t.Run("Roundtrip", func(t *testing.T) {
+		rng := newRandReader(t)
+
+		// Check Decrypt inverts Encrypt
+		before, ciphertext, after := make([]byte, blockSize), make([]byte, blockSize), make([]byte, blockSize)
+
+		rng.Read(before)
+
+		block.Encrypt(ciphertext, before)
+		block.Decrypt(after, ciphertext)
+
+		if !bytes.Equal(after, before) {
+			t.Errorf("plaintext is different after an encrypt/decrypt cycle; got %x, want %x", after, before)
+		}
+
+		// Check Encrypt inverts Decrypt (assumes block ciphers are deterministic)
+		before, plaintext, after := make([]byte, blockSize), make([]byte, blockSize), make([]byte, blockSize)
+
+		rng.Read(before)
+
+		block.Decrypt(plaintext, before)
+		block.Encrypt(after, plaintext)
+
+		if !bytes.Equal(after, before) {
+			t.Errorf("ciphertext is different after a decrypt/encrypt cycle; got %x, want %x", after, before)
+		}
+	})
+
+}
+
+func testCipher(t *testing.T, cipher func(dst, src []byte), blockSize int) {
+	t.Run("AlterInput", func(t *testing.T) {
+		rng := newRandReader(t)
+
+		// Make long src that shouldn't be modified at all, within block
+		// size scope or beyond it
+		src, before := make([]byte, blockSize*2), make([]byte, blockSize*2)
+		rng.Read(src)
+		copy(before, src)
+
+		dst := make([]byte, blockSize)
+
+		cipher(dst, src)
+		if !bytes.Equal(src, before) {
+			t.Errorf("block cipher modified src; got %x, want %x", src, before)
+		}
+	})
+
+	t.Run("Aliasing", func(t *testing.T) {
+		rng := newRandReader(t)
+
+		buff, expectedOutput := make([]byte, blockSize), make([]byte, blockSize)
+
+		// Record what output is when src and dst are different
+		rng.Read(buff)
+		cipher(expectedOutput, buff)
+
+		// Check that the same output is generated when src=dst alias to the same
+		// memory
+		cipher(buff, buff)
+		if !bytes.Equal(buff, expectedOutput) {
+			t.Errorf("block cipher produced different output when dst = src; got %x, want %x", buff, expectedOutput)
+		}
+	})
+
+	t.Run("OutOfBoundsWrite", func(t *testing.T) {
+		rng := newRandReader(t)
+
+		src := make([]byte, blockSize)
+		rng.Read(src)
+
+		// Make a buffer with dst in the middle and data on either end
+		buff := make([]byte, blockSize*3)
+		endOfPrefix, startOfSuffix := blockSize, blockSize*2
+		rng.Read(buff[:endOfPrefix])
+		rng.Read(buff[startOfSuffix:])
+		dst := buff[endOfPrefix:startOfSuffix]
+
+		// Record the prefix and suffix data to make sure they aren't written to
+		initPrefix, initSuffix := make([]byte, blockSize), make([]byte, blockSize)
+		copy(initPrefix, buff[:endOfPrefix])
+		copy(initSuffix, buff[startOfSuffix:])
+
+		// Write to dst (the middle of the buffer) and make sure it doesn't write
+		// beyond the dst slice
+		cipher(dst, src)
+		if !bytes.Equal(buff[startOfSuffix:], initSuffix) {
+			t.Errorf("block cipher did out of bounds write after end of dst slice; got %x, want %x", buff[startOfSuffix:], initSuffix)
+		}
+		if !bytes.Equal(buff[:endOfPrefix], initPrefix) {
+			t.Errorf("block cipher did out of bounds write before beginning of dst slice; got %x, want %x", buff[:endOfPrefix], initPrefix)
+		}
+
+		// Check that dst isn't written to beyond BlockSize even if there is room
+		// in the slice
+		dst = buff[endOfPrefix:] // Extend dst to include suffix
+		cipher(dst, src)
+		if !bytes.Equal(buff[startOfSuffix:], initSuffix) {
+			t.Errorf("block cipher modified dst past BlockSize bytes; got %x, want %x", buff[startOfSuffix:], initSuffix)
+		}
+	})
+
+	// Check that output of cipher isn't affected by adjacent data beyond input
+	// slice scope
+	// For encryption, this assumes block ciphers encrypt deterministically
+	t.Run("OutOfBoundsRead", func(t *testing.T) {
+		rng := newRandReader(t)
+
+		src := make([]byte, blockSize)
+		rng.Read(src)
+		expectedDst := make([]byte, blockSize)
+		cipher(expectedDst, src)
+
+		// Make a buffer with src in the middle and data on either end
+		buff := make([]byte, blockSize*3)
+		endOfPrefix, startOfSuffix := blockSize, blockSize*2
+
+		copy(buff[endOfPrefix:startOfSuffix], src)
+		rng.Read(buff[:endOfPrefix])
+		rng.Read(buff[startOfSuffix:])
+
+		testDst := make([]byte, blockSize)
+		cipher(testDst, buff[endOfPrefix:startOfSuffix])
+		if !bytes.Equal(testDst, expectedDst) {
+			t.Errorf("block cipher affected by data outside of src slice bounds; got %x, want %x", testDst, expectedDst)
+		}
+
+		// Check that src isn't read from beyond BlockSize even if the slice is
+		// longer and contains data in the suffix
+		cipher(testDst, buff[endOfPrefix:]) // Input long src
+		if !bytes.Equal(testDst, expectedDst) {
+			t.Errorf("block cipher affected by src data beyond BlockSize bytes; got %x, want %x", buff[startOfSuffix:], expectedDst)
+		}
+	})
+
+	t.Run("NonZeroDst", func(t *testing.T) {
+		rng := newRandReader(t)
+
+		// Record what the cipher writes into a destination of zeroes
+		src := make([]byte, blockSize)
+		rng.Read(src)
+		expectedDst := make([]byte, blockSize)
+
+		cipher(expectedDst, src)
+
+		// Make nonzero dst
+		dst := make([]byte, blockSize*2)
+		rng.Read(dst)
+
+		// Remember the random suffix which shouldn't be written to
+		expectedDst = append(expectedDst, dst[blockSize:]...)
+
+		cipher(dst, src)
+		if !bytes.Equal(dst, expectedDst) {
+			t.Errorf("block cipher behavior differs when given non-zero dst; got %x, want %x", dst, expectedDst)
+		}
+	})
+
+	t.Run("BufferOverlap", func(t *testing.T) {
+		rng := newRandReader(t)
+
+		buff := make([]byte, blockSize*2)
+		rng.Read((buff))
+
+		// Make src and dst slices point to same array with inexact overlap
+		src := buff[:blockSize]
+		dst := buff[1 : blockSize+1]
+		mustPanic(t, "invalid buffer overlap", func() { cipher(dst, src) })
+
+		// Only overlap on one byte
+		src = buff[:blockSize]
+		dst = buff[blockSize-1 : 2*blockSize-1]
+		mustPanic(t, "invalid buffer overlap", func() { cipher(dst, src) })
+
+		// src comes after dst with one byte overlap
+		src = buff[blockSize-1 : 2*blockSize-1]
+		dst = buff[:blockSize]
+		mustPanic(t, "invalid buffer overlap", func() { cipher(dst, src) })
+	})
+
+	// Test short input/output.
+	// Assembly used to not notice.
+	// See issue 7928.
+	t.Run("ShortBlock", func(t *testing.T) {
+		// Returns slice of n bytes of an n+1 length array.  Lets us test that a
+		// slice is still considered too short even if the underlying array it
+		// points to is large enough
+		byteSlice := func(n int) []byte { return make([]byte, n+1)[0:n] }
+
+		// Off by one byte
+		mustPanic(t, "input not full block", func() { cipher(byteSlice(blockSize), byteSlice(blockSize-1)) })
+		mustPanic(t, "output not full block", func() { cipher(byteSlice(blockSize-1), byteSlice(blockSize)) })
+
+		// Small slices
+		mustPanic(t, "input not full block", func() { cipher(byteSlice(1), byteSlice(1)) })
+		mustPanic(t, "input not full block", func() { cipher(byteSlice(100), byteSlice(1)) })
+		mustPanic(t, "output not full block", func() { cipher(byteSlice(1), byteSlice(100)) })
+	})
+}
+
+func mustPanic(t *testing.T, msg string, f func()) {
+	t.Helper()
+
+	defer func() {
+		t.Helper()
+
+		err := recover()
+
+		if err == nil {
+			t.Errorf("function did not panic for %q", msg)
+		}
+	}()
+	f()
+}