Re: [Lksctp-developers] Patch to handle invalid chunk lengths

[La M. H.P. Y. <pi...@ti...>]

I have detailed architectural and mechanical comments.

My key recommendation is to attach your error handling in the input
pipe (specifically, sctp_inq_pop()) instead of in the association
and endpoint glue.

The basic architecture here is smart pipes attached to a pure state
machine with explicit side-effects.  Event generation is a smart pipe
activity.  A smart pipe may either respond to a pull from the state
machine (e.g. sctp_inq_pop()), or it may initiate a push (e.g.
sctp_generate_t3_rtx_event())

Similarly, the error processing for parameters is NOT routed through
the state machine.  This is a serious architectural violation.  All
inbound packet->outbound packet flows should go through the state
machine.

Below is a more detailed line-by-line commentary.

I have two questions for the group:

Section 6.10 tells us to drop a chunk if it is partial.  Vladislav's
code drops the whole packet.  I tend to agree that Vladislav's code
is correct.  Is this a defect in the spec or merely a trivial difference
in implementation?

Should we silently drop packets if we are in SCTP_STATE_CLOSED?  This
is the most likely blind attacker.

Sridhar Samudrala wrote:

>Hi Vlad,
>
>Your patch looks pretty good.  here are a few minor comments.
>
>1. I was thinking may be you could have added a new subtype to the existing
>   event type of 'other' instead of creating a new event type 'violation'.
>   Currently we have only one subtype defined for 'other' event -
>   SCTP_EVENT_NO_PENDING_TSN. You could add a new subtype
>   SCTP_EVENT_BADCHUNKLEN_VIOLATION to this table.
>

I agree whole-heartedly.  This fits very nicely into Other.

>2. In sctp_process_inv_paramlength() and sctp_sf_violation_chunklen(), you
>   are allocating space for err_str and payload on the stack. Instead
>   can't you get the required space allocated in sctp_make_abort() or
>   sctp_make_op_error() and do a sctp_addto_chunk() of all the additional
>   stuff you want to add to this chunk.
>
I've highlighted one case where you did this correctly and another which 
should be corrected.

>3. This is not a problem with your patch, but in the existing code in
>   sctp_assoc_bh_rcv(). The variable 'subtype' does not reflect its name.
>   Instead of declaring it as sctp_subtype_t, we are using it as int.
>   The way we use it in sctp_endpoint_bh_rcv() seems to be more correct.
>

Yes, the declaration is wrong in sctp_assoc_bh_rcv().

>
>frametest also looks very solid. may be you want to change the copyright to
>HP instead of IBM.
>

Please also add yourself to the appropriate written or modified by lists.

>Thanks
>Sridhar
>
>
>On Thu, 23 Sep 2004, Vladislav Yasevich wrote:
>
>
>>The patch at the following url corrects the handling of
>>invalid chunk and parameter lengths.
>>
>>http://www.csh.rit.edu/~vlad/sctp
>>
>>I'd like to get a the code reviewed by a few developers
>>since it's on a large-ish side (too big to include in email).
>>There is also a frame test for this.
>>
>>The solution used was one suggested by La Monte, with a simplification.
>>I felt that defining our chunk type was not a very clean way
>>,protocol specification wise,  since chunks are really protocol
>>elements.  I simply went with a new event definition and passed
>>the new event to the state machine.
>>
>>All feedback will be appreciated.
>>-vlad
>>--
>>++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>Vladislav Yasevich		Linux and Open Source Lab
>>Hewlett Packard 		Tel: (603) 884-1079
>>Nashua, NH 03062		ZKO3-3/T07
>>
>>
>>-------------------------------------------------------
>>This SF.Net email is sponsored by: YOU BE THE JUDGE. Be one of 170
>>Project Admins to receive an Apple iPod Mini FREE for your judgement on
>>who ports your project to Linux PPC the best. Sponsored by IBM.
>>Deadline: Sept. 24. Go here: http://sf.net/ppc_contest.php
>>_______________________________________________
>>Lksctp-developers mailing list
>>Lks...@li...
>>https://lists.sourceforge.net/lists/listinfo/lksctp-developers
>>
>>
>
>
>-------------------------------------------------------
>This SF.Net email is sponsored by: YOU BE THE JUDGE. Be one of 170
>Project Admins to receive an Apple iPod Mini FREE for your judgement on
>who ports your project to Linux PPC the best. Sponsored by IBM.
>Deadline: Sept. 24. Go here: http://sf.net/ppc_contest.php
>_______________________________________________
>Lksctp-developers mailing list
>Lks...@li...
>https://lists.sourceforge.net/lists/listinfo/lksctp-developers
>
>
Please use the -p option to diff.  It adds the name of the surrounding
function.

When making comments, please capitalize and punctuate correctly.  Spellcheck
your comments.  These guidelines are to uphold the utility of lksctp as a
reference implementation.  Proper grammer, spelling, and typography are
particularly helpful to non-native speakers of English and C.

 > ===== include/net/sctp/constants.h 1.23 vs edited =====
 > --- 1.23/include/net/sctp/constants.h    Fri Jul 23 02:12:40 2004
 > +++ edited/include/net/sctp/constants.h    Thu Sep 23 12:14:02 2004
 > @@ -76,11 +76,12 @@
 >      SCTP_EVENT_T_CHUNK = 1,
 >      SCTP_EVENT_T_TIMEOUT,
 >      SCTP_EVENT_T_OTHER,
 > -    SCTP_EVENT_T_PRIMITIVE
 > +    SCTP_EVENT_T_PRIMITIVE,
 > +    SCTP_EVENT_T_VIOLATION

You can do this as an instance of SCTP_EVENT_T_OTHER without adding a new
event type.

 >  } sctp_event_t;
 > 
 > -#define SCTP_EVENT_T_MAX SCTP_EVENT_T_PRIMITIVE
 > +#define SCTP_EVENT_T_MAX SCTP_EVENT_T_VIOLATION
 >  #define SCTP_EVENT_T_NUM (SCTP_EVENT_T_MAX + 1)
 > 
 >  /* As a convenience for the state machine, we append SCTP_EVENT_* and
 > @@ -123,6 +124,13 @@
 >  #define SCTP_EVENT_PRIMITIVE_MAX    SCTP_PRIMITIVE_ASCONF
 >  #define SCTP_NUM_PRIMITIVE_TYPES    (SCTP_EVENT_PRIMITIVE_MAX + 1)
 > 
 > +typedef enum {
 > +    SCTP_EVENT_VIOLATION_BADCHUNKLEN = 0,
 > +} sctp_event_violation_t;
 > +
 > +#define SCTP_EVENT_VIOLATION_MAX    SCTP_EVENT_VIOLATION_BADCHUNKLEN
 > +#define SCTP_NUM_VIOLATION_TYPES    (SCTP_EVENT_VIOLATION_MAX +1)
 > +
 >  /* We define here a utility type for manipulating subtypes.
 >   * The subtype constructors all work like this:
 >   *
 > @@ -134,6 +142,7 @@
 >      sctp_event_timeout_t timeout;
 >      sctp_event_other_t other;
 >      sctp_event_primitive_t primitive;
 > +    sctp_event_violation_t violation;
 >  } sctp_subtype_t;
 > 
 >  #define SCTP_SUBTYPE_CONSTRUCTOR(_name, _type, _elt) \
 > @@ -145,6 +154,7 @@
 >  SCTP_SUBTYPE_CONSTRUCTOR(TIMEOUT,    sctp_event_timeout_t,    timeout)
 >  SCTP_SUBTYPE_CONSTRUCTOR(OTHER,        sctp_event_other_t,    other)
 >  SCTP_SUBTYPE_CONSTRUCTOR(PRIMITIVE,    sctp_event_primitive_t,    
primitive)
 > +SCTP_SUBTYPE_CONSTRUCTOR(VIOLATION,    sctp_event_violation_t,    
violation)
 > 
 > 
 >  #define sctp_chunk_is_control(a) (a->chunk_hdr->type != SCTP_CID_DATA)
 > @@ -231,6 +241,7 @@
 >  const char *sctp_oname(const sctp_subtype_t);    /* other events */
 >  const char *sctp_tname(const sctp_subtype_t);    /* timeouts */
 >  const char *sctp_pname(const sctp_subtype_t);    /* primitives */
 > +const char *sctp_vname(const sctp_subtype_t);    /* primitives */
 > 
 >  /* This is a table of printable names of sctp_state_t's.  */
 >  extern const char *sctp_state_tbl[], *sctp_evttype_tbl[], 
*sctp_status_tbl[];
 > ===== include/net/sctp/sctp.h 1.58 vs edited =====
 > --- 1.58/include/net/sctp/sctp.h    Fri May 14 22:00:06 2004
 > +++ edited/include/net/sctp/sctp.h    Thu Sep 23 12:14:04 2004
 > @@ -455,7 +455,8 @@
 >  #define _sctp_walk_params(pos, chunk, end, member)\
 >  for (pos.v = chunk->member;\
 >       pos.v <= (void *)chunk + end - sizeof(sctp_paramhdr_t) &&\
 > -     pos.v <= (void *)chunk + end - WORD_ROUND(ntohs(pos.p->length)); \
 > +     pos.v <= (void *)chunk + end - WORD_ROUND(ntohs(pos.p->length)) &&\
 > +     WORD_ROUND(ntohs(pos.p->length)) >= sizeof(sctp_paramhdr_t);\
 >       pos.v += WORD_ROUND(ntohs(pos.p->length)))
 > 
 >  #define sctp_walk_errors(err, chunk_hdr)\
 > @@ -465,10 +466,9 @@
 >  for (err = (sctp_errhdr_t *)((void *)chunk_hdr + \
 >          sizeof(sctp_chunkhdr_t));\
 >       (void *)err <= (void *)chunk_hdr + end - sizeof(sctp_errhdr_t) &&\
 > -     (void *)err <= (void *)chunk_hdr + end - \
 > -            WORD_ROUND(ntohs(err->length));\
 > -     err = (sctp_errhdr_t *)((void *)err + \
 > -        WORD_ROUND(ntohs(err->length))))
 > +     (void *)err <= (void *)chunk_hdr + end - 
WORD_ROUND(ntohs(err->length)) &&\
 > +     WORD_ROUND(ntohs(err->length)) >= sizeof(sctp_errhdr_t); \
 > +     err = (sctp_errhdr_t *)((void *)err + 
WORD_ROUND(ntohs(err->length))))
 > 
 >  #define sctp_walk_fwdtsn(pos, chunk)\
 >  _sctp_walk_fwdtsn((pos), (chunk), ntohs((chunk)->chunk_hdr->length) 
- sizeof(struct sctp_fwdtsn_chunk))
 > ===== include/net/sctp/sm.h 1.36 vs edited =====
 > --- 1.36/include/net/sctp/sm.h    Fri Jul 23 02:12:40 2004
 > +++ edited/include/net/sctp/sm.h    Thu Sep 23 13:22:22 2004
 > @@ -131,6 +131,7 @@
 >  sctp_state_fn_t sctp_sf_shut_8_4_5;
 >  sctp_state_fn_t sctp_sf_pdiscard;
 >  sctp_state_fn_t sctp_sf_violation;
 > +sctp_state_fn_t sctp_sf_violation_chunklen;
 >  sctp_state_fn_t sctp_sf_discard_chunk;
 >  sctp_state_fn_t sctp_sf_do_5_2_1_siminit;
 >  sctp_state_fn_t sctp_sf_do_5_2_2_dupinit;
 > @@ -232,6 +233,10 @@
 >  struct sctp_chunk *sctp_make_abort_user(const struct sctp_association *,
 >                     const struct sctp_chunk *,
 >                     const struct msghdr *);
 > +struct sctp_chunk *sctp_make_abort_violation(const struct 
sctp_association *,
 > +                   const struct sctp_chunk *,
 > +                   const __u8 *,
 > +                   const size_t );
 >  struct sctp_chunk *sctp_make_heartbeat(const struct sctp_association *,
 >                    const struct sctp_transport *,
 >                    const void *payload,
 > @@ -338,6 +343,8 @@
 >  other_event_table[SCTP_NUM_OTHER_TYPES][SCTP_STATE_NUM_STATES];
 >  extern const sctp_sm_table_entry_t
 >  timeout_event_table[SCTP_NUM_TIMEOUT_TYPES][SCTP_STATE_NUM_STATES];
 > +extern const sctp_sm_table_entry_t
 > +violation_event_table[SCTP_NUM_VIOLATION_TYPES][SCTP_STATE_NUM_STATES];
 >  extern sctp_timer_event_t *sctp_timer_events[SCTP_NUM_TIMEOUT_TYPES];
 > 
 >  /* These are some handy utility macros... */
 > ===== net/sctp/associola.c 1.76 vs edited =====
 > --- 1.76/net/sctp/associola.c    Sun Jul 25 01:06:46 2004
 > +++ edited/net/sctp/associola.c    Thu Sep 23 12:14:05 2004
 > @@ -861,6 +861,7 @@
 >      struct sock *sk;
 >      struct sctp_inq *inqueue;
 >      int state, subtype;
 > +    __u16 len;
 >      int error = 0;
 > 
 >      /* The association should be held so we should be safe. */
 > @@ -872,6 +873,7 @@
 >      while (NULL != (chunk = sctp_inq_pop(inqueue))) {

If you detect the event inside sctp_inq_pop() you do not need to inject your
intercept code twice.

It is also better architecturally to put it in the smart pipe code 
rather than
in the state machine glue.

 >          state = asoc->state;
 >          subtype = chunk->chunk_hdr->type;
 > +        len = ntohs(chunk->chunk_hdr->length);
 > 
 >          /* Remember where the last DATA chunk came from so we
 >           * know where to send the SACK.
 > @@ -884,9 +886,27 @@
 >          if (chunk->transport)
 >              chunk->transport->last_time_heard = jiffies;
 > 

I appreciate the full quote.  Would you mind identifying the document?  My
indexing tool can find the quote, but a casual reader has to figure out 
which
of a dozen documents you are talking about.

 > +        /* 6.10 Bundling
 > +        * Partial chunks MUST NOT be placed in an SCTP packet.
 > +        * If the receiver detects a partial chunk, it MUST drop the
 > +        * chunk. 
 > +        */

The text tells us we are supposed discard the chunk, but...

 > +        if (chunk->chunk_end > chunk->skb->tail) {
 > +            chunk->pdiscard = 1;

here we mark the whole packet for discard.  If we are going to do something
other than what the standard requires, we need to have an explicit comment
discussing why.  I think packet discard is the right thing to do at this
point.  You might want to drop a note to sct...@ex...
suggesting that this is a defect in the doc.

 > +            continue;
 > +        }
 > +
 >          /* Run through the state machine. */
 > -        error = sctp_do_sm(SCTP_EVENT_T_CHUNK, SCTP_ST_CHUNK(subtype),
 > -                   state, ep, asoc, chunk, GFP_ATOMIC);
 > +        if (unlikely(len < sizeof(sctp_chunkhdr_t))) {
 > +            subtype = SCTP_EVENT_VIOLATION_BADCHUNKLEN;
 > +            error = sctp_do_sm(SCTP_EVENT_T_VIOLATION,
 > +                       SCTP_ST_VIOLATION(subtype),
 > +                       state, ep, asoc, chunk, GFP_ATOMIC);
 > +        } else {
 > +            error = sctp_do_sm(SCTP_EVENT_T_CHUNK,
 > +                       SCTP_ST_CHUNK(subtype),
 > +                       state, ep, asoc, chunk, GFP_ATOMIC);
 > +        }
 > 
 >          /* Check to see if the association is freed in response to
 >           * the incoming chunk.  If so, get out of the while loop.
 > ===== net/sctp/debug.c 1.10 vs edited =====
 > --- 1.10/net/sctp/debug.c    Mon Apr 19 13:58:09 2004
 > +++ edited/net/sctp/debug.c    Thu Sep 23 12:14:05 2004
 > @@ -134,7 +134,8 @@
 >      "EVENT_T_CHUNK",
 >      "EVENT_T_TIMEOUT",
 >      "EVENT_T_OTHER",
 > -    "EVENT_T_PRIMITIVE"
 > +    "EVENT_T_PRIMITIVE",
 > +    "EVENT_T_VIOLATION"
 >  };
 > 
 >  /* Return value of a state function */
 > @@ -204,4 +205,20 @@
 >      if (id.timeout <= SCTP_EVENT_TIMEOUT_MAX)
 >          return sctp_timer_tbl[id.timeout];
 >      return "unknown_timer";
 > +}
 > +
 > +static const char *sctp_violation_tbl[] = {
 > +    "BADCHUNKLEN_VIOLATION",
 > +};
 > +

When you borrow boilerplate, edit it with care.  If we really did need 
the new
event type, the comment would read:
/* Lookup "violation" debug name.  */

 > +/* Lookup "other" debug name. */
 > +const char *sctp_vname(const sctp_subtype_t id)
 > +{
 > +    if (id.violation < 0)
 > +        return "illegal 'violation' event";
 > +
 > +    if (id.violation <= SCTP_EVENT_VIOLATION_MAX)
 > +        return sctp_violation_tbl[id.violation];
 > +
 > +    return "unknown 'violation' event";
 >  }
 > ===== net/sctp/endpointola.c 1.33 vs edited =====
 > --- 1.33/net/sctp/endpointola.c    Wed Jul  7 18:17:31 2004
 > +++ edited/net/sctp/endpointola.c    Thu Sep 23 12:14:06 2004
 > @@ -345,6 +345,7 @@
 >      sk = ep->base.sk;
 > 
 >      while (NULL != (chunk = sctp_inq_pop(inqueue))) {
 > +        __u16 len = ntohs(chunk->chunk_hdr->length);
 >          subtype.chunk = chunk->chunk_hdr->type;
 > 
 >          /* We might have grown an association since last we
 > @@ -374,8 +375,19 @@
 >          if (chunk->transport)
 >              chunk->transport->last_time_heard = jiffies;

If the defect detection and state machine injection take place in
sctp_inq_pop(), then we do not need this duplicate code.

I also dislike adding error-detection noise to the primary flow.  It 
makes the
code much harder to read.

 > 
 > -        error = sctp_do_sm(SCTP_EVENT_T_CHUNK, subtype, state,
 > -                                   ep, asoc, chunk, GFP_ATOMIC);
 > +        if (chunk->chunk_end > chunk->skb->tail) {
 > +            chunk->pdiscard = 1;
 > +            continue;
 > +        }
 > +
 > +        if (unlikely(len < sizeof(sctp_chunkhdr_t))) {
 > +            subtype.violation = SCTP_EVENT_VIOLATION_BADCHUNKLEN;
 > +            error = sctp_do_sm(SCTP_EVENT_T_VIOLATION, subtype,
 > +                        state, ep, asoc, chunk, GFP_ATOMIC);
 > +        } else {
 > +            error = sctp_do_sm(SCTP_EVENT_T_CHUNK, subtype, state,
 > +                        ep, asoc, chunk, GFP_ATOMIC);
 > +        }
 > 
 >          if (error && chunk)
 >              chunk->pdiscard = 1;
 > ===== net/sctp/input.c 1.39 vs edited =====
 > --- 1.39/net/sctp/input.c    Wed Jul  7 18:17:31 2004
 > +++ edited/net/sctp/input.c    Thu Sep 23 14:18:35 2004
 > @@ -130,6 +130,10 @@
 > 
 >      skb_pull(skb, sizeof(struct sctphdr));

This code rates the comment from section 6.10 again, but we are behaving
differently.  Here we simply drop the packet fragment rather than 
dropping the
whole packet.

 > +    /* make sure we at least have chunk headers worth left */
 > +    if (skb->len < sizeof(struct sctp_chunkhdr))
 > +        goto discard_it;
 > +
 >      family = ipver2af(skb->nh.iph->version);
 >      af = sctp_get_af_specific(family);
 >      if (unlikely(!af))
 > @@ -479,10 +483,10 @@
 >      sctp_errhdr_t *err;
 > 
 >      ch = (sctp_chunkhdr_t *) skb->data;
 > +    ch_end = ((__u8 *) ch) + WORD_ROUND(ntohs(ch->length));
 > 
 >      /* Scan through all the chunks in the packet.  */
 > -    do {
 > -        ch_end = ((__u8 *) ch) + WORD_ROUND(ntohs(ch->length));
 > +    while (ch_end > (__u8 *)ch && ch_end < skb->tail) {
 > 
 >          /* RFC 8.4, 2) If the OOTB packet contains an ABORT chunk, the
 >           * receiver MUST silently discard the OOTB packet and take no
 > @@ -513,7 +517,8 @@
 >          }
 > 
 >          ch = (sctp_chunkhdr_t *) ch_end;
 > -    } while (ch_end < skb->tail);
 > +            ch_end = ((__u8 *) ch) + WORD_ROUND(ntohs(ch->length));
 > +    }
 > 
 >      return 0;
 > 
 > @@ -783,6 +788,14 @@
 >      default:
 >          return NULL;
 >      }
 > +
 > +    /* The code below will attempt to walk the chunk and extract
 > +     * parameter information.  Before we do that, we need to verify
 > +     * that the chunk length is correct.  Otherwie, we'll walk off

                                              Otherwise

 > +     * the end.
 > +     */
 > +    if (WORD_ROUND(ntohs(ch->length)) > skb->len)
 > +        return NULL;
 > 
 >      /*
 >       * This code will NOT touch anything inside the chunk--it is
 > ===== net/sctp/sm_make_chunk.c 1.72 vs edited =====
 > --- 1.72/net/sctp/sm_make_chunk.c    Fri Jul 23 02:18:00 2004
 > +++ edited/net/sctp/sm_make_chunk.c    Thu Sep 23 14:17:25 2004
 > @@ -867,6 +867,25 @@
 >      return retval;
 >  }

This is good code and illustrates the pre-allocate and append strategy used
throughout lksctp.  I strongly recommend replacing most of your uses of
kalloc() with this style.

 > +/* Make an ABORT with a PROTOCOL VIOLATION cause code */
 > +struct sctp_chunk *sctp_make_abort_violation(
 > +    const struct sctp_association *asoc,
 > +    const struct sctp_chunk *chunk,
 > +    const __u8   *payload,
 > +    const size_t paylen)
 > +{
 > +    struct sctp_chunk  *retval = NULL;
 > +
 > +    retval = sctp_make_abort(asoc, chunk, sizeof(sctp_errhdr_t) + 
paylen);
 > +    if (!retval)
 > +        goto end;
 > +
 > +    sctp_init_cause(retval, SCTP_ERROR_PROTO_VIOLATION, payload, 
paylen);
 > +
 > +end:
 > +    return retval;
 > +}
 > +
 >  /* Make a HEARTBEAT chunk.  */
 >  struct sctp_chunk *sctp_make_heartbeat(const struct sctp_association 
*asoc,
 >                    const struct sctp_transport *transport,
 > @@ -1020,7 +1039,6 @@
 >      SCTP_DBG_OBJCNT_INC(chunk);
 >      atomic_set(&retval->refcnt, 1);
 > 
 > -
 >  nodata:
 >      return retval;
 >  }
 > @@ -1529,6 +1547,49 @@
 >      return 0;
 >  }
 > 
 > +static int sctp_process_inv_paramlength(const struct 
sctp_association *asoc,
 > +                    struct sctp_paramhdr *param,
 > +                    const struct sctp_chunk *chunk,
 > +                    struct sctp_chunk **errp)
 > +{
 > +    char        error[] = "The following parameter had invalid length:";
 > +    size_t        error_len = sizeof(error);
 > +    __u8         *payload = NULL;
 > +    size_t         payload_len = WORD_ROUND(error_len +
 > +                         sizeof(sctp_paramhdr_t));
 > +    sctp_paramhdr_t *p = NULL;
 > +
 > +    /* Set up the payload:
 > +     * the encoding will be <error string>\0<param_type><param_len>
 > +     */

This kmalloc()/kfree() pair can be eliminated.

 > +    payload = kmalloc(payload_len, GFP_ATOMIC);
 > +
 > +    if (payload == NULL)
 > +        return -ENOMEM;
 > +
 > +    /* write the error string*/
 > +    memcpy(payload, error, error_len);
 > +
 > +    /* write the parameter header */
 > +    p = (sctp_paramhdr_t *)(payload + error_len);
 > +    p->type = param->type;
 > +    p->length = param->length;
 > +   
 > +    /* create an error chunk and fill it in with our payload */
 > +    if (!*errp)
 > +        *errp = sctp_make_op_error_space(asoc, chunk, payload_len);
 > +
 > +    if (*errp) {
 > +        sctp_init_cause(*errp, SCTP_ERROR_PROTO_VIOLATION, payload,
 > +                payload_len);

  errp = sctp_init_cause(*errp, SCTP_ERROR_PROTO_VIOLATION, payload,
                payload_len);

etc...

 > +    }
 > +
 > +    kfree(payload);

ibid.

 > +
 > +    return 0;
 > +}
 > +
 > +
 >  /* Do not attempt to handle the HOST_NAME parm.  However, do
 >   * send back an indicator to the peer.
 >   */
 > @@ -1705,6 +1766,17 @@
 >              has_cookie = 1;
 > 
 >      } /* for (loop through all parameters) */
 > +
 > +    /* there is a possiblity that a paremeter length was bad and
           There                        parameter
 > +     * in that case we would have stoped walking the parameters.
                                      stopped

 > +     * the current parameter would point at the bad one.
 > +     * current consensus on the mailing list is to send a PROTOCOL
       Current
 > +     * VIOLATION
           VIOLATION.

BTW, is there no discussion of malformed parameters in either the RFC or the
Implementor's Guide?  I was expecting a quote here.

 > +     */
 > +    if (param.v < (void*)chunk->chunk_end - sizeof(sctp_paramhdr_t)) {
 > +        sctp_process_inv_paramlength(asoc, param.p, chunk, errp);

You should be injecting an event into the state machine here rather than
generating packets.  All outbound packets in response to inbound packets
should route through the state machine.

 > +        return 0;
 > +    }
 > 
 >      /* The only missing mandatory param possible today is
 >       * the state cookie for an INIT-ACK chunk.
 > ===== net/sctp/sm_sideeffect.c 1.56 vs edited =====
 > --- 1.56/net/sctp/sm_sideeffect.c    Fri Jul 23 02:15:32 2004
 > +++ edited/net/sctp/sm_sideeffect.c    Thu Sep 23 12:14:07 2004
 > @@ -829,6 +829,7 @@
 > 
 >      static printfn_t *table[] = {
 >          NULL, sctp_cname, sctp_tname, sctp_oname, sctp_pname,
 > +        sctp_vname,
 >      };
 >      printfn_t *debug_fn  __attribute__ ((unused)) = table[event_type];
 > 
 > ===== net/sctp/sm_statefuns.c 1.77 vs edited =====
 > --- 1.77/net/sctp/sm_statefuns.c    Sun Jul 25 01:40:49 2004
 > +++ edited/net/sctp/sm_statefuns.c    Thu Sep 23 14:15:16 2004
 > @@ -3279,6 +3279,82 @@
 >      return SCTP_DISPOSITION_VIOLATION;
 >  }
 > 
 > +/*
 > + * The other end is violating protocol, and in this case we want to 
inform
 > + * them.  We generate an ABORT with a Protocol Violation error code.
 > + *
 > + * Section: Not specified
 > + * Verification Tag:  verify if we have an association.
 > + * Inputs
 > + * (endpoint, asoc, chunk)
 > + *
 > + * Outputs
 > + * (asoc, reply_msg, msg_up, timers, counters)
 > + *
 > + * We simply tag the chunk as a violation.  The state machine will log
 > + * the violation and continue.
 > + */
 > +sctp_disposition_t sctp_sf_violation_chunklen(const struct 
sctp_endpoint *ep,
 > +                     const struct sctp_association *asoc,
 > +                     const sctp_subtype_t type,
 > +                     void *arg,
 > +                     sctp_cmd_seq_t *commands)
 > +{
 > +    struct sctp_chunk *chunk =  arg;
 > +    struct sctp_chunk *abort = NULL;
 > +    struct sctp_packet *packet = NULL;
 > +    char            err_str[]="The following chunk had invalid length:";
 > +    __u8           payload[WORD_ROUND(sizeof(err_str) +
 > +                              sizeof(sctp_paramhdr_t))];
 > +    sctp_paramhdr_t     *p;
 > +
 > +    /* verify vtag. we have the association since we are here. */
           Verify       We

 > +    if (asoc && !sctp_vtag_verify_either(chunk, asoc))
 > +        return sctp_sf_pdiscard(ep, asoc, type, arg, commands);
 > +
 > +    /* initialize payload witht the erorr string */
                              with

 > +    memcpy(payload, err_str, sizeof(err_str));
 > +
 > +    /* encode the chunk type and length of the offending chunk */
           Encode                                            chunk.  */

 > +    p = (sctp_paramhdr_t*)(payload + sizeof(err_str));
 > +    p->type = htons(chunk->chunk_hdr->type);
 > +    p->length = chunk->chunk_hdr->length;
 > +   
 > +    /* make the abort chunk */
           Make           chunk.

 > +    abort = sctp_make_abort_violation(asoc, chunk, payload,
 > +                      sizeof(payload));
 > +    if (!abort)
 > +        goto nomem;
 > +
 > +    /* make the packet and add the abort chunk to it */
           Make                                       it.  */


 > +    packet = sctp_abort_pkt_new(ep, asoc, chunk,
 > +                    (__u8 *)(abort->chunk_hdr) +
 > +                    sizeof(sctp_chunkhdr_t),
 > +                    ntohs(abort->chunk_hdr->length) -
 > +                    sizeof(sctp_chunkhdr_t));
 > +    if (!packet)
 > +        goto nomem;
 > +
 > +    sctp_chunk_free(abort);
 > +
 > +    sctp_add_cmd_sf(commands, SCTP_CMD_SEND_PKT, SCTP_PACKET(packet));
 > +    sctp_add_cmd_sf(commands, SCTP_CMD_DISCARD_PACKET, SCTP_NULL());
 > +    SCTP_INC_STATS(SCTP_MIB_OUTCTRLCHUNKS);
 > +    if (asoc)
 > +        sctp_add_cmd_sf(commands, SCTP_CMD_ASSOC_FAILED,
 > +                SCTP_U32(SCTP_ERROR_PROTO_VIOLATION));
 > +    /* discard the current packet */
           Discard             packet.  */

 > +    SCTP_INC_STATS(SCTP_MIB_ABORTEDS);
 > +    SCTP_DEC_STATS(SCTP_MIB_CURRESTAB);
 > +   
 > +    return SCTP_DISPOSITION_ABORT;
 > +
 > +nomem:
 > +    if (abort)
 > +        sctp_chunk_free(abort);
 > +
 > +    return SCTP_DISPOSITION_NOMEM;
 > +}
 >  
/***************************************************************************
 >   * These are the state functions for handling primitive (Section 10) 
events.
 >   
***************************************************************************/
 > ===== net/sctp/sm_statetable.c 1.22 vs edited =====
 > --- 1.22/net/sctp/sm_statetable.c    Mon Apr 19 13:58:09 2004
 > +++ edited/net/sctp/sm_statetable.c    Thu Sep 23 13:53:36 2004
 > @@ -87,6 +87,13 @@
 >                primitive_event_table);
 >          break;
 > 
 > +    case SCTP_EVENT_T_VIOLATION:
 > +        DO_LOOKUP(SCTP_EVENT_VIOLATION_MAX, violation,
 > +                violation_event_table);
 > +        break;
 > +        /*return &violation_event_table[0][state]; */
 > +
 > +       
 >      default:
 >          /* Yikes!  We got an illegal event type.  */
 >          return &bug;
 > @@ -942,6 +949,40 @@
 >      TYPE_SCTP_EVENT_TIMEOUT_HEARTBEAT,
 >      TYPE_SCTP_EVENT_TIMEOUT_SACK,
 >      TYPE_SCTP_EVENT_TIMEOUT_AUTOCLOSE,
 > +};
 > +
 > +#define TYPE_SCTP_VIOLATION_BAD_CHUNK_LEN { \
 > +    /* SCTP_STATE_EMPTY */ \
 > +    {.fn = sctp_sf_violation_chunklen, \
 > +     .name = "sctp_sf_violation_chunklen"},\

I'm pretty sure SCTP_STATE_EMPTY should call one of the BUG routines.

 > +    /* SCTP_STATE_CLOSED */ \
 > +    {.fn = sctp_sf_violation_chunklen, \
 > +     .name = "sctp_sf_violation_chunklen"}, \

This is the most likely case of a random attacker.  I might make the 
argument
that in THIS case, we should simply drop the whole packet rather than 
generate
an ABORT.

 > +    /* SCTP_STATE_COOKIE_WAIT */ \
 > +    {.fn = sctp_sf_violation_chunklen, \
 > +     .name = "sctp_sf_violation_chunklen"}, \
 > +    /* SCTP_STATE_COOKIE_ECHOED */ \
 > +    {.fn = sctp_sf_violation_chunklen, \
 > +     .name = "sctp_sf_violation_chunklen"}, \
 > +    /* SCTP_STATE_ESTABLISHED */ \
 > +    {.fn = sctp_sf_violation_chunklen, \
 > +     .name = "sctp_sf_violation_chunklen"}, \
 > +    /* SCTP_STATE_SHUTDOWN_PENDING */ \
 > +    {.fn = sctp_sf_violation_chunklen, \
 > +     .name = "sctp_sf_violation_chunklen"}, \
 > +    /* SCTP_STATE_SHUTDOWN_SENT */ \
 > +    {.fn = sctp_sf_violation_chunklen, \
 > +     .name = "sctp_sf_violation_chunklen"}, \
 > +    /* SCTP_STATE_SHUTDOWN_RECEIVED */ \
 > +    {.fn = sctp_sf_violation_chunklen, \
 > +     .name = "sctp_sf_violation_chunklen"}, \
 > +    /* SCTP_STATE_SHUTDOWN_ACK_SENT */ \
 > +    {.fn = sctp_sf_violation_chunklen, \
 > +     .name = "sctp_sf_violation_chunklen"}, \
 > +}
 > +
 > +const sctp_sm_table_entry_t 
violation_event_table[SCTP_NUM_VIOLATION_TYPES][SCTP_STATE_NUM_STATES] = {
 > +    TYPE_SCTP_VIOLATION_BAD_CHUNK_LEN,
 >  };
 > 
 >  const sctp_sm_table_entry_t *sctp_chunk_event_lookup(sctp_cid_t cid,
 >

-- 
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell's sig