Potential security bug in Crypto::GetRandomValues

NEW
Unassigned

Status

()

Core
DOM: Security
P3
normal
2 years ago
a year ago

People

(Reporter: q1, Unassigned)

Tracking

40 Branch
Points:
---
Bug Flags:
sec-bounty -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [domsecurity-backlog2])

(Reporter)

Description

2 years ago
Crypto::GetRandomValues (dom\base\crypto.cpp) sometimes calls ContentChild::SendGetRandomValues. However, it doesn't (except in debug builds) check whether the length of the returned random data == the length it requested (which can vary). It seems that it is, therefore, possible for these lengths sometimes to mismatch. If SendGetRandomValues returns a length < the requested length, GetRandomValues could read data it doesn't own. This could cause an information leak and/or cause GetRandomValues to use low-entropy data, which would then weaken an encryption operation further down the road. It could also cause a crash.

The bug is in line 99, which should compare |!= dataLen| or possibly |< dataLen| instead of |== 0|:

56: void
57: Crypto::GetRandomValues(JSContext* aCx, const ArrayBufferView& aArray,
58:                         JS::MutableHandle<JSObject*> aRetval,
59:                         ErrorResult& aRv)
60: {
...
81:   aArray.ComputeLengthAndData();
82:   uint32_t dataLen = aArray.Length();
83:   if (dataLen == 0) {
84:     NS_WARNING("ArrayBufferView length is 0, cannot continue");
85:     aRetval.set(view);
86:     return;
87:   } else if (dataLen > 65536) {
88:     aRv.Throw(NS_ERROR_DOM_QUOTA_EXCEEDED_ERR);
89:     return;
90:   }
91:
92:   uint8_t* data = aArray.Data();
93: 
94:   if (XRE_GetProcessType() != GeckoProcessType_Default) {
95:     InfallibleTArray<uint8_t> randomValues;
96:     // Tell the parent process to generate random values via PContent
97:     ContentChild* cc = ContentChild::GetSingleton();
98:     if (!cc->SendGetRandomValues(dataLen, &randomValues) ||
99:       randomValues.Length() == 0) {
100:      aRv.Throw(NS_ERROR_FAILURE);
101:      return;
102:    }
103:    NS_ASSERTION(dataLen == randomValues.Length(),
104:                 "Invalid length returned from parent process!");
105:    memcpy(data, randomValues.Elements(), dataLen);
...
Flags: sec-bounty?
ping khuey: you reviewed this, is this yours now?
Flags: needinfo?(khuey)
No :)

I looked at this briefly.  The parent either sends back the right length or nothing at all (see http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#4121) so it would require a parent process compromise (which is much worse) to trigger this.
Flags: needinfo?(khuey)
(Reporter)

Comment 3

2 years ago
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #2)
> No :)
> 
> I looked at this briefly.  The parent either sends back the right length or
> nothing at all (see
> http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.
> cpp#4121) so it would require a parent process compromise (which is much
> worse) to trigger this.

From that link:

4124     uint8_t* buf = Crypto::GetRandomValues(length);
4125     if (!buf) {
4126         return true;
4127     }

Hmm? I guess this is why the client tests |randomValues.Length() == 0|?
I don't think this is a security bug, but why is the ContentParent returning true when it got nothing? Line 98 checks that SendGetRandomValues() is true, but there's no way for the parent code to return false.
Group: core-security

Comment 5

2 years ago
Returning false from any IPC handler will immediately terminate the child process (content/plugin). false return values should be reserved for very fatal kinds of events.
Minusing for bounty since it isn't a security issue.
Flags: sec-bounty? → sec-bounty-
Component: DOM: Security → Security
Component: Security → DOM: Security
Priority: -- → P3
Whiteboard: [domsecurity-backlog2]
You need to log in before you can comment on or make changes to this bug.