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: does not notice if an RSA SignatureAlgorithm is missing parameters #21118

Open
agl opened this Issue Jul 21, 2017 · 4 comments

Comments

Projects
None yet
4 participants
@agl
Contributor

agl commented Jul 21, 2017

We may wish to reject certificates like these:

MIIE/TCCA+egAwIBAgIEUdNARDANBgkqhkiG9w0BAQsFADCBsDELMAkGA1UEBhMC
VVMxFjAUBgNVBAoTDUVudHJ1c3QsIEluYy4xOTA3BgNVBAsTMHd3dy5lbnRydXN0
Lm5ldC9DUFMgaXMgaW5jb3Jwb3JhdGVkIGJ5IHJlZmVyZW5jZTEfMB0GA1UECxMW
KGMpIDIwMDYgRW50cnVzdCwgSW5jLjEtMCsGA1UEAxMkRW50cnVzdCBSb290IENl
cnRpZmljYXRpb24gQXV0aG9yaXR5MB4XDTE0MDkyMjE3MTQ1N1oXDTI0MDkyMzAx
MzE1M1owgb4xCzAJBgNVBAYTAlVTMRYwFAYDVQQKEw1FbnRydXN0LCBJbmMuMSgw
JgYDVQQLEx9TZWUgd3d3LmVudHJ1c3QubmV0L2xlZ2FsLXRlcm1zMTkwNwYDVQQL
EzAoYykgMjAwOSBFbnRydXN0LCBJbmMuIC0gZm9yIGF1dGhvcml6ZWQgdXNlIG9u
bHkxMjAwBgNVBAMTKUVudHJ1c3QgUm9vdCBDZXJ0aWZpY2F0aW9uIEF1dGhvcml0
eSAtIEcyMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAuoS2ctueDGvi
mekwAad26jK4lUEaydphTlhyz/72gnm/c2EGCqUn2LNf00VOHHLWTjLycooP94MZ
0GqAgABFHrDH55q/ElcnHKNoLwqHvWprDl5l8xx31dSFjXAhtLMy54ui1YY5ArG4
0kfO5MlJxDun3vtUfVe+8OhuwnmyOgtV4lCYFjITXC94VsHClLPyWuQnmp8k18bs
0JslguPMwsRFxYyXegZrKhGfqQpuSDtv29QRGUL3jwe/9VNfnD70FyzmaaxOMkxi
d+q36OW7NLwZi66cUee3frVTsTMi5W3PcDwa+uKbZ7aD9I2lr2JMTeBYrGQ0EgP4
to2UYySkcQIDAQABo4IBDzCCAQswDgYDVR0PAQH/BAQDAgEGMBIGA1UdEwEB/wQI
MAYBAf8CAQEwMwYIKwYBBQUHAQEEJzAlMCMGCCsGAQUFBzABhhdodHRwOi8vb2Nz
cC5lbnRydXN0Lm5ldDAzBgNVHR8ELDAqMCigJqAkhiJodHRwOi8vY3JsLmVudHJ1
c3QubmV0L3Jvb3RjYTEuY3JsMDsGA1UdIAQ0MDIwMAYEVR0gADAoMCYGCCsGAQUF
BwIBFhpodHRwOi8vd3d3LmVudHJ1c3QubmV0L0NQUzAdBgNVHQ4EFgQUanImetAe
733nO2lR1GyNn5ASZqswHwYDVR0jBBgwFoAUaJDkZ6SmU4DHhmak8fdLQ/uEvW0w
CwYJKoZIhvcNAQELA4IBAQBpM4P8KHpvfe+dVevFPnqddbPMwzg22TSiKGgY6h5p
073n0HfauACDTkrPb9HxwSI/dOT3mEmem7ae4duYdy1WNLGoPNn9wM3HvwUD1ALF
8eXG2gilE8diIxHRYTAdYIRF73moxiaTpLfNNLhpxRP2kbPJRXN2tpL2dgpb4QNH
t+kpTJEyIzdKnDXYeP0dH+SDiSSArbf5z+RdpdRxxIVbcB/bPxwB6xpFJjEUzGW/
Z97KzDNl5UGR1ze+QRqWneaKl52nzqxOmj29AaBq2U8iAItE1Wliey7rzLrnkn1p
Zz38uHzeQYfQaeq6Chh6GpVDs3lxKHZtoftXSuxNyA4Q
-----END CERTIFICATE-----

@agl agl self-assigned this Jul 21, 2017

@agl agl changed the title from crypto/x509 does not notice if an RSA SignatureAlgorithm is missing parameter to crypto/x509: does not notice if an RSA SignatureAlgorithm is missing parameters Jul 21, 2017

@bradfitz bradfitz added the NeedsFix label Jul 21, 2017

@bradfitz bradfitz added this to the Go1.10 milestone Jul 21, 2017

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jul 21, 2017

Yeah, it's missing the -----BEGIN CERTIFICATE----- line! /s

@rsc rsc modified the milestones: Go1.10, Go1.11 Nov 22, 2017

@odeke-em

This comment has been minimized.

Member

odeke-em commented Jul 7, 2018

How's it going @agl? I'll also ping @pquerna @dchest who perhaps might be interested in this issue, and also @FiloSottile

@odeke-em

This comment has been minimized.

Member

odeke-em commented Jul 20, 2018

@agl I just started taking a look at this issue and noticed that in this repro https://play.golang.org/p/XSYNC9w9C5u or inlined

package main

import (
	"crypto/x509"
	"encoding/pem"
	"log"
)

func main() {
	data := `
MIIE/TCCA+egAwIBAgIEUdNARDANBgkqhkiG9w0BAQsFADCBsDELMAkGA1UEBhMC
VVMxFjAUBgNVBAoTDUVudHJ1c3QsIEluYy4xOTA3BgNVBAsTMHd3dy5lbnRydXN0
Lm5ldC9DUFMgaXMgaW5jb3Jwb3JhdGVkIGJ5IHJlZmVyZW5jZTEfMB0GA1UECxMW
KGMpIDIwMDYgRW50cnVzdCwgSW5jLjEtMCsGA1UEAxMkRW50cnVzdCBSb290IENl
cnRpZmljYXRpb24gQXV0aG9yaXR5MB4XDTE0MDkyMjE3MTQ1N1oXDTI0MDkyMzAx
MzE1M1owgb4xCzAJBgNVBAYTAlVTMRYwFAYDVQQKEw1FbnRydXN0LCBJbmMuMSgw
JgYDVQQLEx9TZWUgd3d3LmVudHJ1c3QubmV0L2xlZ2FsLXRlcm1zMTkwNwYDVQQL
EzAoYykgMjAwOSBFbnRydXN0LCBJbmMuIC0gZm9yIGF1dGhvcml6ZWQgdXNlIG9u
bHkxMjAwBgNVBAMTKUVudHJ1c3QgUm9vdCBDZXJ0aWZpY2F0aW9uIEF1dGhvcml0
eSAtIEcyMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAuoS2ctueDGvi
mekwAad26jK4lUEaydphTlhyz/72gnm/c2EGCqUn2LNf00VOHHLWTjLycooP94MZ
0GqAgABFHrDH55q/ElcnHKNoLwqHvWprDl5l8xx31dSFjXAhtLMy54ui1YY5ArG4
0kfO5MlJxDun3vtUfVe+8OhuwnmyOgtV4lCYFjITXC94VsHClLPyWuQnmp8k18bs
0JslguPMwsRFxYyXegZrKhGfqQpuSDtv29QRGUL3jwe/9VNfnD70FyzmaaxOMkxi
d+q36OW7NLwZi66cUee3frVTsTMi5W3PcDwa+uKbZ7aD9I2lr2JMTeBYrGQ0EgP4
to2UYySkcQIDAQABo4IBDzCCAQswDgYDVR0PAQH/BAQDAgEGMBIGA1UdEwEB/wQI
MAYBAf8CAQEwMwYIKwYBBQUHAQEEJzAlMCMGCCsGAQUFBzABhhdodHRwOi8vb2Nz
cC5lbnRydXN0Lm5ldDAzBgNVHR8ELDAqMCigJqAkhiJodHRwOi8vY3JsLmVudHJ1
c3QubmV0L3Jvb3RjYTEuY3JsMDsGA1UdIAQ0MDIwMAYEVR0gADAoMCYGCCsGAQUF
BwIBFhpodHRwOi8vd3d3LmVudHJ1c3QubmV0L0NQUzAdBgNVHQ4EFgQUanImetAe
733nO2lR1GyNn5ASZqswHwYDVR0jBBgwFoAUaJDkZ6SmU4DHhmak8fdLQ/uEvW0w
CwYJKoZIhvcNAQELA4IBAQBpM4P8KHpvfe+dVevFPnqddbPMwzg22TSiKGgY6h5p
073n0HfauACDTkrPb9HxwSI/dOT3mEmem7ae4duYdy1WNLGoPNn9wM3HvwUD1ALF
8eXG2gilE8diIxHRYTAdYIRF73moxiaTpLfNNLhpxRP2kbPJRXN2tpL2dgpb4QNH
t+kpTJEyIzdKnDXYeP0dH+SDiSSArbf5z+RdpdRxxIVbcB/bPxwB6xpFJjEUzGW/
Z97KzDNl5UGR1ze+QRqWneaKl52nzqxOmj29AaBq2U8iAItE1Wliey7rzLrnkn1p
Zz38uHzeQYfQaeq6Chh6GpVDs3lxKHZtoftXSuxNyA4Q
-----END CERTIFICATE-----`
	block, _ := pem.Decode([]byte(data))
	if block == nil {
		log.Fatal("Failed to decode block")
	}
	cert, err := x509.ParseCertificate(block.Bytes)
	if err != nil {
		log.Fatalf("Failed to parse certificate: %v", err)
	}
	log.Printf("%#v\n", cert)
}

In order for us to even get to x509.ParseCertificate we need to first PEM decode that certificate. Of which the pem.Decode function immediately fails to parse it given that we don't have the -----BEGIN CERTIFICATE----- as @bradfitz noted in #21118 (comment)

as per

if bytes.HasPrefix(data, pemStart[1:]) {
rest = rest[len(pemStart)-1 : len(data)]
} else if i := bytes.Index(data, pemStart); i >= 0 {
rest = rest[i+len(pemStart) : len(data)]
} else {
return nil, data
}

but the starter decode code has never changed since 9 years ago https://github.com/golang/go/blame/master/src/encoding/pem/pem.go#L82-L88
screen shot 2018-07-19 at 4 53 29 pm

and modifying my repro above to even use rest as per

var blockBytes []byte
block, rest := pem.Decode([]byte(data))
if block != nil {
   blockBytes = block.Bytes
} else {
   blockBytes = rest
}

Thus my follow-up question:
a) Was this a real problem that we noticed or a hypothetical word of caution?
b) Perhaps is a separate route that allows such certificates?

One other thing that we could do is tighten our tests both in encoding/pem and crypto/x509 to ensure such certificates never fly. I don't seem to see such tests in https://github.com/golang/go/blob/master/src/encoding/pem/pem_test.go.

Please let me know what you think, and thank you!

@odeke-em

This comment has been minimized.

Member

odeke-em commented Jul 20, 2018

and its SignatureAlgorithm is reported as SHA256-RSA

@rsc rsc modified the milestones: Go1.11, Go1.12 Aug 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment