Subject: Re: [Drip] Implementation questions from students

From: Andrei Gurtov <[email-masked]>
Date: Thu, 18 Feb 2021 17:23:59 +0100

Hej

Final reports from students from fall 2020 on HIPv2 code update and DRIP
implementation are available here

https://www.ida.liu.se/~TDDE21/info/TDDE21_HIPv2_finalreport_2020.pdf

https://www.ida.liu.se/~TDDE21/info/TDDE21_DRIP_finalreport_2020.pdf

There are a few theses ongoing in Spring 2021 term as well.

br Andrei

On 10/12/2020 10:43 PM, Wiethuechter, Adam wrote:
> Andrei,
>
> Comments in line from me as I can respond. Note that this is all
> personal opinion of how I would approach the given problems.
>
> On Mon, Oct 12, 2020 at 5:35 AM Andrei Gurtov <[email-masked]
> <mailto:[email-masked]>> wrote:
>
>     Hello,
>
>     A few questions from a group of students working on DRIP
>     implementation. I guess mostly to Bob. Or it should be sent to
>     hipsec instead?
>
>     br Andrei
>
>
>     -------- Forwarded Message --------
>
>     	
>
>     	
>
>     	
>
>     	
>
>     	
>
>     We had some questions about the HIP project that you could perhaps
>     forward to the correct person:
>
>      1. In the function /build_tlv_hmac()/ in
>         src/protocol/hip_output.c, there is the following bit of code:
>
>         /* get lower 160-bits of HMAC computation */
>
>          memcpy(hmac->hmac,
>
>                  &hmac_md[hmac_md_len-sizeof(hmac->hmac)],
>
>                  sizeof(hmac->hmac));
>
>         (https://bitbucket.org/openhip/openhip/src/37972774633783905ec4f7961d2a5476092ed529/src/protocol/hip_output.c#lines-1373
>         <https://bitbucket.org/openhip/openhip/src/37972774633783905ec4f7961d2a5476092ed529/src/protocol/hip_output.c#lines-1373>)
>
>         The expression used as index into /hmac_md//[]/ seems to be
>         calculated erroneously, as /sizeof(hmac->hmac)/ evaluates to
>         64 bytes and /hmac_md_len/ is the output size of the hash
>         function used. Since SHA256 for example produces 256 bits / 8
>         = 32 bytes as output, this calculation evaluates to 32 - 64 =
>         -32 when treated as signed integers. This results in the data
>         between /hmac_md[-32]/ and /hmac_md[32]/ being read, which
>         reads out of bounds for the array. Using GDB we have observed
>         that this causes other values located on the stack (such as
>         /hmac_md_len/) to be copied into /hmac->hmac//./ Also, per RFC
>         7401 (HIPv2), "The size of the HMAC is the natural size of the
>         hash computation output depending on the used hash function".
>         Thus there does not seem to be a need to truncate this value
>         to 160 bits? At least one implementation
>         (https://www.cryptosys.net/manapi/api_kmac.html
>         <https://www.cryptosys.net/manapi/api_kmac.html>) of the KMAC
>         function (that we have added as an option to HMAC) also states
>         that the output of the KMAC function cannot be truncated when
>         used as a message authentication code.
>
> Judging by how you described the failure it seems like this is a
> change that happened and no one has caught? Someone took the time to
> do this grab of the lower 160 bits because they got more but now it
> sounds like the functions always return what you need (eliminating the
> need for truncation as you said).
>
> I would ask the openhip devs this as I am unclear exactly why this
> would still be used myself.
>  
>
>
>      1. When parsing incoming R1 packets (and probably some other
>         types as well), the peer's HIT from the packet seems to be
>         validated against a locally computed HIT from the peer's HI.
>         This occurs before any parameters other than the puzzle, host
>         ID, and signature have been parsed. In /hip_parse_R1()/,
>         /validate_hit()/ uses /hi_to_hit()/ to compute this and
>         compares the returned value to the one received from the peer.
>         However, since the HIT_SUITE_LIST parameter has not yet been
>         parsed, the variable /hip_a->peer_hi->hit_suite_id/ still has
>         its default value of 0. There is a fallback in /hi_to_hit()/
>         which silently uses SHA256 as the hashing algorithm for
>         creating a HIT from a HI if the HIT suite is set to an invalid
>         value, which enables the verification to succeed if the
>         received HIT happens to also be calculated with SHA256. Other
>         algorithms (such as cSHAKE which we are adding) will cause
>         this to fail, as /hip_a->peer_hi->hit_suite_id/ is only set
>         correctly after the HIT has been verified. We have worked
>         around this issue by first ignoring all parameters other than
>         the HIT_SUITE_LIST (thus ensuring that the HIT suite variable
>         is set before any other parameters are parsed), then
>         continuing with parsing the signature-related parameters and
>         finally the others. Is this an acceptable solution to this
>         problem? Those silent fallbacks seem to cause more problems
>         that they solve, maybe they should be removed or at least emit
>         a warning when triggered?
>
> Your proposed solution is the way I would go about it. Start from the
> atomic values and build from there.
>
>     1.
>
>
>      2. What should the lengths for the key and IV be for River/Lake
>         Keyak? It seems that these lengths are variable and can be
>         chosen at runtime.
>
>      3. There does not seem to be any support for EdDSA25519ph or
>         EdDSA448ph in OpenSSL, and we cannot find any C
>         implementations from a reputable source. Would it be OK if we
>         held off on adding these for the time being? EdDSA25519 and
>         EdDSA448 are implemented in OpenSSL 1.1.1 and seem to work for
>         use in OpenHIP.
>
>
> These were nice to haves I think - so no need to hurt your students
> trying to crowbar them in if nothing exists for them.
>  
>
>     1.
>
>
>      2. In section 6 of
>         https://www.ietf.org/id/draft-moskowitz-hip-new-crypto-05.html
>         <https://www.ietf.org/id/draft-moskowitz-hip-new-crypto-05.html>,
>         there is a comment about how SHAKE, cSHAKE or KMAC could be
>         used as a pseudo-random generator. Should we do something with
>         this information, or is it just there for future reference?
>         Isn't randomness usually handled by the kernel since it has
>         access to higher-quality entropy sources, and if so, is there
>         any place in the OpenHIP code where it would be beneficial to
>         use SHAKE/cSHAKE/KMAC for this purpose?
>
>      3. Should the new encryption- and signature algorithms we are
>         adding be used as default in OpenHIP or just be available as a
>         command line option?
>
> Personally I would put it as a command line option. However others may
> be more inclined to default to the newer crypto for those who are just
> blasting through the setup. I think if you made them all command line
> options then just have the fallback clause be the newer crypto option
> that would kill both birds with one stone. I will need to check the
> code to see if this is even possible but I assume it would be.
>
> I always assume that the user is in a rush, lazy and ignorant of the
> best option (AKA me when I start using my own tools under pressure).
>
>     1.
>
>
>      2. (This is mostly a minor thing) When we are adding KMAC as an
>         alternative to HMAC, is it proper to put this into a function
>         called /build_tlv_*hmac*()/? Seeing as this function will then
>         sometimes calculate a HMAC and other times a KMAC, should it
>         be renamed to /build_tlv_mac()/? Or will it be less intrusive
>         to others (that might be used to the name) to not change it
>         even though the name might be misleading? Or should we add a
>         new function called /build_tlv_kmac()/? (Although this might
>         lead to a lot of switch-case statements everywhere that
>         /build_tlv_hmac()/ is called)
>
> Rename the function to /build_tlv_mac /and have an input parameter
> (the first one preferably) to switch between HMAC and KMAC methods.
> Use a constant to denote the two that can be accessed as needed. Then
> you could have two lower level functions for each (/build_tlv_kmac/
> and /build_tlv_hmac/) to allow for direct calls if needed that are
> really only ever used in /build_tlv_mac/.
>  
>
>
>     -- 
>     Tm-rid mailing list
>     [email-masked] <mailto:[email-masked]>
>     https://www.ietf.org/mailman/listinfo/tm-rid
>     <https://www.ietf.org/mailman/listinfo/tm-rid>
>
>
>
> -- 
> 73's,
> Adam T. Wiethuechter
> AX Enterprize, LLC
>