Page MenuHomePhabricator

Goal: Implement static code analysis for security
Closed, ResolvedPublic

Event Timeline

csteipp created this task.Aug 27 2015, 11:28 PM
csteipp raised the priority of this task from to Needs Triage.
csteipp updated the task description. (Show Details)
csteipp added a project: Security-Team.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 27 2015, 11:28 PM
Addshore added a subscriber: Addshore.
JeroenDeDauw added a subscriber: JeroenDeDauw.EditedSep 2 2015, 3:47 PM

Does this mean we can switch to Hack (or PHP7)? :)

Restricted Application added a subscriber: StudiesWorld. · View Herald TranscriptJan 6 2016, 4:43 PM
csteipp closed this task as Resolved.Jan 6 2016, 5:14 PM
csteipp claimed this task.
csteipp added subscribers: dpatrick, csteipp.

During Q2 we again investigated Veracode, Checkmarx and Pfff.

  • Pfff - taint-tracking is still closed-source at facebook. Variable analysis is too noisy to be useful. As a result, we cannot use pfff for static security analysis at this time.
    • The semantic patching may be useful as part of a preparation pipeline for submitting to other tools.
    • We will continue to evaluate pfff in the future and track their progress. @dpatrick may be able to implement our own version of their taint tracking.
  • Checkmarx - As a tool, Checkmarx gives the most flexibility (allowing us to write our own rules), and most useful information about security flaws (shows full path that triggered security rule through code). Running on our own servers gives us good visibility into how the tool is performing for us. Those debugging logs, however, showed us that the tool was failing to parse significant portions of the MediaWiki core + tarball extensions, due to unexpected syntax. The tool was not able to parse closures as function parameters, assigning object properties using list(), and some other similar issues. These, along with the proposal to move mediawiki to use PHP 5.5 syntax which seems likely to be a challenge for Checkmarx to parse given the types of issues we've seen in 5.3 syntax, precludes us from using the tool.
  • Veracode - Trial scan seemed to give legitimate results for code in core, but it was unable to trace taint issues through extensions. During the evaluation, we found a potential false-negative, and their engineers indicated they would be adding detection for that pattern in a future release. In addition to being responsive to improvement suggestions, Veracode was willing to give us a significant price discount, so we will be using them to scan deployment branches of core each week.

Core + PdfHandler are being submitted to Veracode after each branch cut. This should alert the security team to potential issues within core, and hopefully improvements to the Veracode platform will allow the tool to trace data flows within PdfHandler in the near future. The Security Team will look for ways to automate triage and reporting issues into Phabricator in the future.

This should be documented somewhere on the wiki.

This should be documented somewhere on the wiki.

Ping. I'm still looking for some info so that I can update

This should be documented somewhere on the wiki.

Ping. I'm still looking for some info so that I can update

Just FYI, we are no longer using veracode. The reports had too many false positives, and was not able to trace code execution well enough to catch many things with extensions. The result was nobody was reading the weekly report, so we stopped around nov 2016.

We are instead now moving to our home-grown phan-taint-check static analysis tool.

sbassett moved this task from Backlog to Done on the Security-Team board.Jun 11 2019, 7:13 PM