Closed Bug 1535880 Opened 7 months ago Closed 7 months ago

Potential read of uninitialized memory in RAPL constructor

Categories

(Core :: General, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: mlfbrown, Assigned: njn)

Details

(Keywords: sec-other, Whiteboard: [reporter-external] [client-bounty-form] [verif?][adv-main68-])

Attachments

(1 file)

Potential read of uninitialized memory in RAPL constructor
(function at https://searchfox.org/mozilla-central/source/tools/power/rapl.cpp#544)
Marking security to be on safe side

  1. Declared uint32_t type;

  2. type is passed to ReadValueFromPowerFile for initialization:
    ReadValueFromPowerFile("type", "", "", "%u", &type);

  3. In ReadValueFromPowerFile fails without initializing type if it cannot open a file:
    (function here https://searchfox.org/mozilla-central/source/tools/power/rapl.cpp#430)
    sprintf(filename, "/sys/bus/event_source/devices/power/%s%s%s", aStr1, aStr2, aStr3);
    FILE* fp = fopen(filename, "r");
    if (!fp) return false; // No intialization of type aka aOut
    if (fscanf(fp, aScanfString, aOut) != 1) ...

  4. Back in the RAPL constructor, type is stored as a field via many Domain constructors:
    mPkg = new Domain("pkg", type);
    mCores = new Domain("cores", type);
    mGpu = new Domain("gpu", type, Domain::Optional);
    mRam = new Domain("ram", type, Domain::Optional);

Flags: sec-bounty?

:njn/:glandium, can you help triage this? Thanks.

Group: firefox-core-security → core-security
Component: Security → General
Flags: needinfo?(n.nethercote)
Flags: needinfo?(mh+mozilla)
Product: Firefox → Core

The diagnosis is correct that uninitialized memory could be read if the kernel-provided /sys/bus/event_source/devices/power/type file was missing (perhaps due to a kernel configuration that lacked that file).

However, this uninitialized value only gets passed to the perf_event_open() syscall, where it (presumably) would cause that syscall to fail due to an unexpected attr.type parameter.

    struct perf_event_attr attr;
    memset(&attr, 0, sizeof(attr));
    attr.type = aType;
    attr.size = uint32_t(sizeof(attr));
    attr.config = config;

    // Measure all processes/threads. The specified CPU doesn't matter.
    mFd = perf_event_open(&attr, /* aPid = */ -1, /* aCpu = */ 0,
                          /* aGroupFd = */ -1, /* aFlags = */ 0);
    if (mFd < 0) {
      Abort(
          "perf_event_open() failed\n"
          "- Did you run as root (e.g. with |sudo|) or set\n"
          "  /proc/sys/kernel/perf_event_paranoid to 0, as required?");
    }

So I don't think there's a security problem here; the program would just abort.

Furthermore, RAPL is a standalone utility program used to obtain system-wide power usage estimates from Intel CPUs. It is not part of Firefox, and it is not shipped to users.

I will write a patch that avoids possibility of reading the uninitialized memory. The only effect of this will be to abort slightly earlier with a more useful error message.

Flags: needinfo?(n.nethercote)
Flags: needinfo?(mh+mozilla)

So I don't think there's a security problem here; the program would just abort.

I confirmed this by changing the code to look for a file called type2 instead of type. It aborted in the code snippet shown in comment 2.

This makes RAPL abort with more informative error messages if certain
kernel-provided files aren't present.

Marking this sec-other because it doesn't affect Firefox, and core-sec-release group for the same reason.

Assignee: nobody → n.nethercote
Group: core-security → core-security-release
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: sec-other

I don't think this need to be a security bug at all. As comment 2 says, the outcome of the uninitialized read is benign.

(In reply to Nicholas Nethercote [:njn] from comment #6)

I don't think this need to be a security bug at all. As comment 2 says, the outcome of the uninitialized read is benign.

wfm.

Group: core-security-release
Pushed by opoprus@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/300d3075ae04
Always check ReadValueFromPowerFile() return value. r=glandium
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Flags: sec-bounty? → sec-bounty-
Whiteboard: [reporter-external] [client-bounty-form] [verif?] → [reporter-external] [client-bounty-form] [verif?][adv-main68-]
You need to log in before you can comment on or make changes to this bug.