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

Comments

@Tasssadar
Copy link
Contributor

commented Sep 25, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2017

/cc @agl and @rsc

@agl agl self-assigned this Sep 28, 2017
@agl

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor Author

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 added a commit to avast/apkverifier that referenced this issue Dec 6, 2017
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented 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

This comment has been minimized.

Copy link
Member

commented Sep 30, 2019

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

This comment has been minimized.

Copy link

commented Oct 1, 2019

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
@gopherbot gopherbot closed this in 72dc3a0 Oct 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.