Message ID | 1405620319-2021-1-git-send-email-dborkman@redhat.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
On Thu, Jul 17, 2014 at 08:05:19PM +0200, Daniel Borkmann wrote: > Jason reported an oops caused by SCTP on his ARM machine with > SCTP authentication enabled: > > Internal error: Oops: 17 [#1] ARM > CPU: 0 PID: 104 Comm: sctp-test Not tainted 3.13.0-68744-g3632f30c9b20-dirty #1 > task: c6eefa40 ti: c6f52000 task.ti: c6f52000 > PC is at sctp_auth_calculate_hmac+0xc4/0x10c > LR is at sg_init_table+0x20/0x38 > pc : [<c024bb80>] lr : [<c00f32dc>] psr: 40000013 > sp : c6f538e8 ip : 00000000 fp : c6f53924 > r10: c6f50d80 r9 : 00000000 r8 : 00010000 > r7 : 00000000 r6 : c7be4000 r5 : 00000000 r4 : c6f56254 > r3 : c00c8170 r2 : 00000001 r1 : 00000008 r0 : c6f1e660 > Flags: nZcv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user > Control: 0005397f Table: 06f28000 DAC: 00000015 > Process sctp-test (pid: 104, stack limit = 0xc6f521c0) > Stack: (0xc6f538e8 to 0xc6f54000) > [...] > Backtrace: > [<c024babc>] (sctp_auth_calculate_hmac+0x0/0x10c) from [<c0249af8>] (sctp_packet_transmit+0x33c/0x5c8) > [<c02497bc>] (sctp_packet_transmit+0x0/0x5c8) from [<c023e96c>] (sctp_outq_flush+0x7fc/0x844) > [<c023e170>] (sctp_outq_flush+0x0/0x844) from [<c023ef78>] (sctp_outq_uncork+0x24/0x28) > [<c023ef54>] (sctp_outq_uncork+0x0/0x28) from [<c0234364>] (sctp_side_effects+0x1134/0x1220) > [<c0233230>] (sctp_side_effects+0x0/0x1220) from [<c02330b0>] (sctp_do_sm+0xac/0xd4) > [<c0233004>] (sctp_do_sm+0x0/0xd4) from [<c023675c>] (sctp_assoc_bh_rcv+0x118/0x160) > [<c0236644>] (sctp_assoc_bh_rcv+0x0/0x160) from [<c023d5bc>] (sctp_inq_push+0x6c/0x74) > [<c023d550>] (sctp_inq_push+0x0/0x74) from [<c024a6b0>] (sctp_rcv+0x7d8/0x888) > > While we already had various kind of bugs in that area > ec0223ec48a9 ("net: sctp: fix sctp_sf_do_5_1D_ce to verify if > we/peer is AUTH capable") and b14878ccb7fa ("net: sctp: cache > auth_enable per endpoint"), this one is a bit of a different > kind. > > Giving a bit more background on why SCTP authentication is > needed can be found in RFC4895: > > SCTP uses 32-bit verification tags to protect itself against > blind attackers. These values are not changed during the > lifetime of an SCTP association. > > Looking at new SCTP extensions, there is the need to have a > method of proving that an SCTP chunk(s) was really sent by > the original peer that started the association and not by a > malicious attacker. > > To cause this bug, we're triggering an INIT collision between > peers; normal SCTP handshake where both sides intent to > authenticate packets contains RANDOM; CHUNKS; HMAC-ALGO > parameters that are being negotiated among peers: > > ---------- INIT[RANDOM; CHUNKS; HMAC-ALGO] ----------> > <------- INIT-ACK[RANDOM; CHUNKS; HMAC-ALGO] --------- > -------------------- COOKIE-ECHO --------------------> > <-------------------- COOKIE-ACK --------------------- > > RFC4895 says that each endpoint therefore knows its own random > number and the peer's random number *after* the association has > been established. The local and peer's random number along > with the shared key are then part of the secret used for > calculating the HMAC in the AUTH chunk. > > Now, in our scenario, we have 2 threads with 1 non-blocking > SEQ_PACKET socket each, setting up common shared SCTP_AUTH_KEY > and SCTP_AUTH_ACTIVE_KEY properly, and each of them calling > sctp_bindx(3), listen(2) and connect(2) against each other, > thus the handshake looks similar to this, e.g.: > > ---------- INIT[RANDOM; CHUNKS; HMAC-ALGO] ----------> > <------- INIT-ACK[RANDOM; CHUNKS; HMAC-ALGO] --------- > <--------- INIT[RANDOM; CHUNKS; HMAC-ALGO] ----------- > -------- INIT-ACK[RANDOM; CHUNKS; HMAC-ALGO] --------> > ... > > Since such collisions can also happen with verification tags, > the RFC4895 for AUTH rather vaguely says under section 6.1: > > In case of INIT collision, the rules governing the handling > of this Random Number follow the same pattern as those for > the Verification Tag, as explained in Section 5.2.4 of > RFC 2960 [5]. Therefore, each endpoint knows its own Random > Number and the peer's Random Number after the association > has been established. > > In RFC2960, section 5.2.4, we're eventually hitting Action B: > > B) In this case, both sides may be attempting to start an > association at about the same time but the peer endpoint > started its INIT after responding to the local endpoint's > INIT. Thus it may have picked a new Verification Tag not > being aware of the previous Tag it had sent this endpoint. > The endpoint should stay in or enter the ESTABLISHED > state but it MUST update its peer's Verification Tag from > the State Cookie, stop any init or cookie timers that may > running and send a COOKIE ACK. > > In other words, the handling of the Random parameter is the > same as behavior for the Verification Tag as described in > Action B of section 5.2.4. > > Looking at the code, we exactly hit the sctp_sf_do_dupcook_b() > case which triggers an SCTP_CMD_UPDATE_ASSOC command to the > side effect interpreter, and in fact it copies over > peer_{random, hmacs, chunks} parameter from the newly created > association to update the existing one. > > Also, the old asoc_shared_key is being released and based on the > new params, sctp_auth_asoc_init_active_key() updated. However, > the issue observed in this case is that the previous > asoc->peer.auth_capable was 0 [note, it was 0 first since > peer.auth_capable is only being set on reception of INIT], > and has *not* been updated, so that instead of creating a > new secret, we're doing an early return from the function > sctp_auth_asoc_init_active_key() leaving asoc->asoc_shared_key > as NULL. However, we now have to authenticate chunks from > the updated chunk list (e.g. COOKIE-ACK, ...). > > That in fact causes the server side when responding with ... > > <------------------ AUTH; COOKIE-ACK ----------------- > > ... to trigger a NULL pointer dereference, since in > sctp_packet_transmit(), it discovers that an AUTH chunk is > being queued for xmit, and thus it calls sctp_auth_calculate_hmac(). > > Since the asoc->active_key_id is still inherited from the end > point, and the same as encoded into the chunk, it uses > asoc->asoc_shared_key (which is still NULL) as an asoc_key and > dereferences it in ... > > crypto_hash_setkey(desc.tfm, &asoc_key->data[0], asoc_key->len) > > ... causing an oops. All this happens because sctp_make_cookie_ack() > called with the new association has the peer.auth_capable=1 and > therefore marks the chunk with auth=1 after checking > sctp_auth_send_cid(), but it is *actually* sent later on over > the then *updated* association's transport that didn't initialize > its shared key due to peer.auth_capable=0. > > The correct fix is to update to the new peer.auth_capable > value as well in the collision case via sctp_assoc_update(), > so that in case the collision migrated from 0 -> 1, > sctp_auth_asoc_init_active_key() can properly recalculate > the secret. This therefore fixes the observed server panic. > > Fixes: 730fc3d05cd4 ("[SCTP]: Implete SCTP-AUTH parameter processing") > Reported-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > Signed-off-by: Daniel Borkmann <dborkman@redhat.com> > Tested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> Acked-by: Neil Horman <nhorman@tuxdriver.com> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/17/2014 02:05 PM, Daniel Borkmann wrote: > Jason reported an oops caused by SCTP on his ARM machine with > SCTP authentication enabled: > > Internal error: Oops: 17 [#1] ARM > CPU: 0 PID: 104 Comm: sctp-test Not tainted 3.13.0-68744-g3632f30c9b20-dirty #1 > task: c6eefa40 ti: c6f52000 task.ti: c6f52000 > PC is at sctp_auth_calculate_hmac+0xc4/0x10c > LR is at sg_init_table+0x20/0x38 > pc : [<c024bb80>] lr : [<c00f32dc>] psr: 40000013 > sp : c6f538e8 ip : 00000000 fp : c6f53924 > r10: c6f50d80 r9 : 00000000 r8 : 00010000 > r7 : 00000000 r6 : c7be4000 r5 : 00000000 r4 : c6f56254 > r3 : c00c8170 r2 : 00000001 r1 : 00000008 r0 : c6f1e660 > Flags: nZcv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user > Control: 0005397f Table: 06f28000 DAC: 00000015 > Process sctp-test (pid: 104, stack limit = 0xc6f521c0) > Stack: (0xc6f538e8 to 0xc6f54000) > [...] > Backtrace: > [<c024babc>] (sctp_auth_calculate_hmac+0x0/0x10c) from [<c0249af8>] (sctp_packet_transmit+0x33c/0x5c8) > [<c02497bc>] (sctp_packet_transmit+0x0/0x5c8) from [<c023e96c>] (sctp_outq_flush+0x7fc/0x844) > [<c023e170>] (sctp_outq_flush+0x0/0x844) from [<c023ef78>] (sctp_outq_uncork+0x24/0x28) > [<c023ef54>] (sctp_outq_uncork+0x0/0x28) from [<c0234364>] (sctp_side_effects+0x1134/0x1220) > [<c0233230>] (sctp_side_effects+0x0/0x1220) from [<c02330b0>] (sctp_do_sm+0xac/0xd4) > [<c0233004>] (sctp_do_sm+0x0/0xd4) from [<c023675c>] (sctp_assoc_bh_rcv+0x118/0x160) > [<c0236644>] (sctp_assoc_bh_rcv+0x0/0x160) from [<c023d5bc>] (sctp_inq_push+0x6c/0x74) > [<c023d550>] (sctp_inq_push+0x0/0x74) from [<c024a6b0>] (sctp_rcv+0x7d8/0x888) > > While we already had various kind of bugs in that area > ec0223ec48a9 ("net: sctp: fix sctp_sf_do_5_1D_ce to verify if > we/peer is AUTH capable") and b14878ccb7fa ("net: sctp: cache > auth_enable per endpoint"), this one is a bit of a different > kind. > > Giving a bit more background on why SCTP authentication is > needed can be found in RFC4895: > > SCTP uses 32-bit verification tags to protect itself against > blind attackers. These values are not changed during the > lifetime of an SCTP association. > > Looking at new SCTP extensions, there is the need to have a > method of proving that an SCTP chunk(s) was really sent by > the original peer that started the association and not by a > malicious attacker. > > To cause this bug, we're triggering an INIT collision between > peers; normal SCTP handshake where both sides intent to > authenticate packets contains RANDOM; CHUNKS; HMAC-ALGO > parameters that are being negotiated among peers: > > ---------- INIT[RANDOM; CHUNKS; HMAC-ALGO] ----------> > <------- INIT-ACK[RANDOM; CHUNKS; HMAC-ALGO] --------- > -------------------- COOKIE-ECHO --------------------> > <-------------------- COOKIE-ACK --------------------- > > RFC4895 says that each endpoint therefore knows its own random > number and the peer's random number *after* the association has > been established. The local and peer's random number along > with the shared key are then part of the secret used for > calculating the HMAC in the AUTH chunk. > > Now, in our scenario, we have 2 threads with 1 non-blocking > SEQ_PACKET socket each, setting up common shared SCTP_AUTH_KEY > and SCTP_AUTH_ACTIVE_KEY properly, and each of them calling > sctp_bindx(3), listen(2) and connect(2) against each other, > thus the handshake looks similar to this, e.g.: > > ---------- INIT[RANDOM; CHUNKS; HMAC-ALGO] ----------> > <------- INIT-ACK[RANDOM; CHUNKS; HMAC-ALGO] --------- > <--------- INIT[RANDOM; CHUNKS; HMAC-ALGO] ----------- > -------- INIT-ACK[RANDOM; CHUNKS; HMAC-ALGO] --------> > ... > > Since such collisions can also happen with verification tags, > the RFC4895 for AUTH rather vaguely says under section 6.1: > > In case of INIT collision, the rules governing the handling > of this Random Number follow the same pattern as those for > the Verification Tag, as explained in Section 5.2.4 of > RFC 2960 [5]. Therefore, each endpoint knows its own Random > Number and the peer's Random Number after the association > has been established. > > In RFC2960, section 5.2.4, we're eventually hitting Action B: > > B) In this case, both sides may be attempting to start an > association at about the same time but the peer endpoint > started its INIT after responding to the local endpoint's > INIT. Thus it may have picked a new Verification Tag not > being aware of the previous Tag it had sent this endpoint. > The endpoint should stay in or enter the ESTABLISHED > state but it MUST update its peer's Verification Tag from > the State Cookie, stop any init or cookie timers that may > running and send a COOKIE ACK. > > In other words, the handling of the Random parameter is the > same as behavior for the Verification Tag as described in > Action B of section 5.2.4. > > Looking at the code, we exactly hit the sctp_sf_do_dupcook_b() > case which triggers an SCTP_CMD_UPDATE_ASSOC command to the > side effect interpreter, and in fact it copies over > peer_{random, hmacs, chunks} parameter from the newly created > association to update the existing one. > > Also, the old asoc_shared_key is being released and based on the > new params, sctp_auth_asoc_init_active_key() updated. However, > the issue observed in this case is that the previous > asoc->peer.auth_capable was 0 [note, it was 0 first since > peer.auth_capable is only being set on reception of INIT], > and has *not* been updated, so that instead of creating a > new secret, we're doing an early return from the function > sctp_auth_asoc_init_active_key() leaving asoc->asoc_shared_key > as NULL. However, we now have to authenticate chunks from > the updated chunk list (e.g. COOKIE-ACK, ...). Why is the original value of asoc->peer.auth_capable = 0? In case of collision, asoc is the old association that existed on the system. That association was created as part of sending the INIT. If it is processing a duplicate COOKIE-ECHO as you say, then it has already processed the INIT-ACK and should have determined that the peer is auth capable. Thus the capability of the new and the old associations should be same if we are in fact processing case B (collision). If not, then something else if wrong and my guess is that all other capabilities would be wrong too. -vlad > > That in fact causes the server side when responding with ... > > <------------------ AUTH; COOKIE-ACK ----------------- > > ... to trigger a NULL pointer dereference, since in > sctp_packet_transmit(), it discovers that an AUTH chunk is > being queued for xmit, and thus it calls sctp_auth_calculate_hmac(). > > Since the asoc->active_key_id is still inherited from the end > point, and the same as encoded into the chunk, it uses > asoc->asoc_shared_key (which is still NULL) as an asoc_key and > dereferences it in ... > > crypto_hash_setkey(desc.tfm, &asoc_key->data[0], asoc_key->len) > > ... causing an oops. All this happens because sctp_make_cookie_ack() > called with the new association has the peer.auth_capable=1 and > therefore marks the chunk with auth=1 after checking > sctp_auth_send_cid(), but it is *actually* sent later on over > the then *updated* association's transport that didn't initialize > its shared key due to peer.auth_capable=0. > > The correct fix is to update to the new peer.auth_capable > value as well in the collision case via sctp_assoc_update(), > so that in case the collision migrated from 0 -> 1, > sctp_auth_asoc_init_active_key() can properly recalculate > the secret. This therefore fixes the observed server panic. > > Fixes: 730fc3d05cd4 ("[SCTP]: Implete SCTP-AUTH parameter processing") > Reported-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > Signed-off-by: Daniel Borkmann <dborkman@redhat.com> > Tested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > --- > net/sctp/associola.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/net/sctp/associola.c b/net/sctp/associola.c > index 9de23a2..06a9ee6 100644 > --- a/net/sctp/associola.c > +++ b/net/sctp/associola.c > @@ -1097,6 +1097,7 @@ void sctp_assoc_update(struct sctp_association *asoc, > asoc->c = new->c; > asoc->peer.rwnd = new->peer.rwnd; > asoc->peer.sack_needed = new->peer.sack_needed; > + asoc->peer.auth_capable = new->peer.auth_capable; > asoc->peer.i = new->peer.i; > sctp_tsnmap_init(&asoc->peer.tsn_map, SCTP_TSN_MAP_INITIAL, > asoc->peer.i.initial_tsn, GFP_ATOMIC); > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/18/2014 04:38 PM, Vlad Yasevich wrote: ... > Why is the original value of asoc->peer.auth_capable = 0? > In case of collision, asoc is the old association that > existed on the system. That association was created as part of > sending the INIT. If it is processing a duplicate COOKIE-ECHO > as you say, then it has already processed the INIT-ACK and > should have determined that the peer is auth capable. > > Thus the capability of the new and the old associations should > be same if we are in fact processing case B (collision). > > If not, then something else if wrong and my guess is that all > other capabilities would be wrong too. I agree that they might likely also be flawed. Ok, let me dig further. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/18/2014 03:17 PM, Daniel Borkmann wrote: > On 07/18/2014 04:38 PM, Vlad Yasevich wrote: > ... >> Why is the original value of asoc->peer.auth_capable = 0? >> In case of collision, asoc is the old association that >> existed on the system. That association was created as part of >> sending the INIT. If it is processing a duplicate COOKIE-ECHO >> as you say, then it has already processed the INIT-ACK and >> should have determined that the peer is auth capable. >> >> Thus the capability of the new and the old associations should >> be same if we are in fact processing case B (collision). >> >> If not, then something else if wrong and my guess is that all >> other capabilities would be wrong too. > > I agree that they might likely also be flawed. > > Ok, let me dig further. So I think I know why case D ends up not authenticating the COOKIE-ACK. Most likely the reason is the following statement: repl = sctp_make_cookie_ack(new_asoc, chunk); Note that we use new_asoc, instead of current asoc. Not sure why case B is dumping core yet. -vlad -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/18/2014 11:59 PM, Vlad Yasevich wrote: > On 07/18/2014 03:17 PM, Daniel Borkmann wrote: >> On 07/18/2014 04:38 PM, Vlad Yasevich wrote: >> ... >>> Why is the original value of asoc->peer.auth_capable = 0? >>> In case of collision, asoc is the old association that >>> existed on the system. That association was created as part of >>> sending the INIT. If it is processing a duplicate COOKIE-ECHO >>> as you say, then it has already processed the INIT-ACK and >>> should have determined that the peer is auth capable. >>> >>> Thus the capability of the new and the old associations should >>> be same if we are in fact processing case B (collision). >>> >>> If not, then something else if wrong and my guess is that all >>> other capabilities would be wrong too. >> >> I agree that they might likely also be flawed. >> >> Ok, let me dig further. > > So I think I know why case D ends up not authenticating the COOKIE-ACK. > Most likely the reason is the following statement: > repl = sctp_make_cookie_ack(new_asoc, chunk); > > Note that we use new_asoc, instead of current asoc. Thanks, I will give it a try. Btw, noticed also that when we have AUTH + COOKIE_ECHO collisions, we don't seem to handle them properly either. The normal case works fine, but in case of a collision both sides seem to use wrong RANDOM etc params, and thus discard the handshake due to bad signature. > Not sure why case B is dumping core yet. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/19/2014 12:13 AM, Daniel Borkmann wrote: > On 07/18/2014 11:59 PM, Vlad Yasevich wrote: >> On 07/18/2014 03:17 PM, Daniel Borkmann wrote: >>> On 07/18/2014 04:38 PM, Vlad Yasevich wrote: >>> ... >>>> Why is the original value of asoc->peer.auth_capable = 0? >>>> In case of collision, asoc is the old association that >>>> existed on the system. That association was created as part of >>>> sending the INIT. If it is processing a duplicate COOKIE-ECHO >>>> as you say, then it has already processed the INIT-ACK and >>>> should have determined that the peer is auth capable. >>>> >>>> Thus the capability of the new and the old associations should >>>> be same if we are in fact processing case B (collision). What I can see is the following that leads to this situation: 1) asoc A sends the INIT, goes from CLOSED into COOKIE_WAIT 2) asoc B receives it, calls into sctp_sf_do_5_1B_init() where it actually creates asoc B, responds with INIT_ACK, goes from CLOSED into COOKIE_WAIT 3) asoc A receives INIT, thus collision, calls into sctp_sf_do_5_2_1_siminit() 3.1) asoc A calls into sctp_sf_do_unexpected_init(), creates a temp asoc, does sctp_process_init() on the temp asoc (auth_cap=1, random etc set), replies w/ temp asoc with INIT_ACK 4) asoc B gets INIT_ACK, calls sctp_sf_do_5_1C_ack (and thus SCTP_PEER_INIT via interpreter), sees auth_cap=1, stores random etc; asoc B transitions from COOKIE_WAIT into COOKIE_ECHOED 5) asoc A calls into sctp_sf_do_5_2_4_dupcook(), does the tietag compare, finds action B, creates temp asoc calls sctp_process_init() on it sees auth_cap=1, random etc; then we call into sctp_assoc_update() and migrate all params; what I see there is that random, chunks, hmac migrate from NULL each to the new values stored in the temp asoc (and thus we'd need auth_cap as well to be correct); after that, I see that asoc A goes from COOKIE_WAIT into ESTABLISHED (which seems to be in accordance to the RFC: "The endpoint should stay in or enter the ESTABLISHED state but it MUST ...") 6) later on, asoc B goes from COOKIE_ECHOED into ESTABLISHED So that led me to the resolution of transferring 'caps' over via sctp_assoc_update(). In that case, asoc A transitions from 0 -> 1 as previous 'caps' haven't been stored in the actual asoc. It stayed so far always in a temp asoc that we threw away after a reply. >>>> If not, then something else if wrong and my guess is that all >>>> other capabilities would be wrong too. >>> >>> I agree that they might likely also be flawed. >>> >>> Ok, let me dig further. >> >> So I think I know why case D ends up not authenticating the COOKIE-ACK. >> Most likely the reason is the following statement: >> repl = sctp_make_cookie_ack(new_asoc, chunk); >> >> Note that we use new_asoc, instead of current asoc. > > Thanks, I will give it a try. > > Btw, noticed also that when we have AUTH + COOKIE_ECHO collisions, > we don't seem to handle them properly either. The normal case works > fine, but in case of a collision both sides seem to use wrong RANDOM > etc params, and thus discard the handshake due to bad signature. > >> Not sure why case B is dumping core yet. > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/18/2014 11:59 PM, Vlad Yasevich wrote: > On 07/18/2014 03:17 PM, Daniel Borkmann wrote: >> On 07/18/2014 04:38 PM, Vlad Yasevich wrote: >> ... >>> Why is the original value of asoc->peer.auth_capable = 0? >>> In case of collision, asoc is the old association that >>> existed on the system. That association was created as part of >>> sending the INIT. If it is processing a duplicate COOKIE-ECHO >>> as you say, then it has already processed the INIT-ACK and >>> should have determined that the peer is auth capable. >>> >>> Thus the capability of the new and the old associations should >>> be same if we are in fact processing case B (collision). >>> >>> If not, then something else if wrong and my guess is that all >>> other capabilities would be wrong too. >> >> I agree that they might likely also be flawed. >> >> Ok, let me dig further. > > So I think I know why case D ends up not authenticating the COOKIE-ACK. > Most likely the reason is the following statement: > repl = sctp_make_cookie_ack(new_asoc, chunk); That in fact lets COOKIE-ACK be AUTH'ed which weren't before, so we should add that into the set. What happens though is that subsequent AUTH+HBs from both sides remain unanswered, so no AUTH+HB_ACK. This issue is independant of s/new_asoc/asoc/ though; disabling auth_enabled at both sides makes HB+HB_ACKs work. > Note that we use new_asoc, instead of current asoc. > > Not sure why case B is dumping core yet. > > -vlad > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/18/2014 07:03 PM, Daniel Borkmann wrote: > On 07/19/2014 12:13 AM, Daniel Borkmann wrote: >> On 07/18/2014 11:59 PM, Vlad Yasevich wrote: >>> On 07/18/2014 03:17 PM, Daniel Borkmann wrote: >>>> On 07/18/2014 04:38 PM, Vlad Yasevich wrote: >>>> ... >>>>> Why is the original value of asoc->peer.auth_capable = 0? >>>>> In case of collision, asoc is the old association that >>>>> existed on the system. That association was created as part of >>>>> sending the INIT. If it is processing a duplicate COOKIE-ECHO >>>>> as you say, then it has already processed the INIT-ACK and >>>>> should have determined that the peer is auth capable. >>>>> >>>>> Thus the capability of the new and the old associations should >>>>> be same if we are in fact processing case B (collision). > > What I can see is the following that leads to this situation: > > 1) asoc A sends the INIT, goes from CLOSED into COOKIE_WAIT > 2) asoc B receives it, calls into sctp_sf_do_5_1B_init() where it > actually creates asoc B, responds with INIT_ACK, goes from CLOSED > into COOKIE_WAIT I think this is a race. asoc B doesn't exist yet. we have a listening socket that responds normally to the INIT-ACK. The next thing that happens is the app initiates a connection thus creating asoc B and triggering INIT. > 3) asoc A receives INIT, thus collision, calls into sctp_sf_do_5_2_1_siminit() > 3.1) asoc A calls into sctp_sf_do_unexpected_init(), creates a temp asoc, > does sctp_process_init() on the temp asoc (auth_cap=1, random etc set), > replies w/ temp asoc with INIT_ACK > 4) asoc B gets INIT_ACK, calls sctp_sf_do_5_1C_ack (and thus SCTP_PEER_INIT > via interpreter), sees auth_cap=1, stores random etc; asoc B transitions > from COOKIE_WAIT into COOKIE_ECHOED > 5) asoc A calls into sctp_sf_do_5_2_4_dupcook(), does the tietag compare, > finds action B, creates temp asoc calls sctp_process_init() on it > sees auth_cap=1, random etc; then we call into sctp_assoc_update() > and migrate all params; what I see there is that random, chunks, hmac > migrate from NULL each to the new values stored in the temp asoc > (and thus we'd need auth_cap as well to be correct); after that, I > see that asoc A goes from COOKIE_WAIT into ESTABLISHED (which seems > to be in accordance to the RFC: "The endpoint should stay in or enter > the ESTABLISHED state but it MUST ...") I see. > 6) later on, asoc B goes from COOKIE_ECHOED into ESTABLISHED > > So that led me to the resolution of transferring 'caps' over via > sctp_assoc_update(). In that case, asoc A transitions from 0 -> 1 > as previous 'caps' haven't been stored in the actual asoc. It stayed > so far always in a temp asoc that we threw away after a reply. Thanks for the analysis. The collisions in COOKIE_WAIT state is definitely a hole and it looks like all capabilities need to be updated and we should probably do an audit to make sure we don't miss anything else. -vlad > >>>>> If not, then something else if wrong and my guess is that all >>>>> other capabilities would be wrong too. >>>> >>>> I agree that they might likely also be flawed. >>>> >>>> Ok, let me dig further. >>> >>> So I think I know why case D ends up not authenticating the COOKIE-ACK. >>> Most likely the reason is the following statement: >>> repl = sctp_make_cookie_ack(new_asoc, chunk); >>> >>> Note that we use new_asoc, instead of current asoc. >> >> Thanks, I will give it a try. >> >> Btw, noticed also that when we have AUTH + COOKIE_ECHO collisions, >> we don't seem to handle them properly either. The normal case works >> fine, but in case of a collision both sides seem to use wrong RANDOM >> etc params, and thus discard the handshake due to bad signature. >> >>> Not sure why case B is dumping core yet. >> -- >> To unsubscribe from this list: send the line "unsubscribe netdev" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/19/2014 04:23 AM, Vlad Yasevich wrote: > On 07/18/2014 07:03 PM, Daniel Borkmann wrote: >> On 07/19/2014 12:13 AM, Daniel Borkmann wrote: >>> On 07/18/2014 11:59 PM, Vlad Yasevich wrote: >>>> On 07/18/2014 03:17 PM, Daniel Borkmann wrote: >>>>> On 07/18/2014 04:38 PM, Vlad Yasevich wrote: >>>>> ... >>>>>> Why is the original value of asoc->peer.auth_capable = 0? >>>>>> In case of collision, asoc is the old association that >>>>>> existed on the system. That association was created as part of >>>>>> sending the INIT. If it is processing a duplicate COOKIE-ECHO >>>>>> as you say, then it has already processed the INIT-ACK and >>>>>> should have determined that the peer is auth capable. >>>>>> >>>>>> Thus the capability of the new and the old associations should >>>>>> be same if we are in fact processing case B (collision). >> >> What I can see is the following that leads to this situation: >> >> 1) asoc A sends the INIT, goes from CLOSED into COOKIE_WAIT >> 2) asoc B receives it, calls into sctp_sf_do_5_1B_init() where it >> actually creates asoc B, responds with INIT_ACK, goes from CLOSED >> into COOKIE_WAIT > > I think this is a race. asoc B doesn't exist yet. we have a listening > socket that responds normally to the INIT-ACK. The next thing that happens > is the app initiates a connection thus creating asoc B and triggering INIT. > >> 3) asoc A receives INIT, thus collision, calls into sctp_sf_do_5_2_1_siminit() >> 3.1) asoc A calls into sctp_sf_do_unexpected_init(), creates a temp asoc, >> does sctp_process_init() on the temp asoc (auth_cap=1, random etc set), >> replies w/ temp asoc with INIT_ACK >> 4) asoc B gets INIT_ACK, calls sctp_sf_do_5_1C_ack (and thus SCTP_PEER_INIT >> via interpreter), sees auth_cap=1, stores random etc; asoc B transitions >> from COOKIE_WAIT into COOKIE_ECHOED >> 5) asoc A calls into sctp_sf_do_5_2_4_dupcook(), does the tietag compare, >> finds action B, creates temp asoc calls sctp_process_init() on it >> sees auth_cap=1, random etc; then we call into sctp_assoc_update() >> and migrate all params; what I see there is that random, chunks, hmac >> migrate from NULL each to the new values stored in the temp asoc >> (and thus we'd need auth_cap as well to be correct); after that, I >> see that asoc A goes from COOKIE_WAIT into ESTABLISHED (which seems >> to be in accordance to the RFC: "The endpoint should stay in or enter >> the ESTABLISHED state but it MUST ...") > > I see. > >> 6) later on, asoc B goes from COOKIE_ECHOED into ESTABLISHED >> >> So that led me to the resolution of transferring 'caps' over via >> sctp_assoc_update(). In that case, asoc A transitions from 0 -> 1 >> as previous 'caps' haven't been stored in the actual asoc. It stayed >> so far always in a temp asoc that we threw away after a reply. > > Thanks for the analysis. The collisions in COOKIE_WAIT state is definitely > a hole and it looks like all capabilities need to be updated and we should > probably do an audit to make sure we don't miss anything else. Thanks, I'll look into it and will respin the patch. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Vlad, On 07/18/2014 11:59 PM, Vlad Yasevich wrote: ... > So I think I know why case D ends up not authenticating the COOKIE-ACK. > Most likely the reason is the following statement: > repl = sctp_make_cookie_ack(new_asoc, chunk); > > Note that we use new_asoc, instead of current asoc. Are you sending out a patch for this? Thanks, Daniel -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/22/2014 09:25 AM, Daniel Borkmann wrote: > Hi Vlad, > > On 07/18/2014 11:59 PM, Vlad Yasevich wrote: > ... >> So I think I know why case D ends up not authenticating the COOKIE-ACK. >> Most likely the reason is the following statement: >> repl = sctp_make_cookie_ack(new_asoc, chunk); >> >> Note that we use new_asoc, instead of current asoc. > > Are you sending out a patch for this? I didn't plan on it since you said there are further issues. I thought you were still looking. -vlad > > Thanks, > > Daniel -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/22/2014 06:41 PM, Vlad Yasevich wrote: > On 07/22/2014 09:25 AM, Daniel Borkmann wrote: >> Hi Vlad, >> >> On 07/18/2014 11:59 PM, Vlad Yasevich wrote: >> ... >>> So I think I know why case D ends up not authenticating the COOKIE-ACK. >>> Most likely the reason is the following statement: >>> repl = sctp_make_cookie_ack(new_asoc, chunk); >>> >>> Note that we use new_asoc, instead of current asoc. >> >> Are you sending out a patch for this? > > I didn't plan on it since you said there are further issues. I thought you > were still looking. Ok, understood. Yeah, I'm looking further for the other cases as well. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/sctp/associola.c b/net/sctp/associola.c index 9de23a2..06a9ee6 100644 --- a/net/sctp/associola.c +++ b/net/sctp/associola.c @@ -1097,6 +1097,7 @@ void sctp_assoc_update(struct sctp_association *asoc, asoc->c = new->c; asoc->peer.rwnd = new->peer.rwnd; asoc->peer.sack_needed = new->peer.sack_needed; + asoc->peer.auth_capable = new->peer.auth_capable; asoc->peer.i = new->peer.i; sctp_tsnmap_init(&asoc->peer.tsn_map, SCTP_TSN_MAP_INITIAL, asoc->peer.i.initial_tsn, GFP_ATOMIC);