From e42e82aa1f016cbd0b27c21157548d11e6010ae0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Storsj=C3=B6?= Date: Sun, 26 Jan 2014 14:33:08 +0200 Subject: [PATCH 1/5] Make WelsVsprintf use vsnprintf, to check the buffer size Otherwise builds on platforms other than MSVC might be insecure. Use vsnprintf_s with the _TRUNCATE flag instead of vsprintf_s when using MSVC - this truncates the buffer instead of aborting the whole process in case it's too small. --- codec/common/crt_util_safe_x.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/codec/common/crt_util_safe_x.cpp b/codec/common/crt_util_safe_x.cpp index cb0f451e..ac6db244 100644 --- a/codec/common/crt_util_safe_x.cpp +++ b/codec/common/crt_util_safe_x.cpp @@ -88,7 +88,7 @@ int32_t WelsStrnlen (const str_t* kpStr, int32_t iMaxlen) { } int32_t WelsVsprintf (str_t* pBuffer, int32_t iSizeOfBuffer, const str_t* kpFormat, va_list pArgPtr) { - return vsprintf_s (pBuffer, iSizeOfBuffer, kpFormat, pArgPtr); + return vsnprintf_s (pBuffer, iSizeOfBuffer, _TRUNCATE, kpFormat, pArgPtr); } WelsFileHandle* WelsFopen (const str_t* kpFilename, const str_t* kpMode) { @@ -142,7 +142,7 @@ int32_t WelsStrnlen (const str_t* kpStr, int32_t iMaxlen) { } int32_t WelsVsprintf (str_t* pBuffer, int32_t iSizeOfBuffer, const str_t* kpFormat, va_list pArgPtr) { - return vsprintf (pBuffer, kpFormat, pArgPtr); //confirmed_safe_unsafe_usage + return vsnprintf (pBuffer, iSizeOfBuffer, kpFormat, pArgPtr); //confirmed_safe_unsafe_usage } @@ -210,7 +210,7 @@ int32_t WelsStrnlen (const str_t* kpString, int32_t iMaxlen) { #endif int32_t WelsVsprintf (str_t* pBuffer, int32_t iSizeOfBuffer, const str_t* kpFormat, va_list pArgPtr) { - return vsprintf (pBuffer, kpFormat, pArgPtr); //confirmed_safe_unsafe_usage + return vsnprintf (pBuffer, iSizeOfBuffer, kpFormat, pArgPtr); //confirmed_safe_unsafe_usage } WelsFileHandle* WelsFopen (const str_t* kpFilename, const str_t* kpMode) { From 0ce42ffb895a063000b3e8763248650372c7e56e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Storsj=C3=B6?= Date: Sun, 26 Jan 2014 12:58:43 +0200 Subject: [PATCH 2/5] Rename WelsVsprintf to WelsVsnprintf, to indicate that it actually checks the length --- codec/common/crt_util_safe_x.cpp | 6 +++--- codec/common/crt_util_safe_x.h | 2 +- codec/decoder/plus/src/welsCodecTrace.cpp | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/codec/common/crt_util_safe_x.cpp b/codec/common/crt_util_safe_x.cpp index ac6db244..9edc72ea 100644 --- a/codec/common/crt_util_safe_x.cpp +++ b/codec/common/crt_util_safe_x.cpp @@ -87,7 +87,7 @@ int32_t WelsStrnlen (const str_t* kpStr, int32_t iMaxlen) { return strnlen_s (kpStr, iMaxlen); } -int32_t WelsVsprintf (str_t* pBuffer, int32_t iSizeOfBuffer, const str_t* kpFormat, va_list pArgPtr) { +int32_t WelsVsnprintf (str_t* pBuffer, int32_t iSizeOfBuffer, const str_t* kpFormat, va_list pArgPtr) { return vsnprintf_s (pBuffer, iSizeOfBuffer, _TRUNCATE, kpFormat, pArgPtr); } @@ -141,7 +141,7 @@ int32_t WelsStrnlen (const str_t* kpStr, int32_t iMaxlen) { return strlen (kpStr); //confirmed_safe_unsafe_usage } -int32_t WelsVsprintf (str_t* pBuffer, int32_t iSizeOfBuffer, const str_t* kpFormat, va_list pArgPtr) { +int32_t WelsVsnprintf (str_t* pBuffer, int32_t iSizeOfBuffer, const str_t* kpFormat, va_list pArgPtr) { return vsnprintf (pBuffer, iSizeOfBuffer, kpFormat, pArgPtr); //confirmed_safe_unsafe_usage } @@ -209,7 +209,7 @@ int32_t WelsStrnlen (const str_t* kpString, int32_t iMaxlen) { } #endif -int32_t WelsVsprintf (str_t* pBuffer, int32_t iSizeOfBuffer, const str_t* kpFormat, va_list pArgPtr) { +int32_t WelsVsnprintf (str_t* pBuffer, int32_t iSizeOfBuffer, const str_t* kpFormat, va_list pArgPtr) { return vsnprintf (pBuffer, iSizeOfBuffer, kpFormat, pArgPtr); //confirmed_safe_unsafe_usage } diff --git a/codec/common/crt_util_safe_x.h b/codec/common/crt_util_safe_x.h index b4d35d3b..b83acc66 100644 --- a/codec/common/crt_util_safe_x.h +++ b/codec/common/crt_util_safe_x.h @@ -79,7 +79,7 @@ int32_t WelsSnprintf (str_t* buffer, int32_t sizeOfBuffer, const str_t* form str_t* WelsStrncpy (str_t* dest, int32_t sizeInBytes, const str_t* src, int32_t count); str_t* WelsStrcat (str_t* dest, int32_t sizeInBytes, str_t* src); int32_t WelsStrnlen (const str_t* str, int32_t maxlen); -int32_t WelsVsprintf (str_t* buffer, int32_t sizeOfBuffer, const str_t* format, va_list argptr); +int32_t WelsVsnprintf (str_t* buffer, int32_t sizeOfBuffer, const str_t* format, va_list argptr); WelsFileHandle* WelsFopen (const str_t* filename, const str_t* mode); int32_t WelsFclose (WelsFileHandle* fp); diff --git a/codec/decoder/plus/src/welsCodecTrace.cpp b/codec/decoder/plus/src/welsCodecTrace.cpp index f02df240..c7f95bba 100644 --- a/codec/decoder/plus/src/welsCodecTrace.cpp +++ b/codec/decoder/plus/src/welsCodecTrace.cpp @@ -68,7 +68,7 @@ int32_t CWelsTraceBase::Trace (const int kLevel, const str_t* kpFormat, va_list WelsStrncpy (chBuf, MAX_LOG_SIZE, (const str_t*)"[DECODER]: ", kLen); - WelsVsprintf ((chBuf + kLen), MAX_LOG_SIZE - kLen, (const str_t*)kpFormat, pVl); + WelsVsnprintf ((chBuf + kLen), MAX_LOG_SIZE - kLen, (const str_t*)kpFormat, pVl); WelsStrncpy (chResult, MAX_LOG_SIZE, (const str_t*)chBuf, WelsStrnlen ((const str_t*)chBuf, MAX_LOG_SIZE)); WriteString (kLevel, chResult); From 4a8f54d7679ad1f4dc58e5d621635920694a59ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Storsj=C3=B6?= Date: Sun, 26 Jan 2014 14:44:14 +0200 Subject: [PATCH 3/5] Use vsnprintf in the old MSVC version of WelsSnprintf as well --- codec/common/crt_util_safe_x.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/codec/common/crt_util_safe_x.cpp b/codec/common/crt_util_safe_x.cpp index 9edc72ea..4c2bdbc2 100644 --- a/codec/common/crt_util_safe_x.cpp +++ b/codec/common/crt_util_safe_x.cpp @@ -124,7 +124,7 @@ int32_t WelsSnprintf (str_t* pBuffer, int32_t iSizeOfBuffer, const str_t* kpFor va_start (pArgPtr, kpFormat); - iRc = vsprintf (pBuffer, kpFormat, pArgPtr); //confirmed_safe_unsafe_usage + iRc = vsnprintf (pBuffer, iSizeOfBuffer, kpFormat, pArgPtr); //confirmed_safe_unsafe_usage va_end (pArgPtr); From 7884e77b2dca11583d0e92c779db4f2d0feb8957 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Storsj=C3=B6?= Date: Sun, 26 Jan 2014 14:50:30 +0200 Subject: [PATCH 4/5] Make sure the buffer always is null terminated in the *snprintf calls for old MSVC These functions leave the buffer unterminated in case it was too small. --- codec/common/crt_util_safe_x.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/codec/common/crt_util_safe_x.cpp b/codec/common/crt_util_safe_x.cpp index 4c2bdbc2..edf69733 100644 --- a/codec/common/crt_util_safe_x.cpp +++ b/codec/common/crt_util_safe_x.cpp @@ -125,6 +125,8 @@ int32_t WelsSnprintf (str_t* pBuffer, int32_t iSizeOfBuffer, const str_t* kpFor va_start (pArgPtr, kpFormat); iRc = vsnprintf (pBuffer, iSizeOfBuffer, kpFormat, pArgPtr); //confirmed_safe_unsafe_usage + if (iRc < 0) + pBuffer[iSizeOfBuffer - 1] = '\0'; va_end (pArgPtr); @@ -142,7 +144,10 @@ int32_t WelsStrnlen (const str_t* kpStr, int32_t iMaxlen) { } int32_t WelsVsnprintf (str_t* pBuffer, int32_t iSizeOfBuffer, const str_t* kpFormat, va_list pArgPtr) { - return vsnprintf (pBuffer, iSizeOfBuffer, kpFormat, pArgPtr); //confirmed_safe_unsafe_usage + int32_t iRc = vsnprintf (pBuffer, iSizeOfBuffer, kpFormat, pArgPtr); //confirmed_safe_unsafe_usage + if (iRc < 0) + pBuffer[iSizeOfBuffer - 1] = '\0'; + return iRc; } From c7b74b2b129e02981c1decb0aa98452a4d0381b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Storsj=C3=B6?= Date: Sun, 26 Jan 2014 14:48:38 +0200 Subject: [PATCH 5/5] Make sure the buffer is null terminated after strftime If the buffer is too small, there's no guarantee that it is null terminated. The docs (on both unix and MSVC) say explicitly that the function returns 0 and the buffer contents are indeterminate in this case. --- codec/common/crt_util_safe_x.cpp | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/codec/common/crt_util_safe_x.cpp b/codec/common/crt_util_safe_x.cpp index edf69733..d798fc9d 100644 --- a/codec/common/crt_util_safe_x.cpp +++ b/codec/common/crt_util_safe_x.cpp @@ -110,10 +110,14 @@ int32_t WelsGetTimeOfDay (SWelsTime* pTp) { int32_t WelsStrftime (str_t* pBuffer, int32_t iSize, const str_t* kpFormat, const SWelsTime* kpTp) { struct tm sTimeNow; + int32_t iRc; localtime_s (&sTimeNow, &kpTp->time); - return strftime (pBuffer, iSize, kpFormat, &sTimeNow); + iRc = strftime (pBuffer, iSize, kpFormat, &sTimeNow); + if (iRc == 0) + pBuffer[0] = '\0'; + return iRc; } #else @@ -166,10 +170,14 @@ int32_t WelsGetTimeOfDay (SWelsTime* pTp) { int32_t WelsStrftime (str_t* pBuffer, int32_t iSize, const str_t* kpFormat, const SWelsTime* kpTp) { struct tm* pTnow; + int32_t iRc; pTnow = localtime (&kpTp->time); - return strftime (pBuffer, iSize, kpFormat, pTnow); + iRc = strftime (pBuffer, iSize, kpFormat, pTnow); + if (iRc == 0) + pBuffer[0] = '\0'; + return iRc; } @@ -241,10 +249,14 @@ int32_t WelsGetTimeOfDay (SWelsTime* pTp) { int32_t WelsStrftime (str_t* pBuffer, int32_t iSize, const str_t* kpFormat, const SWelsTime* kpTp) { struct tm* pTnow; + int32_t iRc; pTnow = localtime (&kpTp->time); - return strftime (pBuffer, iSize, kpFormat, pTnow); + iRc = strftime (pBuffer, iSize, kpFormat, pTnow); + if (iRc == 0) + pBuffer[0] = '\0'; + return iRc; } #endif