kudelskisecurity / crystals-go Goto Github PK

While working on #1 , I found the linters gosec and errcheck report several cases of omitted error handling.

Properly addressing these errors requires the extension of the public API. For example, Dilithium.KeyGen() does not return error and (currently) ignores any error return from the internal cryptographic components (rand, sha3).

I understand the library is a "direct" port of the C-reference implementation. I don't know how errors are handled there, yet it is necessary to allow Go-idiomatic error handling.

Due to the necessary API changes the linters are currently excluded. They should be enabled as part of handling this issue to properly find all cases.

Are there any specific concerns, or would a typical extension of error as return value be OK?

Which license is this repository using? If it is missing, it is probably defaulting to a non-permissive license, which is probably not what is intended.

Proposal: MIT license.

Hi, thanks for your repo. I don't know if I'm doing something wrong. When decrypting, characters are truncated and I only get 32 Bytes.

`

kyber := kyber.NewKyber1024()
pk, sk := kyber.PKEKeyGen(nil)
msg := make([]byte, 44)
msg = []byte("Very random message to sign in the benchmark")
encryptMessage := kyber.Encrypt(pk, msg, nil)
decryptedMessage := kyber.Decrypt(sk, encryptMessage)
log.Println("HELLO")
//log.Println(encryptMessage)
log.Println(len(msg)) // 44 Bytes
log.Println(len(decryptedMessage)) // 32 Bytes

`

So 'SEEDBYTES' only represents 32. Therefore, is not possible to use longer messages?

func (d *Dilithium) KeyGen(seed []byte) ([]byte, []byte) {

	if seed == nil || len(seed) != SEEDBYTES {
		seed = make([]byte, SEEDBYTES)
		rand.Read(seed)
	}

	var tr, rho, key [SEEDBYTES]byte
	var rhoprime [2 * SEEDBYTES]byte

	state := sha3.NewShake256()
	state.Write(seed)
	state.Read(rho[:])
	state.Read(rhoprime[:])
	state.Read(key[:])
	state.Reset()

	K := d.params.K
	L := d.params.L
	ETA := d.params.ETA

	Ahat := expandSeed(rho, K, L)

	s1 := make(Vec, L)
	for i := 0; i < L; i++ {
		s1[i] = polyUniformEta(rhoprime, uint16(i), ETA)
	}
	s2 := make(Vec, K)
	for i := 0; i < K; i++ {
		s2[i] = polyUniformEta(rhoprime, uint16(i+L), ETA)
	}
	s1hat := s1.copy()
	s1hat.ntt(L)
	s2hat := s2.copy()
	s2hat.ntt(K)

	t, t1, t0 := make(Vec, K), make(Vec, K), make(Vec, K)
	for i := 0; i < K; i++ {
		t[i] = vecAccPointWise(Ahat[i], s1hat, L)
		s2hat[i].tomont()
		t[i] = add(t[i], s2hat[i])
		t[i].invntt()
		t[i].addQ()
		t1[i], t0[i] = polyPower2Round(t[i])
	}
	state.Write(append(rho[:], packT1(t1, K)...))
	state.Read(tr[:])

	return d.PackPK(PublicKey{T1: t1, Rho: rho}), d.PackSK(PrivateKey{Rho: rho, Key: key, Tr: tr, S1: s1, S2: s2, T0: t0})
}

I think that the seed should be returned alongside with key pair or the seed should be demanded

func (d *Dilithium) KeyGen(seed [SEEDBYTES]byte) ([]byte, []byte) {

if seed == nil || len(seed) != SEEDBYTES {
seed = make([]byte, SEEDBYTES)
rand.Read(seed)

This bit of code is used in compressing a polynomial ring element into a (secret) message:

To do so, it performs a division by Q that might not necessarily compile to a multiplication instruction: looking at the output of some C compilers using https://godbolt.org/z/sKn3TKKGq and https://godbolt.org/z/8GqKoTfYh for example, a division instruction is emitted even when -O3 is specified. Should a division instruction be emitted, its execution time would likely be variable and leak information about its secret input.

We reported a similar issue in the CRYSTALS-Kyber reference implementation; you may want to use their fix: pq-crystals/kyber@dda29cc

The workflow tests contained in tests.yml are failing. @dertseha can you update the file so that it does not cause an error?

Hey,
thank you very much for the package.
I make something wrong. For example, an encryption and decryption with Kyber is not working for me:

	k := kyber.NewKyber1024()
	pk, sk := k.KeyGen(nil)
	encrypt := k.Encrypt(pk, []byte("Das ist eine Testnachricht zum Verschlüsseln."), nil)
	decrypt := k.Decrypt(sk, encrypt)

	fmt.Println("Message")
	fmt.Println(len(encrypt)) // => 1568
	fmt.Println(len(decrypt)) // => 0

Also verification of a message with dilithium will result in false.

	alice := dilithium.NewDilithium2() //Creates a Dilithium instance with recommended security level
	pk, sk := alice.KeyGen([]byte("123456789123456789123456789"))
	msg := []byte("This is a message.")
	sig := alice.Sign(sk, msg)

	bob := dilithium.NewDilithium2()
	verified := bob.Verify(pk, sig, msg) //verified is true for honest executions
	fmt.Println("verified? ")
	fmt.Println(verified) // => false

The file params.go contains some exported constants; Some are in UPPERCASE (should be CamelCase), others are unused within the library.

Are any of these constants required or useful to be exported?
If they are to be exported, they should be documented (see #4 ) - and possibly renamed.

Several exported functions and types (and the packages themselves) are missing documentation.
In order to properly use the library, documentation is necessary.

I found this while working on #1 , and as such the linter revive is disabled, and stylecheck has message ST1000 disabled.

R-use of the information in README.md might be a good start. Is this enough?

While working on #1 , I found the linter gosec report the use of a weak number generator (math/rand instead of crypto/rand).
Several uses are in test, yet this also is in the production code, for example in Dilithium.KeyGen().

Note that because of #2 , the linter gosec is not enabled at all. As part of this issue here, consider the state of the other issue and ensure the linter is enabled.

I do not know the details, only that the simple number generator should not be used.
Is there a specific reason that the weaker generator is in use, or can the better one be used instead?

Implementing cyrpto.Signer helps to use it for x509 Certificates.

https://cs.opensource.google/go/go/+/refs/tags/go1.20.4:src/crypto/x509/x509.go;drc=f375b305c801515b587719490b4be0db1a66e20c;l=1535

Also, the public key should implement crypto.Public key interface (https://pkg.go.dev/crypto#PublicKey)
https://cs.opensource.google/go/go/+/refs/tags/go1.20.4:src/crypto/x509/x509.go;drc=f375b305c801515b587719490b4be0db1a66e20c;l=1597

I'd like to contribute to that and just logging this here.