Implement sha and rand

[qmuntal]

This PR has the following changes (review commit by commit):

• ae520e3: Implements a tool to generate syscall wrappers without having golang.org/x/sys as runtime dependency, which will facilitate vendoring this package into our Go fork. Read inline documentation for more context.
• b520613: Implement all SHA algorithms except SHA224, which is not supported by BCrypt.
• ab022d0: Implement random number generator.
• ca9c71c: Add GH Actions test workflow

[jaredpar]

The USE_SYSTEM_PREFERRED_RNG flag isn't supported on Vista so that puts a lower bound on where our impl would be supported. That seems fine but should note that in our docs.

[qmuntal]

We are getting old. One could think that Vista is still a mainstream version, but it's no longer supported by MSFT since 2017 and the last Go version that supported it was Go 1.10 (see Go minimum requirements).

[qmuntal]

@jaredpar you might want to review c18affd, which I added after your last review.

It reduces boilerplate and makes the code safer by delegating to the autogenerated code the []byte -> *byte+length arguments transform.

[qmuntal]

I've filled an upstream proposal so we can delegate UTF16 string encoding to the autogenerated code: golang/go#51786

[jaredpar]

Aren't we missing the cbOutput uint32 parameter here?

Suggested change

	//sys GetProperty(hObject HANDLE, pszProperty *uint16, pbOutput []byte, pcbResult *uint32, dwFlags uint32) (s error) = bcrypt.BCryptGetProperty
	//sys GetProperty(hObject HANDLE, pszProperty *uint16, pbOutput []byte, cbOutput uint32, pcbResult *uint32, dwFlags uint32) (s error) = bcrypt.BCryptGetProperty

https://docs.microsoft.com/en-us/windows/win32/api/bcrypt/nf-bcrypt-bcryptgetproperty

Noticed this when looking at the getUint32 function and didn't see an input size

[qmuntal]

sys definitions do not need to exactly match Win32 API's. When mkwinsyscall (the tool that autogenerates the wrappers) sees a slice, in this case pbOutput []byte, it transforms it into two parameters: the first element pointer followed by the length of the slice. This is how GetProperty is expanded:

go-crypto-winnative/internal/bcrypt/zsyscall_windows.go

Lines 121 to 131 in b4c072f

	func GetProperty(hObject HANDLE, pszProperty *uint16, pbOutput []byte, pcbResult *uint32, dwFlags uint32) (s error) {
	var _p0 *byte
	if len(pbOutput) > 0 {
	_p0 = &pbOutput[0]
	}
	r0, _, _ := syscall.Syscall6(procBCryptGetProperty.Addr(), 6, uintptr(hObject), uintptr(unsafe.Pointer(pszProperty)), uintptr(unsafe.Pointer(_p0)), uintptr(len(pbOutput)), uintptr(unsafe.Pointer(pcbResult)), uintptr(dwFlags))
	if r0 != 0 {
	s = syscall.Errno(r0)
	}
	return
	}

This not only reduces boilerplate code but is also safer, as the slice is only deferred when it is not empty. The alternative is the define the //sys signature directly the pointer to the first element and the length, which leaves to the caller casting the slice to a pointer, increasing the likelihood of a runtime panic.

[jaredpar]

How do you define a //sys signature where you can use the length pointer parameter? Do you just use a *byte* instead of []byte` in that case? There are some windows APIs where the length pointer parameter is used for both input and output (IIRC).

[qmuntal]

In that case you can't rely on mksyscall doing the []byte -> *byte+len transform, but you can still explicitly define the pointer and length parameters as follows:

GetProperty(hObject HANDLE, pszProperty *uint16, pbOutput *byte, cbOutput *uint32, pcbResult *uint32, dwFlags uint32) (s error) = bcrypt.BCryptGetProperty

The call it in this way:

func getBuf(h bcrypt.HANDLE, name string, buf []byte) (uint32, error) {
	var discard uint32
        var _p0 *byte
        if len(buf ) > 0 {
                // We can't pass &buf[0] directly to GetProperty, as it would panic when len(0) == 0!
		_p0 = &buf[0]
	}
        n := len(buf) // n will wold the input and output length!
	err := bcrypt.GetProperty(h, utf16PtrFromString(name), _p0 , &n, &discard, 0)
	return prop, err
}

[dagood]

A few comments on cmd/mksyscall/main.go itself before getting to the rest.

[dagood]

This looks like it made the function break for values >= 1<<32 when you try to use it like a reader--here's a test I wrote to try repeated reads like I think you'd normally do with a reader (based on https://pkg.go.dev/io#Reader) (with iter just there to prevent infinite loop):

func TestRandReadAllBig(t *testing.T) {
	b := make([]byte, 1<<33+60)
	bb := b
	var iter int
	for iter = 0; len(bb) > 0 && iter <= 9000; iter++ {
		n, err := RandReader.Read(bb)
		if n > 0 {
			bb = bb[n:]
		}
		if err != nil {
			if err == io.EOF {
				break
			}
			t.Fatal(err)
		}
		t.Logf("Iter %v, got %v\n", iter, n)
	}
	t.Logf("Done. %v\n", iter)
}

=== RUN   TestRandReadAllBig
    rand_test.go:40: Iter 0, got 60
    rand_test.go:40: Iter 1, got 0
    rand_test.go:40: Iter 2, got 0
    rand_test.go:40: Iter 3, got 0
...

(io.ReadFull runs into the problem too--but it simply hangs, because it doesn't have an iter limit.)

Should this func actually do something more like this?

// BCryptGenRandom only accepts 2**32-1 bytes at a time, so limit it.
inputLen := len(b)
const maxGenRandomLen = 2<<32 - 1
if inputLen > maxGenRandomLen {
	inputLen = maxGenRandomLen
}
if inputLen == 0 {
	return 0, io.EOF
}

[qmuntal]

I'm aware that it is breaking for values => 1<<32, but I wanted to keep consistency with Go crypto/rand's implementation: https://github.com/golang/go/blob/7eaad60737bc507596c56cec4951b089596ccc9e/src/crypto/rand/rand_windows.go#L19.

What I hadn't seen is that io.ReadFull hangs. I can reproduce it even in Go crypto/rand... I think this deserved at least a CVE and and best a security bounty if we find a service that is passing in buffers whose length is provided by the user.

@dagood @jaredpar thoughts? Do you see the CVE as I do? Is there a MSFT standard procedure for reporting? Should we go for the bounty?

[qmuntal]

I think I fixed it in 7b9094c and 310966f.

[dagood]

A few somewhat related issues (didn't see anything closer):

• crypto/rand: panic in rand.Read() when called from wasm in a browser golang/go#46256
  • Panic when size is too large--the limit is much smaller there.
• crypto/rand: returns wrong number of bytes! golang/go#8362
  • Clarifies that rand.Read has the guarantees, not rand.Reader.Read.

[dagood]

Hmm, -1 yields 0xffffffff, rather than what I'd expect (0). Should the func handle negative numbers, or maybe just panic because that isn't something we expect to ever happen?

[dagood]

I think this solves it for the 1<<32+60 case, but not e.g. 1<<34. In that case the first call fills up the first ~1<<32 positions in the slice, this second call fills up the next ~1<<32 positions, but the remaining positions aren't filled.

func TestRandBigger(t *testing.T) {
	b := make([]byte, 1<<34-1)
	c, err := io.ReadFull(RandReader, b)
	if err != nil {
		return
	}
	t.Logf("Claimed read %x of %x bytes\n", c, len(b))
	var i int
	for i = len(b) - 1; i > 0; i-- {
		if b[i] != 0 {
			break
		}
	}
	t.Logf("Last non0  = %x\n", i)
	t.Logf("len(b)     = %x\n", len(b))
	t.Logf("Got/wanted = %v\n", float64(i)/float64(len(b)))
}

Without seeded rand, I guess we can't technically determine if this means the func isn't working properly 😄, but when I run this locally, I get:

    rand_test.go:42: Claimed read 3ffffffff of 3ffffffff bytes
    rand_test.go:49: Last non0  = fffffffe
    rand_test.go:50: len(b)     = 3ffffffff
    rand_test.go:51: Got/wanted = 0.2499999998981366

[qmuntal]

I'm overcomplicating the randReader.Read implementation. rand.Reader.Read is no guaranteed to return the length if the incoming slice, this guarantee only applies to rand.Read, as ianlancetaylor explains in this comment. Therefore there is no need to call bcrypt.GenRandom more times if the input buffer lengths is clamped, rand.Read will do that for us.

[dagood]

This also doesn't seem to work for values that need more than two iterations:

func TestSHAHugeWriteCollide(t *testing.T) {
	a := make([]byte, 1<<33+32)
	a[len(a)-5] = 5
	t.Logf("%v\n", prettySHA256Sum(t, a)) // 8e706dbcf4ec73f4f06c177146145b502d1a0f7dc42ad967469ec0727fcdcd9c

	b := make([]byte, 1<<33+32)
	b[len(a)-5] = 6
	t.Logf("%v\n", prettySHA256Sum(t, b)) // 8e706dbcf4ec73f4f06c177146145b502d1a0f7dc42ad967469ec0727fcdcd9c
}

[qmuntal]

New attempt: fe562dd

[dagood]

Would it make sense to move this check into the loop just after err is set so the loop condition can be simplified down to for n < len(p) {?

[qmuntal]

IMO having the error check in the loop condition makes the loop content cleaner and easier to read, but it could be just me 😛.

It is also kind of idiomatic.

[dagood]

Interesting, for me, having the error handling anywhere but immediately after where the error is generated makes me immediately want to confirm where the error's going, so I end up taking a much closer look. Here, flow travels upwards to the loop condition and then down afterwards, so I find myself skipping around. Vs. what I think of as normal (when there's no loop involved):

for n < len(p) {
	nn := clamp32(len(p[n:]))
	err = bcrypt.HashData(h.ctx, p[n:n+nn], 0)
	if err != nil {
		// hash.Hash interface mandates Write should never return an error.
		panic(err)
	}
	n += nn
}
runtime.KeepAlive(h)
return len(p), nil

But, as an idiom, I guess I'll get used to it. Maybe this has to do with review, focusing on "how can this fail" rather than "what does this do when successful".

[jaredpar]

Seems all uses of clam32 are on []byte length. Feels like a place where generics could be useful in the future because would allow for a flexible len method.

func len32[T any](slice []T) int32 {
	v := len(slice)
	if v > math.MaxInt32 {
		return math.MaxInt32
	}
	return int32(v)
}

[qmuntal]

It would be cool, but then we won't be able to use this backend in pre-go1.18 toolchains. Anyway, I like the idea of accepting an slice and calling in len32 + using math.Uint32, I'll take that bits!