913760 - Upload static rooting analysis results

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Steve Fink [:sfink] [:s:]]

Hi, I'm back! I need to upload the static rooting analysis results somewhere so people can actually see them.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Upload static rooting analysis results

This requires a buildbotcustom patch to feed through the information necessary for constructing upload paths and getting to the server.

Comment 2

[Steve Fink [:sfink] [:s:]]

Attachment: Pass through more configuration to mozharness jobs

Buildbot knows where the servers and things are, so it should communicate them to the mozharness jobs.

Comment 3

[Steve Fink [:sfink] [:s:]]

Attachment: Allow compressing when copying to upload dir

One of the analysis files is really, really repetitive, and when analyzing the full browser, comes to 159MB. I really don't want to upload all of that. Compressing it brings it all the way down to 3MB (as I said: repetitive). I kind of like having the option to compress built into the copy into the upload area; your opinion may differ wildly.

Comment 4

[Steve Fink [:sfink] [:s:]]

Attachment: imported patch checks

Somewhat silly patch to catch problems sooner and with clearer messages. I thought I had several actions that I could apply this decorator to, but it turns out that this is all in the upload step. Naturally, I wrote this patch *after* tediously working through all of the issues it would have caught. But there's a chance I missed something that will make this fail in a production config.

Ok, mostly I'm wondering what you think of the general idea. I was considering extending it to config and/or buildbot_config entries. It's hard to keep track of what needs what, and I sometimes want to run the script with a subset of the actions that don't need a bunch of the configuration. (Eg, run the analysis locally and don't upload it => no need for SSH keys and servers and stuff.)

Comment 5

[Steve Fink [:sfink] [:s:]]

Callek pointed out that we should consider the security implications of the static rooting analysis. After this bug, we'd be running it on the try server and uploading the results to a publicly-accessible repository.

Note that the results of this analysis are not as sensitive as, say, the ASAN analysis. But this analysis *does* identify defects that could trigger memory corruption and therefore are potentially exploitable. The fuzz posse (gkw, decoder, Jesse) have more experience with these sorts of GC bugs, but I will add some of my own speculation here:

First, none of the hazards identified by this analysis are relevant at all until we turn off the conservative stack scanner. Most problems will also require turning on some sort of moving GC (initially, that will be the generational garbage collector.)

The upload will produce 3 files: gcTypes.txt, gcFunctions.txt, and rootingHazards.txt. The first two are purely information about what functions can potentially trigger a GC, and are not a problem. rootingHazards.txt contains 3 types of entries: unnecessary roots, which are only relevant to performance; actual hazards, where a GC at the wrong time can invalidate a pointer; and unsafe references, which may indicate an actual hazard or may just be a false positive.

We intend to drive actual hazards to zero before enabling exact rooting, or at least manually inspecting them to ensure they are false positives. So these shouldn't pose a problem.

The unsafe references are a little trickier. All of them must be manually inspected to distinguish false positives from real hazards. We may need to put a check in place to ensure that all references reported are known to be false positives. (We will manually inspect all of these before enabling exact rooting.)

If a hazard were to slip through, it could result in a use-after-free or access to the wrong object, possibly one in another compartment/security domain. The difficulty of triggering such a hazard depends on what code it is in. Exploiting a hazard would be fairly hard, but not impossible, if I were to guess. But I'm kind of out of my depth on that question.

Note that this analysis uses all easily-available components, so someone determined could run it themselves. Uploading autogenerated results to a public server obviously makes it far easier to get hold of, of course.

Comment 6

[Steve Fink [:sfink] [:s:]]

CC'ing dveditz on gkw's recommendation. And clarifying that these results will only be relevant when generational garbage collection is turned on. (Strictly speaking, it's when exact rooting is enabled, but we won't do that until we're turning on GGC.)

Comment 7

[Aki Sasaki (not active)]

Comment on attachment 801077 [details] [diff] [review]
Upload static rooting analysis results

I don't think the upload_ssh_* properties are actually set currently, though I may be surprised in a later patch...

Comment 8

[Aki Sasaki (not active)]

Comment on attachment 801078 [details] [diff] [review]
Pass through more configuration to mozharness jobs

And... boom.  I think I ruined the surprise.

Comment 9

[Aki Sasaki (not active)]

Comment on attachment 801079 [details] [diff] [review]
Allow compressing when copying to upload dir

Sure, looks good.

Comment 10

[Aki Sasaki (not active)]

Comment on attachment 801080 [details] [diff] [review]
imported patch checks

I'm not familiar with this, but I can stamp it.

... All of my reviews are dependent on sec review passing.

Comment 11

[Daniel Veditz [:dveditz]]

sfink said in irc:
  we'd start uploading with 1-2 dozen remaining. We won't turn anything on until they're at zero (or identified as false positives)

That sounds like a fine plan as long as someone will be regularly looking at these results in the future.

Comment 12

[Steve Fink [:sfink] [:s:]]

http://hg.mozilla.org/build/buildbotcustom/rev/61b695e32092

Comment 13

[Steve Fink [:sfink] [:s:]]

I still have mixed feelings about the decoration thing, but I'll leave it for now.

http://hg.mozilla.org/build/mozharness/rev/4595f8895ed3
http://hg.mozilla.org/build/mozharness/rev/ef200219ec21
http://hg.mozilla.org/build/mozharness/rev/fe41b7979c01

Comment 14

[Steve Fink [:sfink] [:s:]]

Attachment: Fix upload destination path

Oops, when running in a buildbot setup, files were being uploaded successfully, but to the wrong place.

Comment 15

[Steve Fink [:sfink] [:s:]]

http://hg.mozilla.org/build/mozharness/rev/4ecc13e691a6

Comment 16

[Armen [:armenzg]]

Merged to mozharness' production branch.
It is now live on tbpl.

Comment 17

[Steve Fink [:sfink] [:s:]]

(In reply to Armen Zambrano [:armenzg] (Release Engineering) (EDT/UTC-4) from comment #16)
> Merged to mozharness' production branch.
> It is now live on tbpl.

Except it fails, because I messed up and landed the buildbotcustom change to the production branch by mistake. Aki noticed that the branches didn't match and blew it away in 7be15a497289, which caused these jobs to fail. So I just relanded onto the default branch:

http://hg.mozilla.org/build/buildbotcustom/rev/80f90cac6464