Open Bug 1349527 Opened 3 years ago Updated 1 year ago

[Static Analysis][Out-of-bounds write] In cubeb_log_message::​cubeb_log_message(char const*)

Categories

(Core :: Audio/Video: cubeb, defect, P3)

55 Branch
defect

Tracking

()

People

(Reporter: tristanbourvon, Assigned: tristanbourvon, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: stale-bug, Whiteboard: CID 1402662)

Attachments

(1 file)

Coverity detected an out-of-bounds write in cubeb_log_message::​cubeb_log_message(char const*)

---
Description of the bug
---

> cubeb_log_message(char const str[CUBEB_LOG_MESSAGE_MAX_SIZE])
> {

|str| has size |CUBEB_LOG_MESSAGE_MAX_SIZE|

>   size_t length = strlen(str);

|strlen| returns the length of the string EXCLUDING \0

>   /* paranoia against malformed message */
>   assert(length < CUBEB_LOG_MESSAGE_MAX_SIZE);
>   if (length > CUBEB_LOG_MESSAGE_MAX_SIZE - 1) {
>     return;
>   }

At this point we know that |length < CUBEB_LOG_MESSAGE_MAX_SIZE|

>   PodCopy(storage, str, length);
>   storage[length + 1] = '\0';
> }

* Knowing that |storage| has size |CUBEB_LOG_MESSAGE_MAX_SIZE|
* if |length == CUBEB_LOG_MESSAGE_MAX_SIZE - 1|
=> then, we write \0 at |storage[CUBEB_LOG_MESSAGE_MAX_SIZE]|, which is out-of-bounds.

---
Possible consequences
---

As |cubeb_log_message|s are stored in a |lock_free_queue|, we could potentially be "emptying" the next message in the queue by setting its first byte to \0.

If the |cubeb_log_message| is at the end of the underlying array of the |lock_free_queue|, we could also be writing \0 to:
* |ring_buffer_base::consumer_id|, if NDEBUG is defined
* The next byte in the .data segment of the program (the |lock_free_queue| lives at the very end of |cubeb_async_logger|, which is instantiated statically) otherwise.

It does not seem useful here to further investigate how this could be exploited.

---
A fix
---

Turning
>   storage[length + 1] = '\0';
into
>   storage[length] = '\0';

This will ensure that the \0 byte is always written within the |storage| string.
Whiteboard: CID 1402662
Note that this is my first bug report. Please let me know what can be improved.
Also, please add me as assignee for this bug.
I think this is a 3rd party library that we are using and the fix should be pushed upstream: https://github.com/kinetiknz/cubeb
Component: Untriaged → Audio/Video: cubeb
Product: Firefox → Core
Paul, do you know if this is something we need to push upstream?
Flags: needinfo?(padenot)
Comment on attachment 8850002 [details]
Bug 1349527 - Remove an extra +1 in cubeb_log_message::cubeb_log_message()

https://reviewboard.mozilla.org/r/122758/#review127160
Attachment #8850002 - Flags: review+
Thanks for the patch Tristan, we'll get that upstreamed.
Flags: needinfo?(padenot)
Assignee: nobody → tristanbourvon
Status: UNCONFIRMED → NEW
Ever confirmed: true
Rank: 15
Priority: -- → P1
This is an assigned P1 bug without activity in two weeks. 

If you intend to continue working on this bug for the current release/iteration/sprint, remove the 'stale-bug' keyword.

Otherwise we'll reset the priority of the bug back to '--' on Monday, August 28th.
Keywords: stale-bug
Mass change P1->P2 to align with new Mozilla triage process
Priority: P1 → P2
Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information
Priority: P2 → P3
You need to log in before you can comment on or make changes to this bug.