Navigation Menu

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

Closed
agl opened this issue Jul 21, 2017 · 9 comments
Closed

crypto/x509: does not notice if an RSA SignatureAlgorithm is missing parameters #21118

agl opened this issue Jul 21, 2017 · 9 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unplanned

Comments

@agl
Copy link
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 crypto/x509 does not notice if an RSA SignatureAlgorithm is missing parameter crypto/x509: does not notice if an RSA SignatureAlgorithm is missing parameters Jul 21, 2017
@bradfitz bradfitz added the NeedsFix The path to resolution is known, but the work has not been done. label Jul 21, 2017
@bradfitz bradfitz added this to the Go1.10 milestone Jul 21, 2017
@bradfitz
Copy link
Contributor

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

@rsc rsc modified the milestones: Go1.10, Go1.11 Nov 22, 2017
@odeke-em
Copy link
Member

odeke-em commented Jul 7, 2018 •
edited

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

@odeke-em
Copy link
Member

@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

go/src/encoding/pem/pem.go

Lines 82 to 88 in c814ac4

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
Copy link
Member

and its SignatureAlgorithm is reported as SHA256-RSA

@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 modified the milestones: Go1.14, Unplanned Sep 30, 2019
@rolandshoemaker
Copy link
Member

This appears to be working as expected. The provided example has sha256WithRSAEncryption with NULL parameters, as specified in RFC3279 section-2.2.1 and explicitly called out in the RSA section of the Mozilla root store policy.

@FiloSottile
Copy link
Contributor

I don't see the NULL parameters in the certificate in the issue description, just the sha256WithRSAEncryption OID as the only element in the SEQUENCE.

30 0B 06 09 2A 86 48 86 F7 0D 01 01 0B

https://lapo.it/asn1js/#MIIE_TCCA-egAwIBAgIEUdNARDANBgkqhkiG9w0BAQsFADCBsDELMAkGA1UEBhMCVVMxFjAUBgNVBAoTDUVudHJ1c3QsIEluYy4xOTA3BgNVBAsTMHd3dy5lbnRydXN0Lm5ldC9DUFMgaXMgaW5jb3Jwb3JhdGVkIGJ5IHJlZmVyZW5jZTEfMB0GA1UECxMWKGMpIDIwMDYgRW50cnVzdCwgSW5jLjEtMCsGA1UEAxMkRW50cnVzdCBSb290IENlcnRpZmljYXRpb24gQXV0aG9yaXR5MB4XDTE0MDkyMjE3MTQ1N1oXDTI0MDkyMzAxMzE1M1owgb4xCzAJBgNVBAYTAlVTMRYwFAYDVQQKEw1FbnRydXN0LCBJbmMuMSgwJgYDVQQLEx9TZWUgd3d3LmVudHJ1c3QubmV0L2xlZ2FsLXRlcm1zMTkwNwYDVQQLEzAoYykgMjAwOSBFbnRydXN0LCBJbmMuIC0gZm9yIGF1dGhvcml6ZWQgdXNlIG9ubHkxMjAwBgNVBAMTKUVudHJ1c3QgUm9vdCBDZXJ0aWZpY2F0aW9uIEF1dGhvcml0eSAtIEcyMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAuoS2ctueDGvimekwAad26jK4lUEaydphTlhyz_72gnm_c2EGCqUn2LNf00VOHHLWTjLycooP94MZ0GqAgABFHrDH55q_ElcnHKNoLwqHvWprDl5l8xx31dSFjXAhtLMy54ui1YY5ArG40kfO5MlJxDun3vtUfVe-8OhuwnmyOgtV4lCYFjITXC94VsHClLPyWuQnmp8k18bs0JslguPMwsRFxYyXegZrKhGfqQpuSDtv29QRGUL3jwe_9VNfnD70FyzmaaxOMkxid-q36OW7NLwZi66cUee3frVTsTMi5W3PcDwa-uKbZ7aD9I2lr2JMTeBYrGQ0EgP4to2UYySkcQIDAQABo4IBDzCCAQswDgYDVR0PAQH_BAQDAgEGMBIGA1UdEwEB_wQIMAYBAf8CAQEwMwYIKwYBBQUHAQEEJzAlMCMGCCsGAQUFBzABhhdodHRwOi8vb2NzcC5lbnRydXN0Lm5ldDAzBgNVHR8ELDAqMCigJqAkhiJodHRwOi8vY3JsLmVudHJ1c3QubmV0L3Jvb3RjYTEuY3JsMDsGA1UdIAQ0MDIwMAYEVR0gADAoMCYGCCsGAQUFBwIBFhpodHRwOi8vd3d3LmVudHJ1c3QubmV0L0NQUzAdBgNVHQ4EFgQUanImetAe733nO2lR1GyNn5ASZqswHwYDVR0jBBgwFoAUaJDkZ6SmU4DHhmak8fdLQ_uEvW0wCwYJKoZIhvcNAQELA4IBAQBpM4P8KHpvfe-dVevFPnqddbPMwzg22TSiKGgY6h5p073n0HfauACDTkrPb9HxwSI_dOT3mEmem7ae4duYdy1WNLGoPNn9wM3HvwUD1ALF8eXG2gilE8diIxHRYTAdYIRF73moxiaTpLfNNLhpxRP2kbPJRXN2tpL2dgpb4QNHt-kpTJEyIzdKnDXYeP0dH-SDiSSArbf5z-RdpdRxxIVbcB_bPxwB6xpFJjEUzGW_Z97KzDNl5UGR1ze-QRqWneaKl52nzqxOmj29AaBq2U8iAItE1Wliey7rzLrnkn1pZz38uHzeQYfQaeq6Chh6GpVDs3lxKHZtoftXSuxNyA4Q

@rolandshoemaker
Copy link
Member

Oh d'oh, yup I was looking at the TBSCertificate signature field rather than the Certificate signatureAlgorithm field 🤦.

@gopherbot
Copy link

Change https://golang.org/cl/235118 mentions this issue: crypto/x509: enforce algorithmIdentifier matching/encoding restrictions

@rolandshoemaker rolandshoemaker mentioned this issue Feb 12, 2021
Closed
@gopherbot
Copy link

Change https://golang.org/cl/274234 mentions this issue: crypto/x509: rewrite certificate parser

@golang golang locked and limited conversation to collaborators May 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
8 participants
@bradfitz @agl @rsc @andybons @FiloSottile @odeke-em @rolandshoemaker @gopherbot