Skip to content

Commit

Permalink
Making non-dispatchable packet ignored instead of causing connection …
Browse files Browse the repository at this point in the history 
…rejection. Fixed handling of BADSECRET/NOSECRET states in case of periodic HS (#414)
  • Loading branch information
@ethouris @rndi
ethouris authored and rndi committed Jun 13, 2018
1 parent 7dac65e commit e9e0611
Show file tree
Hide file tree
Showing 2 changed files with 70 additions and 21 deletions.
58 changes: 45 additions & 13 deletions srtcore/core.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2963,7 +2963,8 @@ EConnectStatus CUDT::processAsyncConnectResponse(const CPacket& pkt) ATR_NOEXCEP
cst = processConnectResponse(pkt, &e, false);

HLOGC(mglog.Debug, log << CONID() << "processAsyncConnectResponse: response processing result: "
<< ConnectStatusStr(cst));
<< ConnectStatusStr(cst) << "REQ-TIME LOW to enforce immediate response");
m_llLastReqTime = 0;

return cst;
}
Expand Down Expand Up @@ -3193,21 +3194,52 @@ EConnectStatus CUDT::processRendezvous(ref_t<CPacket> reqpkt, const CPacket& res
size_t msgsize = m_pCryptoControl->getKmMsg_size(0);
if (msgsize == 0)
{
LOGC(mglog.Error, log << "processRendezvous: IPE: PERIODIC HS: NO KMREQ RECORDED.");
return CONN_REJECT;
}
switch (m_pCryptoControl->m_RcvKmState)
{
// If the KMX process ended up with a failure, the KMX is not recorded.
// In this case as the KMRSP answer the "failure status" should be crafted.
case SRT_KM_S_NOSECRET:
case SRT_KM_S_BADSECRET:
{
HLOGC(mglog.Debug, log << "processRendezvous: No KMX recorded, status = NOSECRET. Respond with NOSECRET.");

// Just do the same thing as in CCryptoControl::processSrtMsg_KMREQ for that case,
// that is, copy the NOSECRET code into KMX message.
memcpy(kmdata, &m_pCryptoControl->m_RcvKmState, sizeof(int32_t));
kmdatasize = 1;
}
break;

kmdatasize = msgsize/4;
if (msgsize > kmdatasize*4)
{
// Sanity check
LOGC(mglog.Error, log << "IPE: KMX data not aligned to 4 bytes! size=" << msgsize);
memset(kmdata+(kmdatasize*4), 0, msgsize - (kmdatasize*4));
++kmdatasize;
default:
// Remaining values:
// UNSECURED: should not fall here at alll
// SECURING: should not happen in HSv5
// SECURED: should have received the recorded KMX correctly (getKmMsg_size(0) > 0)
{
// Remaining situations:
// - password only on this site: shouldn't be considered to be sent to a no-password site
LOGC(mglog.Error, log << "processRendezvous: IPE: PERIODIC HS: NO KMREQ RECORDED KMSTATE: RCV="
<< KmStateStr(m_pCryptoControl->m_RcvKmState) << " SND="
<< KmStateStr(m_pCryptoControl->m_SndKmState));
return CONN_REJECT;
}
break;
}
}
else
{
kmdatasize = msgsize/4;
if (msgsize > kmdatasize*4)
{
// Sanity check
LOGC(mglog.Error, log << "IPE: KMX data not aligned to 4 bytes! size=" << msgsize);
memset(kmdata+(kmdatasize*4), 0, msgsize - (kmdatasize*4));
++kmdatasize;
}

HLOGC(mglog.Debug, log << "processRendezvous: getting KM DATA from the fore-recorded KMX from KMREQ, size=" << kmdatasize);
memcpy(kmdata, m_pCryptoControl->getKmMsg_data(0), msgsize);
HLOGC(mglog.Debug, log << "processRendezvous: getting KM DATA from the fore-recorded KMX from KMREQ, size=" << kmdatasize);
memcpy(kmdata, m_pCryptoControl->getKmMsg_data(0), msgsize);
}
}
else
{
Expand Down
33 changes: 25 additions & 8 deletions srtcore/queue.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -932,7 +932,9 @@ void CRendezvousQueue::updateConnStatus(EReadStatus rst, EConnectStatus cst, con
// done when "it's not the time"?
if (CTimer::getTime() >= i->m_ullTTL)
{
HLOGC(mglog.Debug, log << "RendezvousQueue: EXPIRED. removing from queue");
HLOGC(mglog.Debug, log << "RendezvousQueue: EXPIRED ("
<< (i->m_ullTTL ? "enforced on FAILURE" : "passed TTL")
<< ". removing from queue");
// connection timer expired, acknowledge app via epoll
i->m_pUDT->m_bConnecting = false;
CUDT::s_UDTUnited.m_EPoll.update_events(i->m_iID, i->m_pUDT->m_sPollID, UDT_EPOLL_ERR, true);
Expand Down Expand Up @@ -1114,6 +1116,11 @@ void* CRcvQueue::worker(void* param)
cst = self->worker_ProcessAddressedPacket(id, unit, &sa);
}
HLOGC(mglog.Debug, log << self->CONID() << "worker: result for the unit: " << ConnectStatusStr(cst));
if (cst == CONN_AGAIN)
{
HLOGC(mglog.Debug, log << self->CONID() << "worker: packet not dispatched, continuing reading.");
continue;
}
have_received = true;
}
else if (rst == RST_ERROR)
Expand Down Expand Up @@ -1337,8 +1344,8 @@ EConnectStatus CRcvQueue::worker_ProcessAddressedPacket(int32_t id, CUnit* unit,
HLOGC(mglog.Debug, log << CONID() << "Packet for SID=" << id << " asoc with " << SockaddrToString(u->m_pPeerAddr)
<< " received from " << SockaddrToString(addr) << " (CONSIDERED ATTACK ATTEMPT)");
// This came not from the address that is the peer associated
// with the socket. Reject.
return CONN_REJECT;
// with the socket. Ignore it.
return CONN_AGAIN;
}

if (!u->m_bConnected || u->m_bBroken || u->m_bClosing)
Expand Down Expand Up @@ -1378,19 +1385,29 @@ EConnectStatus CRcvQueue::worker_TryAsyncRend_OrStore(int32_t id, CUnit* unit, c
CUDT* u = m_pRendezvousQueue->retrieve(addr, Ref(id));
if ( !u )
{
// XXX this socket is then completely unknown to the system.
// May be nice to send some rejection info to the peer.
// this socket is then completely unknown to the system.
// Note that this situation may also happen at a very unfortunate
// coincidence that the socket is already bound, but the registerConnector()
// has not yet started. In case of rendezvous this may mean that the other
// side just started sending its handshake packets, the local side has already
// run the CRcvQueue::worker thread, and this worker thread is trying to dispatch
// the handshake packet too early, before the dispatcher has a chance to see
// this socket registerred in the RendezvousQueue, which causes the packet unable
// to be dispatched. Therefore simply treat every "out of band" packet (with socket
// not belonging to the connection and not registered as rendezvous) as "possible
// attach" and ignore it. This also should better protect the rendezvous socket
// against a rogue connector.
if ( id == 0 )
{
HLOGC(mglog.Debug, log << CONID() << "AsyncOrRND: no sockets expect connection from "
<< SockaddrToString(addr) << " - POSSIBLE ATTACK");
<< SockaddrToString(addr) << " - POSSIBLE ATTACK, ignore packet");
}
else
{
HLOGC(mglog.Debug, log << CONID() << "AsyncOrRND: no sockets expect socket " << id << " from "
<< SockaddrToString(addr) << " - POSSIBLE ATTACK");
<< SockaddrToString(addr) << " - POSSIBLE ATTACK, ignore packet");
}
return CONN_REJECT;
return CONN_AGAIN; // This means that the packet should be ignored.
}

// asynchronous connect: call connect here
Expand Down

0 comments on commit e9e0611

Please sign in to comment.