I reviewed mini-iso-tools 0.2.1 as checked into lunar. This shouldn't be considered a full audit but rather a quick gauge of maintainability. mini-iso-tools is a package that provides a ncurses TUI for selecting an iso during boot and that allows integration with casper to download and chain-boot the selected iso. - CVE History: - The package currently has no CVEs associated with it as it is a fairly new package. - Upstream is Canonical. - Build-Depends? debhelper-compat (= 13), gcovr, kexec-tools, libcmocka-dev, libjson-c-dev, libncurses-dev, meson, ninja-build, pkg-config - Dependencies seem to not have a huge number of CVEs related to them, and the ones that have a few are mostly patched. - pre/post inst/rm scripts? No - init scripts? No - systemd units? No - dbus services? No - setuid binaries? No - binaries in PATH? No - sudo fragments? No - polkit files? No - udev rules? No - unit tests / autopkgtests? Tests can be run locally easily and there are no autopkgtests. - cron jobs? No - Build logs: - No relevant error warnings or lintian failures - Processes spawned? - a lot of bash scripts work together with the main script in order to provide the menu functionality. One of them is the 'iso-menu-session' script which recovers JSON data to be parsed and then presented through the ncurses menu. - The '30mini-iso-menu' script is added to '/usr/share/initramfs-tools/scripts/casper-premount/' during install. The owner is 'root:root' and permissions are set to '644'. - Memory management? - a lot of memory management follows the standard C way of programming. Memory management is always a tricky thing to do in C, and is always something that ends up introducing many vulnerabilities, so we must be careful when expanding on this code to avoid these types of issues. It is helpful in this situation that upstream is Canonical itself. - main.c: 'args_free' is called after 'args_create' (by the end of 'main'), however, if any intermediary check in the main function fails, this memory actually not freed since 'main' returns first. Therefore, we can have possible memory leaks. Some leaks are detected by valgrind in a normal execution for the program. - json.c: uses 'strcmp' with no checks for size before the comparison, and 'json.c' will be processing data recovered from external sources. This does not seem to be a problem at this time and with the current code we have, however, this is not one of the best programming practices when it comes to security. - from cppcheck/coverity: "common.c:33:29: error: va_list 'ap' was opened but not closed by va_end(). [va_end_missing]". However, documentation mentions that 'va_end' should be called or else the stack can be corrupted. - calling the 'iso-chooser-menu' with empty files or files that do not have the expected contents as input, results in a crash with a segmentation fault, and this seems like a problem that could be easily avoided and not result in a crash. The way the program is meant/expected to be run kind of "guarantees" that this kind of thing won't happen, but it is always best to have checks in place in case we run across unexpected situations. - in function 'choices_extend_from_json', there are many instances where a value is returned, however, this return value is not checked in 'main.c's 'read_iso_choices', nor are the return values of variable 'choices' checked. This can lead to issues, such as the one previously mentioned. - I feel like memory management could be done a bit more carefully, especially considering the size of the code (small). As the code changes, these forgotten things might end up becoming a problem. - The code works in such a way that it "expects" that certain data will always have a certain format, and that is understandable considering how and where it is meant to be executed. However, for security reasons, failures should be addressed regardless of an expected input, to avoid a chain reaction of malicious tampering. By including appropriate checks, we provide less possible ways for an attacker to exploit failures that were caused by other failures. For example, what if we have a situation where an attacker is able to exploit an overflow that in turn allows them to change the value of variable 'capacity' to '0'? We then come across a situation where this is also a point of failure because this value is not being properly checked, as it is expected to always be the hardcoded value of 10. - File IO? - main.c: in this part of the code there is a file that is opened from writing data. The file is open with no flag applied. What is written into the file is information on the ISO that will be downloaded in the future, including checksum information and media URL. The script 'iso-menu-session' calls the code with arguments to generate this file and the file is generated with a specific name under the '/' directory. Since this will be run by the root user, the file will be owned by root, and therefore, in theory, there is no obvious way to exploit this. - when recovering JSON, the application does not parse the JSON itself, it relies on 'json_object_from_file' to parse data from it. The JSON library, therefore is responsible for file I/O in this case. - Use of temp files? - there is one script, 'iso-menu-session' which creates a temporary directory with a predictable name ('/tmp/mini-iso-session') and then downloads files into it using 'wget'. Files in that directory later serve as argument to the menu generation program itself, 'iso-chooser-menu'. These files are supposed to contain the list of ISOs a user can choose from in the menu. Temporary files do not seem to be created safely, and are created with predictable names, when done so through use of the 'iso-menu-session' script. The contents of these files are used as input for 'iso-chooser-menu', which is the program generated from the compiled C code in the package, and the output of this program is added to a file with a fixed name when it is called by 'iso-menu-session', and this fixed named file is later processed by '30mini-iso-menu'. '30-mini-iso-menu' is the script that actually downloads the selected ISO and runs checksum operations to verify the integrity of the file. 'iso-chooser-menu' actually has a hardcoded list of values it references when generating the output data that will contain URLs and SHA256SUMs, this hardcoded list including trusted Ubuntu URLs only and forcing 'iso-chooser-menu' to generate "trusted output". So even if the temporary files consumed as input by 'iso-chooser-menu' are put into a predictable location by 'iso-menu-session' and have predictable names, tampering that does not comply to the predefined accepted values will not result in an output file with malicious content. The input files, however, being saved in '/tmp', could be a point of concern regarding the situation where an attacker adds a new file to the predictably named temporary directory containing bogus/trash data to be processed by 'iso-chooser-menu', since all files in the temporary directory are fed as input to 'iso-chooser-menu'. Considering some of the memory management issues previously identified, this could be used as a point of entry by an attacker to crash something or even run more complicated exploits (of course, this is made less likely considering the moment when the program is expected to run, but it is still something that should be considered). And, as previously tested, it is possible to crash the program by providing an empty file as input to 'iso-chooser-menu'. - Logging? - All logging is done using the syslog function. - Use of 'printf' related functions is done safely (no format string vulnerabilities). - Use of networking? - some of the scripts that activate the main code run 'wget' calls in order to recover certain JSON files parsed by 'json.c'. URLs are hardcoded into the script, and are supposedly safe, since they are Ubuntu related URLs (releases.ubuntu.com). - 'json.c' tries to confirm that a value in a JSON contains a URL that is in a group of pre-selected URLs, meaning there is a check to guarantee that files that will be later downloaded will be recovered from trustworthy sources. - Could we bring more trust to this process other than just relying on hardcoded URLs? - Environment variable usage? No - Use of privileged functions? No - Use of cryptography / random number sources etc? - access to https URLs only. - Maybe it is interesting to consider a more thorough check of the data sources an rely on more than just the CA certificate bundles and TLS when recovering JSONs from the ubuntu.com websites. GPG signature validation is a possible option for this. - Use of WebKit? No - Use of PolicyKit? No - Any significant cppcheck results? - Yes. 'va_start()' called without 'va_end()' in common.c. - Any significant Coverity results? - Memory management issues in test files and the same result as the one described above. - Built with PIE, stack protection, read-only relocations. - loads libraries libc, libjson-c, libncursesw and libtinfo for execution. Security team ACK for promoting mini-iso-tools to main, given that the following issues are addressed/questions are answered: - The cppcheck/coverity result involving the 'va_start()' call with no corresponding 'va_end()' call in common.c be corrected in the code. - Is there a reason why the 'iso-menu-session' saves the recovered JSON files in '/tmp'? Could we consider possibly changing the script to have this data be saved in a better location? - Is there a reason why we wouldn't be able to use GPG signatures in order to guarantee that the contents of the recovered JSON files are trustyworthy? Could we possibly consider using GPG signatures to check that the actual content of the recovered JSON files are trusted, instead of only relying in TLS and hardcoded values only, should there be nothing stopping us from doing so?