[Click] Click Ipsec

[Eddie Kohler]

Hi Markku,

One of Click's great benefits is that its modularity supports code
from different authors... unfortunately that means it's not all
equally well tested. :(

It looks to me like this code was created by taking code from openssl
and then modifying it by "inlining" some calls out to other functions.
For example the OpenSSL version is

		for (i=0; i<HMAC_MAX_MD_CBLOCK; i++)
			pad[i]=0x5c^ctx->key[i];
		if (!EVP_DigestInit_ex(&ctx->o_ctx,md, impl))
			goto err;
		if (!EVP_DigestUpdate(&ctx->o_ctx,pad,EVP_MD_block_size(md)))
			goto err;

and the Click

		for (i=0; i<HMAC_MAX_MD_CBLOCK; i++)
			pad[i]=0x5c^ctx->key[i];
		SHA1_init(&ctx->o_ctx);
		SHA1_update(&ctx->o_ctx,pad,SHA_BLOCK);

And as you guessed, EVP_MD_block_size(md) for the EVP_sha1() structure
is SHA_CBLOCK, not SHA_BLOCK.

Would love it if you created a pull request on github with this fix.

It would be great to have some ipsec tests, or to go back & redo the
sfotware engineering here -- it's often a mistake to copy *and modify*
code to this degree. But at least the pull request is in the right
direction.
Eddie


On Mon, Jan 30, 2012 at 2:48 AM, Markku Savela <Markku.Savela at vtt.fi> wrote:
> Even if based on Openssl, it appears to be some copy/paste error
> then. In sha1_impl
>
> #define SHA_CBLOCK      64
> #define SHA_LBLOCK      16
> #define SHA_BLOCK       16
>
> The code I proposed to be fixed is feeding the HMAC pads into hash
> algorithm. Obviously, the whole pad (SHA_CBLOCK == 64) should be
> fed in, instead of only the first 16 bytes (SHA_BLOCK).
>
>
> On 01/27/2012 05:32 PM, Dimitris Syrivelis wrote:
>> Hi Markku,
>>
>> The code fragment you are referring to, is copied as is from Eric Young's
>> Openssl library Implementation. In comments there is a notice about using this
>> library and i confirm that this code fragment is from there. The last time i
>> checked, IPsec flow was working on click ver 1.8. You have to set up a
>> configuration that uses SA tables as depicted in the example
>> simple_ipsec.click
>> and documentation.
>>
>> Dimitris
>>
>>> Hi,
>>>
>>> Has anyone actually worked on those elements? I just tried the HMAC,
>>> and couldn't get it to match my other implementation. On quick browse,
>>> it looks like it's using wrong constant in few places (SHA_BLOCK where
>>> SHA_CBLOCK should be?). Haven't really tried this yet -- will return
>>> to issue next week...
>>>
>>>
>>>
>>> @@ -97,12 +97,12 @@ void HMAC_Init_ex(HMAC_CTX *ctx, const void *key,
>>> int len)
>>>              for (i=0; i<HMAC_MAX_MD_CBLOCK; i++)
>>>                      pad[i]=0x36^ctx->key[i];
>>>              SHA1_init(&ctx->i_ctx);
>>> -            SHA1_update(&ctx->i_ctx,pad,SHA_BLOCK);
>>> +            SHA1_update(&ctx->i_ctx,pad,SHA_CBLOCK);
>>>
>>>              for (i=0; i<HMAC_MAX_MD_CBLOCK; i++)
>>>                      pad[i]=0x5c^ctx->key[i];
>>>              SHA1_init(&ctx->o_ctx);
>>> -            SHA1_update(&ctx->o_ctx,pad,SHA_BLOCK);
>>> +            SHA1_update(&ctx->o_ctx,pad,SHA_CBLOCK);
>>>              }
>>>      memcpy((void *)&ctx->md_ctx,(void*)&ctx->i_ctx,sizeof(SHA1_ctx));
>>>      }
>>>
>>> _______________________________________________
>>> click mailing list
>>> click at amsterdam.lcs.mit.edu
>>> https://amsterdam.lcs.mit.edu/mailman/listinfo/click
>>>
>>
>>
>
> _______________________________________________
> click mailing list
> click at amsterdam.lcs.mit.edu
> https://amsterdam.lcs.mit.edu/mailman/listinfo/click