[Click] Click Ipsec

[Eddie Kohler]

(But note that SHA_CBLOCK is less than "the whole pad", which is 128B!
I don't know whether that too is an error.

On Mon, Jan 30, 2012 at 3:10 PM, Eddie Kohler <ekohler at gmail.com> wrote:
> 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