I reviewed dhcpcd5 9.4.1-24 as checked into mantic. This shouldn't be considered a full audit but rather a quick gauge of maintainability. dhcpcd5 is a DHCP client (for both IPv4 and IPv6 hosts) and a IPv4LL client. It silently configures the network in a computer and requires mostly no configuration to do so. - CVE History: - Has a few CVEs attributed to it in the past (15). - Patches for CVEs are not as clear/easy to find in older CVEs, but since 2019 it seems like upstream has adopted a more organized system in order to patch vulnerabilities, providing the appropriate commits and including messages that explain the issue. - Most recent CVE is from 2019. - Upstream is active and frequently applying fixes. - Build-Depends: - libudev-dev, debhelper-compat, pkg-config - Does not depend on encryption libraries directly, however, this is because during the configure step, there is a check for the presence of certain libraries, and if they do not exist, they are recovered from the '/compat' directory, which in turn contains a '/crypt' directory. This directory contains C files with hashing functions that are to be used by dhcpcd5. The SHA256 files seems to be recovered from 'libscrypt', which is a package available in Ubuntu. The MD5 and HMAC functions seem to have been implemented for NetBSD and I cannot find a package which implements the functions in those files, only coming across packages that do the same as dhcpcd5 and have an internal copy of the code. Possibly package 'util-linux'? TODO: possibly finding if we have packages which contains the functions recovered from 'compat'. - Other dependencies in this directory include functions available through 'libbsd', like the 'arc4random' random number generator function and some string functions. If the libraries that provide these functions are not set as dependencies for the package, this means that maintenance of this package might involve keeping a close eye in this "outside" functions, as they could be susceptible to vulnerabilities as well. - pre/post inst/rm scripts: - They seem to run regular set up and clean up actions, other than additional commands included in order to prepare the system for privilege separation. - dhcpcd/preinst: sets 'dhcpcd' in 'init.d' as executable if an 'install' command is executed and if the file exists. Ends 'dhcpcd.service' if 'systemd' is in the system and if it is running, and in case of an upgrade. - dhcpdcd/postinst: if a 'configure' is run or an 'abort' (upgrade, configure or remove) command is run, will enable/(set '+x' permission) dhcpcd service/(to 'init.d' dhcpcd file). - dhcpdcd/postrm: disables dhcpcd service in 'init.d' if a 'remove' command (by removing execution flag), and deletes the file completely if the command is a 'purge'. Does the equivalent to the systemd service. - dhcpcd-base/postinst: creates a 'dhcpcd' system user with no shell. Runs the dhcpcd daemon. - dhcpcd-base/postrm: deletes 'dhcpcd' system user IF 'deluser' exists. Presents system message if the user is not deleted. Removes directory where dhcpcd stores leases and DUIDs. - init scripts: - Creates './etc/init.d/dhcpcd' (which is expected since this is a network client to a core network functionality). - systemd units: - Creates 'dhcpcd.service' (which is expected since this is a network client to a core network functionality). - binaries in PATH: - '/usr/sbin/dhcpcd'. This binary is expected to be run by root. - unit tests / autopkgtests: - The package contains a few unit tests, but it would be interesting if it could be tested more extensively, especially to check for regressions after patching. - Autopkgtests exist and were failing, but this issue seems to have been fixed. Since the package itself does not contain a very comprehensive test suite, getting this to work is essential to help the Security Team avoid regressions when patching this package, so it is good that autopkgtests are passing for this package now. - Build logs: - Nothing relevant to report. - Processes spawned: - Spawns hook processes via 'posix_spawn()'. According to the way that Ubuntu installs the package, hooks are under '/usr/lib/dhcpcd'. Hooks installed are the ones under '/debian/hooks' and the standard hooks installed by the system. As per the 'posix_spawn()' manual information, this function exists as a substitute to the 'fork()' system call, which is not present in all systems, such as embedded systems. Provides the combined functionalities of 'fork' and 'exec'. 'posix_spawn()' searches for files in PATH. - Hooks seem to be called into execution in the following manner: functions run by dhcpcd call 'script_runreason'. This function creates the environment through 'make_env', which will recover the PATH environment variable and will set other environment characteristics based on interface and networking information for the running thread. The way that this is dealt with in terms of memory management/string construction seems to be safe, as the code tries to use open_memstream or to replicate the behavior of this function in order to generate a final output that is more surely in the correct format for consumption and processing by other string functions. After the environment string is set, it is sent as an argument to 'script_exec' through 'script_run'. 'script_exec' is the function that calls 'posix_spawn()'. The only place where 'ctx->script', which is the path to the process that will be executed by 'posix_spawn()', seems to be set is in 'if-options.c'. The default value is the path to 'dhcpcd-run-hooks', as it is installed in the system, however, this value can possibly be changed by calling options provided to dhcpcd or through options set in 'dhcpcd.conf'. The value is checked for '/dev/null' so that this is not used. - 'posix_spawn()' uses its last argument as the environment variables that should be considered when it is running the process requested. Considering that these variables are prepared beforehand in 'script.c', this seems like a pretty safe option, all things considered. - Memory management: - Prefers use of strlcpy over strcpy or strncpy, which looks like a wise choice, especially considering that strcmp and strlen are used a lot, so using strlcpy guarantess the presence of the ending '\0'. - Use of calloc and malloc seems to be done carefully, with checks after the attempted allocation for NULL returns and with guarantee that allocation values are not 0 (even if sometimes not happening directly on the function that tries the allocation, but instead happening in the function that calls the function containing the allocation call). Memory is also recursively freed when an allocation fails for some reason or another. - The developer seems to be very careful when handling allocations, which is a positive thing. - A lot of the memcpy instances that I am verifying seem to focus of making the copy for a number of bytes equal to the size of the destination. Various also check for the minimum size in between the source and the destination, which is a prudent choice. A lot of the structures used also seem to have and accompanying _len field, which is used to track the lengths of certain arrays. These are used in memcpy operations various times, and provides a bit more confidence when performing these types of operations. I can also confirm that there are various checks on values to make sure that they have correct size limits or that they are not null. Reallocations are performed when variable sizes are not as expected. - A lot of the cases where memcpy would fail because of the size of the data being transfered, the code returns ENOBUFS, which shows concern when using this type of function and with handling the variables related to it. - This careful memory management may be a combination of concern from part of the developer joined with the fact that for internet protocol packages, data has to be extremely well organized in order to produce the expected result. - 'src/bpf.c' uses defined variables instead of hardcoded values when passing data into a buffer. This is best to avoid miscalculations. - Uses TAILQ to manage double linked queues, which avoids possible memory issues for manually handling those. - Conclusion: memory management is done with extreme care, and it seems like the developer is actively trying to improve the code based on coverity logs (as seen in certain commit messages). - File IO: - in 'script.c', 'mkstemp' is used to open and create a file in '/tmp' for printing of certain data that will later be used as environment for 'posix_spawn()'. The file is created and opened by 'mkstemp' and then immediately unlinked by the C code, making that it will be removed once it is closed. This operation through use of a file only takes place if 'open_memstream' is not present in a system. For my local build, it seems to be the case, so this file is never actually opened. Having open_memstream available in the system, therefore, might be the best option to avoid the opening of unecessary files. - 'src/common.c' contains file handling and functions such as 'readfile' and 'writefile', which are simple functions that focus on opening a file and reading from it into a buffer (there are length checks here to avoid overflows) or reading from a buffer and writing into it. The 'writefile' function accepts as one of its arguments the mode, which will define permissions for the file. It will create the file with the requested permissions if it doesn't exist, and if it already exists the permission is ignored and the file is simply opened for operations. This is not an issue (here) in terms of a possible race condition where someone opens the file before permissions are set after its creation or creates a file beforehand with different permissions in order to tamper with the originally requested permissions (even if O_EXCL is not being used) since all files are written by root in areas that only root has access to (if we consider the default configuration that is set for the application, which is partly defined in the build configurations). 'src/common.c' also contains a function to get lines from a file which exists to specifically avoid buffer overflow issues, as per the comment preceding the function declaration. Functions that read from a file or write to it seem to only be run through other functions, never directly. If the process is not a root process in the privilege separation ecosystem, the request to write to a file is first sent to the root process since the other processes are chrooted. - In src/auth.c, my only concern is the possible race condition that exists in between the time the file is opened until the time the permissions of the file are set in case of the file opened with the 'write' flag. What if someone already has the file opened up and gets to read whatever is being written to it before any permissions are set (since fopen() creates a file with the 0666 mask)? The file will be opened initially by the root thread, and then permissions are set to only allow root reading the file, but the issue is the opening of the file in the time in between. - file 'src/privsep-root.c' contains function 'ps_root_validpath', which attempts to validate a filepath before allowing that a file be written to or read from. However, for this to actually be run, certain the privilege separation mode must be set, which it is, in Ubuntu. Therefore, the 'readfile' and 'writefile' operations in 'src/common.c', when run by unprivileged processes, always go through a root process which will be the one that will actually write to the file, AFTER proper path sanitization. Path sanitization includes verification that there is no '..' in the path and also verification that the file being written to or read from is one of the files that dhcpcd needs to write to or read from only. - Logging: - Will always write log outputs to syslog, according to man. Code analysis confirms this. - Files 'logerr.c' and 'logerr.h' contain information and code regarding log functions (these implement all logging functions, abstracting log interaction to other functions in the code, which avoids possible errors). - Logging seems to be done with care as to avoid format strings vulnerabilities. There were some concerns involving 'dlerror()' returns values being used as arguments (which could be NULL) and calls where the sole argument was a variable called '__func__'. However, further analysis and researched was able to show that this concern did not need to exist as things are being handled appropriately (the print call will not be called with a NULL value due to previous checks, and the '__func__' variable is processed by the compiler as a function name, and function names are set by upstream, which we in theory trust, and will not contain '%' characters) and there indeed is no risk for format string vulnerabilities. - When in privilege separation, chrooted processes can only log through the privileged proxy. When initially forking the process, dhcpcd will set the destination log for the unprivileged processes as being a shared socket between them and the privileged process instead of setting it to the actual log file. The privileged processed is then configured in such a way (with the help of event handling in 'eloop.c') that everytime it receives a log message through this socket, it will call functions that will end up writing to the final log file and to syslog. - Logging seems to happen in a pretty organized manner, following a standard format that considers a fixed size for messages and that follows the syslog RFC. - Environment variable usage: - 'src/script.c' recovers the 'PATH' environment variable and uses it when running hook scripts ('posix_spawn()' requires environment variables). 'PATH' is recovered through 'getenv()' instead of 'secure_getenv()', which would be a better option. Maybe the first one is chosen because it is implicitly expected that the program can only be run as root and that it will not be SUID/GUID. Of course, the hooks directory is only writable by root, but this is relies on the OS configuring it as so as well. Beyond that, the dhcpcd command also accepts scripts as input, which, if it were set to SUID, could lead to issues. 'PATH' is only used with the DUMP option of dhcpcd. - After further analysis of the code, this usage of environment variable seems to not be a problem if the program is run with privilege separation. That is because the dhcpcd process is chrooted to the directory of the dhcpcd user, which is owned by root and can only be written too by root. - Use of privileged functions: - Privilege Separation is enabled during regular build. - In 'src/dhcpcd.c', 'ps_init' initiates privilege separation if PRIVSEP is set. 'ps_init' tries to check if the 'dhcpcd' user exists and if the chroot directory set for that user exists, if not, it disables privilege separation. We are properly setting the user through configure options and properly creating said user in the postinst script, which guarantees privilege separation. - By default, Linux systems are set with 'HAVE_SECCOMP', which results in the sandboxing of 'dhcpcd' to a seccomp sandbox. It sets option 'SECCOMP_MODE_FILTER' when configuring the sandbox, and, as the manual says, "This mode is available only if the kernel is configured with CONFIG_SECCOMP_FILTER enabled", so this needs to be checked in order to allow us to reap the benefits of this sandboxing provided by dhcpcd. - 'dhcpcd', when set with PRIVSEP, chroots to dhcpcd's home directory for several of its threads (and 'chdirs' to the chroot directory). It also sets effective uid and gid to that of the 'dhcpcd' user when it does so. - A privileged proxy process is created in between the main dhcpcd thread and the system, and an 'AF_UNIX' socket is used to allow communication between both these processes. - Privilege separation divides dhcpcd into 4 entities: the main dhcpcd process, the privileged proxy, the controller proxy, and the network proxy. Other threads, such as the ones to handle BPF, are created as well, but the previously mentioned are the main ones. All processes are chrooted except for the privileged proxy, used to perform specific privileged operations. Unprivileged threads are limited in the types of systemcalls that they can run. - 'ps_root_ioctl' is the function that needs to be passed through in order for an unprivileged thread to run an ioctl command. 'src/if.c', for example, must always request the root thread to run ioctl commands when the command aims to set a flag, through 'if_ioctl'. For reading operations, there is no need to go through the root proxy. In 'src/bpf.c' there is a write ioctl call that is performed by one of the functions, however, even in this situation privilege separation is at work: the proxy BPF thread that is created calls the root thread to execute the function that performs that call. The 'ps_root_ioctl' function performs checks that limit the flags that can be used when running ioctl (it allows setting interface addresses, changing the MTU of a network interface, setting the flag word of a device and setting the hardware address of a device. Any other ioctl request is rejected with a permission error). - Communication happens between threads through the use of UNIX sockets. Each thread constantly listen on their own event socket with the help of the ppoll function. A consistent loop runs and polls these sockets, which will be receiving messages from other threads, requesting that certain jobs be executed. When 'ps_dostart' runs for each created thread (controll, inet and root) during their creation process, an event is added for these threads such that everytime the sockets that they read from receive a message a certain function is set to execute. For the root thread, for example: when 'ps_root_start' is run, it creates the log socket and then adds an event to 'eloop' with the log socket descriptor. This means that everytime the log socket has data, the event will trigger and 'ps_root_log' will run and actually write to the log file. Other than the log socket and the log function, the root thread also "tracks" the 'ps_root_fd' file descriptor, which is the descriptor for the socket used by other threads used to request that the root thread run some job for them. When creating the root thread, 'ps_dostart' sets the main root thread listening function to 'ps_root_recvmsg', which will actually carry out root related activities based on the commands sent by other threads (there is an internal dhcpcd protocol that is used here). - 'src/auth.c' function chmods a file, however, if privilege separation is set, this passes through the root thread. It 'chmod's a file to permission '400' in case it doesn't already exist, after opening it for write operations. There seems to have a race condition here, however. It truncates the file before opening it if the file exists. In theory this is not supposed to be a problem as the file will be created with dhcpcd user permissions, and if a user already has those permissions to read the file, they wouldn't need to abuse a race condition. - In 'src/control.c' a UNIX socket is created and it is 'chmod'ed and 'chown'ed to the 'controlgroup' set in the configuration file. According to dhcpdcd's manual page, this is to allow users other than root to connect to dhcpcd, however, this is only allowed in a per-group basis, which is good to limit the set of users to trusted users only. - Use of cryptography / random number sources etc: - 'arc4random' is used for random number generation (available in 'compat' directory if there is no library installed that contains this function). - Uses external libraries to generate SHA256 hashes (once again, these are available in the 'compat' directory if the actual libraries are not present in the system). - It does not attempt to implement its own version of random number generation/cryptography functions. - Use of networking: - Uses BPF to filter packets, the BPF thread is created and later "filtered" by the root thread, as is the inet thread, meaning, both of those threads work as listening proxies but request jobs from the root thread whenever they might need to run privileged operations. - It validates UDP ports when using DHCP protocol. - The root thread is responsible for opening external network sockets for sending messages. The inet thread opens them for receiving messages while it is still unsandboxed. The inet thread is unprivileged and acts as a proxy to the main thread. It waits for messages in sockets and sends them over to the main thread for processing once they arrive. The main thread, being unprivileged, focuses on processing these messages (meaning that untrusted input is processed by unprivileged processes, which is good to avoid various possible issues). - In 'arp.c', BPF handling goes through the BPF thread which in turn calls the root thread to run the BPF operations as needed. - 'if-options.c' checks address format when looking at the options provided by the user in the configuration file and also opens sockets to verify functionalities, closing them right after. - Messages are sent through sockets by the root thread, everytime, through the 'ps_inet_cmd' call. If the root socket receives a message with a certain flag set, it will call the 'ps_inet_cmd' function and if this is not a connection start or stop request, the message will be send through the outgoing socket. The other option for this root originated call is to have it to spawn a listener socket of the requested kind. This is done in the following manner: a new listening thread is started and a socket to communicate internally with this thread is created. The thread itself opens a socket for listening and listens on it. - DHCP code tries to authenticate messages received from external sources. The secret is saved in a file and is only read from by the root thread. - The messages are thoroughly analyzed and comments spread around the code seem to indicate compliance with RFC. - Networking is done in the safely considering the nature of the application, with extensive checking and RFC compliance. Privilege separation forces incoming data to be processed by unprivileged threads, giving the application an extra security layer. - Use of temp files: - Has two instances where temporary files are generated: in 'src/dhcpc6.c' the file is generated with a predictable name, however, preprocessor instructions guarantee that part of the code will never be executed since it is an "#if 0" (this might be used for testing purposes); in 'src/script.c' the creation of a temporary file is used as a way to replicate the behavior of 'open_memstream'. The temporary file is created safely, but it actually doesn't need to be used, since we have open_memstream available, and in my local build it was used instead. Regarding the former, this inclusion of "debug" code in the way that it was included might be a bit dangerous, should the developer accidentally remove the preprocessor instruction in any commit, so it will be something that we would need to monitor to avoid possible issues. - Cppcheck results: - A lot of false positive due to use of the TAILQ_FOREACH macro. TAILQ_FOREACH initializes variables that cppcheck was recognizing as uninitialized. - Coverity results: - Nothing relevant to report. - Quick build. - Upstream seems to run Coverity on their code and fix found issues. - The 'compat' directory contains files with functions needed by the dhcpcd5 but that might not be already installed in the system. - Debian has created its own hooks to install in the system. These are under '/debian/hooks'. Security team ACK for promoting dhcpcd5 to main, given that: - Dependencies for the package are further looked into and are accordingly updated in the best way possible in order to avoid usage of 'compat' directory files if it is not necessary for some type of specific situation (appropriate weighing of pros and cons). - We have CONFIG_SECCOMP_FILTER enabled in order to allow for usage of the seccomp sandbox. - We can confirm that we will have 'open_memstream' available when the code is built in order to avoid opening of unecessary files. - The Security Certification Team also provides their ACK, as the random number generation functions/cryptography functions are related to FIPS.