[Swan-dev] dh and intermediate exchanges

[Andrew Cagney]

On Thu, 12 Nov 2020 at 13:16, Sahana Prasad <sahana.prasad07 at gmail.com> wrote:
>
> Hello Andrew,
>
> The author of the intermediate exchange draft (Valery), confirmed that we always use the last recalculated keys to protect the next
>
>  exchange (whether it is IKE_AUTH or next IKE_INTERMEDIATE). If keys are not recalculated in an IKE_INTERMEDIATE (no KE were exchanged),
>
> then they don’t change and are used in the next exchange. Our case is the same, we don't exchange KE, so we use the same keys for both exchanges.

I'm not sure I follow.

When the IKE_SA_INIT response comes back two calculations are performed:
- the shared DH secret using the local DH secret and remote KE (aka
.st_gr) - see calc_dh_shared_secret()
- the the keying material using shared DH secret and a key derivation
function (KDF/PRF+) - see (the new) calc_v2_keymat()
these keys are then used to protect the next message - either an
IKE_AUTH or IKE_INTERMEDIATE request.

I gather that, after each intermediate exchange, the keying material
needs to be recomputed ready for the next exchange?
However, what's the calculation used?  Is it a variation on the above
or something else specified by the RFC?

> Yes we recalculate the keys the second time, but it should be the same as the first one.
>
> You are right, the recalculation is unnecessary.

My puzzlement is more that when the IKE_INTERMEDIATE response comes
back the code calculates:

- the shared DH secret using the local DH secret and remote KE (aka
.st_gr) - see calc_dh_shared_secret()

but then completely ignores the result -> the re-computed shared DH
secret isn't used

(I can understand the code doing some sort of NOP as a placeholder for
the real intermediate key derivation calculation, but only when the
result is used).

>
> (We did it that way, because we weren't sure how to handle the key calculation.
>
> It was already being done before we knew we wanted to do intermediate, also we use common
>
> functions in state transitions for both IKE_AUTH and IKE_INT. Perhaps that should change too)
>
>
> Added Yulia in case I have missed any details.
>
>
> Thank you,
>
> Regards,
>
> Sahana Prasad
>
>
> On Mon, Nov 9, 2020 at 4:07 PM Andrew Cagney <andrew.cagney at gmail.com> wrote:
>>
>> thanks!
>>
>> As an aside.  I'm currently cleaning up the crypto code by eliminating
>> the remaining pcr style crypto helpers.  A likely side effect is that
>> the memory leak will disappear.  The possibly unnecessary DH
>> calculation will remain, it just won't leak.
>>
>> On Wed, 4 Nov 2020 at 16:22, Sahana Prasad <sahana.prasad07 at gmail.com> wrote:
>> >
>> > Hello Andrew,
>> > Thanks for the analysis.
>> >
>> > I'll get back to you on this one with more details.
>> >
>> > Thank you,
>> > Regards,
>> > Sahana Prasad
>> >
>> > On Wed, 4 Nov 2020, 20:56 Andrew Cagney, <andrew.cagney at gmail.com> wrote:
>> >>
>> >> The immediate problem is that the intermediate exchange is leaking the
>> >> local dh secret.  However there seems to be more going on - someone
>> >> familiar with intermediate code should probably take a look.
>> >>
>> >> - the initiator, on receipt of an IKE_SA_INIT responce calls,
>> >> ikev2_parent_inR1outI2() which finishes with:
>> >>
>> >>     /* If we seen the intermediate AND we are configured to use intermediate */
>> >>     /* for now, do only one Intermediate Exchange round and proceed
>> >> with IKE_AUTH */
>> >>     crypto_req_cont_func (*pcrc_func) = (ike->sa.st_seen_intermediate
>> >> && (md->pbs[PBS_v2N_INTERMEDIATE_EXCHANGE_SUPPORTED] != NULL) &&
>> >> !(md->hdr.isa_xchg == ISAKMP_v2_IKE_INTERMEDIATE)) ?
>> >>             ikev2_parent_inR1outI2_continue : ikev2_parent_inR1outI2_continue;
>> >>     submit_v2_dh_shared_secret(st, "ikev2_inR1outI2 KE",
>> >>                    SA_INITIATOR,
>> >>                    NULL, NULL, &st->st_ike_rekey_spis,
>> >>                    pcrc_func);
>> >>     return STF_SUSPEND;
>> >>
>> >> so it submits a request to finish the DH calculation and compute
>> >> dh-shared-secret#1, and then since it is doing an intermediate change
>> >> chooses to continue with ikev2_parent_inR1out_intermediate().  This
>> >> presumably will send an encrypted IKE_INTERMEDATE request
>> >>
>> >> - the initiator, on receipt of an IKE_INTERMEDIATE response, also
>> >> calls ikev2_parent_inR1outI2(), which submits a request to compute
>> >> dh-shared-secret#2, but this time chooses to continue with
>> >> ikev2_parent_inR1outI2_continue().  This presumably will send an
>> >> encrypted IKE_AUTH request.
>> >>
>> >> It's this second continue that is leaking:
>> >> - ikev2_parent_inR1out_intermediate() calls
>> >> finish_v2_dh_shared_secret() unconditionally which saves
>> >> dh-shared-secret#1 in the state
>> >> - ikev2_parent_inR1outI2_continue(), only calls
>> >> finish_v2_dh_shared_secret() when the response isn't IKE_INTERMEDIATE
>> >> (and here it is) so doesn't save dh-shared-secret#2 leaking it
>> >> (you'll need to trace through a few calls or sprinkle some debug lines
>> >> to see this)
>> >>
>> >> So, does anyone know the back story, and what should be happening?  Is
>> >> the DH needed for instance?
>> >>
>> >> Andrew
>> >> _______________________________________________
>> >> Swan-dev mailing list
>> >> Swan-dev at lists.libreswan.org
>> >> https://lists.libreswan.org/mailman/listinfo/swan-dev