@gopherbot, please consider backport to 1.13.2

Backport issue(s) opened: #34928 (for 1.13).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

Change https://golang.org/cl/201317 mentions this issue: crypto/ecdsa: fix buffer size on s390x for ecdsa

Change https://golang.org/cl/201337 mentions this issue: [release-branch.go1.13] crypto/ecdsa: fix buffer size on s390x for ecdsa

Perhaps I merged the fix a bit too quickly, as I didn't notice the issue here at very little detail or discussion.

Got a repro?

Can this be a security issue? Remote DoS with certain client certs?

/cc @FiloSottile

Yeah, reopening.

• What are the reproduction steps? We'll definitely need a new CL with a test.
• In what situations are the buffers overrun?
• What's the impact? Does the assembly ever write to memory beyond the bounds? Is the computation ever wrong but without a panic?
• Why should we backport this? When was it introduced? How was it not noticed?
• Why did tests not catch this?
• How did you pick the new buffer sizes? Sounds like it should be a constant with a comment.
• Do you think it has security implications?

If we don't get adequate test coverage & confidence of certain assembly, the fallback of course is to just revert that assembly to use the pure Go implementation. That's an option here if it comes to it.

• What are the reproduction steps? We'll definitely need a new CL with a test.

Running the crypto/x509 tests on a z15 is enough to trigger a panic. This assembly is making use of a new instruction and so will not be executed on older machines such as the buildbot (which is a z13).

• In what situations are the buffers overrun?

Potentially every time the KDSA instruction is executed, which will be the case when signing or verifying using P-256, P-384 or P-521.

• What's the impact? Does the assembly ever write to memory beyond the bounds? Is the computation ever wrong but without a panic?

Yes, my understanding is that the assembly is reading and writing to memory out of bounds because the buffer is smaller than the instruction assumes it to be. If a different thread writes to this memory area then we could see the result of the KDSA call change. Overwriting the stack could modify variables in other functions in the call chain. We must therefore assume that it is possible that this could affect the result of a sign or verify call.

• Why should we backport this? When was it introduced? How was it not noticed?

It was introduced in CL 174437 prior to the Go 1.13 release. Go 1.13 and Go 1.13.1 are affected.

The instruction was added based on a draft of the specification for the KDSA instruction which used a smaller size for the params buffer. We did not notice that this change had occurred.

• Why did tests not catch this?

They do, but only when run on a z15. Unfortunately we were not regularly testing on z15 hardware until now. The buildbot is a z13.

In general I don't think we have a good story around testing whether assembly in Go is reading or writing memory out of bounds. Unfortunately if the buffer resides on the heap (or even on the stack) I don't think it will necessarily trigger any sort of panic or error.

• How did you pick the new buffer sizes? Sounds like it should be a constant with a comment.

These are taken directly from the final specification for the instruction. Yes, it should probably be a named type or constant.

• Do you think it has security implications?

Yes, I think it does. The hardware (z15) that is a pre-requisite for exposing this bug in Go is very new and has not been widely deployed yet which limits the likelihood that this is affecting a production application somewhat, but it is still a very nasty bug.

If we don't get adequate test coverage & confidence of certain assembly, the fallback of course is to just revert that assembly to use the pure Go implementation. That's an option here if it comes to it.

I think that this is the best idea for Go 1.13.2. Apologies for all of this. In future we will avoid adding assembly for unreleased hardware.

Ok, thanks for elaborating. We are definitely reverting CL 174437 for Go 1.13. Can you make the CL?

The next version of the assembly policy (which is long overdue, and that's my fault) will explicitly require builders coverage for all assembly paths. We should not have merged that.

We have a security release tomorrow but it's very late to add another change to it. Does this ever work on affected machines? If not, it sounds like no one will be blindsided in production, and we can put the fix in a regular minor release.

Change https://golang.org/cl/201360 mentions this issue: crypto/ecdsa: remove s390x assembly

Ok, thanks for elaborating. We are definitely reverting CL 174437 for Go 1.13. Can you make the CL?

Done, see CL 201360 for the removal in master and CL 201361 for the equivalent backport to Go 1.13.

The next version of the assembly policy (which is long overdue, and that's my fault) will explicitly require builders coverage for all assembly paths. We should not have merged that.

Understood.

We have a security release tomorrow but it's very late to add another change to it. Does this ever work on affected machines? If not, it sounds like no one will be blindsided in production, and we can put the fix in a regular minor release.

I am not 100% certain. The crypto/ecdsa tests pass on affected machines which bothers me and indicates the bug might not completely break applications. I'd like to get the revert into a Go 1.13.x release ASAP however I am not currently aware of any users that might be affected by this issue and so it may be ok to delay it until the normal minor release assuming that is fairly soon (I guess we are due the next one in the next week or two anyway?).

The backport into release-branch.go1.13 already landed, so it will be in the next non-securiry minor release, which should happen on the same day as the security one.

Thanks Filippo.

This is fixed by removing the assembly. Thanks for the quick CL, which shipped as a backport in Go 1.13.3. Let's use a new tracking issue for reintroducing it.