| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 30 Aug 2007 13:42:16 +0800
From: Wei Yongjun <yjwei@...fujitsu.com>
To: Vlad Yasevich <vladislav.yasevich@...com>
CC: lksctp-developers@...ts.sourceforge.net, netdev@...r.kernel.org
Subject: Re: [Lksctp-developers] SCTP: Fix dead loop while received unexpected
chunk with length set to zero
Vlad Yasevich wrote:
> Wei Yongjun wrote:
>
>> Vlad Yasevich wrote:
>>
>>> NACK
>>>
>>> Section 8.4:
>>>
>>> An SCTP packet is called an "out of the blue" (OOTB) packet if it is
>>> correctly formed (i.e., passed the receiver's CRC32c check; see
>>> Section 6.8), but the receiver is not able to identify the
>>> association to which this packet belongs.
>>>
>>>
>>> I would argue that the packet is not correctly formed in this case
>>> and deserves a protocol violation ABORT in return.
>>>
>>> -vlad
>>>
>>>
>> As your comment, patch has been changed.
>> Patch has been split to two, one is resolve this dead loop problem in
>> this mail.
>> And the other is come in another mail to discard partial chunk which
>> chunk length is set to zero.
>>
>
>
> I am starting to question the entire OOTB packet handling. There are way
> too many function that do almost the same thing and all handle OOTB a little
> different.
>
> sctp_sf_do_9_2_reshutack() is also called during sctp_sf_do_dupcook_a()
> processing, so checking for INIT chunk is wrong. Checking for just the
> chunkhdr_t should be enough.
>
This has been changed.
> sctp_sf_tabort_8_4_8 is used directly as well (not just through the state
> machine). Not sure if the header verification is appropriate.
>
It is needed. Because sctp_sf_tabort_8_4_8() is called to handle OOTB
packet before check the header length.
> This is a static function, so any verifications should already have been
> done.
>
Removed.
Thanks.
Regards
Signed-off-by: Wei Yongjun <yjwei@...fujitsu.com>
--- a/net/sctp/sm_statefuns.c 2007-08-17 06:17:14.000000000 -0400
+++ b/net/sctp/sm_statefuns.c 2007-08-18 07:59:25.000000000 -0400
@@ -98,6 +98,7 @@ static sctp_disposition_t sctp_stop_t1_a
struct sctp_transport *transport);
static sctp_disposition_t sctp_sf_abort_violation(
+ const struct sctp_endpoint *ep,
const struct sctp_association *asoc,
void *arg,
sctp_cmd_seq_t *commands,
@@ -192,6 +193,11 @@ sctp_disposition_t sctp_sf_do_4_C(const
if (!sctp_vtag_verify_either(chunk, asoc))
return sctp_sf_pdiscard(ep, asoc, type, arg, commands);
+ /* Make sure that the SHUTDOWN_COMPLETE chunk has a valid length. */
+ if (!sctp_chunk_length_valid(chunk, sizeof(sctp_chunkhdr_t)))
+ return sctp_sf_violation_chunklen(ep, asoc, type, arg,
+ commands);
+
/* RFC 2960 10.2 SCTP-to-ULP
*
* H) SHUTDOWN COMPLETE notification
@@ -2495,6 +2501,11 @@ sctp_disposition_t sctp_sf_do_9_2_reshut
struct sctp_chunk *chunk = (struct sctp_chunk *) arg;
struct sctp_chunk *reply;
+ /* Make sure that the chunk has a valid length */
+ if (!sctp_chunk_length_valid(chunk, sizeof(sctp_chunkhdr_t)))
+ return sctp_sf_violation_chunklen(ep, asoc, type, arg,
+ commands);
+
/* Since we are not going to really process this INIT, there
* is no point in verifying chunk boundries. Just generate
* the SHUTDOWN ACK.
@@ -2938,6 +2949,11 @@ sctp_disposition_t sctp_sf_tabort_8_4_8(
struct sctp_chunk *chunk = arg;
struct sctp_chunk *abort;
+ /* Make sure that the chunk has a valid length. */
+ if (!sctp_chunk_length_valid(chunk, sizeof(sctp_chunkhdr_t)))
+ return sctp_sf_violation_chunklen(ep, asoc, type, arg,
+ commands);
+
packet = sctp_ootb_pkt_new(asoc, chunk);
if (packet) {
@@ -3240,6 +3256,13 @@ sctp_disposition_t sctp_sf_do_8_5_1_E_sa
void *arg,
sctp_cmd_seq_t *commands)
{
+ struct sctp_chunk *chunk = arg;
+
+ /* Make sure that the SHUTDOWN_ACK chunk has a valid length. */
+ if (!sctp_chunk_length_valid(chunk, sizeof(sctp_chunkhdr_t)))
+ return sctp_sf_violation_chunklen(ep, asoc, type, arg,
+ commands);
+
/* Although we do have an association in this case, it corresponds
* to a restarted association. So the packet is treated as an OOTB
* packet and the state function that handles OOTB SHUTDOWN_ACK is
@@ -3654,6 +3677,16 @@ sctp_disposition_t sctp_sf_discard_chunk
void *arg,
sctp_cmd_seq_t *commands)
{
+ struct sctp_chunk *chunk = arg;
+
+ /* Make sure that the chunk has a valid length.
+ * Since we don't know the chunk type, we use a general
+ * chunkhdr structure to make a comparison.
+ */
+ if (!sctp_chunk_length_valid(chunk, sizeof(sctp_chunkhdr_t)))
+ return sctp_sf_violation_chunklen(ep, asoc, type, arg,
+ commands);
+
SCTP_DEBUG_PRINTK("Chunk %d is discarded\n", type.chunk);
return SCTP_DISPOSITION_DISCARD;
}
@@ -3709,6 +3742,13 @@ sctp_disposition_t sctp_sf_violation(con
void *arg,
sctp_cmd_seq_t *commands)
{
+ struct sctp_chunk *chunk = arg;
+
+ /* Make sure that the chunk has a valid length. */
+ if (!sctp_chunk_length_valid(chunk, sizeof(sctp_chunkhdr_t)))
+ return sctp_sf_violation_chunklen(ep, asoc, type, arg,
+ commands);
+
return SCTP_DISPOSITION_VIOLATION;
}
@@ -3716,12 +3756,14 @@ sctp_disposition_t sctp_sf_violation(con
* Common function to handle a protocol violation.
*/
static sctp_disposition_t sctp_sf_abort_violation(
+ const struct sctp_endpoint *ep,
const struct sctp_association *asoc,
void *arg,
sctp_cmd_seq_t *commands,
const __u8 *payload,
const size_t paylen)
{
+ struct sctp_packet *packet = NULL;
struct sctp_chunk *chunk = arg;
struct sctp_chunk *abort = NULL;
@@ -3730,22 +3772,41 @@ static sctp_disposition_t sctp_sf_abort_
if (!abort)
goto nomem;
- sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(abort));
- SCTP_INC_STATS(SCTP_MIB_OUTCTRLCHUNKS);
+ if (asoc) {
+ sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(abort));
+ SCTP_INC_STATS(SCTP_MIB_OUTCTRLCHUNKS);
- if (asoc->state <= SCTP_STATE_COOKIE_ECHOED) {
- sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_STOP,
- SCTP_TO(SCTP_EVENT_TIMEOUT_T1_INIT));
- sctp_add_cmd_sf(commands, SCTP_CMD_SET_SK_ERR,
- SCTP_ERROR(ECONNREFUSED));
- sctp_add_cmd_sf(commands, SCTP_CMD_INIT_FAILED,
- SCTP_PERR(SCTP_ERROR_PROTO_VIOLATION));
+ if (asoc->state <= SCTP_STATE_COOKIE_ECHOED) {
+ sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_STOP,
+ SCTP_TO(SCTP_EVENT_TIMEOUT_T1_INIT));
+ sctp_add_cmd_sf(commands, SCTP_CMD_SET_SK_ERR,
+ SCTP_ERROR(ECONNREFUSED));
+ sctp_add_cmd_sf(commands, SCTP_CMD_INIT_FAILED,
+ SCTP_PERR(SCTP_ERROR_PROTO_VIOLATION));
+ } else {
+ sctp_add_cmd_sf(commands, SCTP_CMD_SET_SK_ERR,
+ SCTP_ERROR(ECONNABORTED));
+ sctp_add_cmd_sf(commands, SCTP_CMD_ASSOC_FAILED,
+ SCTP_PERR(SCTP_ERROR_PROTO_VIOLATION));
+ SCTP_DEC_STATS(SCTP_MIB_CURRESTAB);
+ }
} else {
- sctp_add_cmd_sf(commands, SCTP_CMD_SET_SK_ERR,
- SCTP_ERROR(ECONNABORTED));
- sctp_add_cmd_sf(commands, SCTP_CMD_ASSOC_FAILED,
- SCTP_PERR(SCTP_ERROR_PROTO_VIOLATION));
- SCTP_DEC_STATS(SCTP_MIB_CURRESTAB);
+ packet = sctp_ootb_pkt_new(asoc, chunk);
+
+ if (!packet)
+ goto nomem;
+
+ if (sctp_test_T_bit(abort))
+ packet->vtag = ntohl(chunk->sctp_hdr->vtag);
+
+ abort->skb->sk = ep->base.sk;
+
+ sctp_packet_append_chunk(packet, abort);
+
+ sctp_add_cmd_sf(commands, SCTP_CMD_SEND_PKT,
+ SCTP_PACKET(packet));
+
+ SCTP_INC_STATS(SCTP_MIB_OUTCTRLCHUNKS);
}
sctp_add_cmd_sf(commands, SCTP_CMD_DISCARD_PACKET, SCTP_NULL());
@@ -3786,7 +3847,7 @@ static sctp_disposition_t sctp_sf_violat
{
char err_str[]="The following chunk had invalid length:";
- return sctp_sf_abort_violation(asoc, arg, commands, err_str,
+ return sctp_sf_abort_violation(ep, asoc, arg, commands, err_str,
sizeof(err_str));
}
@@ -3805,7 +3866,7 @@ static sctp_disposition_t sctp_sf_violat
{
char err_str[]="The cumulative tsn ack beyond the max tsn currently sent:";
- return sctp_sf_abort_violation(asoc, arg, commands, err_str,
+ return sctp_sf_abort_violation(ep, asoc, arg, commands, err_str,
sizeof(err_str));
}
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists