Skip to content

Commit 5889a2c

Browse files
maxsharabaykomaxsharabayko
authored
[core] AES-GCM payload length check (#2591).
The negotiated MTU size value is used to determine the maximum allowed payload size. Fixed CSndBuffer::addBufferFromFile.
1 parent 45232ad commit 5889a2c

File tree

5 files changed

+80
-40
lines changed

5 files changed

+80
-40
lines changed

‎srtcore/buffer_snd.cpp‎

Lines changed: 30 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -132,14 +132,12 @@ CSndBuffer::~CSndBuffer()
132132

133133
void CSndBuffer::addBuffer(const char* data, int len, SRT_MSGCTRL& w_mctrl)
134134
{
135-
int32_t& w_msgno = w_mctrl.msgno;
136-
int32_t& w_seqno = w_mctrl.pktseq;
137-
int64_t& w_srctime = w_mctrl.srctime;
138-
const int& ttl = w_mctrl.msgttl;
139-
const int iPktLen = m_iBlockLen - m_iAuthTagSize; // Payload length per packet.
140-
int iNumBlocks = len / iPktLen;
141-
if ((len % m_iBlockLen) != 0)
142-
++iNumBlocks;
135+
int32_t& w_msgno = w_mctrl.msgno;
136+
int32_t& w_seqno = w_mctrl.pktseq;
137+
int64_t& w_srctime = w_mctrl.srctime;
138+
const int& ttl = w_mctrl.msgttl;
139+
const int iPktLen = getMaxPacketLen();
140+
const int iNumBlocks = countNumPacketsRequired(len, iPktLen);
143141

144142
HLOGC(bslog.Debug,
145143
log << "addBuffer: needs=" << iNumBlocks << " buffers for " << len << " bytes. Taken=" << m_iCount << "/" << m_iSize);
@@ -239,20 +237,18 @@ void CSndBuffer::addBuffer(const char* data, int len, SRT_MSGCTRL& w_mctrl)
239237

240238
int CSndBuffer::addBufferFromFile(fstream& ifs, int len)
241239
{
242-
const int iPktLen = m_iBlockLen - m_iAuthTagSize; // Payload length per packet.
243-
int iNumBlocks = len / iPktLen;
244-
if ((len % m_iBlockLen) != 0)
245-
++iNumBlocks;
240+
const int iPktLen = getMaxPacketLen();
241+
const int iNumBlocks = countNumPacketsRequired(len, iPktLen);
246242

247243
HLOGC(bslog.Debug,
248244
log << "addBufferFromFile: size=" << m_iCount << " reserved=" << m_iSize << " needs=" << iPktLen
249245
<< " buffers for " << len << " bytes");
250246

251247
// dynamically increase sender buffer
252-
while (iPktLen + m_iCount >= m_iSize)
248+
while (iNumBlocks + m_iCount >= m_iSize)
253249
{
254250
HLOGC(bslog.Debug,
255-
log << "addBufferFromFile: ... still lacking " << (iPktLen + m_iCount - m_iSize) << " buffers...");
251+
log << "addBufferFromFile: ... still lacking " << (iNumBlocks + m_iCount - m_iSize) << " buffers...");
256252
increase();
257253
}
258254

@@ -262,7 +258,7 @@ int CSndBuffer::addBufferFromFile(fstream& ifs, int len)
262258

263259
Block* s = m_pLastBlock;
264260
int total = 0;
265-
for (int i = 0; i < iPktLen; ++i)
261+
for (int i = 0; i < iNumBlocks; ++i)
266262
{
267263
if (ifs.bad() || ifs.fail() || ifs.eof())
268264
break;
@@ -282,7 +278,7 @@ int CSndBuffer::addBufferFromFile(fstream& ifs, int len)
282278
s->m_iMsgNoBitset = m_iNextMsgNo | MSGNO_PACKET_INORDER::mask;
283279
if (i == 0)
284280
s->m_iMsgNoBitset |= PacketBoundaryBits(PB_FIRST);
285-
if (i == iPktLen - 1)
281+
if (i == iNumBlocks - 1)
286282
s->m_iMsgNoBitset |= PacketBoundaryBits(PB_LAST);
287283
// NOTE: PB_FIRST | PB_LAST == PB_SOLO.
288284
// none of PB_FIRST & PB_LAST == PB_SUBSEQUENT.
@@ -296,7 +292,7 @@ int CSndBuffer::addBufferFromFile(fstream& ifs, int len)
296292
m_pLastBlock = s;
297293

298294
enterCS(m_BufLock);
299-
m_iCount += iPktLen;
295+
m_iCount += iNumBlocks;
300296
m_iBytesCount += total;
301297

302298
leaveCS(m_BufLock);
@@ -556,6 +552,22 @@ int CSndBuffer::getCurrBufSize() const
556552
return m_iCount;
557553
}
558554

555+
int CSndBuffer::getMaxPacketLen() const
556+
{
557+
return m_iBlockLen - m_iAuthTagSize;
558+
}
559+
560+
int CSndBuffer::countNumPacketsRequired(int iPldLen) const
561+
{
562+
const int iPktLen = getMaxPacketLen();
563+
return countNumPacketsRequired(iPldLen, iPktLen);
564+
}
565+
566+
int CSndBuffer::countNumPacketsRequired(int iPldLen, int iPktLen) const
567+
{
568+
return (iPldLen + iPktLen - 1) / iPktLen;
569+
}
570+
559571
namespace {
560572
int round_val(double val)
561573
{
@@ -590,7 +602,7 @@ void CSndBuffer::updAvgBufSize(const steady_clock::time_point& now)
590602
m_mavg.update(now, pkts, bytes, timespan_ms);
591603
}
592604

593-
int CSndBuffer::getCurrBufSize(int& w_bytes, int& w_timespan)
605+
int CSndBuffer::getCurrBufSize(int& w_bytes, int& w_timespan) const
594606
{
595607
w_bytes = m_iBytesCount;
596608
/*

‎srtcore/buffer_snd.h‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -161,7 +161,22 @@ class CSndBuffer
161161

162162
void updAvgBufSize(const time_point& time);
163163
int getAvgBufSize(int& bytes, int& timespan);
164-
int getCurrBufSize(int& bytes, int& timespan);
164+
int getCurrBufSize(int& bytes, int& timespan) const;
165+
166+
167+
/// Het maximum payload length per packet.
168+
int getMaxPacketLen() const;
169+
170+
/// @brief Count the number of required packets to store the payload (message).
171+
/// @param iPldLen the length of the payload to check.
172+
/// @return the number of required data packets.
173+
int countNumPacketsRequired(int iPldLen) const;
174+
175+
/// @brief Count the number of required packets to store the payload (message).
176+
/// @param iPldLen the length of the payload to check.
177+
/// @param iMaxPktLen the maximum payload length of the packet (the value returned from getMaxPacketLen()).
178+
/// @return the number of required data packets.
179+
int countNumPacketsRequired(int iPldLen, int iMaxPktLen) const;
165180

166181
/// @brief Get the buffering delay of the oldest message in the buffer.
167182
/// @return the delay value.

‎srtcore/core.cpp‎

Lines changed: 18 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -871,13 +871,10 @@ string srt::CUDT::getstreamid(SRTSOCKET u)
871871

872872
// XXX REFACTOR: Make common code for CUDT constructor and clearData,
873873
// possibly using CUDT::construct.
874+
// Initial sequence number, loss, acknowledgement, etc.
874875
void srt::CUDT::clearData()
875876
{
876-
// Initial sequence number, loss, acknowledgement, etc.
877-
int udpsize = m_config.iMSS - CPacket::UDP_HDR_SIZE;
878-
879-
m_iMaxSRTPayloadSize = udpsize - CPacket::HDR_SIZE;
880-
877+
m_iMaxSRTPayloadSize = m_config.iMSS - CPacket::UDP_HDR_SIZE - CPacket::HDR_SIZE;
881878
HLOGC(cnlog.Debug, log << CONID() << "clearData: PAYLOAD SIZE: " << m_iMaxSRTPayloadSize);
882879

883880
m_iEXPCount = 1;
@@ -6442,15 +6439,20 @@ int srt::CUDT::sendmsg2(const char *data, int len, SRT_MSGCTRL& w_mctrl)
64426439
// to modify m_pSndBuffer and m_pSndLossList
64436440
const int iPktsTLDropped SRT_ATR_UNUSED = sndDropTooLate();
64446441

6445-
int minlen = 1; // Minimum sender buffer space required for STREAM API
6446-
if (m_config.bMessageAPI)
6442+
// For MESSAGE API the minimum outgoing buffer space required is
6443+
// the size that can carry over the whole message as passed here.
6444+
// Otherwise it is allowed to send less bytes.
6445+
const int iNumPktsRequired = m_config.bMessageAPI ? m_pSndBuffer->countNumPacketsRequired(len) : 1;
6446+
6447+
if (m_bTsbPd && iNumPktsRequired > 1)
64476448
{
6448-
// For MESSAGE API the minimum outgoing buffer space required is
6449-
// the size that can carry over the whole message as passed here.
6450-
minlen = (len + m_iMaxSRTPayloadSize - 1) / m_iMaxSRTPayloadSize;
6449+
LOGC(aslog.Error,
6450+
log << CONID() << "Message length (" << len << ") can't fit into a single data packet ("
6451+
<< m_pSndBuffer->getMaxPacketLen() << " bytes max).");
6452+
throw CUDTException(MJ_NOTSUP, MN_XSIZE, 0);
64516453
}
64526454

6453-
if (sndBuffersLeft() < minlen)
6455+
if (sndBuffersLeft() < iNumPktsRequired)
64546456
{
64556457
//>>We should not get here if SRT_ENABLE_TLPKTDROP
64566458
// XXX Check if this needs to be removed, or put to an 'else' condition for m_bTLPktDrop.
@@ -6463,15 +6465,15 @@ int srt::CUDT::sendmsg2(const char *data, int len, SRT_MSGCTRL& w_mctrl)
64636465

64646466
if (m_config.iSndTimeOut < 0)
64656467
{
6466-
while (stillConnected() && sndBuffersLeft() < minlen && m_bPeerHealth)
6468+
while (stillConnected() && sndBuffersLeft() < iNumPktsRequired && m_bPeerHealth)
64676469
m_SendBlockCond.wait(sendblock_lock);
64686470
}
64696471
else
64706472
{
64716473
const steady_clock::time_point exptime =
64726474
steady_clock::now() + milliseconds_from(m_config.iSndTimeOut);
64736475
THREAD_PAUSED();
6474-
while (stillConnected() && sndBuffersLeft() < minlen && m_bPeerHealth)
6476+
while (stillConnected() && sndBuffersLeft() < iNumPktsRequired && m_bPeerHealth)
64756477
{
64766478
if (!m_SendBlockCond.wait_until(sendblock_lock, exptime))
64776479
break;
@@ -6497,7 +6499,7 @@ int srt::CUDT::sendmsg2(const char *data, int len, SRT_MSGCTRL& w_mctrl)
64976499
* we test twice if this code is outside the else section.
64986500
* This fix move it in the else (blocking-mode) section
64996501
*/
6500-
if (sndBuffersLeft() < minlen)
6502+
if (sndBuffersLeft() < iNumPktsRequired)
65016503
{
65026504
if (m_config.iSndTimeOut >= 0)
65036505
throw CUDTException(MJ_AGAIN, MN_XMTIMEOUT, 0);
@@ -9876,7 +9878,8 @@ int srt::CUDT::handleSocketPacketReception(const vector<CUnit*>& incoming, bool&
98769878
else
98779879
m_pRcvBuffer->dropMessage(u->m_Packet.getSeqNo(), u->m_Packet.getSeqNo(), SRT_MSGNO_NONE);
98789880

9879-
LOGC(qrlog.Error, log << CONID() << "AEAD decryption failed, breaking the connection.");
9881+
LOGC(qrlog.Error, log << CONID() << "AEAD decryption failed. Pkt seqno %" << u->m_Packet.getSeqNo()
9882+
<< ", msgno " << u->m_Packet.getMsgSeq(m_bPeerRexmitFlag) << ". Breaking the connection.");
98809883
m_bBroken = true;
98819884
m_iBrokenCounter = 0;
98829885
}

‎srtcore/socketconfig.cpp‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ struct CSrtConfigSetter<SRTO_MSS>
6969
{
7070
static void set(CSrtConfig& co, const void* optval, int optlen)
7171
{
72-
int ival = cast_optval<int>(optval, optlen);
72+
const int ival = cast_optval<int>(optval, optlen);
7373
if (ival < int(CPacket::UDP_HDR_SIZE + CHandShake::m_iContentSize))
7474
throw CUDTException(MJ_NOTSUP, MN_INVAL, 0);
7575

@@ -611,7 +611,7 @@ struct CSrtConfigSetter<SRTO_PAYLOADSIZE>
611611

612612
if (val > SRT_LIVE_MAX_PLSIZE)
613613
{
614-
LOGC(aclog.Error, log << "SRTO_PAYLOADSIZE: value exceeds SRT_LIVE_MAX_PLSIZE, maximum payload per MTU.");
614+
LOGC(aclog.Error, log << "SRTO_PAYLOADSIZE: value exceeds " << SRT_LIVE_MAX_PLSIZE << ", maximum payload per MTU.");
615615
throw CUDTException(MJ_NOTSUP, MN_INVAL, 0);
616616
}
617617

@@ -632,12 +632,22 @@ struct CSrtConfigSetter<SRTO_PAYLOADSIZE>
632632
if (size_t(val) > efc_max_payload_size)
633633
{
634634
LOGC(aclog.Error,
635-
log << "SRTO_PAYLOADSIZE: value exceeds SRT_LIVE_MAX_PLSIZE decreased by " << fc.extra_size
635+
log << "SRTO_PAYLOADSIZE: value exceeds " << SRT_LIVE_MAX_PLSIZE << " bytes decreased by " << fc.extra_size
636636
<< " required for packet filter header");
637637
throw CUDTException(MJ_NOTSUP, MN_INVAL, 0);
638638
}
639639
}
640640

641+
// Not checking AUTO to allow defaul 1456 bytes.
642+
if ((co.iCryptoMode == CSrtConfig::CIPHER_MODE_AES_GCM)
643+
&& (val > (SRT_LIVE_MAX_PLSIZE - HAICRYPT_AUTHTAG_MAX)))
644+
{
645+
LOGC(aclog.Error,
646+
log << "SRTO_PAYLOADSIZE: value exceeds " << SRT_LIVE_MAX_PLSIZE << " bytes decreased by " << HAICRYPT_AUTHTAG_MAX
647+
<< " required for AES-GCM.");
648+
throw CUDTException(MJ_NOTSUP, MN_INVAL, 0);
649+
}
650+
641651
co.zExpPayloadSize = val;
642652
}
643653
};

‎srtcore/strerror_defs.cpp‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -73,10 +73,10 @@ const char* strerror_msgs_notsup [] = {
7373
"Operation not supported: Socket is not in listening state", // MN_NOLISTEN = 6
7474
"Operation not supported: Listen/accept is not supported in rendezvous connection setup", // MN_ISRENDEZVOUS = 7
7575
"Operation not supported: Cannot call connect on UNBOUND socket in rendezvous connection setup", // MN_ISRENDUNBOUND = 8
76-
"Operation not supported: Incorrect use of Message API (sendmsg/recvmsg).", // MN_INVALMSGAPI = 9
77-
"Operation not supported: Incorrect use of Buffer API (send/recv) or File API (sendfile/recvfile).", // MN_INVALBUFFERAPI = 10
76+
"Operation not supported: Incorrect use of Message API (sendmsg/recvmsg)", // MN_INVALMSGAPI = 9
77+
"Operation not supported: Incorrect use of Buffer API (send/recv) or File API (sendfile/recvfile)", // MN_INVALBUFFERAPI = 10
7878
"Operation not supported: Another socket is already listening on the same port", // MN_BUSY = 11
79-
"Operation not supported: Message is too large to send (it must be less than the SRT send buffer size)", // MN_XSIZE = 12
79+
"Operation not supported: Message is too large to send", // MN_XSIZE = 12
8080
"Operation not supported: Invalid epoll ID", // MN_EIDINVAL = 13
8181
"Operation not supported: All sockets removed from epoll, waiting would deadlock", // MN_EEMPTY = 14
8282
"Operation not supported: Another socket is bound to that port and is not reusable for requested settings", // MN_BUSYPORT = 15

0 commit comments

Comments
 (0)