Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto/x509: checkSignature why the loop doesn't break after finding algo? #52955

Open
dchaofei opened this issue May 18, 2022 · 3 comments · May be fixed by #52987
Open

crypto/x509: checkSignature why the loop doesn't break after finding algo? #52955

dchaofei opened this issue May 18, 2022 · 3 comments · May be fixed by #52987
Labels
NeedsFix

Comments

@dchaofei
Copy link

@dchaofei dchaofei commented May 18, 2022

What version of Go are you using (go version)?

$ go version
go version go1.18 darwin/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOVERSION="go1.18"

What did you do?

I read the go source code and found a performance issue, checkSignature why the loop doesn't break after finding algo?

go/src/crypto/x509/x509.go

Lines 818 to 829 in aedf298

func checkSignature(algo SignatureAlgorithm, signed, signature []byte, publicKey crypto.PublicKey, allowSHA1 bool) (err error) {
var hashType crypto.Hash
var pubKeyAlgo PublicKeyAlgorithm
for _, details := range signatureAlgorithmDetails {
if details.algo == algo {
hashType = details.hash
pubKeyAlgo = details.pubKeyAlgo
}
}
switch hashType {

What did you expect to see?

The for of the checkSignature function requires an early break

	for _, details := range signatureAlgorithmDetails {
		if details.algo == algo {
			hashType = details.hash
			pubKeyAlgo = details.pubKeyAlgo
			break
		}
	}

What did you see instead?

If this needs to be fixed, I'd be happy to submit a pr

@dchaofei dchaofei changed the title crypto/X509: checkSignature why the loop doesn't break after finding algo? crypto/x509: checkSignature why the loop doesn't break after finding algo? May 18, 2022
dchaofei added a commit to dchaofei/go that referenced this issue May 18, 2022
… matching `algo`

The loop should be terminated immediately when `algo` has been found

Fixes golang#52955
@seankhliao seankhliao added the NeedsInvestigation label May 18, 2022
@seankhliao
Copy link
Member

@seankhliao seankhliao commented May 18, 2022

cc @golang/security

dchaofei added a commit to dchaofei/go that referenced this issue May 18, 2022
The loop should be terminated immediately when `algo` has been found

Fixes golang#52955
dchaofei added a commit to dchaofei/go that referenced this issue May 18, 2022
The loop should be terminated immediately when `algo` has been found

Fixes golang#52955
@rolandshoemaker
Copy link
Member

@rolandshoemaker rolandshoemaker commented May 18, 2022

Adding a break seems reasonable, there should only ever be a single matching algorithm and this doesn't need to be constant time. Feel free to send a CL, although be aware that since we are in the freeze window at the moment, your change will not be merged until ~August, when the tree re-opens for 1.20 changes.

@seankhliao seankhliao added NeedsFix and removed NeedsInvestigation labels May 18, 2022
@gopherbot
Copy link

@gopherbot gopherbot commented May 19, 2022

Change https://go.dev/cl/407215 mentions this issue: crypto/x509: optimize the performance of checkSignature

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants