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)

defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: tsmith, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-uninitialized, testcase)

Attachments

(2 files, 1 obsolete file)

Attached file test_case.264
      No description provided.
Attached file call_stack.txt (obsolete) —
Depends on: 1170319
We have fixed this bug in the master branch commit b37cda2 and openh264v1.5 branch commit d6b1680, please help to verify it.
Attached file call_stack.txt
Attachment #8669256 - Attachment is obsolete: true
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"?
Yes, I can still reproduce the issue with the attached test case.
(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.
Verified with commit: af6a9a838fa
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Group: media-core-security → core-security-release
Group: core-security-release
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.

Attachment

General

Created:
Updated:
Size: