[VRP] Untrusted fuzzer UI#5329
Conversation
37f5d31 to
8d2a70a
Compare
6deb047 to
7feb0e9
Compare
7feb0e9 to
ec90403
Compare
letitz
left a comment
There was a problem hiding this comment.
Thanks for sending this! Overall I think we should invert the trusted/untrusted bit to make trust explicit instead of implicit, and default to safety in the absence of an explicit trust signal.
| if not job: | ||
| raise helpers.EarlyExitError(f'Job {job_name} not found.', 400) | ||
| if job.platform.lower() != 'linux': | ||
| raise helpers.EarlyExitError( | ||
| f'Untrusted fuzzers can only be run on Linux jobs. ' | ||
| f'Job "{job_name}" has platform "{job.platform}".', 400) |
There was a problem hiding this comment.
There is a similar check for untrusted testcases:
clusterfuzz/src/appengine/handlers/upload_testcase.py
Lines 416 to 423 in 5f7e9d7
We should factor the check out into a single method somewhere to ensure there is no accidental divergence in behavior as the code evolves:
def check_job_supports_untrusted_workloads(job_name: str) -> None:
"""Checks that the named job can run untrusted testcases or fuzzers.
Raises:
ValueError: if the job can run on privileged bots and should never run
an untrusted workload.
"""
if not ...:
raise ValueError(f'Job "{job_name}" does not support running untrusted workloads. $reason")Structuring it with an exception allows this helper to define the error message to show to users, which can evolve over time as the condition changes (maybe one day we won't support untrusted execution on all linux jobs, or maybe we'll support it for windows jobs etc.).
| <paper-checkbox checked="{{fuzzer.external_contribution}}">External contribution</paper-checkbox> | ||
| </div> | ||
| <div class="inline narrow"> | ||
| <paper-checkbox checked="{{fuzzer.untrusted}}">Untrusted</paper-checkbox> |
There was a problem hiding this comment.
We should default to safe behavior: if someone forgets to check the box, we should be conservative and assume the fuzzer is not trusted. Let's invert the meaning of the box to "trusted" instead.
| # Does it run un-trusted content ? Examples including running live sites. | ||
| untrusted_content = ndb.BooleanProperty(default=False) |
There was a problem hiding this comment.
This is interesting! Have you looked into what this does?
| untrusted_content = ndb.BooleanProperty(default=False) | ||
|
|
||
| # Is this fuzzer untrusted? (VRP support) | ||
| untrusted = ndb.BooleanProperty(default=False) |
There was a problem hiding this comment.
Same deal here, let's default to safe behavior and assume jobs are untrusted if not specifically told otherwise. Let's invert this boolean to "trusted" (default false) just like for Testcase:
We can run a migration script to set the property to true for all existing fuzzers in the database.
Problem
We need a way to identify fuzzers that are "untrusted" (e.g., external fuzzers provided by VRP researchers) so we can apply different security policies to them (like restricting where they can run). Currently, there is no field in the UI or Datastore to mark a fuzzer as untrusted.
Proposed Solution
Add an
untrustedboolean field to theFuzzermodel in Datastore and expose it as a simple checkbox in the Fuzzer management UI. This allows admins to easily mark specific fuzzers as untrusted.UI Change
Trying to upload a untrusted fuzzer for a non-linux job gives an error:
Testing
Validated by deploying to dev and verifying that the checkbox state is correctly persisted to and loaded from Datastore.
Validation
I have uploaded a fuzzer with
untrustedset to true; here is a screenshot.