Closed Bug 1008786 Opened 5 years ago Closed 5 years ago

Potential uninitialized memory read from nr_reg_local_get_child_count()

Categories

(Core :: WebRTC: Networking, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: ekr, Assigned: bwc)

Details

Attachments

(1 file)

Static analysis by Sylvestre Ledru turned up a UMR if

nr_reg_local_get_child_count() is called under certain conditions.

The code path is that nr_reg_local_get_child_count() calls
nr_reg_fetch_node(parent, NR_REG_TYPE_REGISTRY, &ignore1, &ignore2)

with ignore2 being an uninitialized stack variable.

nr_reg_fetch_node() then fails at the line marked HERE below.

However, because nr_reg_local_get_child_count checks for
nr_reg_is_valid() on parent (which is now name) and never
gets here if that is invalid I believe this may be a false
positive.

http://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nrappkit/src/registry/registry_local.c#125


int
nr_reg_fetch_node(char *name, unsigned char type, nr_registry_node **node, int *free_node)
{
    int r, _status;

    if ((r=nr_reg_is_valid(name)))   <-- HERE
      ABORT(r);

    *node = 0;
    *free_node = 0;

    if ((r=r_assoc_fetch(nr_registry, name, strlen(name)+1, (void*)node)))
      ABORT(r);

    if ((*node)->type != type)
      ABORT(R_FAILED);

    _status=0;
  abort:
    if (_status) {
        if (*node)
            r_log(NR_LOG_REGISTRY, LOG_DEBUG, "Couldn't fetch node '%s' ('%s'), found '%s' instead",
              name, nr_reg_type_name(type), nr_reg_type_name((*node)->type));
        else
            r_log(NR_LOG_REGISTRY, LOG_DEBUG, "Couldn't fetch node '%s' ('%s')",
              name, nr_reg_type_name(type));
    }
    else {
        r_log(NR_LOG_REGISTRY, LOG_DEBUG, "Fetched node '%s' ('%s')",
              name, nr_reg_type_name(type));
    }
    return(_status);
}


If I am incorrect, we dereference ((*node)->type which is then passed to nr_reg_type_name(). This seems to check it's arguments, but I haven't double
checked.

Byron, please make your own analysis. If you feel this is not security
related, please have it unmarked. Even if it's not a bug, it would probably
be better to zero *node on input to be double-sure.
I'm pretty sure we're doubly-protected here; we don't try to use either ignore1 or ignore2 if the call to nr_reg_fetch_node fails, which is always the case when they are left uninitted.
Someone else will need to unmark this. I'll try to get a patch up once I get done with the current ICE PCP code review cycle.
Assignee: nobody → docfaraday
Group: core-security
Comment on attachment 8437868 [details] [diff] [review]
Zero outparams in nr_reg_fetch_node before we do anything that might abort.

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

This is probably trivial enough to do without a try push, right?
Attachment #8437868 - Flags: review?(ekr)
Attachment #8437868 - Flags: review?(ekr) → review?(drno)
Comment on attachment 8437868 [details] [diff] [review]
Zero outparams in nr_reg_fetch_node before we do anything that might abort.

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

LGTM
Attachment #8437868 - Flags: review?(drno) → review+
https://hg.mozilla.org/mozilla-central/rev/78518d0a8e82
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.