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

[Andrew Cagney]

So, I got curious and went looking for the word 'containing'.  In
IKEv2 I found the following under "3.2.  Generic Payload Header" a use
of the word:

   o  Next Payload (1 octet) - Identifier for the payload type of the
      next payload in the message.  If the current payload is the last
      in the message, then this field will be 0.  This field provides a
      "chaining" capability whereby additional payloads can be added to
      a message by appending each one to the end of the message and
      setting the Next Payload field of the preceding payload to
      indicate the new payload's type.  An Encrypted payload, which must
      always be the last payload of a message, is an exception.  It
      contains data structures in the format of additional payloads.  In
      the header of an Encrypted payload, the Next Payload field is set
      to the payload type of the first contained payload (instead of 0);
      conversely, the Next Payload field of the last contained payload
      is set to zero.

so while it makes it very clear that it is a "chain" it does describe
the SK payload as something of an exception.

So what about IKEv1?  In IKEv1's  "3.2 Generic Payload Header":

    o  Next Payload (1 octet) - Identifier for the payload type of the
       next payload in the message.  If the current payload is the last
       in the message, then this field will be 0.  This field provides
       the "chaining" capability.

the description is even simpler (presumably because IKEv1 doesn't have
an SK payload and instead uses an encrypted bit in the message
header).

But then, in IKEv1's "3.4 Security Association Payload" I find:

    o  Next Payload (1 octet) - Identifier for the payload type of the
       next payload in the message.  If the current payload is the last
       in the message, then this field will be 0.  This field MUST NOT
       contain the values for the Proposal or Transform payloads as they
       are considered part of the security association negotiation.  For
       example, this field would contain the value "10" (Nonce payload)
       in the first message of a Base Exchange (see Section 4.4) and the
       value "0" in the first message of an Identity Protect Exchange
       (see Section 4.5).

ok, that's more than little weird (IKEv2 has no such text).  Digging
further, IKEv1's "3.5 Proposal Payload" and "3.6 Transform Payload"
have  identical descriptions (2 vs 3, Proposal vs Transform):

    o  Next Payload (1 octet) - Identifier for the payload type of the
       next payload in the message.  This field MUST only contain the
       value "3" or "0".  If there are additional Transform payloads in
       the proposal, then this field will be 3.  If the current
       Transform payload is the last within the proposal, then this
       field will be 0.

So IKEv1 is using the field "next payload" in two very different ways:
- to form a  "chain" of payloads that starts with the message header
and terminates with the last payload having 0
- to group identical structures (payloads) (within some other
structure/payload) where the last structure in the group has 0

But why is this weird?  Because, in IKEv2, the Proposal/Transform
_payloads_ were replaced by Proposal/Transform _structures_, and these
structures replaced the loaded "next payload" field with "last
substruc":

   o  Last Substruc (1 octet) - Specifies whether or not this is the
      last Proposal Substructure in the SA.  This field has a value of 0
      if this was the last Proposal Substructure, and a value of 2 if
      there are more Proposal Substructures.  This syntax is inherited
      from ISAKMP, but is unnecessary because the last Proposal could be
      identified from the length of the SA.  The value (2) corresponds
      to a payload type of Proposal in IKEv1, and the first four octets
      of the Proposal structure are designed to look somewhat like the
      header of a payload.

so, from the IKEv2 POV, a mechanism to handled nested structures might
be useful (I really have my doubts, IKEv1 and IKEv2 are managing to
generate these substructures just fine); it is orthogonal to the
next-payload chain.

On Wed, 26 Sep 2018 at 13:44, Andrew Cagney <andrew.cagney at gmail.com> wrote:
>
> 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