Bug 36324 - SSLStream sends incorrect intermediate certificate chain, when Authenticates as server
Summary: SSLStream sends incorrect intermediate certificate chain, when Authenticates ...
Status: RESOLVED FIXED
Alias: None
Product: Class Libraries
Classification: Mono
Component: Mono.Security ()
Version: 4.2.0 (C6)
Hardware: Macintosh Mac OS
: --- normal
Target Milestone: Untriaged
Assignee: Martin Baulig
URL:
Depends on:
Blocks:
 
Reported: 2015-11-28 05:52 UTC by toin
Modified: 2017-12-11 20:04 UTC (History)
3 users (show)

Tags:
Is this bug a regression?: ---
Last known good build:


Attachments
Patch for this bug (1.15 KB, application/octet-stream)
2015-11-28 11:20 UTC, toin
Details


Notice (2018-05-24): bugzilla.xamarin.com is now in read-only mode.

Please join us on Visual Studio Developer Community and in the Xamarin and Mono organizations on GitHub to continue tracking issues. Bugzilla will remain available for reference in read-only mode. We will continue to work on open Bugzilla bugs, copy them to the new locations as needed for follow-up, and add the new items under Related Links.

Our sincere thanks to everyone who has contributed on this bug tracker over the years. Thanks also for your understanding as we make these adjustments and improvements for the future.


Please create a new report on GitHub or Developer Community with your current version information, steps to reproduce, and relevant error messages or log files if you are hitting an issue that looks similar to this resolved bug and you do not yet see a matching new report.

Related Links:
Status:
RESOLVED FIXED

Description toin 2015-11-28 05:52:04 UTC
Current situation
-----------------
mono sends a random intermediate certificate chain containing N-1 intermediate certificates, where N is the number of total intermediate certificates in the certificate store.

This leads to 2 problems:
1) 
If you have 20 different intermediate certificates in the store and authenticate as a server to the client (AuthenticateAsServer), the server sends 19 intermediate certificates as an unlogically chain. Sort of funny; it even crashes WebKit on an iPad, btw :-p

2) If the server certificate is signed by the last intermediate certificate in the store, the client can't build it's own chain at all, cauz - even if it receives 19 random intermediate certificates as a chain, the intermediate certificate, which is the issuer of the server certificate wasn't send at all.

So mono's current implementation of the handling with intermediate certificate's misses the whole point of the concept of a trusted chain...


Correct Situation
-----------------
The chain of intermediate certificates must only contain the issuer of the server certificate as well as the issuer(s) of the "following" intermediate certificates except the root certificate,

Example:
Root CA
|_ Intermediate CA Class 1
   |_ Intermediate CA Class 1 For Server Certificates only
      |_ The Server Certificate

Expected chain is:
Intermediate CA Class 1 => Intermediate CA Class 1 Server Certificates only => The Server Certificate

Cauz now, the client can check:
1)Oh, The server certificate is signed by "Intermediate CA Class 1 Server Certificates", which I don't know, but, hey, it's in the chain provided by the server!

2)Oh, and this intermediate certificate is signed by "Intermediate CA Class 1", which I still don't know, but, hey, it's in the chain, too!

3)Oh, and this intermediate certificate is signed by "Root CA", which is NOT in the chain, but it's a Root Certificate, which is marked as Trusted in my own certificate store. So I trust also all intermediate certificates as well as the server certificate!

So even, if they are 200 certificates in the intermediate CA store of the server, the client is only interested in a chain, which leads from the received server certificate down to a CA-Certificate, which the client knows as "Trusted"; so moslty a Root CA certificate.


What todo?
----------
I see two changes that are required here:

1) /mcs/class/corlib/Mono.Security.X509/X509Chain.cs
The class should contain - same as the class /mcs/class/System/System.Security.Cryptography.X509Certificates/X509Chain.cs - a public property: 

public X509ChainElementCollection ChainElements { get; }
(Could return a normal X509CertificateCollection, too)

Implementation must be equal to the implementation of X509Chain in System.Security.Cryptography.X509Certificates. If the chain is build via "Build( LeafCertificate )", it should store the concret chain, which leads to that leaf certificate in ChainElements. So, the whole code is already correctly implemented in System.Security.Cryptography.X509Certificates/X509Chain.cs and should be pretty easy to adopt.

2) /mcs/class/Mono.Security/Mono.Security.Protocol.Tls/ServerContext.cs
The constructor of this class is the place, where the certificate chain is setup, build and used in the ServerSettings for the authentication. At the moment, it's doing it wrong with a 

for (int j = chain.Chain.Count - 1; ... ) 

loop. Which is wrong, because it iterates over ALL!!!! intermediate certificates in the store (exept the last one) and adds them... The correct loop must go over the newly created property ChainElements, because this are the only chain elements, which the client is interested in and - it contains for sure - the intermediate certificate which is the issuer of the server certificate, otherwise the previous Build( LeafCertificate ) wouldn't return true at all!

Warning: If I remember correctly, the ChainElements of X509Chain are already in the correct order, so I think, the loop over ChainElements should go from 1 to {END} instead of {END} to 1. But I am not 100% sure about it. Never checked in the RFC anyway, if the chain must sent in some sort of "correct" order, or only contains all required certificates anyhow. But I never saw a "real world" server, which doesn't provides the chain in the "correct" logically order for the client and it makes debugging way easier, if the chain gets longer...


Any help?
--------
I'm pretty sure, I'll patch it in the next days for my own usage, cauz I really need it to be fixed; it's sort of a show stopper for me. Never worked with github and co; is it okay to post a patch file here and someone will review and hopefully merge it?!?
Comment 1 toin 2015-11-28 06:45:09 UTC
oh, sorry for the whole mis-information in "what todo"
maybe, I should first start reading sources and then write "what todo's" :-p
shame on me!

the property Chain in Mono.Security.X509/X509Chain.cs is already a "sort of" ChainElements property. Dunno, why don't name things equal; makes debugging lots easier.

Problem is, that in Mono.Security.Protocol.Tls/ServerContext.cs it's get initialized in the constructor with ALL intermediate certificates:

//This is wrong, cauz it fills the private _chain variable with ALL Intermediate Certificates
MonoX509.X509Chain chain = new MonoX509.X509Chain (MonoX509.X509StoreManager.IntermediateCACertificates);


Correct code must be:

//This will initialize an empty _chain variable
MonoX509.X509Chain chain = new MonoX509.X509Chain ();

//initializes certs variable (= an pool of all certificates, which could be used for building the final chain)
chain.LoadCertificates( MonoX509.X509StoreManager.IntermediateCACertificates )

//on Build( leafCertificate ), the _chain will build correctly with the use of all intermediate certificates in certs

The loop is correct in this implementation, btw.
Comment 2 toin 2015-11-28 11:20:52 UTC
Created attachment 14022 [details]
Patch for this bug

Added the patch file
Comment 3 Andi McClure 2015-12-07 18:55:28 UTC
Martin, could you take a look at this?
Comment 4 Martin Baulig 2016-11-11 10:11:21 UTC
Bump, need to check whether this problem still exists.
Comment 5 Martin Baulig 2017-12-11 20:04:17 UTC
Old bug from 2015 using legacy code that doesn't exist anymore.