[Fall 2021] Step 3: Add a `pyre validate-taint-config` command and make errors in taintConfiguration.ml typed

[onionymous]

Outdated, please read later comment.

Pysa rules, sources, sinks and other taint information are specified in taint.config files. Multiple taint.config files can be specified in one project. When Pysa is run, it looks at all the taint.config files specified in the "taint_models_path" of the .pyre_configuration file for that project, and reads the rules, source/sink names, etc. from all of these files.

The goal of this project is to have some method to validate these taint.config files. This can be in the form of a Python script that we can run standalone, or something that is run at the start of invoking the Python client.

Some validation ideas to get started (feel free to think of more!):

1. Make sure there are no duplicate warning codes. All Pysa rules should have a unique warning code. When there are multiple taint.config files, it can be confusing to keep track of the codes across different files, so we want to make sure no codes are repeated.
2. Make sure the source/sink names used in rules exist.
3. A regex validator for implicit string sources/sinks. Pyre/Pysa is written in OCaml and the regex engine used is re2. This is a non-backtracking engine which doesn't support a lot of features in other regex flavors like lookaheads, etc. (see https://github.com/google/re2/wiki/Syntax) so we want to make sure the regexes are valid re2 syntax. In addition, we want to make sure backslashes are escaped properly in these regexes, etc.

[abishekvashok]

Okay, so I was looking at the ocaml source,
It seems we have the warning code uniqueness validator

pyre-check/source/interprocedural_analyses/taint/taintConfiguration.ml

Lines 208 to 212 in b21dede

	let validate_code_uniqueness code =
	if Hash_set.mem seen_rules code then
	failwith (Format.sprintf "Multiple rules share the same code `%d`." code);
	Hash_set.add seen_rules code
	in

But we don't have that applied across multiple taint.config files (or do we?)

And for sources and sinks, we validate them via this function:

pyre-check/source/interprocedural_analyses/taint/annotationParser.ml

Lines 20 to 48 in b21dede

	let parse_source ~allowed ?subkind name =
	match
	List.find allowed ~f:(fun { name = source_name; _ } -> String.equal source_name name), subkind
	with
	(* In order to support parsing parametric sources and sinks for rules, we allow matching
	parametric source kinds with no subkind. *)
	| Some _, None -> Ok (Sources.NamedSource name)
	| Some { kind = Parametric; _ }, Some subkind ->
	Ok (Sources.ParametricSource { source_name = name; subkind })
	| _ -> Error (Format.sprintf "Unsupported taint source `%s`" name)
	let parse_sink ~allowed ?subkind name =
	let create = function
	| "LocalReturn" -> Ok Sinks.LocalReturn
	| update when String.is_prefix update ~prefix:"ParameterUpdate" ->
	let index = String.chop_prefix_exn update ~prefix:"ParameterUpdate" in
	Ok (ParameterUpdate (Int.of_string index))
	| name -> Error (Format.sprintf "Unsupported taint sink `%s`" name)
	in
	match
	List.find allowed ~f:(fun { name = sink_name; _ } -> String.equal sink_name name), subkind
	with
	(* In order to support parsing parametric sources and sinks for rules, we allow matching
	parametric source kinds with no subkind. *)
	| Some _, None -> Ok (Sinks.NamedSink name)
	| Some { kind = Parametric; _ }, Some subkind ->
	Ok (Sinks.ParametricSink { sink_name = name; subkind })
	| _ -> create name

However, it doesn't display line number and column number.

@onionymous So as you specified yesterday, this issue can be changed to tweak the error codes and display line number and column number with taint.config errors? Just confirming before I proceed further :)

[onionymous]

Yup, as discussed internally with @0xedward and @arthaud, we want to do this on the OCaml side. We already have an existing taint.config parser and does some validation, so this task will be pretty much scoped to having more descriptive error messages on:

1. duplicate warning code or duplicate source/sink names
2. when there is a regex error in an implicit string source/sink

Thankfully, since OCaml already uses re2, this means we don't have to deal with packaging an external Python dependency for it - just compiling the regex in OCaml should perform the validation :)

We want this to display useful error messages so that it can be used for integration with e.g. the VSCode plugin in the future, so we probably need information like the line and column number. Right now pyre validate-models actually does a pretty good job for this, but the messages aren't descriptive enough:

e.g. Duplicate source:

λ  distillery  (33b689571|remote/master) pyre validate-models
ƛ Using unstable client. If you encounter issues, you can use the stable client via `export PYRE_CLIENT_RELEASE="stable"`.
ƛ Exception occured during model validation: Invalid payload for model validation: `{'error': '(Failure "Duplicate entry for source: `IP`")'}`.

Malformed regex:

ƛ Exception occured during model validation: Invalid payload for model validation: `{'error': '("Taint__TaintConfiguration.MalformedConfiguration(\\"/data/users/sym/instagram/instagram-server/distillery/stubs/taint/core_privacy_security/open_source/taint.config\\", \\"Line 647, bytes 5-38:\\\\nInvalid token \']\\\\n  },\\\\n  \\\\\\"implicit_sinks\\\\\\": {\\\\n    \'\\")")'}`.

Also going to paste Maxime's internal comment here:

If we are going to extend pyre validate-models there is a small caveat though: I think it requires type checking the whole project. That would make it pretty slow for the VSCode Integration.
We could make this a separate command if needed. We could also check if pyre validate-models could work with an incremental type check (I don't know what it does right now).

We can also think about adding a pyre validate-config command, which skips the typechecking part to make it faster for something like VSCode integration.

[abishekvashok]

After facebook#732 here's a plan of action:

• Migrate from yojson to jsonm to parse taint configuration files, ie, implement parsing into an AST that is loosely defined in Add position information to taint.config errors facebook/pyre-check#732 with utility functions in a wrapper module for Jsonm: Implement json parser and migrate taintConfiguration facebook/pyre-check#734

• Add location support for rule duplicate errors: Display positions in RuleCodeDuplicate errors facebook/pyre-check#735

• Add location support for source duplicate errors: Display positions in SourceDuplicate errors facebook/pyre-check#741

• Add location support for sink duplicate errors: Display positions in SinkDuplicate errors facebook/pyre-check#756

• Add location support for implicit_sources

• Add location support for implicit_sinks

• Add location support for transforms: Display position in TransformDuplicate errors facebook/pyre-check#775

• Add location support for features

• Add location support for MalformedRegex: Adds InvalidRegex TaintConfiguration Error type facebook/pyre-check#743

• Add --verify-taint-configuration command that just does taint model verification: Adds verify taint config only option facebook/pyre-check#740

• Switch from yojson to the new module in other parts of the codebase, in a one by one fashion.