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

Proto_IkePacket.c: fix MD5, SHA-256, SHA-384 and SHA-512 implementation #694

Merged
merged 1 commit into from
Sep 11, 2018

Conversation

davidebeatrici
Copy link
Member

@davidebeatrici davidebeatrici commented Sep 10, 2018

Fixes #552.

#294 added SHA-256, SHA-384, and SHA-512 support to the protocol, but part of it was removed in faee11f, because it caused a buffer over-read crash.

It also broke the MD5 implementation because the switch-case block doesn't handle the type anymore:

switch (h->HashId)
{
case IKE_HASH_SHA1_ID:
case IKE_HASH_SHA2_256_ID:
hmac_block_size = HMAC_BLOCK_SIZE;
break;
case IKE_HASH_SHA2_384_ID:
case IKE_HASH_SHA2_512_ID:
hmac_block_size = HMAC_BLOCK_SIZE_1024;
break;
default:
return;
}

This pull request fixes all the implementations and improves the IkeHMac() function by using the dedicated hashing functions.


SoftEther VPN Patch Acceptance Policy:
http://www.softether.org/5-download/src/9.patch

I choose option 1.

@davidebeatrici davidebeatrici changed the title Proto_IkePacket.c: fix SHA-256, SHA-384 and SHA-512 implementation Proto_IkePacket.c: fix MD5, SHA-256, SHA-384 and SHA-512 implementation Sep 11, 2018
Copy link
Contributor

@paulmenzel paulmenzel left a comment

Choose a reason for hiding this comment

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

Thank you for tracking this down.

// Validate arguments
if (h == NULL || dst == NULL || (key == NULL && key_size != 0) || (data == NULL && data_size != 0))
{
return;
}

switch (h->HashId)
{
switch(h->HashId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

No idea if the coding style guidelines changed, but let’s keep the existing coding style for consistency?

Copy link
Member

Choose a reason for hiding this comment

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

shall we add .clang-format with "BreakBeforeBraces: Stroustrup" ? (or any other value, I do not care personally)

and make travis-ci check the format

Copy link
Member Author

Choose a reason for hiding this comment

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

No idea if the coding style guidelines changed, but let’s keep the existing coding style for consistency?

Thank you, I wrote it like this by mistake.

shall we add .clang-format with "BreakBeforeBraces: Stroustrup" ? (or any other value, I do not care personally)

and make travis-ci check the format

I think that we should create the clang-format configuration according to the current code style, as even moving the brackets would require a lot of changes throughout the code.

return;
}
else if (h->HashId == IKE_HASH_MD5_ID)
if (md == NULL)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Can a debug message be printed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Pull request SoftEtherVPN#294 added SHA-256, SHA-384, and SHA-512 support to the protocol, but part of it was removed in faee11f, because it caused a buffer over-read crash.

It also broke the MD5 implementation because the switch-case block didn't handle the type anymore.

This pull request fixes all the implementations and improves the IkeHMac() function by using the dedicated hashing functions.
@davidebeatrici davidebeatrici merged commit 64b68c1 into SoftEtherVPN:master Sep 11, 2018
@slytimer
Copy link

Hi. It seems that it doesn't work again on windows xp. Build 9744

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Windows XP L2TP/IPsec connection not working with recent SoftEther VPN Server versions
4 participants