Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Display position in TransformDuplicate errors #775

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

abishekvashok
Copy link
Contributor

@abishekvashok abishekvashok commented Aug 10, 2023

Pre-submission checklist

  • I've ran the linters locally and fixed lint errors related to the files I modified in this PR. You can install the linters by running pip install -r requirements-dev.txt && pre-commit install
  • pre-commit run

Summary

Previously when TransformDuplicate errors occured, we just left a note specifying the duplicate name of the transform. Now we can specify the locations where the duplicates occur.

Modifies taintTransform to have a new type that holds location information (for parsed transforms, it is made None). The location information will only be present for transform node definitions in taint.config.

Modifies and updates tests to confirm and check the new type of TransformDuplicate error.

Test Plan

  • After the changes, ran on documentation/pysa_tutorial/exercise1 with default taint.config:
Screenshot 2023-08-10 at 7 18 12 PM
  • Changed taint.config to:
{
  "sources": [
    {
      "name": "CustomUserControlled",
      "comment": "use to annotate user input"
    }
  ],

  "sinks": [
    {
      "name": "CodeExecution",
      "comment": "use to annotate execution of python code"
    }
  ],

  "transforms": [
    {
      "name": "Test",
      "comment": "Test transform"
    },
    {
      "name": "Test",
      "comment": "Test tranform duplicate"
    }
  ],

  "features": [],

  "rules": [
    {
      "name": "Possible RCE:",
      "code": 5001,
      "sources": [ "CustomUserControlled" ],
      "sinks": [ "CodeExecution" ],
      "message_format": "User specified data may reach a code execution sink"
    }
  ]
}

Before the changes
before

After the changes

after
  • Ran tests with make test

Fixes part of MLH-Fellowship#82
Signed-off-by: Abishek V Ashok [email protected]

Previously when TransformDuplicate errors occured, we just left a note
specifying the duplicate name of the transform. Now we can specify the
locations where the duplicates occur.

Modifies taintTransform to have a new type that holds location
information (for parsed transforms, it is made None). The location
information will only be present for transform node definitions in
taint.config.

Modifies and updates tests to confirm and check the new type of
TransformDuplicate error.

Signed-off-by: Abishek V Ashok <[email protected]>
type t =
| Named of string
| Named of named_transform
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't change this, since this is the actual transform used during the analysis. We don't need to keep the location around during the analysis.
We do something similar for sources and sinks: we use AnnotationParser.source_or_sink during model parsing but we actually use Sources.t or Sinks.t in the analysis.
TL, DR: I would define a new type in annotationParser.ml to represent those. or just use AnnotationParser.source_or_sink?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes! Will make the change :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants