bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Enable static rooting analysis on mobile

ASSIGNED
Assigned to

Status

()

Core
JavaScript Engine
ASSIGNED
5 years ago
2 years ago

People

(Reporter: terrence, Assigned: sfink, NeedInfo)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [leave open)

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
Several mobile peers have told us that there is no mobile-specific code that interacts with JSAPI and Steve's manual run showed that we are totally clean in this build. Running these on every checkin would be a waste of resources.

On the other hand, if any hazards do creep in, they would be invisible sec-crit bugs -- probably zero-day, given that the analysis is public. Not running the analysis would be grossly irresponsible.

As a compromise, we'd like to run the hazard analysis on mobile nightly. Given that the analysis makes it extremely easy to track down the error, finding the regressing patch will still be trivial.
(Assignee)

Comment 1

5 years ago
Created attachment 8370904 [details] [diff] [review]
Add a mobile hazard analysis build to the buildbot config

This only adds an optional try build, not a nightly.
Attachment #8370904 - Flags: review?(bhearsum)
(Assignee)

Updated

5 years ago
Assignee: nobody → sphink
Status: NEW → ASSIGNED
(Assignee)

Comment 2

5 years ago
Hm, looks like I'll need to rebase the mozharness changes past bug 961314 before I can post them.
(In reply to Steve Fink [:sfink] from comment #1)
> Created attachment 8370904 [details] [diff] [review]
> Add a mobile hazard analysis build to the buildbot config
> 
> This only adds an optional try build, not a nightly.

Terrence's comment suggests that we need to be running these regularly to reduce the risk that we get 0 day'ed. Is somebody going to be pushing these jobs to try regularly to fulfill this?
(Assignee)

Comment 4

5 years ago
(In reply to Ben Hearsum [:bhearsum] from comment #3)
> (In reply to Steve Fink [:sfink] from comment #1)
> > Created attachment 8370904 [details] [diff] [review]
> > Add a mobile hazard analysis build to the buildbot config
> > 
> > This only adds an optional try build, not a nightly.
> 
> Terrence's comment suggests that we need to be running these regularly to
> reduce the risk that we get 0 day'ed. Is somebody going to be pushing these
> jobs to try regularly to fulfill this?

It's a cost/benefit tradeoff. These builds are defending against the possibility that mobile firefox devs will add some new JSAPI-using code with JS rooting problems. That would be very bad, and could result in a 0-day.

On the other hand, mobile firefox devs have never yet added *any* JSAPI-using code that needs to deal with rooting at all, and apparently have no intention of doing so. All the code uses a higher level Gecko interface. So right now, this is guarding against a purely theoretical problem. The question is how many build resources are worth guarding against the theoretical problem. A nightly build feels about right to me. I don't trust us to dependably do manual try pushes regularly.

But I've never added a new nightly, so I'd need to either figure out how or get that bhearsum guy to do it for me. (This is a mozharness-based build, btw.)
(Assignee)

Comment 5

5 years ago
This bug will need multiple patches.
Whiteboard: [leave open
Comment on attachment 8370904 [details] [diff] [review]
Add a mobile hazard analysis build to the buildbot config

(In reply to Steve Fink [:sfink] from comment #4)
> (In reply to Ben Hearsum [:bhearsum] from comment #3)
> > (In reply to Steve Fink [:sfink] from comment #1)
> > > Created attachment 8370904 [details] [diff] [review]
> > > Add a mobile hazard analysis build to the buildbot config
> > > 
> > > This only adds an optional try build, not a nightly.
> > 
> > Terrence's comment suggests that we need to be running these regularly to
> > reduce the risk that we get 0 day'ed. Is somebody going to be pushing these
> > jobs to try regularly to fulfill this?
> 
> It's a cost/benefit tradeoff. These builds are defending against the
> possibility that mobile firefox devs will add some new JSAPI-using code with
> JS rooting problems. That would be very bad, and could result in a 0-day.
> 
> On the other hand, mobile firefox devs have never yet added *any*
> JSAPI-using code that needs to deal with rooting at all, and apparently have
> no intention of doing so. All the code uses a higher level Gecko interface.
> So right now, this is guarding against a purely theoretical problem. The
> question is how many build resources are worth guarding against the
> theoretical problem. A nightly build feels about right to me. I don't trust
> us to dependably do manual try pushes regularly.

I think you're in a better place to make this call than me. This patch seems like a fine first step though.

> But I've never added a new nightly, so I'd need to either figure out how or
> get that bhearsum guy to do it for me. (This is a mozharness-based build,
> btw.)

If it's going to be nightly only, who is going to look at it? Nothing that is nightly only is allowed to be shown on TBPL by default. If we still want to do it, you'll be treading new ground here. We don't have nightly only things that are implemented as a platform like this.
Attachment #8370904 - Flags: review?(bhearsum) → review+
(Assignee)

Comment 7

4 years ago
Created attachment 8430930 [details] [diff] [review]
mobile browser static rooting analysis

Old patch I have lying around for implementing the mozharness job. Haven't run it in ages. Might need some rebasing.
(Reporter)

Updated

4 years ago
Blocks: 1008341
No longer blocks: 834909
(Reporter)

Comment 8

2 years ago
https://dxr.mozilla.org/mozilla-central/source/widget/android/NativeJSContainer.cpp

We do have code using JSAPI in Android-only C++ code. We do need to ship the analysis there. How hard is it going to be to whip up a TC build for this?
Flags: needinfo?(sphink)
You need to log in before you can comment on or make changes to this bug.