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

test_strings.py is failing on windows #369

Closed
terriko opened this issue Feb 20, 2020 · 19 comments
Closed

test_strings.py is failing on windows #369

terriko opened this issue Feb 20, 2020 · 19 comments
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@terriko
Copy link
Contributor

terriko commented Feb 20, 2020

I've enabled the windows CI! Unfortunately, since @wzao1515 's hard work on windows last summer, we've had a few regressions and the port is currently not working as expected.

  • test/test_cli.py 👎
  • test/test_csv2cve.py ✔️
  • test/test_cvedb.py ✔️
  • test/test_definitions.py ::heavy_check_mark:
    • but 0 tests running
  • test/test_extract.py 🛑
    • disabled because this is where the loop happens
  • test/test_file.py ✔️
  • test/test_json.py ✔️
    • technically not running any tests because LONG_TESTS not enabled
  • test/test_scanner.py 👎
  • test/test_strings.py 👎
  • test/test_util.py ✔️

This issue is to track the problem that we're encountering in test_strings.py. It looks to me like the problem is that the strings have an extra \r in them. Here's the beginning of the AssertionError that occurs:

AssertionError: '!This program cannot be run in DOS mode.\r' not found in ['MZ', '@', '!', 'L', '!This program cannot be run in DOS mode.', '$', 'PE', 'd',

As you can tell, the string "'!This program cannot be run in DOS mode." does appear, but the string "'!This program cannot be run in DOS mode.\r" does not. On linux, newlines are \n but on windows newlines are \r\n. We can almost certainly ignore any \r that we come across in the string.

The fix here needs to happen so that it works not only in the tests, but also in the parser itself so that we don't always fail to find strings because of an extra \r character.

@terriko terriko added the bug Something isn't working label Feb 20, 2020
@terriko
Copy link
Contributor Author

terriko commented Feb 20, 2020

Note for students: I don't regularly work on Windows, so if you do use Windows primarily, this is a great chance to show the benefits of operating system diversity!

@Niraj-Kamdar
Copy link
Contributor

one of the error in test_scanner.py is in test_does_not_scan_symlinks function because in windows os.symlink is privilege operation.

@terriko
Copy link
Contributor Author

terriko commented Mar 20, 2020

@Niraj-Kamdar Good find. We can probably have that test disabled when on windows, then.

@Niraj-Kamdar
Copy link
Contributor

Niraj-Kamdar commented Mar 21, 2020

one other error in test_scanner.py is while asserting package in list(cves.keys()) because package ffmpeg isn't in cve.keys()

@Niraj-Kamdar
Copy link
Contributor

What is pstring here? I didn't find pstring.py in cve_bin_tool

try:
    import cve_bin_tool.pstring as pstring
except ImportError:
    pstring = None

@Niraj-Kamdar
Copy link
Contributor

In test_cli why are we asserting not equal here.

    def test_extract_curl_7_20_0(self):
        """Scanning curl-7.20.0"""
        self.assertNotEqual(
            main(["cve-bin-tool", "-l", "debug", "-x", self.tempdir]), 0
        )

It should be assertEqual zero case successful function call returns 0

@terriko
Copy link
Contributor Author

terriko commented Mar 22, 2020

Cve-bin-tool returns work like this:

  • 0 means it ran and found no cves
  • positive numbers indicate a number of cves (this was a specific request from a user so that CI systems would fail when cves were found)
  • negative numbers indicate errors that caused the tool not to run

So yes, scanning on that old version of curl should find cves and return a positive non-zero number.

@terriko
Copy link
Contributor Author

terriko commented Mar 22, 2020

Pstring is the strings implementation for Windows. It's in the .c file because it was converted to c to improve performance (considerably).

@terriko
Copy link
Contributor Author

terriko commented Mar 22, 2020

Don't know what's up with the ffmpeg error though.

@Niraj-Kamdar
Copy link
Contributor

Niraj-Kamdar commented Mar 22, 2020

@terriko test_extract_curl_7_20_0 in test_cli is failing because of the same problem of "\r" in the end. Here's truncated output of scanner ['/lib64/ld-linux-x86-64.so.2\r', 's{^^\r']

@Niraj-Kamdar
Copy link
Contributor

Niraj-Kamdar commented Mar 22, 2020

@terriko Why output from strings command and our Strings library is different

        s = Strings(filename)
        o = s.parse()
        lines = o.split("\n")
        t = subprocess.check_output(['strings', filename]).decode().split("\n")[0:2]
        LOGGER.debug(f"\n\n\n{t} ,{lines[0:2]}\n\n")

debug output looks like this....

['/lib64/ld-linux-x86-64.so.2\r', 's{^^\r'] ,['\x7fELF', '>']

@terriko
Copy link
Contributor Author

terriko commented Mar 26, 2020

It shouldn't be different. This is likely a bug. We should probably strip all \r from output in both cases.

@Niraj-Kamdar
Copy link
Contributor

@terriko Why don't we use compiled strings binary from GNU mingw? Its size is only 1MB and results are similar to linux GNU strings.

@wzao1515
Copy link
Contributor

@terriko Why don't we use compiled strings binary from GNU mingw? Its size is only 1MB and results are similar to linux GNU strings.

Because previously we were thinking about using a single method to support all platforms, so that we could get rid of both linux strings and mingw strings. But for now I think it is fine to use packages from third party.

@Niraj-Kamdar
Copy link
Contributor

Niraj-Kamdar commented Mar 29, 2020

@wzao1515 glad to hear that because using strings from GNU, we can avoid some problems.

@Niraj-Kamdar
Copy link
Contributor

Niraj-Kamdar commented Mar 29, 2020

@terriko I have open PR which fixes test_checker, test_strings and test_extract but there is still problem with test_scanner and test_cli . extractor isn't extracting cpio file in Windows and there is also problem of "\r\n" in cli module and fixing it the way I fixed test_string breaking bunch of things.

@Niraj-Kamdar
Copy link
Contributor

New python string parser is working fine on windows.

@Niraj-Kamdar
Copy link
Contributor

test_cli , test_scanner and test_string are working fine now on windows. so, only thing left is test_extract and for that i have opened issue #667. We should close this issue now.

@terriko
Copy link
Contributor Author

terriko commented May 26, 2020

Closing!

@terriko terriko closed this as completed May 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants