Make usage of all configured interfaces safer #30
Conversation
Move the ifinit() calls into config.c's configure_interface() during parse time. This removes dependence on UB and is safe across more systems. This also removes the need to parse the config file twice. Fixes use after free / crash on OpenBSD with -O >0. Additionally this simplifies SIGHUP behaviour and makes it not force reload all configured interfaces if interfaces were originally passed on the commandline.
jrmithdobbs
force-pushed
the
fix_hup_loading_all_interfaces
branch
from
2fe9379
to
d72b0e6
Compare
jrmithdobbs
changed the title
|
This fixes both loading all interfaces on HUP when invoked with interfaces on the command line as well as a segfault that is described in #31 . |
|
Updated the summary to clarify that this is a use after free security issue that should really be fixed. It is not an issue with running on OpenBSD, that is a symptom. The original code explicitly copies out a pointer to This feature was implemented as an explicit use after free vulnerability by design. I do not currently see an obvious way to use this to lead to code execution, or I would have already opened a CVE, but since users of the opnsense web interface with limited privileges can technically poke arbitrary binary data into memory using the raw option extension also added by opnsense it seems prudent to fix this. |
Improvements include the following: - Reload the client configuration on SIGHUP - Removed all unused binaries except dhcp6c - Raw option send and receive support - PDINFO delegated prefix environment variable - Accept interfaces from the configuration file - Increased log verbosity. - Fix socket leakage by setting FD_CLOEXEC. - Call a configuration script after addresses and prefixes are set on an interface. - Update ifid on interface restart, it may have changed. - "-n" flag to prevent address release from being sent to the DHCP server upon restart. - Merged PR : opnsense/dhcp6c#28 - Merged PR : opnsense/dhcp6c#29 - Merged PR : opnsense/dhcp6c#30
| @@ -196,33 +200,21 @@ main(int argc, char *argv[]) | |||
| client6_init(); | |||
|
|
|||
| /* | |||
| * Doing away with the need for command line interfaces | |||
| * We need to read the config file to get the names of the interfaces. | |||
| * Doing away with the need for command line interfaces If this is set | |||
There was a problem hiding this comment.
| * Doing away with the need for command line interfaces If this is set | |
| * Doing away with the need for command line interfaces. If this is set |
Move the ifinit() calls into config.c's configure_interface() during parse
time. This removes dependence on UB and is safe across more systems. This also
removes the need to parse the config file twice and fixes a memory leak.
Fixes a memory leak of the entire config structure besides the interface list in main.
Fixes the same memory leak again that was duplicated into the HUP handler.
Fixes two use after free security issues. (one in main, one in HUP handler)
These use after frees cause crashes on OpenBSD with -O >0 but is a security issue
everywhere.
The use after free does not seem to exploitable as the only things done with the
free'ed pointers is dereferencing and access to the name member in ifinit when
calling strdup.
Additionally this simplifies SIGHUP behaviour and makes it not force reload all
configured interfaces if interfaces were originally passed on the command line.