-
Notifications
You must be signed in to change notification settings - Fork 187
Cherry pick PR #8865: Fix MediaKeys.GetMetrics failures in RDK #9056
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Bug: 474508805 **Issue[RDK]:** mediakeys.getmetrics fails() as - C26 : FAIL - c25 newly built (25.lts.1+) FAIL - c25 with default RDK image: PASS This PR fixes the above YTS failure --------- Co-authored-by: Jonas Tsai <jonastsai@google.com> (cherry picked from commit 8cacfce)
🤖 Gemini Suggested Commit Message💡 Pro Tips for a Better Commit Message:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request cherry-picks a fix for MediaKeys.GetMetrics failures on RDK platforms by introducing a fallback mechanism using a new opencdm_get_metrics function and improving promise handling for unavailable metrics. However, a critical heap-based buffer overflow vulnerability has been identified in drm_system_ocdm.cc. This vulnerability, stemming from improper uint16_t usage for length calculations, can lead to integer truncation and overflow, resulting in an undersized buffer allocation during Base64 encoding, potentially causing denial of service or arbitrary code execution. It is strongly recommended to use size_t for all length variables and validate input sizes. Please address this critical vulnerability before merging.
| uint16_t buffer_length = buffer.size(); | ||
| uint16_t out_length = (((buffer_length * 8) / 6) + 4) * sizeof(TCHAR); | ||
| metrics_.resize(out_length, '\0'); | ||
| out_length = WPEFramework::Core::URL::Base64Encode( | ||
| reinterpret_cast<const uint8_t*>(buffer.data()), buffer_length, | ||
| reinterpret_cast<char*>(metrics_.data()), out_length, false); | ||
| metrics_.resize(out_length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A critical heap-based buffer overflow vulnerability exists in the GetMetrics function. This occurs because buffer.size() (a size_t) is truncated to a uint16_t for buffer_length, which can lead to incorrect size calculations if the input exceeds 65,535 bytes. Consequently, the out_length calculation for Base64 encoding can also overflow a uint16_t, causing metrics_.resize() to allocate an undersized buffer. The subsequent Base64Encode call then writes beyond this buffer's bounds, leading to a heap overflow, which could result in denial of service or arbitrary code execution. It is crucial to use size_t for all length variables and to validate input sizes to prevent this.
size_t buffer_length = buffer.size();
// Add a reasonable size limit check to prevent extreme memory allocation.
const size_t kMaxMetricsSize = 1024 * 1024; // 1MB limit, adjust as needed
if (buffer_length > kMaxMetricsSize) {
SB_LOG(ERROR) << "Metrics data exceeds size limit.";
return;
}
size_t out_length = (((buffer_length * 8) / 6) + 4) * sizeof(TCHAR);
metrics_.resize(out_length, '\0');
size_t encoded_length = WPEFramework::Core::URL::Base64Encode(
reinterpret_cast<const uint8_t*>(buffer.data()), buffer_length,
reinterpret_cast<char*>(metrics_.data()), out_length, false);
metrics_.resize(encoded_length);| cdm_promise_adapter_.ResolvePromise(promise_id, | ||
| metrics_string.value_or(std::string())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you're already checking if metrics_string has a value with if (metrics_string), you can directly dereference it using *metrics_string. This is slightly more efficient as it avoids the value_or call which may create a temporary std::string.
cdm_promise_adapter_.ResolvePromise(promise_id, *metrics_string);
Refer to the original PR: #8865
Bug: 474508805
Issue[RDK]:
mediakeys.getmetrics fails() as
This PR fixes the above YTS failure