Improve on-stack const TArray whitelisting for CanRunScriptChecker (MOZ_CAN_RUN_SCRIPT)
Categories
(Developer Infrastructure :: Source Code Analysis, task, P3)
Tracking
(Not tracked)
People
(Reporter: bzbarsky, Assigned: masayuki)
References
(Depends on 3 open bugs)
Details
Attachments
(9 files, 1 obsolete file)
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review |
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:
- The
varDecl
's type islValueReferenceType(pointee(isSmartPtrToRefCounted()))
(possibly with someisConstQualified()
thrown in. - It's a
varDecl
that is thehasLoopVariable
of a ranged-for statement or so. - 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".)
Reporter | ||
Comment 1•4 years ago
|
||
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.
Updated•3 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Assignee | ||
Comment 3•1 year ago
|
||
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...
Assignee | ||
Comment 4•1 year ago
|
||
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.
Updated•1 year ago
|
Assignee | ||
Comment 5•1 year ago
|
||
Depends on D174840
Updated•1 year ago
|
Updated•1 year ago
|
Updated•1 year ago
|
Assignee | ||
Comment 6•1 year ago
|
||
I think that I can make all members of smat pointers in MOZ_STACK_CLASS
as safe once the patch is landed.
Comment 8•1 year ago
|
||
Heh, sorry, it was a long holiday in Germany until this Monday.
Updated•1 year ago
|
Assignee | ||
Comment 9•1 year ago
|
||
They make the following patches smaller (and look like simpler).
Assignee | ||
Comment 10•1 year ago
|
||
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
Assignee | ||
Comment 11•1 year ago
|
||
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
Assignee | ||
Comment 12•1 year ago
|
||
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
Assignee | ||
Comment 13•1 year ago
|
||
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
Assignee | ||
Comment 14•1 year ago
|
||
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
Assignee | ||
Comment 15•1 year ago
|
||
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
Updated•1 year ago
|
Assignee | ||
Comment 16•1 year ago
|
||
Depends on D175318
Updated•1 year ago
|
Comment 17•11 months ago
|
||
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.
Assignee | ||
Comment 18•11 months ago
|
||
(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•11 months ago
|
||
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•11 months ago
|
||
Extra AddRef/Release is very significant.
Comment 21•8 months ago
|
||
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.
Assignee | ||
Comment 22•8 months ago
|
||
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.
Description
•