800502 - NULL deref in ccsip_subsmanager.c:add_content()

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

eventBody has to be NULL on the first pass through the loop

I suspect we're never calling this code, currently, so impact is minimal.

Comment 1

[Ethan Hugg [:ehugg]]

Attachment: Signaling - protect strlen from NULL input in add_content

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 670509 [details] [diff] [review]
Signaling -  protect strlen from NULL input in add_content

Review of attachment 670509 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/sipcc/core/sipstack/ccsip_subsmanager.c
@@ +1511,5 @@
>  
>          switch (eventData->type) {
>          case EVENT_DATA_RAW:
>              // Assume body is of type CMXML for now
>              (void) sippmh_add_message_body(request, eventBody, len,

Still wrong, as this assumes eventBody isn't null.  Probably should be some type of buffer or allocation.

Comment 3

[Ethan Hugg [:ehugg]]

Comment on attachment 670509 [details] [diff] [review]
Signaling -  protect strlen from NULL input in add_content


You're right this function is broken.  Checking original as well as latest version.

Comment 4

[Ethan Hugg [:ehugg]]

Broken during removal of libxml2 dependency  Original encodes the eventData in XML.

    while (eventData) {
        eventBody = xmlEncodeEventData(eventData);
        if (!eventBody) {
            CCSIP_DEBUG_ERROR(SIP_F_PREFIX"%s: Framing XML Body failed\n", fname1, fname);
            return (FALSE);
        }
        len = strlen(eventBody);

I'm now considering #if 0 of the function body and returning FALSE.

Comment 5

[Ethan Hugg [:ehugg]]

Attachment: Signaling - Removed unused code that required XML handling

Comment 6

[Ethan Hugg [:ehugg]]

Comment on attachment 670530 [details] [diff] [review]
Signaling - Removed unused code that required XML handling


This function cannot be repaired without XML handling.  It is used in sipSPISendSubscribe, sipSPISendSubNotify, and sipSPISendPublish.  We could do something like this, or remove more of Sub/Pub/Notify.

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7cb0e7444c55

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7cb0e7444c55

Should this have a test?

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

No testsuite needed; the code has no way currently to call it (part of the SIP functionality not currently hooked up), and the solution to the bug caused by removing the XML config code was to stub this function out.