I have a wip patch that works, but I'm not sure it's right, because I'm not familiar with the certificate thing. So, there are 3 certificates in the chain, as you can see in the output of gnutls-cli. We check all of them in build_certificate_chain(), and none of them are PINNED, ANCHORED or self-signed. In the last iteration of the certificate chain loop, we look for an issuer in the database (g_tls_database_lookup_certificate_issuer) because the last certificate returns NULL for the issuer (as expected) and we don't find any so build_certificate_chain() returns with STATUS_INCOMPLETE. The thing is that for the first issuer, that we get it's not PINNED, ANCHORED not sefl-signed, if we search for an issuer in the database, we do find a valid one (ANCHORED). So, what I did was changing the order in which we get the issuer for the current certificate in the loop, checking first if there's an issuer in the database, and if there isn't then try with the certificate issuer. It makes https://fbcdn-dragon-a.akamaihd.net work (while still failing in case of actually invalid certs), and all test pass, but I'm not sure that's the right thing.

Created attachment 310040 [details] [review]
gnutls: Build the certificate chain recursively instead of using a loop

This fixes the problem for me and all test keep passing.

Review of attachment 310040 [details] [review]:

I am not a real reviewer here, but one thing I would explicitly audit for in converting a loop into recursion is ensuring attacker-supplied input can't cause an infinite recursion.

(In reply to Colin Walters from comment #3)
> Review of attachment 310040 [details] [review] [review]:
> 
> I am not a real reviewer here, but one thing I would explicitly audit for in
> converting a loop into recursion is ensuring attacker-supplied input can't
> cause an infinite recursion.

Good advice. I believe it can, in this case.

it would be good to add tests for the case this patch is supposed to solve, and for the infinite-loop case

FYI, this is not going to be easy to test. I've never managed to send an arbitrary chain of certificates with the current test server based on GTlsServerConnection, which can only send a GTlsCertificate, and I haven't managed to get an arbitrary chain into a GTlsCertificate. Or if I did, it didn't work as expected. Last time I tried to get this working, I gave up when I decided that the next step I would try would be to use an Apache web server (which would allow us to send whatever chain we want).

I agree it's very important to test, but it would be a significant project. :(

(In reply to Michael Catanzaro from comment #6)
> FYI, this is not going to be easy to test. I've never managed to send an
> arbitrary chain of certificates with the current test server based on
> GTlsServerConnection, which can only send a GTlsCertificate, and I haven't
> managed to get an arbitrary chain into a GTlsCertificate. Or if I did, it
> didn't work as expected.

If I'm remembering right, this should be possible since bug 724708 was fixed. You should now be able to create individual certificates, and manually string together the issuer chain, and set that as the certificate on a GTlsServerConnection, and it should send the cert chain in the order you provided it.

(In reply to Michael Catanzaro from comment #4)
> (In reply to Colin Walters from comment #3)
> > Review of attachment 310040 [details] [review] [review] [review]:
> > 
> > I am not a real reviewer here, but one thing I would explicitly audit for in
> > converting a loop into recursion is ensuring attacker-supplied input can't
> > cause an infinite recursion.
> 
> Good advice. I believe it can, in this case.

Yes, well, the thing is that we don't know the amount of certificates in the chain at that point, but yes, we could still limit the recursion to a reasonable number. The current code uses an infinite loop, so you can do the same and ensure it never breaks.

In any case, my main concern is whether the approach is correct or not, I'm afraid trying to find an issuer in the db for every certificate in the chain might be just a work around that hides the actual bug. Because as Michael suggested yesterday, the third certificate in the chain should be the same than the issuer of the second certificate in the db (if I've understood the whole thing correctly).

(In reply to Carlos Garcia Campos from comment #9)
> In any case, my main concern is whether the approach is correct or not, I'm
> afraid trying to find an issuer in the db for every certificate in the chain
> might be just a work around that hides the actual bug.

No, that's completely correct. That is what NSS has always done and what OpenSSL and GnuTLS are nowadays doing, which is why the chain is accepted by everyone except us.

> Because as Michael
> suggested yesterday, the third certificate in the chain should be the same
> than the issuer of the second certificate in the db (if I've understood the
> whole thing correctly).

That is what I would expect. I'm quite surprised the third certificate (Baltimore CyberTrust Root) is not a trusted root. I have a Baltimore CyberTrust Root in my trust store, but it has different PEM from the one sent in the chain. Maybe it has the same public key as the trusted root? It would be good to understand what is going on here.

But anyway, if we just follow the chain of trust, the third certificate in the chain (Baltimore CyberTrust Root) would be rejected, since it's not signed by a trusted root in our database, but by the no-longer-trusted GTE CyberTrust Global Root. The only way to make this chain work is to either (a) note that the second certificate in the chain is signed by a trusted root in our database (which apparently it is), or (b) note that the third certificate in the chain signed the second, and that the third certificate is a trusted root (which I thought would be the case, but apparently is not?). 


For reference, a bit more information on this chain:

$ gnutls-cli fbcdn-dragon-a.akamaihd.net -V
Processed 180 CA certificate(s).
Resolving 'fbcdn-dragon-a.akamaihd.net'...
Connecting to '216.229.80.72:443'...
- Certificate type: X.509
- Got a certificate list of 3 certificates.
- Certificate[0] info:
 - X.509 Certificate Information:
	Version: 3
	Serial Number (hex): 03bf3aef2fa27c96b8ca8fb959cd332c9d501138
	Issuer: C=NL,L=Amsterdam,O=Verizon Enterprise Solutions,OU=Cybertrust,CN=Verizon Akamai SureServer CA G14-SHA1
	Validity:
		Not Before: Fri Jun 19 16:52:07 UTC 2015
		Not After: Sun Jun 19 16:52:05 UTC 2016
	Subject: C=US,ST=MA,L=Cambridge,O=Akamai Technologies Inc.,CN=a248.e.akamai.net
	Subject Public Key Algorithm: RSA
	Algorithm Security Level: Medium (2048 bits)
		Modulus (bits 2048):
			00:d9:a2:c4:90:e0:90:c6:41:34:9d:f3:d5:95:fa:da
			c3:81:bb:e4:ee:09:11:e4:a5:45:6d:73:2a:19:f9:3a
			20:9e:8d:14:4f:17:b8:5a:d3:82:3c:d0:d5:f3:a4:b0
			3f:b7:3a:6c:b5:7a:3a:ea:d3:14:89:b2:ac:1c:b6:08
			6d:5b:41:f2:84:88:a7:1f:3a:c4:a7:aa:f0:1a:25:cb
			13:78:07:7b:fb:04:2f:5f:73:5e:ed:19:d2:54:ec:f7
			9b:ec:e9:14:f3:ca:53:46:15:54:88:e4:1f:bc:8f:18
			c4:c5:35:c9:cc:b1:b6:7e:8b:ef:21:75:ad:55:e9:52
			08:8c:47:dc:48:a0:c7:8f:b6:b9:87:c2:6c:45:3e:20
			63:8f:51:62:e4:37:9a:9b:8f:80:b9:ee:17:02:1d:39
			16:c9:8a:6b:69:fc:eb:2a:d5:99:17:ad:6d:3f:db:29
			13:c1:7d:4b:ab:39:56:8d:59:43:bb:7f:81:71:7e:28
			8a:9a:88:3b:08:ec:bc:f0:d8:5e:e8:4b:09:4d:27:66
			07:b9:20:de:2f:90:81:cc:de:a8:c8:bb:77:c6:26:c3
			5e:c8:38:35:e0:a2:b0:a5:a9:14:08:19:d4:c8:5e:73
			21:0b:ad:c2:84:a4:57:c9:c6:59:00:24:1b:54:61:4f
			2b
		Exponent (bits 24):
			01:00:01
	Extensions:
		Basic Constraints (critical):
			Certificate Authority (CA): FALSE
		Certificate Policies (not critical):
			1.3.6.1.4.1.6334.1.50
				URI: https://secure.omniroot.com/repository
		Authority Information Access (not critical):
			Access Method: 1.3.6.1.5.5.7.48.1 (id-ad-ocsp)
			Access Location URI: http://vassg141.ocsp.omniroot.com
			Access Method: 1.3.6.1.5.5.7.48.2 (id-ad-caIssuers)
			Access Location URI: https://cacert.a.omniroot.com/vassg141.crt
			Access Method: 1.3.6.1.5.5.7.48.2 (id-ad-caIssuers)
			Access Location URI: https://cacert.a.omniroot.com/vassg141.der
		Subject Alternative Name (not critical):
			DNSname: a248.e.akamai.net
			DNSname: *.akamaihd.net
			DNSname: *.akamaihd-staging.net
			DNSname: *.akamaized.net
			DNSname: *.akamaized-staging.net
		Key Usage (critical):
			Digital signature.
			Key encipherment.
		Key Purpose (not critical):
			TLS WWW Server.
			TLS WWW Client.
		Authority Key Identifier (not critical):
			dd6c807cbab53217a5844140f0d20466132fa990
		CRL Distribution points (not critical):
			URI: http://vassg141.crl.omniroot.com/vassg141.crl
		Subject Key Identifier (not critical):
			03b64a9c800c6018880a64cdae28628a7a6cc018
	Signature Algorithm: RSA-SHA1
	Signature:
		1c:64:ce:c3:76:4d:8c:29:fc:76:d1:3c:24:83:57:8e
		3e:77:21:0e:d6:83:f1:42:b9:2e:21:9d:14:96:c1:53
		49:e8:16:20:53:40:f2:e5:01:b7:df:01:07:77:49:6d
		ea:53:10:c9:00:05:0f:bb:c8:21:1d:38:9c:07:78:9c
		0a:ad:e1:91:91:8b:95:f9:a8:e4:02:64:e2:15:0b:a9
		7f:13:b8:03:ae:95:c5:45:47:33:fb:65:dd:30:bc:6c
		cc:96:bb:c3:bc:52:77:74:03:86:ab:9d:dc:16:6f:04
		49:b9:9f:8f:3c:b6:1e:5b:97:e9:f1:8e:e9:ba:59:da
		76:d4:7c:a6:7a:ce:2f:5e:d8:66:62:06:ff:c1:18:60
		f8:ad:1e:31:d3:ba:ee:06:b2:75:1a:0f:05:6a:a9:61
		7a:27:eb:a6:bd:f7:7c:05:c7:2c:bb:fd:ff:2d:1e:b4
		b5:b4:a9:cf:91:5b:0e:9e:e3:de:94:fa:95:b6:99:26
		be:e5:7c:27:03:e9:b8:96:fa:17:6b:85:e9:1e:ed:d4
		e3:41:9f:db:be:89:76:ed:e8:86:85:c1:86:1d:29:2b
		17:d1:2c:0b:cf:07:cd:8a:52:89:93:e1:72:79:c5:31
		7d:f1:fa:34:ce:d9:37:94:50:0b:71:c7:49:c8:6a:cb
Other Information:
	SHA1 fingerprint:
		ea5a209578d709605ca1e4caa52bbdc178fb2323
	SHA256 fingerprint:
		43645b3d5fa69b53750deab982c6ed6aca06d17452589f20bbb8b31d09500b48
	Public Key ID:
		3e7c3e4d97de81efd9a96dcf7e172cb245836d5d
	Public key's random art:
		+--[ RSA 2048]----+
		|                 |
		|                E|
		|            o . .|
		|           . = . |
		|        S   o +. |
		|       o   ..+o+ |
		|        + .o+oo.o|
		|         +... o+B|
		|          .. .+*O|
		+-----------------+


-----BEGIN CERTIFICATE-----
MIIFvDCCBKSgAwIBAgIUA7867y+ifJa4yo+5Wc0zLJ1QETgwDQYJKoZIhvcNAQEF
BQAwgY0xCzAJBgNVBAYTAk5MMRIwEAYDVQQHEwlBbXN0ZXJkYW0xJTAjBgNVBAoT
HFZlcml6b24gRW50ZXJwcmlzZSBTb2x1dGlvbnMxEzARBgNVBAsTCkN5YmVydHJ1
c3QxLjAsBgNVBAMTJVZlcml6b24gQWthbWFpIFN1cmVTZXJ2ZXIgQ0EgRzE0LVNI
QTEwHhcNMTUwNjE5MTY1MjA3WhcNMTYwNjE5MTY1MjA1WjBtMQswCQYDVQQGEwJV
UzELMAkGA1UECBMCTUExEjAQBgNVBAcTCUNhbWJyaWRnZTEhMB8GA1UEChMYQWth
bWFpIFRlY2hub2xvZ2llcyBJbmMuMRowGAYDVQQDExFhMjQ4LmUuYWthbWFpLm5l
dDCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBANmixJDgkMZBNJ3z1ZX6
2sOBu+TuCRHkpUVtcyoZ+Togno0UTxe4WtOCPNDV86SwP7c6bLV6OurTFImyrBy2
CG1bQfKEiKcfOsSnqvAaJcsTeAd7+wQvX3Ne7RnSVOz3m+zpFPPKU0YVVIjkH7yP
GMTFNcnMsbZ+i+8hda1V6VIIjEfcSKDHj7a5h8JsRT4gY49RYuQ3mpuPgLnuFwId
ORbJimtp/Osq1ZkXrW0/2ykTwX1LqzlWjVlDu3+BcX4oipqIOwjsvPDYXuhLCU0n
Zge5IN4vkIHM3qjIu3fGJsNeyDg14KKwpakUCBnUyF5zIQutwoSkV8nGWQAkG1Rh
TysCAwEAAaOCAjEwggItMAwGA1UdEwEB/wQCMAAwTAYDVR0gBEUwQzBBBgkrBgEE
AbE+ATIwNDAyBggrBgEFBQcCARYmaHR0cHM6Ly9zZWN1cmUub21uaXJvb3QuY29t
L3JlcG9zaXRvcnkwga8GCCsGAQUFBwEBBIGiMIGfMC0GCCsGAQUFBzABhiFodHRw
Oi8vdmFzc2cxNDEub2NzcC5vbW5pcm9vdC5jb20wNgYIKwYBBQUHMAKGKmh0dHBz
Oi8vY2FjZXJ0LmEub21uaXJvb3QuY29tL3Zhc3NnMTQxLmNydDA2BggrBgEFBQcw
AoYqaHR0cHM6Ly9jYWNlcnQuYS5vbW5pcm9vdC5jb20vdmFzc2cxNDEuZGVyMG4G
A1UdEQRnMGWCEWEyNDguZS5ha2FtYWkubmV0gg4qLmFrYW1haWhkLm5ldIIWKi5h
a2FtYWloZC1zdGFnaW5nLm5ldIIPKi5ha2FtYWl6ZWQubmV0ghcqLmFrYW1haXpl
ZC1zdGFnaW5nLm5ldDAOBgNVHQ8BAf8EBAMCBaAwHQYDVR0lBBYwFAYIKwYBBQUH
AwEGCCsGAQUFBwMCMB8GA1UdIwQYMBaAFN1sgHy6tTIXpYRBQPDSBGYTL6mQMD4G
A1UdHwQ3MDUwM6AxoC+GLWh0dHA6Ly92YXNzZzE0MS5jcmwub21uaXJvb3QuY29t
L3Zhc3NnMTQxLmNybDAdBgNVHQ4EFgQUA7ZKnIAMYBiICmTNrihiinpswBgwDQYJ
KoZIhvcNAQEFBQADggEBABxkzsN2TYwp/HbRPCSDV44+dyEO1oPxQrkuIZ0UlsFT
SegWIFNA8uUBt98BB3dJbepTEMkABQ+7yCEdOJwHeJwKreGRkYuV+ajkAmTiFQup
fxO4A66VxUVHM/tl3TC8bMyWu8O8Und0A4arndwWbwRJuZ+PPLYeW5fp8Y7pulna
dtR8pnrOL17YZmIG/8EYYPitHjHTuu4GsnUaDwVqqWF6J+umvfd8Bccsu/3/LR60
tbSpz5FbDp7j3pT6lbaZJr7lfCcD6biW+hdrheke7dTjQZ/bvol27eiGhcGGHSkr
F9EsC88HzYpSiZPhcnnFMX3x+jTO2TeUUAtxx0nIass=
-----END CERTIFICATE-----

- Certificate[1] info:
 - X.509 Certificate Information:
	Version: 3
	Serial Number (hex): 0727a46a
	Issuer: C=IE,O=Baltimore,OU=CyberTrust,CN=Baltimore CyberTrust Root
	Validity:
		Not Before: Wed Apr 02 14:32:45 UTC 2014
		Not After: Fri Apr 02 14:31:39 UTC 2021
	Subject: C=NL,L=Amsterdam,O=Verizon Enterprise Solutions,OU=Cybertrust,CN=Verizon Akamai SureServer CA G14-SHA1
	Subject Public Key Algorithm: RSA
	Algorithm Security Level: Medium (2048 bits)
		Modulus (bits 2048):
			00:91:eb:2d:35:9b:0c:64:6c:ca:8b:2d:d5:39:b6:33
			b4:fc:49:0a:16:78:d7:a8:3d:37:10:02:76:db:d7:db
			4f:9c:56:4b:a1:69:b4:4b:2d:4a:e3:43:f6:6f:16:08
			b5:ef:74:45:8f:74:56:16:2b:15:a5:16:4c:7d:6f:0a
			99:b4:20:e7:8a:7b:9c:46:e9:ae:31:fb:13:4f:f0:97
			16:61:e3:ba:2b:a5:24:d8:dc:4b:96:c1:be:d3:ab:bf
			66:d8:44:26:f9:16:96:ed:9c:36:2c:df:29:34:2a:b5
			3d:13:d2:b1:41:2d:53:93:cb:b5:74:28:06:f4:63:8d
			5e:de:b2:a8:1b:d7:7d:4f:24:66:33:ed:c6:8a:b0:84
			6e:c6:09:5a:96:28:06:8c:e4:51:cb:a1:c5:4b:bb:df
			44:3f:0a:5a:c3:56:11:8b:f5:3c:95:db:09:7b:d6:34
			88:51:a2:fd:4b:aa:49:14:77:a9:b1:24:ab:25:4f:62
			9c:b9:f9:c9:07:2d:10:c8:70:b7:18:b1:56:32:1d:3f
			a9:82:52:a7:b0:b2:61:24:6a:5f:8c:74:f6:2c:78:2c
			20:59:70:0b:76:ce:fc:50:ad:52:eb:9c:a2:45:36:9a
			8b:81:1b:4d:ff:4c:76:9b:de:29:ec:f5:bd:e6:08:5e
			eb
		Exponent (bits 24):
			01:00:01
	Extensions:
		Basic Constraints (critical):
			Certificate Authority (CA): TRUE
			Path Length Constraint: 2
		Certificate Policies (not critical):
			1.3.6.1.4.1.6334.1.50
				URI: https://secure.omniroot.com/repository
		Authority Information Access (not critical):
			Access Method: 1.3.6.1.5.5.7.48.1 (id-ad-ocsp)
			Access Location URI: http://ocsp.omniroot.com/baltimoreroot
			Access Method: 1.3.6.1.5.5.7.48.2 (id-ad-caIssuers)
			Access Location URI: https://cacert.omniroot.com/baltimoreroot.crt
			Access Method: 1.3.6.1.5.5.7.48.2 (id-ad-caIssuers)
			Access Location URI: https://cacert.omniroot.com/baltimoreroot.der
		Key Usage (critical):
			Digital signature.
			Non repudiation.
			Certificate signing.
			CRL signing.
		Authority Key Identifier (not critical):
			e59d5930824758ccacfa085436867b3ab5044df0
		CRL Distribution points (not critical):
			URI: http://cdp1.public-trust.com/CRL/Omniroot2025.crl
		Subject Key Identifier (not critical):
			dd6c807cbab53217a5844140f0d20466132fa990
	Signature Algorithm: RSA-SHA1
	Signature:
		58:41:22:ff:4d:42:55:ae:c8:06:87:45:3f:8b:59:d3
		07:39:9a:b2:a0:91:81:d2:0c:c1:67:4f:4f:71:28:5a
		90:1c:69:3a:9a:f6:9b:c0:08:f4:5a:22:89:55:df:6f
		af:45:b8:a2:13:79:18:b5:aa:d3:c2:fe:cb:70:73:85
		25:5b:d5:cc:6f:06:90:a9:d8:af:76:8e:74:07:50:83
		10:4d:1d:79:30:96:d9:e9:02:1f:e3:21:d6:ec:d2:3c
		30:2c:b5:53:c9:12:a7:f2:c3:63:ee:f6:ab:fb:e0:7b
		85:fd:84:d6:5b:57:50:38:f4:d3:19:29:3e:11:f0:66
		40:e4:9f:c1:bb:f0:6c:e2:6f:24:3e:a6:6b:37:21:f5
		27:bb:a7:02:22:b9:fc:43:da:ed:13:0f:e7:83:36:55
		91:7a:00:1c:d6:f8:19:6c:3a:0f:44:73:93:95:ca:01
		d6:e6:5f:85:47:1a:27:7e:05:12:34:9d:48:cd:97:04
		84:3a:75:fa:76:14:98:5d:a8:69:6b:d9:f6:ed:d1:c6
		ca:1f:1d:c3:62:a9:c5:f0:31:4d:33:6f:8b:26:91:4b
		40:0d:f1:e7:d0:a7:3b:8f:a7:32:9d:de:9d:a9:71:10
		df:f4:47:4c:c7:2d:a9:21:34:8e:e9:0b:6f:39:f1:c9
Other Information:
	SHA1 fingerprint:
		3209cc3ce4f1c22ab64b2e4284100b0022ad2739
	SHA256 fingerprint:
		9cfb49f8520572fda343698bc04307c856b4f1819b1d734bcd40aac8f1e16c2d
	Public Key ID:
		ebbb9fdabbcb57cd81a307df60fa4ba454db575f
	Public key's random art:
		+--[ RSA 2048]----+
		|                 |
		|                 |
		|             .. E|
		|           ..=o.+|
		|        S  .*o+++|
		|         ..ooo..+|
		|        .  .oo   |
		|       . o .o.   |
		|        ==O= ..  |
		+-----------------+


-----BEGIN CERTIFICATE-----
MIIFHzCCBAegAwIBAgIEByekajANBgkqhkiG9w0BAQUFADBaMQswCQYDVQQGEwJJ
RTESMBAGA1UEChMJQmFsdGltb3JlMRMwEQYDVQQLEwpDeWJlclRydXN0MSIwIAYD
VQQDExlCYWx0aW1vcmUgQ3liZXJUcnVzdCBSb290MB4XDTE0MDQwMjE0MzI0NVoX
DTIxMDQwMjE0MzEzOVowgY0xCzAJBgNVBAYTAk5MMRIwEAYDVQQHEwlBbXN0ZXJk
YW0xJTAjBgNVBAoTHFZlcml6b24gRW50ZXJwcmlzZSBTb2x1dGlvbnMxEzARBgNV
BAsTCkN5YmVydHJ1c3QxLjAsBgNVBAMTJVZlcml6b24gQWthbWFpIFN1cmVTZXJ2
ZXIgQ0EgRzE0LVNIQTEwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQCR
6y01mwxkbMqLLdU5tjO0/EkKFnjXqD03EAJ229fbT5xWS6FptEstSuND9m8WCLXv
dEWPdFYWKxWlFkx9bwqZtCDninucRumuMfsTT/CXFmHjuiulJNjcS5bBvtOrv2bY
RCb5FpbtnDYs3yk0KrU9E9KxQS1Tk8u1dCgG9GONXt6yqBvXfU8kZjPtxoqwhG7G
CVqWKAaM5FHLocVLu99EPwpaw1YRi/U8ldsJe9Y0iFGi/UuqSRR3qbEkqyVPYpy5
+ckHLRDIcLcYsVYyHT+pglKnsLJhJGpfjHT2LHgsIFlwC3bO/FCtUuucokU2mouB
G03/THab3ins9b3mCF7rAgMBAAGjggG3MIIBszASBgNVHRMBAf8ECDAGAQH/AgEC
MEwGA1UdIARFMEMwQQYJKwYBBAGxPgEyMDQwMgYIKwYBBQUHAgEWJmh0dHBzOi8v
c2VjdXJlLm9tbmlyb290LmNvbS9yZXBvc2l0b3J5MIG6BggrBgEFBQcBAQSBrTCB
qjAyBggrBgEFBQcwAYYmaHR0cDovL29jc3Aub21uaXJvb3QuY29tL2JhbHRpbW9y
ZXJvb3QwOQYIKwYBBQUHMAKGLWh0dHBzOi8vY2FjZXJ0Lm9tbmlyb290LmNvbS9i
YWx0aW1vcmVyb290LmNydDA5BggrBgEFBQcwAoYtaHR0cHM6Ly9jYWNlcnQub21u
aXJvb3QuY29tL2JhbHRpbW9yZXJvb3QuZGVyMA4GA1UdDwEB/wQEAwIBxjAfBgNV
HSMEGDAWgBTlnVkwgkdYzKz6CFQ2hns6tQRN8DBCBgNVHR8EOzA5MDegNaAzhjFo
dHRwOi8vY2RwMS5wdWJsaWMtdHJ1c3QuY29tL0NSTC9PbW5pcm9vdDIwMjUuY3Js
MB0GA1UdDgQWBBTdbIB8urUyF6WEQUDw0gRmEy+pkDANBgkqhkiG9w0BAQUFAAOC
AQEAWEEi/01CVa7IBodFP4tZ0wc5mrKgkYHSDMFnT09xKFqQHGk6mvabwAj0WiKJ
Vd9vr0W4ohN5GLWq08L+y3BzhSVb1cxvBpCp2K92jnQHUIMQTR15MJbZ6QIf4yHW
7NI8MCy1U8kSp/LDY+72q/vge4X9hNZbV1A49NMZKT4R8GZA5J/Bu/Bs4m8kPqZr
NyH1J7unAiK5/EPa7RMP54M2VZF6ABzW+BlsOg9Ec5OVygHW5l+FRxonfgUSNJ1I
zZcEhDp1+nYUmF2oaWvZ9u3RxsofHcNiqcXwMU0zb4smkUtADfHn0Kc7j6cynd6d
qXEQ3/RHTMctqSE0jukLbznxyQ==
-----END CERTIFICATE-----

- Certificate[2] info:
 - X.509 Certificate Information:
	Version: 3
	Serial Number (hex): 07278eed
	Issuer: C=US,O=GTE Corporation,OU=GTE CyberTrust Solutions\, Inc.,CN=GTE CyberTrust Global Root
	Validity:
		Not Before: Wed Apr 18 16:36:18 UTC 2012
		Not After: Mon Aug 13 16:35:17 UTC 2018
	Subject: C=IE,O=Baltimore,OU=CyberTrust,CN=Baltimore CyberTrust Root
	Subject Public Key Algorithm: RSA
	Algorithm Security Level: Medium (2048 bits)
		Modulus (bits 2048):
			00:a3:04:bb:22:ab:98:3d:57:e8:26:72:9a:b5:79:d4
			29:e2:e1:e8:95:80:b1:b0:e3:5b:8e:2b:29:9a:64:df
			a1:5d:ed:b0:09:05:6d:db:28:2e:ce:62:a2:62:fe:b4
			88:da:12:eb:38:eb:21:9d:c0:41:2b:01:52:7b:88:77
			d3:1c:8f:c7:ba:b9:88:b5:6a:09:e7:73:e8:11:40:a7
			d1:cc:ca:62:8d:2d:e5:8f:0b:a6:50:d2:a8:50:c3:28
			ea:f5:ab:25:87:8a:9a:96:1c:a9:67:b8:3f:0c:d5:f7
			f9:52:13:2f:c2:1b:d5:70:70:f0:8f:c0:12:ca:06:cb
			9a:e1:d9:ca:33:7a:77:d6:f8:ec:b9:f1:68:44:42:48
			13:d2:c0:c2:a4:ae:5e:60:fe:b6:a6:05:fc:b4:dd:07
			59:02:d4:59:18:98:63:f5:a5:63:e0:90:0c:7d:5d:b2
			06:7a:f3:85:ea:eb:d4:03:ae:5e:84:3e:5f:ff:15:ed
			69:bc:f9:39:36:72:75:cf:77:52:4d:f3:c9:90:2c:b9
			3d:e5:c9:23:53:3f:1f:24:98:21:5c:07:99:29:bd:c6
			3a:ec:e7:6e:86:3a:6b:97:74:63:33:bd:68:18:31:f0
			78:8d:76:bf:fc:9e:8e:5d:2a:86:a7:4d:90:dc:27:1a
			39
		Exponent (bits 24):
			01:00:01
	Extensions:
		Basic Constraints (critical):
			Certificate Authority (CA): TRUE
			Path Length Constraint: 3
		Certificate Policies (not critical):
			2.5.29.32.0
				URI: http://cybertrust.omniroot.com/repository
		Key Usage (critical):
			Certificate signing.
			CRL signing.
		Authority Key Identifier (not critical):
			directoryName: C=US,O=GTE Corporation,OU=GTE CyberTrust Solutions\, Inc.,CN=GTE CyberTrust Global Root
			serial: 01a5
		CRL Distribution points (not critical):
			URI: http://www.public-trust.com/cgi-bin/CRL/2018/cdp.crl
	Signature Algorithm: RSA-SHA1
	Signature:
		93:1d:fe:8b:ae:46:ec:cb:a9:0f:ab:e5:ef:ca:b2:68
		16:68:d8:8f:fa:13:a9:af:b3:cb:2d:e7:4b:6e:8e:69
		2a:c2:2b:10:0a:8d:f6:ae:73:b6:b9:fb:14:fd:5f:6d
		b8:50:b6:c4:8a:d6:40:7e:d7:c3:cb:73:dc:c9:5d:5b
		af:b0:41:b5:37:eb:ea:dc:20:91:c4:34:6a:f4:a1:f3
		96:9d:37:86:97:e1:71:a4:dd:7d:fa:44:84:94:ae:d7
		09:04:22:76:0f:64:51:35:a9:24:0f:f9:0b:db:32:da
		c2:fe:c1:b9:2a:5c:7a:27:13:ca:b1:48:3a:71:d0:43
Other Information:
	SHA1 fingerprint:
		4d34ea92764b3a3149119952f41930ca11348361
	SHA256 fingerprint:
		940b8920f4071c23bb32b2b572abb7d7ffbc114481a697e5d14f862e25069804
	Public Key ID:
		30a4e64fde768afced5a9084283046792c291570
	Public key's random art:
		+--[ RSA 2048]----+
		|==E.  .          |
		|oB o + .         |
		|. + + + .        |
		|   +   + .       |
		|    . . S        |
		|     + . .       |
		|      o o o      |
		|     . o =       |
		|      o.+oo      |
		+-----------------+


-----BEGIN CERTIFICATE-----
MIIEFTCCA36gAwIBAgIEByeO7TANBgkqhkiG9w0BAQUFADB1MQswCQYDVQQGEwJV
UzEYMBYGA1UEChMPR1RFIENvcnBvcmF0aW9uMScwJQYDVQQLEx5HVEUgQ3liZXJU
cnVzdCBTb2x1dGlvbnMsIEluYy4xIzAhBgNVBAMTGkdURSBDeWJlclRydXN0IEds
b2JhbCBSb290MB4XDTEyMDQxODE2MzYxOFoXDTE4MDgxMzE2MzUxN1owWjELMAkG
A1UEBhMCSUUxEjAQBgNVBAoTCUJhbHRpbW9yZTETMBEGA1UECxMKQ3liZXJUcnVz
dDEiMCAGA1UEAxMZQmFsdGltb3JlIEN5YmVyVHJ1c3QgUm9vdDCCASIwDQYJKoZI
hvcNAQEBBQADggEPADCCAQoCggEBAKMEuyKrmD1X6CZymrV51Cni4eiVgLGw41uO
KymaZN+hXe2wCQVt2yguzmKiYv60iNoS6zjrIZ3AQSsBUnuId9Mcj8e6uYi1agnn
c+gRQKfRzMpijS3ljwumUNKoUMMo6vWrJYeKmpYcqWe4PwzV9/lSEy/CG9VwcPCP
wBLKBsua4dnKM3p31vjsufFoREJIE9LAwqSuXmD+tqYF/LTdB1kC1FkYmGP1pWPg
kAx9XbIGevOF6uvUA65ehD5f/xXtabz5OTZydc93Uk3zyZAsuT3lySNTPx8kmCFc
B5kpvcY67Oduhjprl3RjM71oGDHweI12v/yejl0qhqdNkNwnGjkCAwEAAaOCAUcw
ggFDMBIGA1UdEwEB/wQIMAYBAf8CAQMwSgYDVR0gBEMwQTA/BgRVHSAAMDcwNQYI
KwYBBQUHAgEWKWh0dHA6Ly9jeWJlcnRydXN0Lm9tbmlyb290LmNvbS9yZXBvc2l0
b3J5MA4GA1UdDwEB/wQEAwIBBjCBiQYDVR0jBIGBMH+heaR3MHUxCzAJBgNVBAYT
AlVTMRgwFgYDVQQKEw9HVEUgQ29ycG9yYXRpb24xJzAlBgNVBAsTHkdURSBDeWJl
clRydXN0IFNvbHV0aW9ucywgSW5jLjEjMCEGA1UEAxMaR1RFIEN5YmVyVHJ1c3Qg
R2xvYmFsIFJvb3SCAgGlMEUGA1UdHwQ+MDwwOqA4oDaGNGh0dHA6Ly93d3cucHVi
bGljLXRydXN0LmNvbS9jZ2ktYmluL0NSTC8yMDE4L2NkcC5jcmwwDQYJKoZIhvcN
AQEFBQADgYEAkx3+i65G7MupD6vl78qyaBZo2I/6E6mvs8st50tujmkqwisQCo32
rnO2ufsU/V9tuFC2xIrWQH7Xw8tz3MldW6+wQbU36+rcIJHENGr0ofOWnTeGl+Fx
pN19+kSElK7XCQQidg9kUTWpJA/5C9sy2sL+wbkqXHonE8qxSDpx0EM=
-----END CERTIFICATE-----

- Status: The certificate is trusted. 
- Description: (TLS1.2)-(ECDHE-RSA-SECP256R1)-(AES-256-GCM)
- Session ID: 08:98:D5:69:F2:14:5B:34:D3:E6:0D:40:CA:0D:DA:08:2E:D5:F6:34:3E:BE:76:C8:0E:6C:05:60:7F:10:C2:49
- Ephemeral EC Diffie-Hellman parameters
 - Using curve: SECP256R1
 - Curve size: 256 bits
- Version: TLS1.2
- Key Exchange: ECDHE-RSA
- Server Signature: RSA-SHA256
- Cipher: AES-256-GCM
- MAC: AEAD
- Compression: NULL
- Options:
- Channel binding 'tls-unique': deabdfcea97e4927ee13a848
- Handshake was completed

- Simple Client Mode:

(In reply to Dan Winship from comment #7)
> If I'm remembering right, this should be possible since bug 724708 was
> fixed. You should now be able to create individual certificates, and
> manually string together the issuer chain, and set that as the certificate
> on a GTlsServerConnection, and it should send the cert chain in the order
> you provided it.

I will post a WIP (i.e. not working yet) patch in bug #754129.

(In reply to Dan Winship from comment #5)
> it would be good to add tests for the case this patch is supposed to solve,
> and for the infinite-loop case

For the infinite recursion -> stack overflow case... I tested Carlos's patch with a 40 MB certificate chain (30000 certificates). The test took over a minute to run, and didn't crash (so glib handled it properly). I think Carlos's patch must be wrong in theory -- we need a limit on the recursion if we recurse, since the attacker supplies the chain -- but if 30000 certificates wasn't enough to cause stack overflow, I don't know what would be enough to test this.

I can't attach the patch on Bugzilla due to the file size limit, and I don't think it should be committed anyway, since the test is so slow that it's not reasonable to add it to the test suite.

(In reply to Michael Catanzaro from comment #10)
> I'm quite surprised the third certificate (Baltimore CyberTrust Root) is not a > trusted root.

The root Baltimore CyberTrust Root certificate is self-signed. The Baltimore CyberTrust Root sent in the chain is issued by the legacy CA. So they are different certificates, but they have the same public key.

Created attachment 310298 [details] [review]
Add tests for construction of alternative trust paths

Carlos's patch makes this test pass.

(We still need to limit the recursion.)

(In reply to Michael Catanzaro from comment #12)
> (In reply to Dan Winship from comment #5)
> > it would be good to add tests for the case this patch is supposed to solve,
> > and for the infinite-loop case
> 
> For the infinite recursion -> stack overflow case... I tested Carlos's patch
> with a 40 MB certificate chain (30000 certificates). The test took over a
> minute to run, and didn't crash (so glib handled it properly). I think
> Carlos's patch must be wrong in theory -- we need a limit on the recursion
> if we recurse, since the attacker supplies the chain -- but if 30000
> certificates wasn't enough to cause stack overflow, I don't know what would
> be enough to test this.
> 
> I can't attach the patch on Bugzilla due to the file size limit, and I don't
> think it should be committed anyway, since the test is so slow that it's not
> reasonable to add it to the test suite.

We could limit the recursion to n certificates and add a test with n+1 certificates, I guess. What would be a reasonable limit?

Created attachment 310327 [details] [review]
Updated patch, limit the recursion to 10

I used 10 for now.

(In reply to Michael Catanzaro from comment #12)
> For the infinite recursion -> stack overflow case... I tested Carlos's patch
> with a 40 MB certificate chain (30000 certificates). The test took over a
> minute to run, and didn't crash (so glib handled it properly). I think
> Carlos's patch must be wrong in theory -- we need a limit on the recursion
> if we recurse, since the attacker supplies the chain -- but if 30000
> certificates wasn't enough to cause stack overflow, I don't know what would
> be enough to test this.

I used the same cert 30000 times, but to trigger the potential stack overflow in the original patch, I would have needed to create an actual valid chain of trust that long, so that test was bogus.

Created attachment 310377 [details] [review]
Add tests for construction of alternative trust paths

*** Bug 754135 has been marked as a duplicate of this bug. ***

*** Bug 755013 has been marked as a duplicate of this bug. ***

Comment on attachment 310327 [details] [review]
Updated patch, limit the recursion to 10

OK, I'm not convinced this is actually the right fix (as opposed to just matching anchors by key rather than by the entire cert), but eventually all of this code is going to go away and just use gnutls directly anyway, so let's commit it. Sorry I didn't get around to this before 2.46.0 went out.

Comment on attachment 310327 [details] [review]
Updated patch, limit the recursion to 10

Pushed to git master and glib-2-44 branches.

Should this get a unit test? That way I could check this issue on the openssl backend.

Comment on attachment 310377 [details] [review]
Add tests for construction of alternative trust paths

oops, there was a test case already, it just didn't get committed

(In reply to Dan Winship from comment #24)
> Comment on attachment 310377 [details] [review] [review]
> Add tests for construction of alternative trust paths
> 
> oops, there was a test case already, it just didn't get committed

Ah, sorry, I forgot about it.

Let's mark it as fixed now, then.

(In reply to Dan Winship from comment #21)
> Comment on attachment 310327 [details] [review] [review]
> Updated patch, limit the recursion to 10
> 
> OK, I'm not convinced this is actually the right fix (as opposed to just
> matching anchors by key rather than by the entire cert)

I thought it was matching by key. :/ That would indeed be the ideal approach.

Anyway, great to know this code is planned to be removed. I trust Nikos to keep GnuTLS as safe as any other TLS library, but this big mess of historical code in glib-networking adds significant risk.

(In reply to Michael Catanzaro from comment #27)
> I thought it was matching by key. :/ That would indeed be the ideal approach.

It is matching by key; otherwise, my test would not pass. :-)

(In reply to Ignacio Casal Quinteiro (nacho) from comment #23)
> Should this get a unit test? That way I could check this issue on the
> openssl backend.

Glad these are helping you. :)

(In reply to Michael Catanzaro from comment #28)
> (In reply to Michael Catanzaro from comment #27)
> > I thought it was matching by key. :/ That would indeed be the ideal approach.
> 
> It is matching by key; otherwise, my test would not pass. :-)

"it" here refers to GTlsDatabaseGnutls, and it does not match issuers by key, it matches by full certificate.

(In reply to Dan Winship from comment #29)
> "it" here refers to GTlsDatabaseGnutls, and it does not match issuers by
> key, it matches by full certificate.

Hm, that test case I added ensures that it matches issuers by the key. In the test case, a new certificate with the same key as a root certificate is generated, then issues an intermediate cert. The intermediate cert is accepted as valid, even though it is not issued by any of the root certs, but by a completely different cert that happens to use the same key as one of the root certs. So I think it is fine...?

(In reply to Michael Catanzaro from comment #30)
> (In reply to Dan Winship from comment #29)
> > "it" here refers to GTlsDatabaseGnutls, and it does not match issuers by
> > key, it matches by full certificate.
> 
> Hm, that test case I added ensures that it matches issuers by the key.

Sorry, I should have said "does not match anchors by key" there (like I did originally). It does find issuers by matching on the key (because that's the only way to find them). It does not match certificates in the presented chain against certificates in the CA list by matching on the key (which would allow it to directly match the Baltimore cert in Facebook's chain against the Baltimore cert in the CA list). Instead, it ends up effectively pretending that Facebook only sent the first certificate in the chain (and then seeing if it can find an issuer for it).

OK, not a huge difference, but my point is that there shouldn't need to be any recursion here.

But anyway, this hopefully won't matter soon because we'll get rid of all this code and just use gnutls.

Michael, thanks a lot for the tests, they are very appreciated. I just rebase the openssl branch and they seem to pass without any changes on the code which is kind of unexpected since the openssl backend follows quite a lot the logic from the gnutls one which makes me wonder if the tests are wrong or if this just works magically on the openssl side...

(In reply to Dan Winship from comment #31)
> OK, not a huge difference, but my point is that there shouldn't need to be
> any recursion here.

OK, I see.

(In reply to Ignacio Casal Quinteiro (nacho) from comment #32)
> Michael, thanks a lot for the tests, they are very appreciated. I just
> rebase the openssl branch and they seem to pass without any changes on the
> code which is kind of unexpected since the openssl backend follows quite a
> lot the logic from the gnutls one which makes me wonder if the tests are
> wrong or if this just works magically on the openssl side...

It's plausible that you indeed don't need any changes in the OpenSSL backend. OpenSSL and GnuTLS will both accept these chains by default. Our GnuTLS backend breaks this by manually parsing and reordering all the certificates (to allow for looking for issuers in the GTlsDatabase) before sending them to GnuTLS, which is why we need to handle this ourselves. I would have expected your code to be equivalently broken, but I guess your backend is just better than ours. :D  Using the GnuTLS backend, my test does not pass without Carlos's patch.