Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Populate DataHash key size in SslConnectionInfo #3916

Closed
shrutigarg wants to merge 1 commit into from
Closed

Populate DataHash key size in SslConnectionInfo #3916

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 40 additions & 1 deletion src/Common/src/Interop/Unix/libssl/SslConnectionInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,13 @@ internal class SslConnectionInfo
internal const int SSL_GOST89 = 229411;
internal const int SSL_AEAD = 229412;

//Data Hash Key Sizes
internal const int MD5_HashKeySize = 128;
internal const int SHA1_HashKeySize = 160;
internal const int SHA256_HashKeySize = 256;
internal const int SHA384_HashKeySize = 384;
internal const int GOST_HashKeySize = 256;

public readonly int Protocol;
public readonly int DataCipherAlg;
public readonly int DataKeySize = 0;
Expand All @@ -46,7 +53,12 @@ internal SslConnectionInfo(Interop.libssl.SSL_CIPHER cipher, string protocol)
KeyExchangeAlg = (int) MapExchangeAlgorithmType((Interop.libssl.KeyExchangeAlgorithm) cipher.algorithm_mkey);
DataHashAlg = (int) MapHashAlgorithmType((Interop.libssl.DataHashAlgorithm) cipher.algorithm_mac);
DataKeySize = cipher.alg_bits;
// TODO (Issue #3362) map key sizes
DataHashKeySize = GetHashAlgorithmBitSize((Interop.libssl.DataHashAlgorithm) cipher.algorithm_mac);
Copy link
Member

@stephentoub stephentoub Oct 16, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove the = 0 from the declaration of DataHashKeySize earlier.

Copy link
Contributor Author

@shrutigarg shrutigarg Oct 16, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep , will fix


// TODO (Issue #3362) Map keyExchKeySize.
//There is no way we can get the key exchange size from the Openssl.
//It internally generates the key size in *_send_client_key_exchange method on the basis of the algorithm used.

}

private SslProtocols MapProtocolVersion(string protocolVersion)
Expand Down Expand Up @@ -190,5 +202,32 @@ private static HashAlgorithmType MapHashAlgorithmType(Interop.libssl.DataHashAlg
return HashAlgorithmType.None;
}
}

private static int GetHashAlgorithmBitSize(Interop.libssl.DataHashAlgorithm mac)
{
switch (mac)
{
case Interop.libssl.DataHashAlgorithm.SSL_MD5:
return MD5_HashKeySize;

case Interop.libssl.DataHashAlgorithm.SSL_SHA1:
return SHA1_HashKeySize;

case Interop.libssl.DataHashAlgorithm.SSL_GOST94:
return GOST_HashKeySize;

case Interop.libssl.DataHashAlgorithm.SSL_GOST89MAC:
return GOST_HashKeySize;

case Interop.libssl.DataHashAlgorithm.SSL_SHA256:
return SHA256_HashKeySize;

case Interop.libssl.DataHashAlgorithm.SSL_SHA384:
return SHA384_HashKeySize;

default:
return 0;
Copy link
Member

@stephentoub stephentoub Oct 16, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if another algorithm ends up being used and we return 0 here?

Copy link
Contributor Author

@shrutigarg shrutigarg Oct 16, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stephentoub apart from the macs mentioned there is one more mac which is AEAD .. which is as per the explanation in Openssl is not a real mac .
AEAD is only used with *GCM Cipher Algorithm.. if *GCM encryption algorthim (cipher algo) is used, it typically does both the work encryption and hashing.

"TLSv1.2 adds a new class of ciphers using AEAD modes such as GCM, which is a single crypto operation that does both encryption and integrity protection, instead of having separate encryption and HMAC e.g. AES128-CBC plus HMAC-SHA1. Mac=AEAD means really no HMAC is needed because of AEAD."

So I think returning some default thing would be the better idea.

Copy link
Member

@stephentoub stephentoub Oct 19, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bartonjs, opinion?

Copy link
Member

@bartonjs bartonjs Oct 19, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CIPop wrote a little tool for printing the details of an SSL connection.

On the server side I ran openssl s_server -cert eccombined.pem -cipher ECDHE-ECDSA-AES128-GCM-SHA256 -accept 8000, forcing it into a GCM connection. The Windows connection information says

        PROTOCOL=Tls12 , KEYEXCHG=( 256bit      44550), CYPHER=( 128bit     Aes128), HASH=(   0bit      32780)

I boosted it a bit:

 KeyEx: Class=5 Type=7 Sid=6
Cipher: Class=3 Type=3 Sid=14
  Hash: Class=4 Type=0 Sid=12

The hash appears to be reported as a 0-bit strong hash (class=4) any (type=0) SHA-256 (sid=12). The relevant part for this function is that it agrees that the hash size should be 0 for AEAD.

What's probably better is to report 0 for AEAD (# define SSL_AEAD 0x00000040L) and -1 or throw, or something on otherwise unknown inputs.

Copy link
Member

@bartonjs bartonjs Oct 19, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be sure this is doing something sensible on OSX.

Our struct looks like OpenSSL Master, but might be different than OpenSSL 0.9.8zg (which is what OSX has). I'm not sure that we're doing anything sensible on OSX.

Copy link
Contributor Author

@shrutigarg shrutigarg Oct 20, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bartonjs : yes, to make it work in veriosn0.9.8 we would have to change the logic for other algorithms too . keyEx and DataCipher.. ..
Btw I don't find "SSL_CIPHER_get_digest_nid " in 1.0.* existing versions. Which version is this present?
Instead we can use

#if IMNOTSUREWHAT // OpenSSL 0.9.8
switch (cipher->algorithms & SSL_MAC_MASK)
{
case SSL_MD5:
cipherMac = NID_md5;
break;
case SSL_SHA1:
cipherMac = NID_sha1;
break;
default:
// Handle this unknown case
break;
}
#else
switch (cipher->algorithms_mac)
{
//do mapping
}
#endif
..
otherwise we would have to maintain 2 #if s ..?

Copy link
Member

@eerhardt eerhardt Oct 20, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a similar macro to indicate "modern libssl" so that #if can be inverted?

There aren't any macros/defines at all yet that I'm aware of, I was suggesting we create one named OPENSSL_0_9_8 to use on OSX.
If you wanted it inverted, you could use #ifndef OPENSSL_0_9_8. I'm not sure how many versions of OpenSsl we are planning on supporting at one time. I would assume we are supporting 0.9.8 for use on OSX, and the last known good version for use on Linux. On my Ubuntu machine, it looks like I have 1.0.1. @bartonjs - can you confirm?

Btw I don't find "SSL_CIPHER_get_digest_nid " in 1.0.* existing versions. Which version is this present?

According to https://www.openssl.org/docs/manmaster/ssl/SSL_CIPHER_get_name.html, it exists in the latest, i.e. "master", version but not in 1.0.2 or 1.0.1.

Copy link
Member

@eerhardt eerhardt Oct 20, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vijaykota and @shrutigarg - Please see the changes I am proposing at eerhardt@4fc8515 to support differing APIs in OpenSSL.

Copy link
Member

@eerhardt eerhardt Oct 21, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI - see #4026 for how I shimmed the SSL_CIPHER struct.

Copy link
Contributor Author

@shrutigarg shrutigarg Oct 27, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

closing this and sending a new PR for this after shim update

}
}
}
}