1620312 - Improve on-stack const TArray whitelisting for CanRunScriptChecker (MOZ_CAN_RUN_SCRIPT)

Status: ASSIGNED

(Developer Infrastructure :: Source Code Analysis, task, P3)

Description

[Boris Zbarsky [:bzbarsky]]

When I fixed bug 1535530, I had to add a bunch of MOZ_KnownLive annotations for this situation:

  const nsTArray<RefPtr<something>> stackArr(thingICopy);
  for (auto& item : stackArr) {
    item->CanRunScriptFunction();
  }

which is a bit unfortunate. We should be able to whitelist this specific case, by changing StackSmartPtr to also match when:

1. The varDecl's type is lValueReferenceType(pointee(isSmartPtrToRefCounted())) (possibly with some isConstQualified() thrown in.
2. It's a varDecl that is the hasLoopVariable of a ranged-for statement or so.
3. The ranged-for hasRangeInit which is an on-stack const variable. If we can restrict to on-stack const nsTArray, even better.

(This is in addition to the case it matches now, which is "type of varDecl is isSmartPtrToRefCounted and has stack lifetime".)

Comment 1

[Boris Zbarsky [:bzbarsky]]

Tests that ought to pass with this fixed (in TestCanRunScript.cpp):

MOZ_CAN_RUN_SCRIPT void test_ranged_for_1() {
  RefPtr<RefCountedBase> arr[2];
  for (auto item : arr) {
    test2(item);
    item->method_test();
  }
}

MOZ_CAN_RUN_SCRIPT void test_ranged_for_2() {
  RefPtr<RefCountedBase> arr[2];
  for (auto& item : arr) {
    test2(item); // expected-error {{arguments must all be strong refs or caller's parameters when calling a function marked as MOZ_CAN_RUN_SCRIPT (including the implicit object argument).  'item' is neither.}}
    item->method_test(); // expected-error {{arguments must all be strong refs or caller's parameters when calling a function marked as MOZ_CAN_RUN_SCRIPT (including the implicit object argument).  'item' is neither.}}
  }
}

MOZ_CAN_RUN_SCRIPT void test_ranged_for_3() {
  const RefPtr<RefCountedBase> arr[2];
  for (auto item : arr) {
    test2(item);
    item->method_test();
  }
}

MOZ_CAN_RUN_SCRIPT void test_ranged_for_4() {
  const RefPtr<RefCountedBase> arr[2];
  for (auto& item : arr) {
    test2(item);
    item->method_test();
  }
}

though depending on how we handle "random const thing" vs "TArray" those might need adjusting.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oh, despite of the name, AutoTArray is not a MOZ_STACK_CLASS. Therefore, fixing this does not help the cases that an array comes from the caller...

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1620312 - Make CanRunScriptChecker treat smart pointers in array in stack as safe r=andi!,saschanaz!

For example, currently, this is now allowed, but this should be allowed for
avoiding MOZ_KnownLive hacks which may cause security risk if used in unsafe
cases.

AutoTArray<Element, 10> elements;
for (const RefPtr<Element>& element : elements) {
  element->may_run_script();
}

Unfortunately, this patch needs to allow the reference which is initialized with
mutable array because arrays are usually mutable unless it's a param of the
method. I think that we should prohibit modifying the array in range-based for
loops instead, but this is not also covered by this patch. Anyway, such tricky
loops should be banned by reviewers.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: WIP: Bug 1620312 - part 9: Remove unnecessary `MOZ_KnownLive`s in the tree

Depends on D174840

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I think that I can make all members of smat pointers in MOZ_STACK_CLASS as safe once the patch is landed.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

ping to review...

Comment 8

[Kagami Rosylight 🏳️‍🌈🏳️‍⚧️ [:saschanaz] (they/them)]

Heh, sorry, it was a long holiday in Germany until this Monday.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1620312 - part 1: Alias smart pointer types in CanRunScriptChecker::registerMatchers r=saschanaz!

They make the following patches smaller (and look like simpler).

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1620312 - part 2: Allow to call a method of/with an element reference of immutable smart pointer array in the stack r=andi!,saschanaz!

This allows the following cases:

const nsTArray<RefPtr<T>> array;
array.ElementAt(0)->can_run_script();
array[0]->can_run_script();
can_run_script(array.ElementAt(0));
can_run_script(array[0]);

Depends on D175312

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1620312 - part 3: Treat references initialized with known live thing as safe r=saschanaz!

Since references are immutable (different from pointers), we can think that
the references are safe if they are initialized with known live things.
For example, the following calls are safe:

const nsTArray<RefPtr<T>> array;
const RefPtr<T>& firstElement = array[0];
firstElement->can_run_script();
can_run_script(firstElement);

Depends on D175313

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1620312 - part 4: Assume instances of classes marked as `MOZ_STACK_CLASS` are always in the stack r=andi!,saschanaz!

MOZ_STACK_CLASS guarantees that the class is instantiated only in the stack.
Therefore, if a class is marked as so, we can assume that it's in the stack
even if it comes from parameter of the method. Therefore, this allows the
following cases:

class MOZ_STACK_CLASS StackSmartPtrTArray : public AutoTArray<RefPtr<T>, 10> {};

MOZ_CAN_RUN_SCRIPT MayRunScript(const StackSmartPtrTArray& aArray) {
  aArray.ElementAt(0)->can_run_script();
  aArray[0]->can_run_script();
  can_run_script(aElement.ElementAt(0));
  can_run_script(aElement[0]);
}

And also the references to the elements of aArray in this case.

Depends on D175314

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1620312 - part 5: Allow method call of smart pointers in mutable array if the array is in the stack r=andi!,saschanaz!

While calling a method, we can assume that the array keeps grabbing the
ref-counted object if the array is in the stack even if it's mutable.
However, if the result of a element getter is a mutable reference, it's not
safe to pass it to a method taking the mutable reference as-is. I.e.,

nsTArray<RefPtr<T>> array;
array.ElementAt(0)->can_run_script();
array[0]->can_run_script();

are always safe.

However,

can_run_script(array.ElementAt(0));
can_run_script(array[0]);

is safe if can_run_script takes T* or const RefPtr<T>&, but it's not safe
if it takes RefPtr<T>&.

Unfortunately, it's difficult to check whether the method takes RefPtr<T>& or
not because it requires to check the callee declaration, but fortunately, we
don't have the cases which hit this. Therefore, we don't need to allow this
tricky case. (Note that if the array is const nsTArray<RefPtr<T>>, the
element getter always returns const RefPtr<T>&, so, this issue appears only
with a mutable array.)

Depends on D175315

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1620312 - part 6: Allow smart pointer reference declared in a range-based for statement and initialized with safe array r=andi!,saschanaz!

This is the main goal of this bug. References of smart pointer which is
initialized in a range-based for loop with a known safe array should be
treated as safe. I.e., the following cases should be safe.

for (const RefPtr<T>& element : immutableArray) {
  element->can_run_script();
  can_run_script(element);
}

And also, even if the array is a mutable array, we should allow it if wrapped in
std::as_const. I.e.,

for (const RefPtr<T>& element : std::as_const(mutableArray)) {
  element->can_run_script();
  can_run_script(element);
}

Finally, if mozilla::Reversed is used, there is only a CallExpr in the
for-rage-initializer. Therefore, we need to handle it separately.

Depends on D175316

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1620312 - part 7: Allow references declared in for-range-declaration with temporary array r=andi!,saschanaz!

Then, the following code are also safe:

for (const RefPtr<T>& element : mutableArray.Clone()) {
  element->can_run_script();
  can_run_script(element);
}

and

for (const RefPtr<T>& element :
     CopyableAutoTArray<RefPtr<T>, N>(mutableArray)) {
  element->can_run_script();
  can_run_script(element);
}

Depends on D175317

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1620312 - part 8: Rename some variables in `CanRunScriptChecker::registerMatchers` r=saschanaz!

Depends on D175318

Comment 17

[Kagami Rosylight 🏳️‍🌈🏳️‍⚧️ [:saschanaz] (they/them)]

Sorry for the delay, I'm looking again.

But I'm not sure it's worth all the complexity not only for the array but also for the general MOZ_CAN_RUN_SCRIPT setup. I wonder it could be simpler if we could just force additional addref-ing for every parameter and the class instance, instead of trying to carefully maintain a complex allowlist that is becoming harder and harder to read. (See also https://bugzilla.mozilla.org/show_bug.cgi?id=1661815#c20)

This is security related analysis and I fear that adding more complexity would not just make the analysis not readable but also cause a hole.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Kagami [:saschanaz] from comment #17)

But I'm not sure it's worth all the complexity not only for the array but also for the general MOZ_CAN_RUN_SCRIPT setup.

I think the main motivation of that is, extra addref/release appear in profiles in hot paths, therefore, it should be avoided in explicitly safe cases. Additionally, the cases which will be fixed in part 9 are safe and using MOZ_KnownLive is riskier than after applying the patches because MOZ_KnownLive allows any dangerous cases, but the static analysis bug can be fixed with updating the CaunRunScriptChecker and that detects the points where are not safe. Therefore, I believe that it's worthwhile to fix this kind of bugs.

This is security related analysis and I fear that adding more complexity would not just make the analysis not readable but also cause a hole.

I think that without my patches, MOZ_KnownLive will appear in same situations. That would make the reviewers carefully review the code, but anyway not good steps to review patches.

I have now some ideas to make the code design simpler, like adding MOZ_LONGER_LIVE_RESULT or something to mark methods which may return a refcountable pointer, reference or smart pointer reference and check the parent object's lifetime like my patches. However, it will allow not to use MOZ_KnownLive in more situations, so doing it is riskier than only allowing the array class cases and the code may be simpler, but must be bigger than my patches because it needs to be careful for more things.

I understand your concern about regressions, but MOZ_KnownLive is riskier. It also tells everybody "here may be unsafe".

Comment 19

[Kagami Rosylight 🏳️‍🌈🏳️‍⚧️ [:saschanaz] (they/them)]

Oh sure, I agree that MOZ_KnownLive can be very well a footgun and that's why I started to touch this. I'll review the patch stack, but I still wanted to put another option in the table.

I think the main motivation of that is, extra addref/release appear in profiles in hot paths, therefore, it should be avoided in explicitly safe cases.

The question is how significant that is. Might be worth to implement a PoC and run some perf tests.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Extra AddRef/Release is very significant.

Comment 21

[BugBot [:suhaib / :marco]]

The following patches are waiting for review from an inactive reviewer:

ID	Title	Author	Reviewer Status
D175316	Bug 1620312 - part 5: Allow method call of smart pointers in mutable array if the array is in the stack r=andi!,saschanaz!	masayuki	saschanaz: Back Sep 10, 2023
D175317	Bug 1620312 - part 6: Allow smart pointer reference declared in a range-based for statement and initialized with safe array r=andi!,saschanaz!	masayuki	saschanaz: Back Sep 10, 2023
D175318	Bug 1620312 - part 7: Allow references declared in for-range-declaration with temporary array r=andi!,saschanaz!	masayuki	saschanaz: Back Sep 10, 2023
D175970	Bug 1620312 - part 8: Rename some variables in CanRunScriptChecker::registerMatchers r=saschanaz!	masayuki	saschanaz: Back Sep 10, 2023

:masayuki, could you please find another reviewer?

For more information, please visit BugBot documentation.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

No, Kagami is the best reviewer for them, and they have already reviewed some of them, but I currently do not have much time to update the patches due to my condition.