Update OS Command Injection Plugin to Include All Test Cases

[shayancha]

Went through all OS command injection test cases and added patched sources to the taintmonkey() fixture.

All test cases execute predictably when only the necessary sink function is included in the SINKS list.

However, there is a bug for the use case where multiple sink functions are included in SINKS and the last element is not the appropriate sink function for the test case being executed.

[bliutech]

However, there is a bug for the use case where multiple sink functions are included in SINKS and the last element is not the appropriate sink function for the test case being executed.

Can you elaborate on this bug a bit more? Would it occur for something like this?

SINKS = [
   "os.popen",
   "os.system"
]

With only os.system being patched? I suspect this may be due to some issues with cache invalidation in taintmonkey.patch.import_module.

[bliutech]

Looks like a good start. I think we need to also add harnesses for each test case to this plugin as well.

[shayancha]

However, there is a bug for the use case where multiple sink functions are included in SINKS and the last element is not the appropriate sink function for the test case being executed.

Can you elaborate on this bug a bit more? Would it occur for something like this?

SINKS = [
   "os.popen",
   "os.system"
]

With only os.system being patched? I suspect this may be due to some issues with cache invalidation in taintmonkey.patch.import_module.

Yes, exactly!

[bliutech]

Got it. I think we need to tie the lifetime of the import cache directly to the TaintMonkey object. This way we don't need to invalidate the cache on every import and can just do it at the destruction of the instance. We probably want to implement a similar approach to pytest with how they have a MonkeyPatch.undo() interface. https://github.com/pytest-dev/pytest/blob/main/src/_pytest/monkeypatch.py#L57

[bliutech]

Good progress so far!

[shayancha]

I adjusted open_file_command so that it acts as a propagator, not a source.

However, is_safe_path does not seem to properly sanitize the input, even after I try to monkey patch it manually. I noticed that if I even try to do something like

@tm.patch.function(
        "dataset.cwe_78_os_command_injection.secure_novalidation.app.is_safe_path"
    )
    def patched_is_safe_path(path):
        return original_function(path)

I get different behavior than if I don't monkey patch, which shouldn't be the case. When I monkey patch, is_safe_path isn't actually blocking any inputs from continuing to a sink.

[SeabassMarket]

We are going to rewrite plugins soon to follow the new streamlined format. Just clean it up a bit, and then we'll merge.