Skip to content

Conversation

wchresta
Copy link
Contributor

@wchresta wchresta commented Mar 23, 2019

Remember import aliases for built-in and black box functions.

  • This allows sinks to be fully qualified
  • I.e. we can use os.system and it will match even if the import us done using from os import system

This is an incompatible change. The labels for function calls changed (they are now fully qualified, regardless of how the function was imported).

This solves #177

* Allow trigger words to be fully qualified to reduce false positives
* This will give fully qualified names for blackboxes like flask
* Improve readability by using keyword arguments
@wchresta
Copy link
Contributor Author

Please review this change carefully; It changes the contents of import_alias_mapping and I'm not 100% this has no unintended side-effects.

@KevinHock KevinHock requested review from KevinHock and bcaller March 23, 2019 21:02
Copy link
Collaborator

@KevinHock KevinHock left a comment

Choose a reason for hiding this comment

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

Looks great to me, thank you so much for making this 👍 Very high-quality code

I left a few optional feedback comments, but the functionality looks great

edit: Don't worry about Codeclimate, it complains about everything

package_name,
local_names,
import_alias_mapping,
module=(module[0], init_file_location),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for doing this 🙏 :) kwargs always improve readability

uninspectable_modules.add(alias.name) # Don't repeatedly warn about this
return IgnoredNode()

def visit_ImportFrom(self, node):
# Is it relative?
if node.level > 0:
return self.handle_relative_import(node)
else:
Copy link
Collaborator

Choose a reason for hiding this comment

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

You're definitely improving the readability of this code, thank you :) The less indentation the better

Reassigned in:
File: examples/vulnerable_code/path_traversal_sanitised_2.py
> Line 8: image_name = ~call_1
File: examples/vulnerable_code/path_traversal_sanitised_2.py
> Line 12: ~call_3 = ret_os.path.join(~call_4, image_name)
File: examples/vulnerable_code/path_traversal_sanitised_2.py
> reaches line 12, sink "send_file(":
~call_2 = ret_send_file(~call_3)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This makes the output more readable as well, awesome job 🎉

module,
None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

The main thing I'm unsure about is, why I was passing None for real_names, but it was probably wrong :)

I do remember I wrote a lot of tests in the import PRs, so I'm somewhat confident this code is well covered. (Granted it was when I was an even worse engineer than I am now 😁 ).

@@ -482,6 +482,21 @@ def test_list_append_taints_list(self):
)
self.assert_length(vulnerabilities, expected_length=1)

def test_import_bb_or_bi_with_alias(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great test 🎈 🥇

@KevinHock KevinHock merged commit 98a7b6b into python-security:master Mar 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants