Improve code quality using static analysis

[langweg]

The code should be checked for typical programming errors and omissions. To this end, static analysis tools could be used. Get in touch with Felix Schuckert (F-001) to discuss under what conditions the use of industry-grade tools is possible that we have at hand for teaching purposes.

[SailReal]

We're going to have a meeting with Felix on Tuesday, July 24th to perform the analyses. The results will be uploaded here.

[SailReal]

The first static analysis is finished 🎉 :

• The coverty report is accessible using the following link: https://141.37.122.64:8443 (the server is inside the HTWG so use VPN)
• The output of Fortify can be downloaded as pdf using this link: SharkCageFortifyReport

We will now go through all the problems, categorize them accordingly and fix the ones considered as errors.

[DonatJR]

There is now a PR which addresses issues found by both tools here. And a point-by-point analysis of the issues:

Static analysis using Fortify

• ASP.NET Bad Practices: Leftover Debug Code:
  • CageConfigurator/CageConfigurator.cs:12: false positive, not a web project, main function is needed
• Command Injection
  • CageChooser/CageChooserForm.cs:168: can only exploited as admin -> no point in addressing it
  • CageChooser/CageChooserForm.cs:178: same as above
  • CageChooser/CageChooserForm.cs:178: code no longer in project
  • CageConfigurator/CageConfiguratorForm.cs:361: same as the first two above
  • CageConfigurator/CageConfiguratorForm.cs:370: same as above
  • CageChooser/CageChooser.cs:25: debug only
• Dead code: all in external library (https://github.com/nlohmann/json extensivly used and well tested / maintained)
• Dead Code: Unused Field
  • CageServiceInstaller/ServiceInstaller.cs:10: fixed
• Null Dereference
  • CageChooser/CageChooserForm.cs:170: fixed
  • CageConfigurator/CageConfiguratorForm.cs:363: fixed
• Path Manipulation
  • CageConfigurator/CageConfiguratorForm.cs:419: we have to let the user input a path for the config at some point. also, processes on the desktop should have normal user rights once the token manipulation is correctly implemented
  • CageConfigurator/CageConfiguratorForm.cs:476: nothing will be done with the opened file except read the bytes -> okay
  • CageConfigurator/CageConfiguratorForm.cs:417: same as l. 419
  • CageConfigurator/CageConfiguratorForm.cs:499: only way to check if binary has a signature
  • CageChooser/CageChooserForm.cs:81: code no longer in project
  • CageChooser/CageChooserForm.cs:91: same as above
  • CageConfigurator/CageConfiguratorForm.cs:146: same as above
  • CageConfigurator/CageConfiguratorForm.cs:90: not critical
  • CageChooser/CageChooserForm.cs:29: code no longer in project
  • CageChooser/CageChooserForm.cs:35: code no longer in project
  • CageChooser/CageChooserForm.cs:37: code no longer in project
  • CageChooser/Properties/Settings.Designer.cs:42: code no longer in project
  • CageChooser/Properties/Settings.Designer.cs:31: code no longer in project
  • CageConfigurator/CageConfigurator.cs:12: needed, not critical
  • CageConfigurator/CageConfiguratorForm.cs:90: not critical
  • CageChooser/CageChooser.cs:31: debug only
  • CageConfigurator/CageConfiguratorForm.cs:249: coarser catch does not make any sense, we are now displaying an error message to the user
  • CageConfigurator/CageConfiguratorForm.cs:502: only for checking if binary is signed / unsigned, exception handling not necessary
  • CageConfigurator/CageConfiguratorForm.cs:219: coarser catch does not make any sense
• Poor Style: Variable Never Used
  • CageManager/SecuritySetup.cpp:67: false positive
  • remaining issues of this category: code in same library as above, false positives
• Privacy Violation: Heap Inspection
  • CageConfigurator/CageConfiguratorForm.cs:478: has to be done at some point and is just a hash (not security critical), heap should only be readable from processes with same privileges (Admin)
• Type Mismatch: Signed to Unsigned
  • CageManager/base64.cpp:29: library code, fixme maybe check if fixable
• Unsafe Native Invoke
  • CageChooser/CageChooserForm.cs:149: recommendation is to ensure that differences in bounds checking and other behavior between managed and native code are accounted for and handled correctly -> done

Static analsysis using Coverty

sorted by file on result website:

• CageChooserForm.cs:

  • code of all issues no longer in project -> ignore

• CageConfiguratorForm.cs:

  • resource leaks: all from file dialogs -> fixed
  • l. 267 (null pointer dereference) -> fixed
  • l. 387 (null pointer dereference) -> fixed
  • l. 207 (null pointer dereference) -> ignore, will be caught by try-catch and is expected if config not correct

• CageDesktop.cpp:

  • l. 23, 32 (error handling issues) -> ignore for now, if operator<< throws I'm willing to accept an application termination (throw lists are deprecated, alternative(?): surround all std::cout calls with try-catch). Addition: This will be removed as soon as we have an logging framework.

• CageLabeler.cpp:

  • l. 117 (control flow issues) -> already fixed

• CageManager.cpp:

  • errors / exceptions thrown in main are not declared -> ignore, if an exception occurs in main an application termination is okay
  • getchar() -> code no longer in project (was for debugging purposes)

• FullWorkArea.cpp:

  • errors / exceptions thrown are not declared -> ignore
  • l. 9 (uninitialized members) -> fixed

• SecuritySetup.cpp:

  • l. 10 (rule violations) -> fixed

• CageService.cpp:

  • l. 166, 224 (leaked handle) -> fixed
  • l. 180 (explicit null dereferenced) -> fixed

• CageServiceMain.cpp:

  • errors / exceptions thrown are not declared -> ignore

• tokenLib.cpp:

  • l. 584 (memory - illegal access) -> fixed
  • l. 581 (resource leak) -> false positive, handles to modules do not need to be freed or closed (CloseHandle does not even accept HMODULE)
  • l. 461 (control flow issues) -> fixed
  • l. 575 (exceptional resource leak) -> false positive, memory is freed in destructor
  • l. 232 (exceptional resource leak) -> not fixed, ask peter how to proceed
  • l. 34 (rule violations) -> fixed
  • l. 336 (exceptional resource leak) -> not fixed, ask peter how to proceed

• Microsoft (VC++), asio and json.hpp stuff: ignore for now, library code

[bencikpeter]

Have you considered adding Coverity/Fortify to our CI system? (Not sure about appveyor, but some CI systems support running static analysis automatically with each build)

[DonatJR]

I don't think we would be able to use Coverty / Fortify for this (they are not free and we don't have a personal / student license, we were just able to run them on a specific computer at the HTWG). We can think about some other tool, though. :+1:

[bencikpeter]

Actually, Coverity is free for open source projects... If the comply to the list of this requirenments:

https://scan.coverity.com/faq#how-get-project-included-in-scan

[DonatJR]

Nice 👍 created an issue #98 I had some bad info then. It was probably a misunderstanding and only Fortify needs a paid-for license