Permalink
Browse files

Fix vulnerabilities found and reported by Mark Dowd

- limit length of memcpy
- limit number of offered algorithms in Hello packet
- length check in PING packet
- fix a small coding error
  • Loading branch information...
1 parent 2c9a33d commit c8617100f359b217a974938c5539a1dd8a120b0e @wernerd committed Jun 25, 2013
Showing with 18 additions and 5 deletions.
  1. +4 −0 clients/ccrtp/ZrtpQueue.cpp
  2. +6 −4 zrtp/ZRtp.cpp
  3. +5 −0 zrtp/ZrtpPacketHello.cpp
  4. +3 −1 zrtp/ZrtpStateClass.cpp
View
4 clients/ccrtp/ZrtpQueue.cpp
@@ -161,6 +161,10 @@ ZrtpQueue::takeInDataPacket(void)
// if ZRTP processing is enabled. Because valid RTP packets are
// already handled we delete any packets here after processing.
if (enableZrtp && zrtpEngine != NULL) {
+ // Fixed header length + smallest ZRTP packet (includes CRC)
+ if (rtn < (12 + sizeof(HelloAckPacket_t))) // data too small, dismiss
+ return 0;
+
// Get CRC value into crc (see above how to compute the offset)
uint16_t temp = rtn - CRC_SIZE;
uint32_t crc = *(uint32_t*)(buffer + temp);
View
10 zrtp/ZRtp.cpp
@@ -1167,7 +1167,8 @@ ZrtpPacketError* ZRtp::prepareError(uint32_t errMsg) {
}
ZrtpPacketPingAck* ZRtp::preparePingAck(ZrtpPacketPing* ppkt) {
-
+ if (ppkt->getLength() != 6) // A PING packet must have a length of 6 words
+ return NULL;
// Because we do not support ZRTP proxy mode use the truncated ZID.
// If this code shall be used in ZRTP proxy implementation the computation
// of the endpoint hash must be enhanced (see chaps 5.15ff and 5.16)
@@ -1467,14 +1468,14 @@ AlgorithmEnum* ZRtp::findBestSASType(ZrtpPacketHello *hello) {
if (num == 0) {
return &zrtpSasTypes.getByName(mandatorySasType);
}
- // Buildlist of configured SAS algorithm names
+ // Build list of configured SAS algorithm names
numAlgosConf = configureAlgos.getNumConfiguredAlgos(SasType);
for (i = 0; i < numAlgosConf; i++) {
algosConf[i] = &configureAlgos.getAlgoAt(SasType, i);
}
// Build list of offered known algos in Hello,
for (numAlgosOffered = 0, i = 0; i < num; i++) {
- algosOffered[numAlgosOffered] = &zrtpSasTypes.getByName((const char*)hello->getSasType(i++));
+ algosOffered[numAlgosOffered] = &zrtpSasTypes.getByName((const char*)hello->getSasType(i));
if (!algosOffered[numAlgosOffered]->isValid())
continue;
numAlgosOffered++;
@@ -2310,7 +2311,8 @@ void ZRtp::setClientId(std::string id, HelloPacketVersion* hpv) {
}
void ZRtp::storeMsgTemp(ZrtpPacketBase* pkt) {
- int32_t length = pkt->getLength() * ZRTP_WORD_SIZE;
+ uint32_t length = pkt->getLength() * ZRTP_WORD_SIZE;
+ length = (length > sizeof(tempMsgBuffer)) ? sizeof(tempMsgBuffer) : length;
memset(tempMsgBuffer, 0, sizeof(tempMsgBuffer));
memcpy(tempMsgBuffer, (uint8_t*)pkt->getHeaderBase(), length);
lengthOfMsgData = length;
View
5 zrtp/ZrtpPacketHello.cpp
@@ -105,10 +105,15 @@ ZrtpPacketHello::ZrtpPacketHello(uint8_t *data) {
uint32_t temp = zrtpNtohl(t);
nHash = (temp & (0xf << 16)) >> 16;
+ nHash &= 0x7; // restrict to max 7 algorithms
nCipher = (temp & (0xf << 12)) >> 12;
+ nCipher &= 0x7;
nAuth = (temp & (0xf << 8)) >> 8;
+ nAuth &= 0x7;
nPubkey = (temp & (0xf << 4)) >> 4;
+ nPubkey &= 0x7;
nSas = temp & 0xf;
+ nSas &= 0x7;
// +2 : the MAC at the end of the packet
computedLength = nHash + nCipher + nAuth + nPubkey + nSas + sizeof(HelloPacket_t)/ZRTP_WORD_SIZE + 2;
View
4 zrtp/ZrtpStateClass.cpp
@@ -123,7 +123,9 @@ void ZrtpStateClass::processEvent(Event_t *ev) {
else if (first == 'p' && middle == ' ' && last == ' ') {
ZrtpPacketPing ppkt(pkt);
ZrtpPacketPingAck* ppktAck = parent->preparePingAck(&ppkt);
- parent->sendPacketZRTP(static_cast<ZrtpPacketBase *>(ppktAck));
+ if (ppktAck != NULL) { // ACK only to valid PING packet, otherwise ignore it
+ parent->sendPacketZRTP(static_cast<ZrtpPacketBase *>(ppktAck));
+ }
parent->synchLeave();
return;
}

0 comments on commit c861710

Please sign in to comment.