theupdateframework / go-tuf

Go implementation of The Update Framework (TUF)
https://theupdateframework.com
Apache License 2.0
625 stars 105 forks source link

Should the scrypt parameters be updated? #467

Closed Zenithar closed 1 year ago

Zenithar commented 1 year ago

Hi,

Should we update the scrypt cost parameters (2^15, 8, 1) to be aligned with the requirement of being able to derive a key in approximatively 100ms (http://www.tarsnap.com/scrypt/scrypt-slides.pdf - Slide 17)?

Based in Filippo Valsorda blog post about scrypt - https://words.filippo.io/the-scrypt-parameters/ - I reused its benchmark on my current laptop (M1 10-Core).

func main() {
    for n := uint8(14); n < 22; n++ {
        b := testing.Benchmark(func(b *testing.B) {
            for i := 0; i < b.N; i++ {
                scrypt.Key([]byte("password"), []byte("salt"), 1<<n, 8, 1, 32)
            }
        })
        t := b.T / time.Duration(b.N)
        fmt.Printf("N = 2^%d\t%dms\n", n, t/time.Millisecond)
    }
}

And the results are :

N = 2^14    25ms
N = 2^15    52ms (current settings)
N = 2^16    106ms (Colin Percival, aka the creator, recommendation)
N = 2^17    213ms (OWASP recommendation)
N = 2^18    432ms
N = 2^19    889ms
N = 2^20    1740ms
N = 2^21    3633ms

In conclusion, from the benchmark, on my machine, the appropriate parameter should be 2^16. But the OWASP Password storage recommends 2^17.

I also noticed that this code is directly referenced by cosign to store the private key using this crypto system.https://github.com/sigstore/cosign/blob/main/pkg/cosign/keys.go#L154. Actually, I don't see any threat unless the ability to speedup the brute force attacks of this derivation.

What do you think?

trishankatdatadog commented 1 year ago

Thanks for filing this issue, and I don't see any reason why we shouldn't increase security! However, as we discussed offline, how would this affect backwards-compatibility?

Zenithar commented 1 year ago

Something like that could help to do progressive enhancements - https://github.com/Zenithar/go-tuf/commit/658ca7bc7cb54240b3e097bfa223d6fa561ac50d

trishankatdatadog commented 1 year ago

That makes sense to me, thanks. We can make this backwards-compatible by making the level legacy by default.

@rdimitrov Do you think this issue should go into 0.7?

Zenithar commented 1 year ago

Legacy level by default? I was more targeting Standard by default so that already encrypted envelope will be normally processed for backward compatibility, but new encryption request will use the Standard level.

WDYT?

trishankatdatadog commented 1 year ago

Legacy level by default? I was more targeting Standard by default so that already encrypted envelope will be normally processed for backward compatibility, but new encryption request will use the Standard level.

Oh, I see, I mistakenly thought Legacy is what Standard actually means. NVM, yes, you're right.