1521360 - (CVE-2019-9805) Two potential loads from uninitialized memory in Prio serial_read_mp_array and read_packet_client

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[mlfbrown]

The same potential issue exists in both serial_read_mp_array and read_packet_client:

SECStatus rv = SECSuccess;
P_CHECKCB(upk != NULL); // NOTE: potential problem
... possibly more P_CHECKCB conditions
msgpack_unpacked res;

The P_CHECKCB macro goes to cleanup, which does this:

cleanup:
msgpack_unpacked_destroy(&res);

The msgpack_unpacked_destroy function does this with the uninitialized memory:

static inline void msgpack_unpacked_destroy(msgpack_unpacked* result)
{
if(result->zone != NULL) {
msgpack_zone_free(result->zone);
result->zone = NULL;
memset(&result->data, 0, sizeof(msgpack_object));
}
}

Functions at:
https://dxr.mozilla.org/mozilla-central/source/third_party/prio/prio/serial.c?q=%2Bfunction%3A%22serial_read_packet_client%28msgpack_unpacker+%2A%2C+PrioPacketClient%2C+const_PrioConfig%29%22&redirect_type=single#117 (serial_read_mp_array)

and

https://dxr.mozilla.org/mozilla-central/source/third_party/prio/prio/serial.c?q=%2Bfunction%3A%22serial_read_packet_client%28msgpack_unpacker+%2A%2C+PrioPacketClient%2C+const_PrioConfig%29%22&redirect_type=single#325 (serial_read_packet_client)

Comment 1

[Johann Hofmann [:johannh]]

Rob, do you know who's the right person to talk to for security issues in the prio library?

Comment 2

[Robert Helmer [:rhelmer]]

(In reply to Johann Hofmann [:johannh] from comment #1)

Rob, do you know who's the right person to talk to for security issues in the prio library?

Probably me for now, I'll follow up. cc'ing Henry who might be interested in taking a look.

Comment 3

[Henry Corrigan-Gibbs [:henrycg]]

Yup, looks like a bug to me! I used the same faulty design pattern in a bunch of places in serial.c, so we should fix them all in one go.

An attacker would only be able to trigger this behavior if one of these buggy assertions (e.g., P_CHECKCB(upk != NULL);) fails, which shouldn't happen unless there is also a bug in the code that calls these functions.

In any event, we should fix this. Rob, do you want me to open a PR for this against the mozilla/libprio repo?

Comment 4

[Robert Helmer [:rhelmer]]

(In reply to Henry Corrigan-Gibbs [:henrycg] from comment #3)

Yup, looks like a bug to me! I used the same faulty design pattern in a bunch of places in serial.c, so we should fix them all in one go.

An attacker would only be able to trigger this behavior if one of these buggy assertions (e.g., P_CHECKCB(upk != NULL);) fails, which shouldn't happen unless there is also a bug in the code that calls these functions.

In any event, we should fix this. Rob, do you want me to open a PR for this against the mozilla/libprio repo?

We should take this opportunity to figure out a good workflow for handling security bugs in libprio. Even though Prio is not enabled by default, we may start enabling it for some users so we should treat these like we would any security issue in Firefox.

I'm thinking we should have a private fork of libprio that we uplift from, then we can land patches in the public libprio repo when Mozilla is ready to announce security bugs. This would be a little more work since we'd need to keep both repos in sync but I don't think it'd be too bad.

I am waiting for input from our security folks to find out if we already have best practices around this, do you mind holding off opening a PR on the public repo for now?

Comment 5

[Henry Corrigan-Gibbs [:henrycg]]

(In reply to Robert Helmer [:rhelmer] from comment #4)

I am waiting for input from our security folks to find out if we already have best practices around this, do you mind holding off opening a PR on the public repo for now?

For sure. I'll wait to hear back from you.

Comment 6

[Robert Helmer [:rhelmer]]

(In reply to Henry Corrigan-Gibbs [:henrycg] from comment #5)

(In reply to Robert Helmer [:rhelmer] from comment #4)

I am waiting for input from our security folks to find out if we already have best practices around this, do you mind holding off opening a PR on the public repo for now?

For sure. I'll wait to hear back from you.

OK so state-of-the art is to land a patch first, ship, then land in github when we've shipped. An example where this was done is libvorbis in bug 1446062.

So I think there's no real point in bothering with a private github repo, the amount of work for the patch author is roughly the same, except for having to use two review systems :)

Henry, would you mind reviewing a patch? I think you just need to log in to phabricator with your bugzilla account to be able to do so:
https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html

Comment 7

[Robert Helmer [:rhelmer]]

Attachment: Bug 1521360 - ensure that Prio early cleanup runs correctly r?henrycg

Comment 8

[Henry Corrigan-Gibbs [:henrycg]]

I just reviewed the patch and approved it in Phabricator. If there is anything else I should do here, just let me know.

Comment 9

[Henry Corrigan-Gibbs [:henrycg]]

Rob, after reviewing your patch, I grep'd through the libprio source to look for other places where I may have misused the msgpack API.

The only other place we use it is in client.c and it looks like there are indeed two similar bugs there. In particular, in PrioClient_encode and PrioPacketClient_decrypt, a failure early on in the function could cause the program to invoke msgpack_*_destroy on an uninitialized object.

• In PrioClient_encode, pulling the msgpack_*_init function calls up above the assertions should fix the bug.
• In PrioPacketClient_decrypt, line 346, we should do something like this to make sure that we don't call msgpack_unpacker_destroy after initialization has failed:

  if (!msgpack_unpacker_init(&upk, data_len)) {
    return SECFailure;
  }
  ...
  

Since we don't use the msgpack API anywhere else in libprio, I am cautiously optimistic that we will not find more instances of this particular type of bug elsewhere in the library.

Comment 10

[Robert Helmer [:rhelmer]]

(In reply to Henry Corrigan-Gibbs [:henrycg] from comment #9)

Rob, after reviewing your patch, I grep'd through the libprio source to look for other places where I may have misused the msgpack API.

The only other place we use it is in client.c and it looks like there are indeed two similar bugs there. In particular, in PrioClient_encode and PrioPacketClient_decrypt, a failure early on in the function could cause the program to invoke msgpack_*_destroy on an uninitialized object.

• In PrioClient_encode, pulling the msgpack_*_init function calls up above the assertions should fix the bug.
• In PrioPacketClient_decrypt, line 346, we should do something like this to make sure that we don't call msgpack_unpacker_destroy after initialization has failed:

  if (!msgpack_unpacker_init(&upk, data_len)) {
    return SECFailure;
  }
  ...
  

Since we don't use the msgpack API anywhere else in libprio, I am cautiously optimistic that we will not find more instances of this particular type of bug elsewhere in the library.

Thanks for catching those! I'll amend the current patch.

Comment 11

[Robert Helmer [:rhelmer]]

Comment on attachment 9038404 [details]
Bug 1521360 - ensure that Prio early cleanup runs correctly r?henrycg

Security Approval Request

How easily could an exploit be constructed based on the patch?

Per bug 1521360 comment 3, this should only be exploitable if there is also a bug in the code calling these functions, which we do not believe to be the case.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Yes

Which older supported branches are affected by this flaw?

release (65.0)

If not all supported branches, which bug introduced the flaw?

None

Do you have backports for the affected branches?

Yes

If not, how different, hard to create, and risky will they be?

How likely is this patch to cause regressions; how much testing does it need?

This feature (Prio) has complete unit tests and is preffed off by default, it probably does not need additional manual testing in addition to the automated tests.

Comment 12

[Al Billings [:abillings - ex-MoCo]]

This doesn't need sec-approval to land on trunk as a sec-moderate issue. I'm setting status flags and clearing the request. Land away!

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/99c2a6b3e3bdc4d0d8b8290fee7a52b099aece48

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/99c2a6b3e3bd

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Did you want to request Beta approval on this?

Comment 16

[Robert Helmer [:rhelmer]]

Comment on attachment 9038404 [details]
Bug 1521360 - ensure that Prio early cleanup runs correctly r?henrycg

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

Bug 1521360

User impact if declined

An attacker would only be able to trigger this behavior if one of these buggy assertions (e.g., P_CHECKCB(upk != NULL);) fails, which shouldn't happen unless there is also a bug in the code that calls these functions.

Is this code covered by automated tests?

Yes

Has the fix been verified in Nightly?

No

Needs manual test from QE?

No

If yes, steps to reproduce

List of other uplifts needed

None

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

String changes made/needed

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9038404 [details]
Bug 1521360 - ensure that Prio early cleanup runs correctly r?henrycg

Mitigation for potential issue, passes tests, ok in nightly.
Let's uplift for beta 10.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c5bcd3213da6