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: Truncate DSA signed data's hash before verification #22017

Closed
Tasssadar opened this issue Sep 25, 2017 · 6 comments
Closed

crypto/x509: Truncate DSA signed data's hash before verification #22017

Tasssadar opened this issue Sep 25, 2017 · 6 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@Tasssadar
Copy link
Contributor

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

go version go1.9 linux/amd64

Does this issue reproduce with the latest release?

Yes

The x509.Certificate's CheckSignature function does not truncate the signed data's hash (as dsa's docs say should be done), resulting in wrong check result. When I truncated the hash, the check passes, as can be seen below.

I'm not sure whether the hash should be truncated for all x509 certs. This is tested with Android APK signatures, which do require the truncation (the data in code below were extracted from this apk and there are more like it).

I can submit a pull request if you think it is correct to do the truncation.

Raw full source of the code (~4k lines)

package main

import (
	"fmt"
	"crypto/x509"
	"crypto/sha256"
	"crypto/dsa"
	"math/big"
	"errors"
	"encoding/asn1"
)

type dsaSignature struct {
	R, S *big.Int
}

func checkWithTruncatedHash(cert *x509.Certificate) error {
	hash := sha256.Sum256(signed)
	pub := cert.PublicKey.(*dsa.PublicKey)

	reqLen := pub.Q.BitLen() / 8

	if reqLen > len(hash) {
		return fmt.Errorf("Digest algorithm is too short for given DSA parameters.")
	}
	digest := hash[:reqLen]

	dsaSig := new(dsaSignature)
	if rest, err := asn1.Unmarshal(signature, dsaSig); err != nil {
		return err
	} else if len(rest) != 0 {
		return errors.New("x509: trailing data after DSA signature")
	}
	if dsaSig.R.Sign() <= 0 || dsaSig.S.Sign() <= 0 {
		return errors.New("x509: DSA signature contained zero or negative values")
	}
	if !dsa.Verify(pub, digest, dsaSig.R, dsaSig.S) {
		return errors.New("x509: DSA verification failure")
	}
	return nil
}

func main() {
	cert, _ := x509.ParseCertificate(rawCert)

	fmt.Printf("Result of check with original crypto/x509: %v\n", cert.CheckSignature(algo, signed, signature))
	fmt.Printf("Result of check with truncated hash: %v\n", checkWithTruncatedHash(cert))
}

var algo = x509.DSAWithSHA256

var rawCert = []byte { ... }
var signature = []byte { ... }
var signed = []byte { ... }
@valyala
Copy link
Contributor

valyala commented Sep 25, 2017

/cc @agl and @rsc

@agl agl self-assigned this Sep 28, 2017
@agl
Copy link
Contributor

agl commented Oct 13, 2017

(Man, I should never have supported DSA in that code.)

I'm not sure that it's valid to have the hash mismatch with the DSA key type, as you have here. DSA was originally specified only with SHA-1 and with 1024-bit keys. That's what you have in the certificate, but your signature algorithm is DSAWithSHA256. For that, you should have a 2048/256 key.

@Tasssadar
Copy link
Contributor Author

Tasssadar commented Oct 14, 2017

Unfortunately, it's not really my key - I just need to verify the signature is correct. The signature check in Android passes without a problem, so it's "valid" in that way at least.

I'm fine with having that extra checkWithTruncatedHash function in my code, I just saw that dsa docs say the hash should be truncated, but it isn't in x509 and thought it might be a bug.

Tasssadar pushed a commit to avast/apkverifier that referenced this issue Dec 6, 2017
@ianlancetaylor
Copy link
Contributor

CC @FiloSottile

@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 29, 2018
@ianlancetaylor ianlancetaylor added this to the Go1.11 milestone Mar 29, 2018
@rsc rsc unassigned agl Aug 17, 2018
@rsc rsc modified the milestones: Go1.11, Go1.12 Aug 17, 2018
@andybons andybons modified the milestones: Go1.12, Go1.13 Feb 12, 2019
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@FiloSottile FiloSottile 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 Sep 30, 2019
@FiloSottile
Copy link
Contributor

Alright, I would like to let DSA rot in a corner, and using SHA-256 with 1024 bit keys is weird, but as far as I can tell we are indeed doing this wrong. I would take a PR to fix this with a test case that passes OpenSSL verification.

@gopherbot
Copy link

Change https://golang.org/cl/198138 mentions this issue: crypto/dsa: truncate the hash in Verify and Sign

Tasssadar added a commit to Tasssadar/go that referenced this issue Oct 4, 2019
According to spec, the hash must be truncated, but crypto/dsa
does not do it. We can't fix it in crypto/dsa, because it would break
verification of previously generated signatures.
In crypto/x509 however, go can't generate DSA certs, only verify them,
so the fix here should be safe.

Fixes golang#22017
@golang golang locked and limited conversation to collaborators Oct 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants