I reviewed iwd 1.27-1 as checked into kinetic.  This shouldn't be considered a full audit but rather a quick gauge of maintainability.

Iwd, the iNet wireless daemon, is a wireless daemon for Linux that aims to implement wireless network functionalities and simplify network configuration, management and use for users. Iwd attempts to achieve this in a simple manner, using Linux Kernel functionalities and not depending on any external libraries, which also allows for optimized resource utilization (storage, runtime memory and link-time costs). It is written by Intel, and is an alternative to wpa_supplicant.

- CVE History: there are two main CVEs associated with this package, CVE-2020-17497 and CVE-2020-8689. According to our prioritization, none of these is a HIGH or a CRITICAL, and according to NVD, they are classified respectively as HIGH and MEDIUM. Upstream seems to have responded to the vulnerability reports quickly, and patches were provided.

- Build-Depends: debhelper-compat, libell-dev, libreadline-dev, libdbus-1-dev, systemd.

- Run-Depends: init-system-helpers (>= 1.51), libc6 (>= 2.34), libell0 (>= 0.50), libreadline8 (>= 6.0), dbus. Focuses on mostly depending on the Linux Kernel and the C runtime library.

- There is a postinst script and there is a postrm script. For the postinst script: creates and enables the iwd systemd service in /etc/init.d. The service seems to not be automatically started upon install, which is confirmed by the fact that the postinst scripts calls update-rc.d with option defaults-disabled. For the postrm script: removes the installed systemd service, reloads systemd and masks the iwd service, in case of a 'remove'; or purges and unmasks it, in case of a 'purge'. No configuration files seem to be removed. The postrm script cleans up what is done by the postinst script.

- File ./etc/init.d/iwd is created during install. This contains the iwd service that is to be run in the system. Permissions for this file are the default for init.d scripts, which seems appropriate. The script sets environment variables which will point to iwd's state directory and configuration directory (places where iwd extracts data from). It creates the directory for the state configurations with 0700 permissions. It starts the daemon. It sets the location of its pid file to /var/run/iwd-sysd2v.pid. The daemon is set to run in the background. It looks like iwd creates the directory it needs everytime it starts running, maybe to guarantee that there will be no errors if any configuration or state file is missing. This could be a positive thing, as iwd sets the permissions of the state directory everytime it starts, which means that once it starts, only whoever is the owner of the file (which we can assume is expected to be root) can edit it, which would avoid issues where there is an attempt to tamper with this file.

- /lib/systemd/system/iwd.service: defines the DBus service net.connman.iwd which has the following network capabilities: CAP_NET_ADMIN, CAP_NET_RAW and CAP_NET_BIND_SERVICE. It sets the StateDirectoryMode to 0700.  NoNewPrivileges is set to true, which is a security plus. Sets PrivateTmp, which is useful to secure access to temporary files of the process. Therefore, from these and other configurations set in the file, it looks like there are active measures being taken to prevent the service from being a point for exploits in the system.

- /lib/systemd/network/80-iwd.link: systemd network configuration file, where predictable renaming of wireless devices is disabled (prevents udev from renaming the interface to wlp#s#. This was done to resolve a race condition, see https://iwd.wiki.kernel.org/interface_lifecycle#udev_interface_renaming).

- Creates a DBus service called net.connman.iwd connected to the iwd daemon that is started by systemd. 


- Binaries in PATH: all hardened (PIE, RELRO, stack protection, etc), none of them is suid (755, owned by root).
  - /usr/bin/iwctl - the client application used to communicate with the daemon.
  - /usr/bin/iwmon - the monitoring application.
  - /usr/libexec/iwd - the daemon itself.

- The package contains unit tests that can be run with 'make check'. These tests are run during build. Results are clear and easy to understand.

- Build logs contained no relevant errors or warnings other than lintian failures that can be ignored.

- Processes spawned: the code makes many calls to functions in the exec() family. A few of these calls simply execute hardcoded commands or execute commands which include data that is recovered from kernel controlled files, and these are not an issue. In the iwmon utility, there is an instance of exec where a shell is opened to run a command which is set by the user through the PAGER environment variable. The value in this variable is simply sanitized to check for the 'cat' command, since use of the cat command in the situation in question might lead to a problem with excessive memory consumption. It is therefore possible to run commands through this environment variable, however, this might not be as dangerous as it seems considering that whoever is running iwmon, a command line utility, most likely already has access to a command line and can simply run commands directly instead of doing it through an environment variable. The iwmon binary also does not have any suid privileges, which also causes this to be less of a problem. In file src/resolve.c, there are two calls to popen which also seems to take as arguments external values. One of them is the path location for 'resolvconf'. Iwd searches for 'resolvconf' based on the contents of the environment variable $PATH, which could be altered by a malicious user, but once again, this might not be too much of a problem considering that this part of the code is run by root, in a root environment, started by systemd. The second argument in this popen call also seems to come from an external source, this argument being the interface name. No sanitization is done for the data associated with the argument, and this is a bit of a concerning thing, however, from this general analysis of the iwd code, no possible ways to exploit (or at least quick and easy ways to exploit) this was found. 


- Memory management: there seems to be a lot of places where checks are missing and a lot of pĺaces where variables are not being properly initialized. However, it seems like the iwd code has this behavior because a lot of the functions are dealing with hardcoded values or values that are extracted from the kernel/kernel controlled files. Even if there is a lot of data that is "expected" to behave, there are a lot of cases where there is no reason to not do the checks, even if this is a performance focused tool. Ell, for example, also focuses on being fast and it does checks in a lot of cases, using the 'unlikely' and 'likely' functions to help speed up and optimize the process. The configuration file will contain user provided data, so there we have one reason why checks could be necessary. This is not to say that iwd performs no checks: a lot of checks are indeed done properly, however, there were cases where missing checks were identified and the reason for said checks to not be made were not understood (if said reason exists and the lack of a check is not just a bug instead). A few cases also show some checks being done in an "obscure manner". To better explain this, consider the following example, which is based on something actually seen in the iwd code: a possible overflow can happen when function X is called by function Y, however, because of some other set variable in function Y that is also sent as an argument to X, the code in X which would cause the overflow is not actually executed. This means that iwd seems to be relying on a variable B to prevent an overflow in A, when it would be better if checks were done directly in A. There are also cases where variable checks for data that is going to be processed by a function X seem to be done by a function way up in the chain, meaning, they are done by the caller of a caller of a caller…and so on. This can also be considered a bit dangerous as future newly introduced code might call X but then forget to do the check because it expects X to do it, when X is expecting one of the previous callers to do it. The bigger the code gets, the higher the chances are of this happening. All of that being said, looking at the log for iwd in its git, there at least seems to be a concern from upstream to fix bugs of this nature when they are identified, and in C code, memory related bugs are expected to happen.

- File IO: most files accessed are system files (data provided by the kernel), with hardcoded paths defined in the code. Usage of flag O_CLOEXEC was verified, and there is good management of file permissions for files that are created (iwmon can generate pcap files, for example). In general, it seems like file access is done safely enough. What could be a problem at times is the lack of sanitization of certain path names, especially considering that iwd is a daemon run by root. However, as previously mentioned, since a lot of the paths seem to be hardcoded, it is less concerning that there is no sanitization of input, although doing so is always recommended if data is recovered from possibly untrusted sources.

- Logging: mainly uses ell functions to perform logging. Looking at the messages, it seems like messages are crafted in such a way that they will not cause format string vulnerabilities. The messages are usually short and direct, and they do not seem to expose unnecessary information to the user. Log messages related to passwords do not seem to expose passwords. A lot of the error messages are actually associated with debug mode.

- Environment variables: most environment variables checked by the code are related to debugging and the creation of debugging logs, and these do not seem to be too much of an issue, as their contents are checked, and, if the variable is set, debugging messages are set as well. These specific values do not seem to be used for anything else. There are, however, environment variable values which are used for other purposes, and for which the value being stored is used for more than just defining if debug output will be printed to a file. The previously mentioned exec call which starts a shell instance references the contents of environment variable PAGER with very little sanitization. Usage of this variable to exploit the system does not seem to be viable (considering this general analysis) and at least there is a check to avoid excessive memory consumption (which happens if PAGER is set to command 'cat'), however, should the code change in the future, might this lack of sanitization open iwd to any unwanted vulnerabilities? Environment variables such as CREDENTIALS_DIRECTORY and STATUS_DIRECTORY are also not extensively sanitized. The reason behind this could be because the function which checks these variables is related to the iwd daemon which will in theory be run as root, meaning that these values will most likely be appropriately set. Considering this, the danger is lessened.

- Use of privileged functions: the only privileged function used is ioctl (according to the audit results). It is used by the monitor module to recover information about the terminal size. The client module also uses it for the same purpose. The monitor module as well as one of the iwd tools (in the tool/ directory) also call this function, but for other purposes. The monitor module recovers the index of an interface related to a recently created socket, whilst the probe-req tool seems to gather information on the flag word for a newly created socket device. For the latter case, one of the flags is used in an if statement and that is the extent of usage of the output, previously open sockets are closed before this if statement, and the statement returns from the called function if flag IFF_UP is not set, so there seems to be little room for exploits here. In all cases, ioctl is being used for read operations only. The output for ioctl is checked for errors in 3 out of the 4 cases. When checking for the window size in the client module, no return value is checked, which could be an  issue since the value of an uninitialized pointer that is supposed to be populated by ioctl is checked right after, so we have a possible bug.

- Cryptography and random number sources usage: iwd is mainly using cryptographic functions that are implemented by ell, which in turn, uses the Kernel Crypto API. It implements a few things directly, such as AES Key-Unwrap from RFC 3394, but mostly using the ell functions as the underlying way to achieve its result. According to code comments, functions implemented in crypto.c seem to be following RFC standards, as those are referenced in said comments, so there is the concern to follow the RFC. This part of the code specifically is very well commented.

- Temporary file usage: there is one instance where a temporary file is created, and it is created with a predictable name, since it is hardcoded (/tmp/iwd-tls-debug-server-cert.pem). Iwd sets to print the contents of a certificate chain to this file if a debug environment variable is set. In theory, the contents of a pem file are public. In ell, the file is created with 0600 permissions, and closed after usage. What if someone deletes the file while information is still being printed to it? Not an issue, the memory area will be written to (the inode), and maybe after the file is closed it is actually deleted, but then such a thing would only interfere with debugging. Reading the file is not an option as it is at least created with 0600 permissions, which is good. All of the ell functions seem to parse the file. The main concern here would be a symlink attack, where the attacker links this file to a logrotate file, for example, which in turn will try to execute any commands in it. However, from the ell sequence of executions, it would need to be a VERY specially crafted file in order to get past all of the parsing functions without returning a false value before any dumps of some possible command is made to the obviously named temporary file.

- Networking: socket management seems to be done well, with socket descriptors being closed when an error occurs or when desired filters fail to be applied correctly. IP address comparisons are done at the binary level and not at the string level, and verification of IP address data is done (the code even includes checks that verify if an IPv6 address is a link local address). This is mainly a network utility, so a lot of the data is parsed in order for network functionality to be implemented.

- No use of WebKit or PolicyKit.

- cppcheck results: the relevant cppcheck results that were analyzed contained 4 false positives for NULL pointer dereferencing cases. The NULL pointer dereferencing does not actually occur, and in 3 of the 4 cases it is because the function calls that would cause this dereferencing to happen also have other arguments in the call set appropriately in order to avoid it. However, looking at the code and analyzing the previously mentioned cases, the code does seem to be "dangerously" programmed, because instead of actually checking if a pointer is NULL, the functions use checks made on the values of other variables in order to define the path that would not result in the NULL pointer dereferencing issue. What if we have a situation where this other value is not in an expected range AND the pointer is NULL? For a more secure code, it seems like the check should be done in the pointer itself. The 4th case is simply a weird false positive, checks are being done correctly. Other than the NULL pointer dereferencing cases, there are cases of uninitialized variables, which seems to not be a false positives, and could cause issues.

- Coverity results: there are a few false positives for cases involving a lack of variable initialization and for possible buffer overflows. For code related to the former cases, the initializations are being indirectly performed by other functions, while for code related to the latter cases, the bounds checks performed in order to avoid overflows are being done by functions that call the one that is supposedly vulnerable (but is not actually vulnerable, as this is a false positive).  For cases that do not look like false positives, there seem to be quite a few involving uninitialized variables, where the program could end up having undefined behavior if said variables are not properly initialized. There are also a few cases where return values are not checked, and the analysis did not show any reason why said return values shouldn't be checked, as the lack of verification could cause other issues if the returned data indicates a possible error in the execution of the function returning them. There are also a few overflow issues which do not seem to be false positives. Checks are not being made in the functions that execute the operations that overflow and there are explicit out-of-bounds accesses being made, with calls to the supposedly vulnerable functions being made with hardcoded offsets that will result in errors. As an example, there is a situation where a call to a "vulnerable" function sends a buffer of size 6 (the buffer size is hardcoded into the caller) as an argument. The vulnerable function then copies data to this buffer specifying that the copy should be of 8 bytes. In cases such as these, it is hard to tell if the choice to "overflow" the buffer is being done deliberately because there is guarantee that the copy will not take place when, for example, the buffer size is 6, due to the copy instruction being inside a conditional statement and this statement evaluating to false whenever the caller function is the one that hardcodes the buffer size to 6. There is another analyzed case where something similar to this happens, and the possible overflow is "allowed" because it will never actually be executed when the call is made from the point in the code where we have the arguments that would cause this being defined. However, this seems like a dangerous way to go about things, security wise, especially considering that this is code that is being actively maintained. For some situations, there is no way of telling what is the real intention because some of the programming choices made are very obscure, and there are no comments explaining why that is possibly happening. Finally, there was also a divide-by-zero error found where a check is made, but when it is, nothing else is done about it, except a message being added to a log to inform the issue.

- TODO comments in the code indicate concerns with security issues such as race conditions. A few messages also indicate plans to comply with specifications even if the functionality might not be currently useful to iwd itself. In general, TODO comments contain good explanations on why something is still not yet implemented.

- Uses BPF filters in certain situations, like in src/netdev.c.

- It seems that upstream is worried about fixing issues such as the ones similar to results found by cppcheck and Coverity, as shown by https://git.kernel.org/pub/scm/network/wireless/iwd.git/commit/?id=f2fe9206c6b10b6e7996957c3f4618c0ec81aced. There is a concern regarding compiler warnings, with warnings not being simply ignored for some of the studied cases.

- Upstream is active.

- Upstream has a TODO list, with all TODO tasks documented and classified, which seems like a good start to get these implemented. Continuous updates seem to have been made regarding this list, and items were removed from the list ever since it was created, however, it seems like the last TODO item removed from it was removed about a year ago.

Even with the concerns regarding memory management and the issues identified by cppcheck and Coverity, it seems like iwd is well coded and well maintained. Code is expected to contain bugs, and C code is usually expected to contain a few memory management bugs. So long as upstream is working to fix these whenever possible, it seems like maintaining the package would not be a problem. Therefore, Security team ACK for promoting iwd to main.