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: Trailing data in the IssuerAlternativeName extension value does not return an error when parsing certificate. #23016

Closed
szank opened this issue Dec 6, 2017 · 5 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@szank
Copy link

szank commented Dec 6, 2017

Please answer these questions before submitting your issue. Thanks!

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

go version go1.9.2 darwin/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

osx

What did you do?

I was investigating some test certificates, and noticed this one:

-----BEGIN CERTIFICATE-----
MIIGPjCCBSigAwIBAgIFBDFmk+0wCwYJKoZIhvcNAQELMFYxCzAJBgNVBAYTAlVT
MRYwFAYDVQQKEw1Nb3RoZXIgTmF0dXJlMRMwEQYDVQQLEwpFdmVyeXRoaW5nMRgw
FgYDVQQDEw9OYW1lIGNvbnN0cmFpbnQxADAiGA8yMDU1MTIwMTA2MDcwOFoYDzIw
NTYxMDA1MjM1MjU4WjCBmzELMAkGA1UEBhMCVVMxGDAWBgNVBAoTD0V4dHJlbWUg
RGlzY29yZDEOMAwGA1UECxMFQ2hhb3MxFDASBgNVBAcTC1RhbGxhaGFzc2VlMQsw
CQYDVQQIEwJGTDEcMBoGA1UECRMTMzIxMCBIb2xseSBNaWxsIFJ1bjEOMAwGA1UE
ERMFMzAwNjIxDzANBgNVBAMTBmdvdi51czEAMIIBIjANBgkqhkiG9w0BAQEFAAOC
AQ8AMIIBCgKCAQEA2lVo8m23yLGU5GBObxhGAMYJNfMxLXwmGaYDQGN/Zr82ZK+Y
5oYwDd8OpZmLxU1aRfWoLSs6JQ/LbftdvT5nns+xFD/ZpboeghlZvpA8espM5zDR
5i4Qs8pmfqbrARdKV1khTDfgFw6aGTmatdHcwfJ+U6jOaXPTI8S4olN4LsyV0sCa
JT/c1SBXOdfQjLGe2cxwIyBzvwxj07guuYzq3Kj72d9sKyWPAdTdBngtKH2jvVq9
XncFo+juD+nhQOsRPCqAaE5cdsdSF+YTuJF/ZrAe5ohY0UiyTg9FrxVe8hf3PB3r
h9yjHuC7J0D0M39+dmhmyfYrAzDp9NnXsWXVvQIDAQABo4ICyzCCAscwDgYDVR0P
AQH/BAQDAgCkMB0GA1UdJQQWMBQGCCsGAQUFBwMCBggrBgEFBQcDATAPBgNVHRMB
Af8EBTADAQH/MA4GA1UdIwQHMAWAAwECAzBiBggrBgEFBQcBAQRWMFQwIQYIKwYB
BQUHMAGGFWh0dHA6Ly90aGVjYS5uZXQvb2NzcDAvBggrBgEFBQcwAoYjaHR0cDov
L3RoZWNhLm5ldC90b3RhbGx5dGhlY2VydC5jcnQwOAYDVR0RBDEwL4YgaXJyZWxl
dmFudGluZm8vL3VzZXJAMTkyLjE2OC4xLjGCC3d3dy5kbnMuY29tMAsGA1UdEgQE
MACCADAbBgNVHSAEFDASMAgGBmeBDAECAjAGBgQqAwQFMIIBqwYDVR0eBIIBojCC
AZ6ggc4wE4ERZ29vZF9lbWFpbEBnZy5jb20wCYEHTHVsTWFpbDAPgg1wZXJtaXR0
ZWQuY29tMIGOpIGLMIGIMQswCQYDVQQGEwJVUzENMAsGA1UEChMEVUlVQzEMMAoG
A1UECxMDRUNFMRIwEAYDVQQHEwlDaGFtcGFpZ24xCzAJBgNVBAgTAklMMRYwFAYD
VQQJEw02MDEgV3JpZ2h0IFN0MQ4wDAYDVQQREwU2MTgyMDERMA8GA1UEAxMIdWl1
Yy5uZXQxADAKhwhKfeBI//8AAKGByjASgRBiYWRfZW1haWxAZ2cuY29tMAmBB0x1
bE1haWwwDIIKYmFubmVkLmNvbTCBjqSBizCBiDELMAkGA1UEBhMCVVMxDjAMBgNV
BAoTBVVtaWNoMQswCQYDVQQLEwJDUzESMBAGA1UEBxMJQW5uIEFyYm9yMQswCQYD
VQQIEwJNSTEVMBMGA1UECRMMNTAwIFN0YXRlIFN0MQ4wDAYDVQQREwU0ODEwOTES
MBAGA1UEAxMJdW1pY2gubmV0MQAwCocIwKgBAf//AAAwCwYJKoZIhvcNAQELA4IB
AQDfA7wx83CZo+SG+revr32LacHA/EQFHGI4lyHQWbBdVsDCb6N7w9fNXCC3+MRo
cWYNNWctlvyEZfDoU/mshh2vpAQcL56toMJYCX8+6GeAJCGIS4yD7ks+j7GY+kzR
OnW1A3irglDycF3blQoaGJsLp94VjyOzESh0+nNwypfF1lGZFzjBpXGsL04hKzAR
7CAADZ7qSHvB39o8VdEX0toWk791G+yys7ugx/pAxWIpmYU68lco9S860KxA66ln
7s3g2KgdqC4kU9auUM3tvHFqdnXfbBYSYC1qvihKwmeFbCmAgsKH1rm4x4OuEohb
ewclEGevcJayEHyQjYNwtXfg
-----END CERTIFICATE-----

This looks strange, as the issuer alternative name looks malformed.
Here is the asn.1 decoder link:

The issuer alt name AttributeTypeAndValue value contains two concatanated ASN.1 structs.
One is an empty sequence (0x30,0x00) followed by empty primitive value (context specific tag) (0x82,0x00)

Here is the openssl asn1parse dump (relevant part):

857:d=5  hl=2 l=   3 prim: OBJECT            :X509v3 Issuer Alternative Name
  862:d=5  hl=2 l=   4 prim: OCTET STRING      [HEX DUMP]:30008200

Shouldn't it return an error? Openssl doesn't complain, but it is very permissive anyway. Go is much more strict, so I thought you guys might want to guard against this kind of encoding errors.

If possible, provide a recipe for reproducing the error.
A complete runnable program is good.
A link on play.golang.org is best.
https://play.golang.org/p/SCivmG9m88

What did you expect to see?

error

What did you see instead?

no errors

@odeke-em
Copy link
Member

odeke-em commented Dec 6, 2017

/cc @agl

@bradfitz bradfitz added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 6, 2017
@bradfitz bradfitz added this to the Go1.11 milestone Dec 6, 2017
@agl agl self-assigned this Dec 6, 2017
@szank
Copy link
Author

szank commented Dec 7, 2017

FWIW Issuer Alternative Names extension is not recognised/parsed by golang, so it ends up in Extensions slice.
As such go is not obliged to parse it or verify it.

@szank
Copy link
Author

szank commented Dec 7, 2017

I think this issue can be closed after all.

@odeke-em
Copy link
Member

odeke-em commented Dec 7, 2017

@szank thanks for the investigation. For purposes of posterity and aiding @agl and others during determining what can be closed, could you please post perhaps a reference to where you got that information? Thank you!

@titanous
Copy link
Member

titanous commented Dec 7, 2017

@szank is correct: https://play.golang.org/p/-HbEmkotsS

Note the unparsed extension:

pkix.Extension{Id:asn1.ObjectIdentifier{2, 5, 29, 18}, Critical:false, Value:[]uint8{0x30, 0x0, 0x82, 0x0}}

Since Go is not aware of the correct format of the extension and does not parse it, there is no reason to return an error. There is also no Certificate.IssuerAltName (or similar) field for this data to go in.

Closing as working as intended.

@titanous titanous closed this as completed Dec 7, 2017
@golang golang locked and limited conversation to collaborators Dec 7, 2018
@rsc rsc unassigned agl Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants