microsoft / go-crypto-winnative

Go crypto backend for Windows using CNG
MIT License
28 stars 3 forks source link

Implement shaXHash.Clone() #21

Closed qmuntal closed 2 years ago

qmuntal commented 2 years ago

⚠️ This PR supersedes #20

This PR adds a Clone method to all supported SHAs. It is necessary because some standard library Go crypto logic relies on the hash to be cloneable, more specifically the TLS 1.3 server handshake: https://github.com/golang/go/blob/de8101d21bcf5f1097bcfaf3a1b55820ba70dce9/src/crypto/tls/handshake_server_tls13.go#L329.

The problem is that Go TLS 1.3 clones the hash via the binary marshal interface, and CNG does not support it. The Clone() method is a workaround for supporting TLS 1.3 without providing the binary marshalling interface.

It will be used like this:

func cloneHash(in hash.Hash, h crypto.Hash) hash.Hash {
  // Recreate the interface to avoid importing encoding.
  type binaryMarshaler interface {
    MarshalBinary() (data []byte, err error)
    UnmarshalBinary(data []byte) error
  }
  marshaler, ok := in.(binaryMarshaler)
  if !ok {
    // CNGCrypto hashes do not implement the binaryMarshaler interface,
    // but do implement the Clone method.
    if cloner, ok := in.(interface{ Clone() (hash.Hash, error) }); ok {
       if out, err := cloner.Clone(); err != nil {
         return out
       }
    }
    return nil
  }
  ...
}
jaredpar commented 2 years ago

I'm not connecting the dots on how the new Clone method will end up being called from cloneHash. Are we going to use a patch file in the main go repo to update cloneHash?

qmuntal commented 2 years ago

I'm not connecting the dots on how the new Clone method will end up being called from cloneHash. Are we going to use a patch file in the main go repo to update cloneHash?

Yes, cloneHash will have to be patched to fallback to Clone if the hash does not implement the binary marshaller interface.

jaredpar commented 2 years ago

Would we be patching the cloneHash to essentially only change behavior when goexperiment=wincryptnative?

qmuntal commented 2 years ago

Would we be patching the cloneHash to essentially only change behavior when goexperiment=wincryptnative?

Yep. It is unfortunate but CNG does not plan to support this functionality in the near future. Quoting @NielsFerguson:

CNG goes not support exporting of partially computed hash states. ...

The requirement to actually save intermediate hash states is very rare; I’ve seen only one application do it, and that was a bad design for many reasons. It is almost always easy to avoid that requirement during the design phase...

The case of TLS 1.3 cloneHash does not fall into the category of bad design because it is a private function working with private hash types under the Go crypto team control, and we are the intruders trying to fit CNG into it. BUT, cloneHash is conceptually stretching the hash API to its limits, as it is serializing and deserializing a hash to fake an in-memory copy.