[Swan-dev] why do we bother checking out_raw() et.al.'s result?

[Andrew Cagney]

On Mon, 24 Sep 2018 at 00:44, D. Hugh Redelmeier <hugh at mimosa.com> wrote:

> Some problems are created by underdocumented designs.  For example,
> when you implemented NP backpatching, it appears that you didn't know
> about the tree-structure-in-time of the PBSes.  Backpatching code that
> reflected that is simpler than what you wrote.  Any sticky error
> conditions would need to flow toward the root of that tree.

Er, no.  While the output PBS can nest, the .np fields do not.  Here's
the IKEv2 RFC:

   Following the header are
   one or more IKE payloads each identified by a Next Payload field in
   the preceding payload.  Payloads are identified in the order in which
   they appear in an IKE message by looking in the Next Payload field in
   the IKE header, and subsequently according to the Next Payload field
   in the IKE payload itself until a Next Payload field of zero
   indicates that no payloads follow.

This is describing a single chain of .np fields that thread their way
through the payloads.  The chain starts in the header, and ends with
the last payload having .np=0.  It doesn't mention anything about
nesting. Consequently, at any point there is only on location to
patch, and hence only one previous .np location needs to be maintained
and the best place for that is in the packet's PBS.

The text then goes on:

   If a payload of type "Encrypted"
   is found, that payload is decrypted and its contents parsed as
   additional payloads.

I'm still not seeing anything here about how an SK's .np is nested.
Just that if an SK payload is encountered, it needs to be decrypted
before parsing can continue (admittedly the IKEv2 code doesn't handle
things this way - it first parses the un-encrypted packet and then
goes back and re-parses the encrypted bits; dumping this would make
things so much simpler).

Finally there's the text:

   An Encrypted payload MUST be the last payload
   in a packet and an Encrypted payload MUST NOT contain another
   Encrypted payload.

Carefully avoiding the question of where does the .np for a payload
following an SK payload go.  Per above, encrypted or un-encrypted, the
last payload's .np field is 0.

BTW:

My first simplistic attempt at back-patching .np was closer to your
code - and of course it didn't work - reading the RFC pointed to my
mistaken assumption and a simple fix.  And, unlike
move_pbs_previous_np(), one that didn't require invasive changes to
the IKEv2 code.

If I were to do anything different, it would be to add a separate
'struct out_packet' so that 'struct out_pbs' could directly point at
the relevant fields (which would include 'cur').

So while much of the massive change below was a good idea, this part was not:

    - backpatch target info is now in the PBS that will receive the payload.
      No searching is required.

    - ft_mnp (message next payload) is renamed ft_fcp (first contained
      payload) and can be used in more places, not just the IKE message
      header.

    - use an empty struct (ikev2_encrypted_portion) as a wrapper PBS for
      encrypted payloads.  This requires some consideration of backpatching
      mechanism (see calls to move_pbs_previous_np()).

Is there a good reason to not go back to the simpler mechanism?

Andrew

commit c7cc3dbdd0782c157d2676740212c24e35c18415
Author: D. Hugh Redelmeier <hugh at mimosa.com>
Date:   Fri Aug 10 16:07:20 2018 -0400

    pluto: improve next payload backpatching and start using it in IKEv1 code

    - backpatch target info is now in the PBS that will receive the payload.
      No searching is required.

    - backpatching better supports payloads within payloads

    - rename struct_desc's "np" (next payload) to "pt" (payload type)
      since this is about the current payload, not the next one.

    - make sure that every struct_desc used for payload output has a
      correct pt.  This meant the generic struct_desc should not be used.
      pt should only be explicitly initialized in a struct_desc for a
      payload.

    - the struct_desc pt field is now used at the start of out_struct,
      before the field loop.

    - ft_mnp (message next payload) is renamed ft_fcp (first contained
      payload) and can be used in more places, not just the IKE message
      header.

    - some backpatch problems are reported as expectation failures.
      This is an escalation from just appearing in debug logging.
      More checking is performed.

    - IKEv1 Vendor ID emitting is handled better: common routines are used
      instead of replicated and accidentally mutated code.

    - even more np calculation could be eliminated

    - scatter a lot more "const"s

    - use an empty struct (ikev2_encrypted_portion) as a wrapper PBS for
      encrypted payloads.  This requires some consideration of backpatching
      mechanism (see calls to move_pbs_previous_np()).

    - simplify ikev2_create_psk_auth() by eliminating a parameter

    - simplify ikev2_calculate_psk_sighash()

    - eliminate non-static array bounds ("hash_len") from
      ikev2_create_psk_auth() and ikev2_verify_psk_auth().
      This is (optonally) supported by compilers but the
      C Committee seems to consider it a mistake.

    - rename ikev2_np_cp_or_sa to ikev2_np_cp_or (awkward, but better
      reflects what it does).  This routine should be elminated.

    - simplify nat_traversal_insert_vid() by replacing struct state *
      parameter with a const struct connection * parameter

    - rename ikev2_record_fragments to ikev2_record_outbound_fragments
      to better reflect its function.

    - improved some logging