Closed
Bug 1211070
Opened 9 years ago
Closed 9 years ago
OpenH264: MSan use-of-uninitialized-value in WelsStrcat
Categories
(Core :: Audio/Video: GMP, defect)
Core
Audio/Video: GMP
Tracking
()
RESOLVED
FIXED
People
(Reporter: tsmith, Unassigned)
References
(Blocks 1 open bug)
Details
(Keywords: csectype-uninitialized, testcase)
Attachments
(2 files, 1 obsolete file)
No description provided.
Reporter | ||
Comment 1•9 years ago
|
||
We have fixed this bug in the master branch commit b37cda2 and openh264v1.5 branch commit d6b1680, please help to verify it.
Reporter | ||
Comment 3•9 years ago
|
||
Attachment #8669256 -
Attachment is obsolete: true
Reporter | ||
Comment 4•9 years ago
|
||
This bug does not appear to be fixed. Tested with commit b37cda2. > char* WelsStrcat (char* pDest, int32_t iSizeInBytes, const char* kpSrc) { <- codec/common/src/crt_util_safe_x.cpp:245 WelsStrcat is made up of a call to strlen and a call to WelsStrncpy. This is the equivalent of call a to strcpy since the length value used in strncpy comes from a call to strlen. If this is only being used with trusted input then it's no big deal... but that could always change. Or there could be other bugs like this MSan issue. Consider the following: > int32_t iCurLen = (int32_t)strlen (pDest); <- codec/common/src/crt_util_safe_x.cpp:246 Since the value iCurLen is coming from a call to strlen made on uninitialized memory in this case we may get an unknown potentially large (or negative) value. Then: > return WelsStrncpy (pDest + iCurLen, iSizeInBytes - iCurLen, kpSrc) <- codec/common/src/crt_util_safe_x.cpp:247 iSizeInBytes - iCurLen in the above statement could evaluate to a large (or negative) value which is used in WelsStrncpy here ... > strncpy (pDest, kpSrc, iSizeInBytes); //confirmed_safe_unsafe_usage <- codec/common/src/crt_util_safe_x.cpp:246 strncpy takes a size_t which is unsigned but is passed int32_t iSizeInBytes which is signed. The value passed could trigger an overflow (stack based in this case) leading to a crash or worse. I believe to fix the MSan error all that needs to be done is to change line 53 in codec/common/src/utils.cpp:53 to: > char pTraceTag[MAX_LOG_SIZE] = {0}; BUT after looking in to this and reviewing the string operations I think at the very least unsigned values for length parameters should be used. There may not be issues at the moment but this MSan issue is a good example of how easily things can go wrong and lead to potentially exploitable security vulnerabilities.
Hi Tyson, is the input you used to get attachment 8672034 [details] the same as Attachment 8669255 [details] "test_case.264"?
Reporter | ||
Comment 6•9 years ago
|
||
Yes, I can still reproduce the issue with the attached test case.
Reporter | ||
Comment 7•9 years ago
|
||
(In reply to Tyson Smith [:tsmith] from comment #6) > Yes, I can still reproduce the issue with the attached test case. Really it looks like any valid test case will trigger this on a MSan build since the issue is triggered by a call to WelsCreateDecoder before any test case processing is done.
Thanks for Tyson's help. I think it is a false alarm. Although the "char pTraceTag[MAX_LOG_SIZE]" uninitialized, it will be assigned in the following switch statement by WelsSnprintf() which actually call vsnprintf_s(). To make the MSAN check pass, we have fixed this issue in commit 410689f in master branch and commit 97ba688 in openh264v1.5 branch. Please help to verify it, thanks.
Reporter | ||
Comment 9•9 years ago
|
||
Verified with commit: af6a9a838fa
Updated•9 years ago
|
Group: media-core-security → core-security-release
Updated•8 years ago
|
Group: core-security-release
Assignee | ||
Updated•2 years ago
|
Component: OpenH264 → Audio/Video: GMP
Product: External Software Affecting Firefox → Core
You need to log in
before you can comment on or make changes to this bug.
Description
•