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

Jump to bottom

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

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

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 The path to resolution is known, but the work has not been done. Performance
Milestone
Go1.22

Comments

@dchaofei
Copy link
Contributor

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
@dchaofei
crypto/x509: optimize the performance of checkSignature function loop…
d22de31 
… matching `algo`

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

Fixes golang#52955
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 18, 2022
@seankhliao
Copy link
Member

cc @golang/security

dchaofei added a commit to dchaofei/go that referenced this issue May 18, 2022
@dchaofei
crypto/x509: optimize the performance of checkSignature
23998a6 
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
@dchaofei
crypto/x509: optimize the performance of checkSignature
7213227 
The loop should be terminated immediately when `algo` has been found

Fixes golang#52955
@rolandshoemaker
Copy link
Member

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 The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels May 18, 2022
@dchaofei dchaofei mentioned this issue May 19, 2022
Open
@gopherbot
Copy link

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

@seankhliao seankhliao added this to the Go1.20 milestone Aug 20, 2022
@gopherbot gopherbot modified the milestones: Go1.20, Go1.21 Feb 1, 2023
@gopherbot gopherbot modified the milestones: Go1.21, Go1.22 Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsFix The path to resolution is known, but the work has not been done. Performance
Projects
None yet
Milestone
Go1.22
Development

Successfully merging a pull request may close this issue.

crypto/x509: optimize the performance of checkSignature dchaofei/go
5 participants
@dmitshur @rolandshoemaker @gopherbot @seankhliao @dchaofei