From bddac12707d797001e5bad3c5e7657d18e672b4c Mon Sep 17 00:00:00 2001 From: sherla Date: Sun, 29 Mar 2026 20:33:51 -0700 Subject: [PATCH 1/2] Fix: * rfc3489 does not use magic cookie in transaction id * the last 4 bytes of transaction id were not populated --- stuncore/stunbuilder.cpp | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/stuncore/stunbuilder.cpp b/stuncore/stunbuilder.cpp index 2cc456e..23f8f1d 100644 --- a/stuncore/stunbuilder.cpp +++ b/stuncore/stunbuilder.cpp @@ -93,7 +93,6 @@ HRESULT CStunMessageBuilder::AddTransactionId(const StunTransactionId& transid) HRESULT CStunMessageBuilder::AddRandomTransactionId(StunTransactionId* pTransId) { StunTransactionId transid; - uint32_t stun_cookie_nbo = htonl(STUN_COOKIE); uint32_t entropy=0; @@ -135,10 +134,18 @@ HRESULT CStunMessageBuilder::AddRandomTransactionId(StunTransactionId* pTransId) srand(entropy); - // the first four bytes of the transaction id is always the magic cookie - // followed by 12 bytes of the real transaction id - memcpy(transid.id, &stun_cookie_nbo, sizeof(stun_cookie_nbo)); - for (int x = 4; x < (STUN_TRANSACTION_ID_LENGTH-4); x++) + // rfc3489 (legacy mode) does not use magic cookie + int x = 0; + if (!_fLegacyMode) + { + // the first four bytes of the transaction id is always the magic cookie + // followed by 12 bytes of the real transaction id + uint32_t stun_cookie_nbo = htonl(STUN_COOKIE); + memcpy(transid.id, &stun_cookie_nbo, sizeof(stun_cookie_nbo)); + x = 4; + } + + for (; x < STUN_TRANSACTION_ID_LENGTH; x++) { transid.id[x] = (uint8_t)(rand() % 256); } From f1fe894070d250317967755be1eda01a98167080 Mon Sep 17 00:00:00 2001 From: sherla Date: Sat, 4 Apr 2026 20:09:46 -0700 Subject: [PATCH 2/2] Make sure legacy mode transaction id does not start with the magic cookie. Add unit tests to validate transaction id in legacy and non-legacy modes. --- stuncore/stunbuilder.cpp | 14 ++++++++++++-- testcode/testbuilder.cpp | 42 ++++++++++++++++++++++++++++++++++++++++ testcode/testbuilder.h | 2 ++ 3 files changed, 56 insertions(+), 2 deletions(-) diff --git a/stuncore/stunbuilder.cpp b/stuncore/stunbuilder.cpp index 23f8f1d..dbc3651 100644 --- a/stuncore/stunbuilder.cpp +++ b/stuncore/stunbuilder.cpp @@ -93,6 +93,7 @@ HRESULT CStunMessageBuilder::AddTransactionId(const StunTransactionId& transid) HRESULT CStunMessageBuilder::AddRandomTransactionId(StunTransactionId* pTransId) { StunTransactionId transid; + uint32_t stun_cookie_nbo = htonl(STUN_COOKIE); uint32_t entropy=0; @@ -134,13 +135,11 @@ HRESULT CStunMessageBuilder::AddRandomTransactionId(StunTransactionId* pTransId) srand(entropy); - // rfc3489 (legacy mode) does not use magic cookie int x = 0; if (!_fLegacyMode) { // the first four bytes of the transaction id is always the magic cookie // followed by 12 bytes of the real transaction id - uint32_t stun_cookie_nbo = htonl(STUN_COOKIE); memcpy(transid.id, &stun_cookie_nbo, sizeof(stun_cookie_nbo)); x = 4; } @@ -150,6 +149,17 @@ HRESULT CStunMessageBuilder::AddRandomTransactionId(StunTransactionId* pTransId) transid.id[x] = (uint8_t)(rand() % 256); } + if (_fLegacyMode) + { + // rfc3489 (legacy mode) does not use magic cookie + // if the generated txn id happens to start with the magic cookie, keep + // re-generating the 1st byte until it doesn't + while (memcmp(transid.id, &stun_cookie_nbo, sizeof(stun_cookie_nbo)) == 0) + { + transid.id[0] = (uint8_t)(rand() % 256); + } + } + if (pTransId) { *pTransId = transid; diff --git a/testcode/testbuilder.cpp b/testcode/testbuilder.cpp index f928cf8..3e07224 100644 --- a/testcode/testbuilder.cpp +++ b/testcode/testbuilder.cpp @@ -27,6 +27,8 @@ HRESULT CTestBuilder::Run() HRESULT hr = S_OK; Chk(Test1()) Chk(Test2()); + Chk(Test3()); + Chk(Test4()); Cleanup: return hr; } @@ -138,4 +140,44 @@ HRESULT CTestBuilder::Test2() return hr; } +// This test validates the non-legacy mode transaction id generated by builder. +HRESULT CTestBuilder::Test3() +{ + HRESULT hr = S_OK; + CStunMessageBuilder builder; + CStunMessageReader reader; + StunTransactionId transid = {}; + CRefCountedBuffer spBuffer; + + builder.SetLegacyMode(false); + ChkA(builder.AddBindingRequestHeader()); + ChkA(builder.AddRandomTransactionId(&transid)); + ChkA(builder.GetResult(&spBuffer)); + + ChkIfA(CStunMessageReader::BodyValidated != reader.AddBytes(spBuffer->GetData(), spBuffer->GetSize()), E_FAIL); + ChkIf(reader.IsMessageLegacyFormat() == true, E_FAIL); + +Cleanup: + return hr; +} +// This test validates the legacy mode transaction id generated by builder. +HRESULT CTestBuilder::Test4() +{ + HRESULT hr = S_OK; + CStunMessageBuilder builder; + CStunMessageReader reader; + StunTransactionId transid = {}; + CRefCountedBuffer spBuffer; + + builder.SetLegacyMode(true); + ChkA(builder.AddBindingRequestHeader()); + ChkA(builder.AddRandomTransactionId(&transid)); + ChkA(builder.GetResult(&spBuffer)); + + ChkIfA(CStunMessageReader::BodyValidated != reader.AddBytes(spBuffer->GetData(), spBuffer->GetSize()), E_FAIL); + ChkIf(reader.IsMessageLegacyFormat() == false, E_FAIL); + +Cleanup: + return hr; +} \ No newline at end of file diff --git a/testcode/testbuilder.h b/testcode/testbuilder.h index 59c216b..2163104 100644 --- a/testcode/testbuilder.h +++ b/testcode/testbuilder.h @@ -26,6 +26,8 @@ class CTestBuilder : public IUnitTest public: HRESULT Test1(); HRESULT Test2(); + HRESULT Test3(); + HRESULT Test4(); virtual HRESULT Run();