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 #4169

Merged
stephentoub merged 2 commits into from
Oct 29, 2015
Merged

Populate DataHash key size in SslConnectionInfo #4169

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
86dec92
Populate DataHash key size in SslConnectionInfo
shrutigarg Oct 27, 2015
02e3992
Fixed PR Comments and Synced code
shrutigarg Oct 29, 2015
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
3 changes: 2 additions & 1 deletion src/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Ssl.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,8 @@ internal static extern bool GetSslConnectionInfo(
out int dataCipherAlg,
out int keyExchangeAlg,
out int dataHashAlg,
out int dataKeySize);
out int dataKeySize,
out int hashKeySize);

[DllImport(Libraries.CryptoNative)]
internal static unsafe extern int SslWrite(SafeSslHandle ssl, byte* buf, int num);
Expand Down
10 changes: 7 additions & 3 deletions 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 @@ -12,7 +12,7 @@ internal class SslConnectionInfo
public readonly int DataCipherAlg;
public readonly int DataKeySize;
public readonly int DataHashAlg;
public readonly int DataHashKeySize = 0;
public readonly int DataHashKeySize;
public readonly int KeyExchangeAlg;
public readonly int KeyExchKeySize = 0;

Expand All @@ -26,11 +26,15 @@ internal SslConnectionInfo(SafeSslHandle sslContext)
out DataCipherAlg,
out KeyExchangeAlg,
out DataHashAlg,
out DataKeySize))
out DataKeySize,
out DataHashKeySize))
{
throw Interop.OpenSsl.CreateSslException(SR.net_ssl_get_connection_info_failed);
}
Copy link
Member

@eerhardt eerhardt Oct 27, 2015

Choose a reason for hiding this comment

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

Can we update the // TODO (Issue #3362) map key sizes comment below?

Copy link
Contributor Author

@shrutigarg shrutigarg Oct 28, 2015

Choose a reason for hiding this comment

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

Seems openssl doesnot provide a way to return a exchangekey size.. in dtls1_send_server_key_exchange and similar function for other sslProtocols , it internally calculates the key exchange size and generate key.. It is not a constant either that we can hardcode and return.
That's why its still pending .. not sure how to fix this.

Copy link
Member

@eerhardt eerhardt Oct 28, 2015

Choose a reason for hiding this comment

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

I think this would be good to note in the code. Also opening an issue for this and using that number in the TODO comment is a good idea since #3362 is closed and #4128 is going to come through and just delete these TODO comments.

Copy link
Contributor Author

@shrutigarg shrutigarg Oct 29, 2015

Choose a reason for hiding this comment

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

seems this todo we cannot proceed in , we have already done the investigations around. So just making a note for this. No point of opening another TODO.
Will delete this TODO comment.

// TODO (Issue #3362) map key sizes

//Openssl does not provide a way to return a exchange key size.
//It internally does calculate the key size before generating key to exchange
//It is not a constant (Algorthim specific) either that we can hardcode and return.
}

private SslProtocols MapProtocolVersion(string protocolVersion)
Expand Down
69 changes: 44 additions & 25 deletions src/Native/System.Security.Cryptography.Native/pal_ssl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,7 @@ enum class SSL_DataHashAlgorithm : int64_t
#endif
};

static HashAlgorithmType MapHashAlgorithmType(const SSL_CIPHER* cipher)
static void GetHashAlgorithmTypeAndSize(const SSL_CIPHER* cipher, HashAlgorithmType* dataHashAlg, DataHashSize* hashKeySize)
{
unsigned long mac;
#if HAVE_SSL_CIPHER_SPLIT_ALGORITHMS
Expand All @@ -377,42 +377,59 @@ static HashAlgorithmType MapHashAlgorithmType(const SSL_CIPHER* cipher)
SSL_DataHashAlgorithm sslMac = static_cast<SSL_DataHashAlgorithm>(mac);
switch (sslMac)
{
case SSL_DataHashAlgorithm::SSL_MD5:
return HashAlgorithmType::Md5;
case SSL_DataHashAlgorithm::SSL_MD5:
*dataHashAlg = HashAlgorithmType::Md5;
*hashKeySize = DataHashSize::MD5_HashKeySize;
return;

case SSL_DataHashAlgorithm::SSL_SHA1:
return HashAlgorithmType::Sha1;
case SSL_DataHashAlgorithm::SSL_SHA1:
*dataHashAlg = HashAlgorithmType::Sha1;
*hashKeySize = DataHashSize::SHA1_HashKeySize;
return;

#if HAVE_SSL_CIPHER_SPLIT_ALGORITHMS
case SSL_DataHashAlgorithm::SSL_GOST94:
return HashAlgorithmType::SSL_GOST94;

case SSL_DataHashAlgorithm::SSL_GOST89MAC:
return HashAlgorithmType::SSL_GOST89;

case SSL_DataHashAlgorithm::SSL_SHA256:
return HashAlgorithmType::SSL_SHA256;

case SSL_DataHashAlgorithm::SSL_SHA384:
return HashAlgorithmType::SSL_SHA384;

case SSL_DataHashAlgorithm::SSL_AEAD:
return HashAlgorithmType::SSL_AEAD;
case SSL_DataHashAlgorithm::SSL_GOST94:
*dataHashAlg = HashAlgorithmType::SSL_GOST94;
*hashKeySize = DataHashSize::GOST_HashKeySize;
return;

case SSL_DataHashAlgorithm::SSL_GOST89MAC:
*dataHashAlg = HashAlgorithmType::SSL_GOST89;
*hashKeySize = DataHashSize::GOST_HashKeySize;
return;

case SSL_DataHashAlgorithm::SSL_SHA256:
*dataHashAlg = HashAlgorithmType::SSL_SHA256;
*hashKeySize = DataHashSize::SHA256_HashKeySize;
return;

case SSL_DataHashAlgorithm::SSL_SHA384:
*dataHashAlg = HashAlgorithmType::SSL_SHA384;
*hashKeySize = DataHashSize::SHA384_HashKeySize;
return;

case SSL_DataHashAlgorithm::SSL_AEAD:
*dataHashAlg = HashAlgorithmType::SSL_AEAD;
*hashKeySize = DataHashSize::Default;
return;
#endif
}

return HashAlgorithmType::None;
*dataHashAlg = HashAlgorithmType::None;
*hashKeySize = DataHashSize::Default;
return;
}
Copy link
Member

@stephentoub stephentoub Oct 27, 2015

Choose a reason for hiding this comment

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

What about the default case where sslMac doesn't match any of these?

Copy link
Contributor Author

@shrutigarg shrutigarg Oct 28, 2015

Choose a reason for hiding this comment

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

Will add at end to return 0 for rest of the cases.


extern "C" int32_t GetSslConnectionInfo(SSL* ssl,
CipherAlgorithmType* dataCipherAlg,
ExchangeAlgorithmType* keyExchangeAlg,
CipherAlgorithmType* dataCipherAlg,
ExchangeAlgorithmType* keyExchangeAlg,
HashAlgorithmType* dataHashAlg,
int32_t* dataKeySize)
int32_t* dataKeySize,
DataHashSize* hashKeySize)
{
const SSL_CIPHER* cipher;

if (!ssl || !dataCipherAlg || !keyExchangeAlg || !dataHashAlg || !dataKeySize)
if (!ssl || !dataCipherAlg || !keyExchangeAlg || !dataHashAlg || !dataKeySize || !hashKeySize)
{
goto err;
}
Expand All @@ -425,8 +442,8 @@ extern "C" int32_t GetSslConnectionInfo(SSL* ssl,

*dataCipherAlg = MapCipherAlgorithmType(cipher);
*keyExchangeAlg = MapExchangeAlgorithmType(cipher);
*dataHashAlg = MapHashAlgorithmType(cipher);
*dataKeySize = cipher->alg_bits;
GetHashAlgorithmTypeAndSize(cipher, dataHashAlg, hashKeySize);

return 1;

Expand All @@ -441,6 +458,8 @@ extern "C" int32_t GetSslConnectionInfo(SSL* ssl,
*dataHashAlg = HashAlgorithmType::None;
if (dataKeySize)
*dataKeySize = 0;
if (hashKeySize)
*hashKeySize = DataHashSize::Default;

return 0;
}
Expand Down
16 changes: 15 additions & 1 deletion src/Native/System.Security.Cryptography.Native/pal_ssl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
#include "pal_crypto_types.h"

#include <openssl/ssl.h>
#include <openssl/md5.h>
#include <openssl/sha.h>

/*
These values should be kept in sync with System.Security.Authentication.SslProtocols.
Expand Down Expand Up @@ -88,6 +90,16 @@ enum class HashAlgorithmType : int32_t
SSL_AEAD = 229412,
};

enum class DataHashSize : int32_t
{
MD5_HashKeySize = 8 * MD5_DIGEST_LENGTH,
SHA1_HashKeySize = 8 * SHA_DIGEST_LENGTH,
SHA256_HashKeySize = 8 * SHA256_DIGEST_LENGTH,
SHA384_HashKeySize = 8 * SHA384_DIGEST_LENGTH,
GOST_HashKeySize = 256,
Default = 0,
};

enum SslErrorCode : int32_t
{
PAL_SSL_ERROR_NONE = 0,
Expand Down Expand Up @@ -214,11 +226,13 @@ Returns the connection information for the SSL instance.

Returns 1 upon success, otherwise 0.
*/

extern "C" int32_t GetSslConnectionInfo(SSL* ssl,
CipherAlgorithmType* dataCipherAlg,
ExchangeAlgorithmType* keyExchangeAlg,
HashAlgorithmType* dataHashAlg,
int32_t* dataKeySize);
int32_t* dataKeySize,
DataHashSize* hashKeySize);

/*
Shims the SSL_write method.
Expand Down
16 changes: 3 additions & 13 deletions src/System.Net.Security/src/System/Net/SslStreamPal.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ public static SecurityStatusPal DecryptMessage(SafeDeleteContext securityContext

public static SafeFreeContextBufferChannelBinding QueryContextChannelBinding(SafeDeleteContext phContext, ChannelBindingKind attribute)
{
// TODO (Issue #3362) To be implemented
// TODO (Issue #3954) To be implemented
throw NotImplemented.ByDesignWithMessage(SR.net_MethodNotImplementedException);
}

Expand All @@ -75,19 +75,9 @@ public static void QueryContextStreamSizes(SafeDeleteContext securityContext, ou
streamSizes = new StreamSizes();
}

public static int QueryContextConnectionInfo(SafeDeleteContext securityContext, out SslConnectionInfo connectionInfo)
public static void QueryContextConnectionInfo(SafeDeleteContext securityContext, out SslConnectionInfo connectionInfo)
Copy link
Member

@stephentoub stephentoub Oct 27, 2015

Choose a reason for hiding this comment

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

If we're no longer returning an int, any reason not to change the out parameter to be the return value?

Copy link
Member

@eerhardt eerhardt Oct 27, 2015

Choose a reason for hiding this comment

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

This is a PAL function, so the signature needs to match Windows. (Windows currently has a void return). If we did this we would need to update the Windows PAL function and the call site. (Not saying that we can't do that though...)

Copy link
Member

@bartonjs bartonjs Oct 27, 2015

Choose a reason for hiding this comment

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

That's why I like PAL interfaces vs PAL static implicit call conventions, personally.

{
connectionInfo = null;
try
{
connectionInfo = new SslConnectionInfo(securityContext.SslContext);

return 0;
}
catch
{
return -1;
}
connectionInfo = new SslConnectionInfo(securityContext.SslContext);
}

private static SecurityStatusPal HandshakeInternal(SafeFreeCredentials credential, ref SafeDeleteContext context,
Expand Down