Regular expression models for literals #139

pkesseli · 2023-08-07T12:56:46Z

As discussed in the workplace group I added support for source models for literals matching configurable regular expressions. The models.md documentation outlines what that would look like:

Example literal models:

[
  {
    "pattern": "SELECT \\*.*",
    "description": "Potential SQL Query",
    "sources": [
      {
        "kind": "SqlQuery"
      }
    ]
  },
  {
    "pattern": "AI[0-9A-Z]{16}",
    "description": "Suspected Google API Key",
    "sources": [
      {
        "kind": "GoogleAPIKey"
      }
    ]
  }
]

Example code:

void testRegexSource() {
  String prefix = "SELECT * FROM USERS WHERE id = ";
  String aci = getAttackerControlledInput();
  String query = prefix + aci; // Sink
}

void testRegexSourceGoogleApiKey() {
  String secret = "AIABCD1234EFGH5678";
  sink(secret);
}

Reviewer note

In order to test these changes, I added new classes and methods to source/tests/integration/end-to-end/library_classes, which causes a significant amount of unrelated changes to other tests' expected_xxx.json. I tried to constrain those changes to the commit Update library_classes.

yuhshin-oss

Thanks! Looks pretty good to me :D Just small comments/nits

yuhshin-oss · 2023-08-07T23:25:01Z

source/LiteralModel.h

+  void join_with(const LiteralModel& other);
+
+  /**
+   * Indicats whether model contains any taints.


nit: spelling "Indicates"

🐱‍👓

yuhshin-oss · 2023-08-07T23:29:41Z

source/LiteralModel.cpp

+static constexpr const char* PATTERN = "pattern";
+
+static constexpr const char* SOURCES = "sources";


nit: we generally don't define json keys as constants in our code base (reasoning being that it adds another level of indirection for something that is quite localized). We just use the string literal at the call-site directly

yuhshin-oss · 2023-08-07T23:32:44Z

source/LiteralModel.cpp

+LiteralModel LiteralModel::from_json(
+    const Json::Value& value,
+    Context& context) {
+  JsonValidation::validate_object(value);


nit: I think check_unexpected_members already calls validate_object

source/LiteralModel.cpp

yuhshin-oss · 2023-08-07T23:42:32Z

source/Registry.cpp

+  for (const auto& literal_model : literal_models_) {
+    if (literal_model.second.matches(literal)) {


nit: think we usually do:

for (const auto& [pattern, model] : literal_models_) { if (pattern.matches(literal)) { // ... } }

source/Registry.h

yuhshin-oss · 2023-08-07T23:52:46Z

source/tests/integration/end-to-end/code/literal_source/RegexSource.java

+  // -------------------------------------------------------
+  //  Issues
+  // -------------------------------------------------------
+  public static void testRegexSource() {


nit: we usually have the word "issue" somewhere in the method name if we're expecting an issue there... so maybe testRegexSourceIssue() or something? (and testRegexSourceNoMatchNoIssue() when there are no issues expected)

pkesseli · 2023-08-09T21:22:11Z

I also realised since I updated library_classes I also affected source/tests/integration/end-to-end/code/check_cast_types, which doesn't get run by default in tests.yml. I updated those expected JSON files in my latest push as well now.

I manually compiled check_cast_types/KotlinCheckCast.kt and added it to the JAR - not sure whether that's intended way of running this test, but it produced the same JSON output files again except for my new models in library_classes. 🙂

facebook-github-bot · 2023-08-14T14:34:37Z

@yuhshin-oss has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

yuhshin-oss · 2023-08-14T22:10:01Z

source/ForwardTaintTransfer.cpp

@@ -5,6 +5,8 @@
 * LICENSE file in the root directory of this source tree.
 */

+#include <regex>


Missed this before, but let's use <re2/re2.h> instead

yuhshin-oss · 2023-08-14T22:18:39Z

source/ForwardTaintTransfer.cpp

@@ -5,6 +5,8 @@
 * LICENSE file in the root directory of this source tree.
 */

+#include <regex>


Missed this before, but let's use <re2/re2.h> instead

yuhshin-oss · 2023-08-14T22:19:30Z

source/ForwardTaintTransfer.cpp

+  if (model.empty())
+    return false;


nit: style:

if (model.empty()) { return false; }

yuhshin-oss · 2023-08-14T22:19:53Z

source/LiteralModel.h

+
+#pragma once
+
+#include <regex>


Similarly, use <re2/re2.h> instead.

yuhshin-oss · 2023-08-14T22:20:59Z

source/LiteralModel.h

+  Taint sources_;
+};
+
+}


nit:

} // namespace marianatrench

yuhshin-oss

Ugh, couple of clangtidy/format issues showing up internally that's too tedious to type all of them here. If you can address the RE2 one, we should be able to auto-format after importing

pkesseli · 2023-08-16T13:51:54Z

Ugh, couple of clangtidy/format issues showing up internally that's too tedious to type all of them here. If you can address the RE2 one, we should be able to auto-format after importing

Thanks, all addressed. I wasn't sure how to run the linter myself, otherwise I would have checked for that!

facebook-github-bot · 2023-08-17T16:54:40Z

@yuhshin-oss has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

arthaud

Sorry for coming up late on this, I saw a few things after @yuhshin-oss that I think are worth improving. Please bear with me.

arthaud · 2023-08-18T10:17:16Z

source/ForwardTaintTransfer.cpp

+  environment->write(
+      aliasing.result_memory_locations(),
+      model.sources(),
+      UpdateKind::Weak);


I think we should use a strong update here. This could matter if this is in a loop.

arthaud · 2023-08-18T10:20:28Z

source/LiteralModel.cpp

+  for (const auto& source : sources) {
+    mt_assert(source.is_leaf());
+    check_taint_config_consistency(pattern, source);
+    all_sources.join_with(Taint{std::move(source)});


You are using std::move on a const ref, that does nothing. might as well remove that std::move.

Thanks, I changed this to a const ref when I migrated to re2 and I missed the move.

arthaud · 2023-08-18T10:22:59Z

source/LiteralModel.cpp

+  return all_sources;
+}
+
+LiteralModel::LiteralModel() : regex_(pattern_) { }


so we create a LiteralModel with an empty string and a pattern for the empty string?
I would prefer if this was more clear in the internal state, for instance we could use a std::optional<re2::RE2>.

I now used an optional, and we do not match anything if default.

arthaud · 2023-08-18T10:26:22Z

source/LiteralModel.cpp

+LiteralModel::LiteralModel() : regex_(pattern_) { }
+
+LiteralModel::LiteralModel(
+    std::string&& pattern,


We usually take by copy in those cases.
See https://www.youtube.com/watch?v=PNRju6_yn3o

Also, as a rule of thumb, let's be consistent with the rest of the codebase.

Replaced by a std::string_view.

Well technically std::string_view is worse in the case where we move a std::string, it will make an extra copy.
but performance is not a problem here unless proven otherwise, so that's fine.

I am not sure I am seeing an extra copy in that case? std::string_view doesn't have a constructor for std::string &&, so won't it just do the same as usual and just copy the pointer and length? And the constructor of RE2 will always copy exactly once, irrespective of whether we use const std::string & or const StringPiece &/std::string_view, since StringPiece has a string_view constructor.

I thus defaulted to std::string_view since it doesn't require the caller to use std::move. Having said that, we invoke this constructor exactly once with an rvalue anyway, so I just changed it to an std::string to keep things simple and consistent. 🙂

arthaud · 2023-08-18T10:27:56Z

source/LiteralModel.cpp

+  if (this != &other) {
+    sources_.join_with(other.sources_);
+  }


If the patterns are different I think it would make sense to set it to null/empty?

Now checking if they have the same pattern, resetting regex_ otherwise.

arthaud · 2023-08-18T10:29:48Z

source/LiteralModel.h

+  Json::Value to_json(Context& context) const;
+
+ private:
+  std::string pattern_;


We don't need to store the pattern since RE2 already does it. We could get it with regex_.pattern().

Thanks, this was still an artifact from using std::regex before I migrated to RE2.

arthaud · 2023-08-18T10:30:52Z

source/Registry.cpp

@@ -176,13 +200,25 @@ void Registry::join_with(const FieldModel& field_model) {
  }
 }

+void Registry::join_with(const LiteralModel& literal_model) {
+  auto iterator = literal_models_.find(literal_model.pattern());
+  if (iterator != end(literal_models_)) {


nitpick: we usually use literal_models_.end(). Let's be consistent with the rest of the codebase.

arthaud · 2023-08-18T10:33:04Z

source/Registry.cpp

        for (std::size_t i = batch_size * batch; i < batch_size * (batch + 1) &&
-             i < models.size() + field_models.size();
+             i < models.size() + field_models.size() + literal_models.size();
             i++) {
          if (i < models.size()) {
            writer->write(models[i].to_json(context_), &batch_stream);
-          } else {
+          } else if (i < models.size() + field_models.size()) {
            writer->write(
                field_models[i - models.size()].to_json(context_),
                &batch_stream);
+          } else {
+            writer->write(
+                literal_models[i - models.size() - field_models.size()].to_json(context_),
+                &batch_stream);


pre-existing but we should probably rewrite this with a proper iterator. These index computations are error prone.

I'm not sure what refactoring you have in mind here. Presently these models are stored in three different vectors and accessed based on the current batch number. Is there an iterator-based API in sparta that I should use?

Just to be clear, I'm not expecting you to implement this here. This could be done in another PR, or we could do this internally.

Anyway, I changed my mind here. Yes we could probably make this work with iterators but the fact that iterators hide the indexing will make this code unreadable.

What might make it easier to read is abstracting away "models + field_models + literal_models". We could have:

class ModelVector { public: std::size_t size() const { return _models.size() + _field_models.size() + _literal_models.size(); } // TODO: find a better name.. Json::Value json_for_index(std::size_t i) { if (i < _models.size()) { return _models[i].to_json(); } i -= _models.size(); if (i < _field_models.size()) { return _field_models[i].to_json(); } i -= _field_models.size(); if (i < _literal_models.size()) { return _literal_models[i].to_json(); } mt_unreachable("invalid index"); } private: std::vector<Model> _models; std::vector<FieldModel> _field_models; std::vector<LiteralModel> _literal_models; }

arthaud · 2023-08-18T10:34:56Z

source/Registry.h

@@ -63,6 +65,7 @@ class Registry final {
  /* These are thread-safe. */
  Model get(const Method* method) const;
  FieldModel get(const Field* field) const;
+  LiteralModel get(std::string_view literal) const;


Could we change the name of this method?
When I read get I usually expect a O(1) but here we loop over all literal models.
Also, we might want another API latter that is an actual get for a given pattern.
I would call it match_literal or something.
Also nitpick: the comment above says "These are thread-safe" but your implementation of get is not thread-safe (iterating on a ConcurrentMap is not thread safe). I think it's fine though as long as there are no mutations in parallel.

Technically not my comment, but definitely surprised to learn that concurrent traversal in redex' ConcurrentMap is not safe. Not what I'm used to from concurrent_unordered_map. 🙂

Since I renamed the method, I now moved it away from those two get mehods and gave it a comment of its own.

arthaud · 2023-08-18T10:37:51Z

source/tests/integration/end-to-end/library_classes/StringBuilder.java

+public class StringBuilder {
+
+  public StringBuilder append(String str) {
+    return this;
+  }
+}


Could all changes to library_classes be in a different PR? This causes a lot of changes in integration tests that are not related to the PR, making it hard to review.

Not a problem, here is the extracted PR:
#140

Thanks, could you rebase this on top of master now that #140 was merged?

Summary: Add library_classes to later test additional literal sources. Extracted from: #139 Pull Request resolved: #140 Reviewed By: anwesht Differential Revision: D48519654 Pulled By: arthaud fbshipit-source-id: 6f1b4795ae932f257009978bebd8f8bb35b59441

Match static regex for new source in `analyze_const_string`.

arthaud · 2023-08-22T12:32:09Z

source/LiteralModel.cpp

+}
+
+LiteralModel::LiteralModel(const LiteralModel& other)
+    : regex_(other.regex_ ? std::optional(other.regex_->pattern()) : std::nullopt), sources_(other.sources_) {


I guess this is because re2::RE2 is not copyable? That's annoying.

Correct:

// Not copyable. // RE2 objects are expensive. You should probably use std::shared_ptr<RE2> // instead. If you really must copy, RE2(first.pattern(), first.options()) // effectively does so: it produces a second object that mimics the first. RE2(const RE2&) = delete;

One alternative would indeed be to use a shared_ptr instead of an optional, RE2 objects are safe for concurrent use after all. Happy to apply that change if preferred.

I think that's fine as is :)

arthaud · 2023-08-22T12:32:38Z

source/LiteralModel.cpp

+std::string LiteralModel::pattern() const {
+  return regex_ ? regex_->pattern() : "(no pattern)";
+}


Could we return a std::optional instead? Not a fan of implicitly returning "no pattern".

arthaud · 2023-08-22T12:33:49Z

source/LiteralModel.cpp

+}
+
+void LiteralModel::join_with(const LiteralModel& other) {
+  if (this != &other) {


nitpick: We generally prefer the early-return pattern, instead of nesting if blocks.

Suggested change

if (this != &other) {

if (this == &other) {

return;

}

arthaud · 2023-08-22T12:34:43Z

source/LiteralModel.cpp

+Json::Value LiteralModel::to_json(const ExportOriginsMode export_origins_mode) const {
+  auto value = Json::Value(Json::objectValue);
+
+  value["pattern"] = pattern();


We should check beforehand if the pattern is empty

Omitted now if the pattern is empty.

arthaud · 2023-08-22T12:35:07Z

source/LiteralModel.cpp

+
+Json::Value LiteralModel::to_json(Context& context) const {
+  auto value = to_json(context.options->export_origins_mode());
+  value["position"] = context.positions->unknown()->to_json();


Fine, let's keep it for consistency.

arthaud · 2023-08-22T12:36:22Z

source/Registry.cpp

@@ -176,13 +200,25 @@ void Registry::join_with(const FieldModel& field_model) {
  }
 }

+void Registry::join_with(const LiteralModel& literal_model) {
+  auto iterator = literal_models_.find(literal_model.pattern());


Maybe assert that the pattern is non-empty?

Now using mt_assert:

const std::optional<std::string> pattern{literal_model.pattern()}; mt_assert(pattern);

Allow configuring regex sources using JSON config.

Add Google API key test for literal model.

Do not log tainting a register if model is empty.

Allow unused `description` property in literal model.

Add literal models doc and examples to `models.md`.

Expose models, field models and literal models in options and shim.

facebook-github-bot · 2023-08-23T14:11:09Z

@arthaud has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2023-08-23T19:10:38Z

@arthaud merged this pull request in c266c18.

facebook-github-bot added the cla signed label Aug 7, 2023

yuhshin-oss requested changes Aug 7, 2023

View reviewed changes

pkesseli force-pushed the pkesseli/literal-sources branch 2 times, most recently from aed2108 to 5019e0a Compare August 9, 2023 21:17

pkesseli requested a review from yuhshin-oss August 10, 2023 08:38

yuhshin-oss reviewed Aug 14, 2023

View reviewed changes

yuhshin-oss requested changes Aug 14, 2023

View reviewed changes

pkesseli force-pushed the pkesseli/literal-sources branch from 5019e0a to 90a463b Compare August 16, 2023 13:51

pkesseli requested a review from yuhshin-oss August 16, 2023 13:51

arthaud requested changes Aug 18, 2023

View reviewed changes

pkesseli mentioned this pull request Aug 21, 2023

chore(COD-1378): Update library_classes #140

Closed

pkesseli requested a review from arthaud August 21, 2023 15:26

Support Regexp as sinks and/or sources

ea755e6

Match static regex for new source in `analyze_const_string`.

pkesseli force-pushed the pkesseli/literal-sources branch from 90a463b to 7ceaec5 Compare August 22, 2023 11:39

arthaud requested changes Aug 22, 2023

View reviewed changes

Pascal Kesseli added 6 commits August 23, 2023 12:32

Introduce literal_models.json

a456a1a

Allow configuring regex sources using JSON config.

Add API key test

ab6d623

Add Google API key test for literal model.

Skip logging empty literal models

a568847

Do not log tainting a register if model is empty.

Allow description in literal model

20fe5dd

Allow unused `description` property in literal model.

Document literal models

f041bc0

Add literal models doc and examples to `models.md`.

Expose models options

3939f0d

Expose models, field models and literal models in options and shim.

pkesseli force-pushed the pkesseli/literal-sources branch from 44f5954 to 3939f0d Compare August 23, 2023 10:32

arthaud approved these changes Aug 23, 2023

View reviewed changes

facebook-github-bot closed this in c266c18 Aug 23, 2023

facebook-github-bot added the Merged label Aug 23, 2023

pkesseli deleted the pkesseli/literal-sources branch October 4, 2023 16:59

		static constexpr const char* PATTERN = "pattern";

		static constexpr const char* SOURCES = "sources";

		for (const auto& literal_model : literal_models_) {
		if (literal_model.second.matches(literal)) {


		#pragma once

		#include <regex>

Regular expression models for literals #139

Regular expression models for literals #139

Conversation

pkesseli commented Aug 7, 2023 • edited Loading

Reviewer note

yuhshin-oss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkesseli commented Aug 9, 2023

facebook-github-bot commented Aug 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuhshin-oss left a comment

Choose a reason for hiding this comment

pkesseli commented Aug 16, 2023

facebook-github-bot commented Aug 17, 2023

arthaud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arthaud Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkesseli Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arthaud Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkesseli Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkesseli Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Aug 23, 2023

facebook-github-bot commented Aug 23, 2023

pkesseli commented Aug 7, 2023 •

edited

Loading

arthaud Aug 22, 2023 •

edited

Loading

pkesseli Aug 22, 2023 •

edited

Loading

arthaud Aug 22, 2023 •

edited

Loading

pkesseli Aug 21, 2023 •

edited

Loading

pkesseli Aug 23, 2023 •

edited

Loading