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

Status: RESOLVED FIXED

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

Description

[Tristan Bourvon]

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.

Comment 1

[Tristan Bourvon]

Note that this is my first bug report. Please let me know what can be improved.

Comment 2

[Tristan Bourvon]

Also, please add me as assignee for this bug.

Comment 3

[Tristan Bourvon]

Attachment: Bug 1349527 - Remove an extra +1 in cubeb_log_message::cubeb_log_message()

Fix an out-of-bounds write by removing and extra +1 which was causing a \0
to be written out of |storage| in edge cases.

Review commit: https://reviewboard.mozilla.org/r/122758/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/122758/

Comment 4

[Andi [:andi]]

I think this is a 3rd party library that we are using and the fix should be pushed upstream: https://github.com/kinetiknz/cubeb

Comment 5

[Michael Froman [:mjf]]

Paul, do you know if this is something we need to push upstream?

Comment 6

[Paul Adenot (: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

Comment 7

[Paul Adenot (:padenot)]

Thanks for the patch Tristan, we'll get that upstreamed.

Comment 8

[Paul Adenot (:padenot)]

Now upstream at https://github.com/kinetiknz/cubeb/commit/192b65548a0402df8f94904f68a191b390beca7f, we'll get that in gecko shortly.

Comment 9

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

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.

Comment 10

[Bulk Bug Changes for mreavy's org]

Mass change P1->P2 to align with new Mozilla triage process

Comment 11

[Sylvestre Ledru [:Sylvestre]]

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

Comment 12

[Paul Adenot (:padenot)]

This has been fixed for some time by regular cubeb uplift into Gecko.