RDKCOM-5624: RDKBNETWOR-80 Transform to Nftables from Iptables - #292
RDKCOM-5624: RDKBNETWOR-80 Transform to Nftables from Iptables#292vsai1990 wants to merge 14 commits into
Conversation
|
b'## WARNING: A Blackduck scan failure has been waived A prior failure has been upvoted
|
Reason for change: 1) Translate all the RDKB IPtables rules to nftables 2) write into /tmp/.nft and /tmp/.nft_v6 files and apply into netfilter 3) all the nftables rules are added under firewall_nft dir Test Procedure: RDKB Firewall functionality Risks: Medium
There was a problem hiding this comment.
Pull request overview
This PR introduces an nftables-based firewall implementation alongside the existing iptables-based firewall, with build-time support (--enable-firewall-nft) and runtime selection via syscfg nft_enable.
Changes:
- Adds a new
source/firewall_nft/implementation (firewall + nfqueue handler + support headers) intended to generate/apply nftables rules. - Updates build system (autotools + Makefile conditionals) to optionally build the nftables firewall and to build the legacy firewall binary as
firewall_ipt. - Updates runtime launcher script and a utapi port-forwarding path to conditionally use nft vs iptables.
Reviewed changes
Copilot reviewed 14 out of 16 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| source/utapi/lib/utapi.c | Adds runtime switch to attempt nft rules for ephemeral port forwarding. |
| source/utapi/lib/Makefile.am | Adds -DNFT_ENABLE when FIREWALL_NFT is enabled. |
| source/scripts/init/service.d/service_firewall/firewall_log_handle.sh | Switches between legacy and nft firewall binaries based on syscfg nft_enable. |
| source/Makefile.am | Adds firewall_nft subdir when FIREWALL_NFT is enabled. |
| source/firewall/Makefile.am | Builds legacy firewall as firewall_ipt under FIREWALL_NFT. |
| source/firewall_nft/raw_socket_send.c | Adds raw packet send helper (copied from legacy). |
| source/firewall_nft/nfq_handler_nft.c | Adds nft-oriented nfqueue handler implementation. |
| source/firewall_nft/Makefile.am | Builds firewall_nft and an nfqueue handler binary. |
| source/firewall_nft/firewallnft.h | Adds nft firewall header/API surface. |
| source/firewall_nft/firewall_priv_nft.c | Adds nft versions of custom rule helpers. |
| source/firewall_nft/firewall_ipv6_nft.c | Adds nft IPv6 firewall rule generation. |
| source/firewall_nft/firewall_interface_nft.c | Adds weak stubs for platform hooks in nft firewall. |
| source/firewall_nft/firewall_ext_nft.c | Adds extender-mode nft firewall logic. |
| source/firewall_nft/firewall_custom.h | Adds nft firewall custom header and shared declarations/macros. |
| configure.ac | Adds --enable-firewall-nft configure option and generates source/firewall_nft/Makefile. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 16 changed files in this pull request and generated 3 comments.
Suppressed comments (3)
source/utapi/lib/utapi.c:7785
- Same brace imbalance exists in the UDP branch:
if (!isNatRedirectionBlocked)andif (0 == strcmp("none", fromip))are opened but not closed before the router-mode forwarding rule block, which can break compilation or unintentionally gate forwarding rules behind NAT-redirection conditions.
}
/* it will applicable during router mode */
if( 0 == isBridgeMode )
{
source/firewall_nft/Makefile.am:31
- With
FIREWALL_NFTenabled, the build includes bothsource/firewallandsource/firewall_nft, and both Makefiles declare anfq_handlerinbin_PROGRAMS. This creates an install/build name collision fornfq_handler(two different implementations produce the same binary name).
bin_PROGRAMS = firewall_nft nfq_handler
firewall_nft_SOURCES = firewall_nft.c firewall_ipv6_nft.c firewall_priv_nft.c firewall_interface_nft.c firewall_ext_nft.c
if CPC_FIREWALL_ENABLE
firewall_nft_SOURCES += firewall_lib.c firewall_dsl.c rabid.c
AM_LDFLAGS += -lrdkconfig
endif
nfq_handler_SOURCES = raw_socket_send.c nfq_handler_nft.c
firewall_nft_LDADD = $(top_builddir)/source/syscfg/lib/libsyscfg.la \
configure.ac:61
--enable-firewall-nfthelp text says the value istrue/false, but the parser only acceptsyes/no(and errors otherwise). This makes the documented invocation fail.
#Enable NFT compile time flags if firewall-nft is set as yes
AC_ARG_ENABLE([firewall-nft],
[ --enable-firewall-nft=val Turn on nft Feature, val=true or false],
[case "${enableval}" in
yes) firewall_nft=true ;;
no) firewall_nft=false ;;
*) AC_MSG_ERROR([bad value ${enableval} for --enable-firewall-nft]) ;;
esac],[firewall_nft=false])
AM_CONDITIONAL(FIREWALL_NFT, test x"$firewall_nft" = x"true")
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 16 changed files in this pull request and generated 1 comment.
Suppressed comments (7)
source/firewall/Makefile.am:32
- With
FIREWALL_NFTenabled, bothsource/firewallandsource/firewall_nftbuild and install a binary namednfq_handler, which will cause an install-time collision. Only onenfq_handlershould be built/installed for the nft build.
if FIREWALL_NFT
bin_PROGRAMS = firewall_ipt nfq_handler
else
bin_PROGRAMS = firewall nfq_handler
endif
source/utapi/lib/utapi.c:7729
if (isNatReady)is never closed after adding the UDP prerouting_fromwan rule, so the NAT redirection and router-mode forwarding rules become unintentionally scoped underisNatReady. This changes behavior vs the TCP path and can skip LAN-side DNAT/SNAT and filter rules when WAN is not ready.
#endif
if ( !isNatRedirectionBlocked )
{
configure.ac:60
- The
--enable-firewall-nfthelp text says the value istrue/false, but the implementation only acceptsyes/no. This is confusing for users of./configure. Either accepttrue|falseas aliases or update the help string to match.
#Enable NFT compile time flags if firewall-nft is set as yes
AC_ARG_ENABLE([firewall-nft],
[ --enable-firewall-nft=val Turn on nft Feature, val=true or false],
[case "${enableval}" in
yes) firewall_nft=true ;;
no) firewall_nft=false ;;
*) AC_MSG_ERROR([bad value ${enableval} for --enable-firewall-nft]) ;;
esac],[firewall_nft=false])
source/firewall_nft/nfq_handler_nft.c:490
main()readsargv[1]to decide IPv4 vs IPv6 without validatingargc, which can segfault if the program is started without arguments.
int main(int argc, char *argv[])
{
struct nfq_handle *nfqHandle;
struct nfq_q_handle *queueHandle;
int fd, rv;
source/firewall_nft/nfq_handler_nft.c:519
- The error message has a typo (“maxium”) and doesn’t explain what the maximum allowed length is, which makes debugging misconfiguration harder.
if (strlen(argv[2]) >= sizeof(srcMac))
{
fprintf(stderr, "nfq_handler: maxium length of srcMac %s\n", __FUNCTION__);
exit(1);
}
source/firewall_nft/raw_socket_send.c:248
- In the IPv4 branch of
CreateIPHeader(),malloc()return is not checked and the allocated header is not zero-initialized before fields are written. This can lead to NULL dereference on allocation failure and leaves unspecified fields uninitialized.
struct iphdr *ip_header;
ip_header = (struct iphdr *)malloc(sizeof(struct iphdr));
ip_header->version = 4;
source/firewall_nft/raw_socket_send.c:289
CreateTcpHeader()does not checkmalloc()and does not zero-initializestruct tcphdr, so TCP flag/bitfield members that aren’t explicitly set may contain garbage. This can produce malformed packets.
static struct tcphdr *CreateTcpHeader(int family, unsigned short sport, unsigned short dport, unsigned long seqNum, unsigned long ackNum, unsigned char fin)
{
struct tcphdr *tcp_header;
/* Check /usr/include/linux/tcp.h for header definiation */
tcp_header = (struct tcphdr *)malloc(sizeof(struct tcphdr));
tcp_header->source = htons(sport);
tcp_header->dest = htons(dport);
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 16 changed files in this pull request and generated 4 comments.
Suppressed comments (2)
source/firewall_nft/Makefile.am:24
- With FIREWALL_NFT enabled, both
source/firewall/Makefile.amandsource/firewall_nft/Makefile.ambuild/install anfq_handlerbinary. Since both subdirs are included under FIREWALL_NFT (source/Makefile.am:21-26), this will cause an install/build collision in $(bindir) and it’s ambiguous which handler ends up on the image.
AM_CPPFLAGS = -I$(top_srcdir)/source/include -I$(top_srcdir)/source/util/utils $(DBUS_CFLAGS)
AM_LDFLAGS = -lccsp_common -lsecure_wrapper -lnetfilter_queue -lnfnetlink $(DBUS_LIBS) -pthread -lrt
bin_PROGRAMS = firewall_nft nfq_handler
configure.ac:59
- The help text says the flag value is "true or false", but the case statement only accepts "yes"/"no"; passing "--enable-firewall-nft=true" will currently error out. Either update the help text to match, or accept true/false (and optionally 1/0) as synonyms.
[ --enable-firewall-nft=val Turn on nft Feature, val=true or false],
[case "${enableval}" in
yes) firewall_nft=true ;;
no) firewall_nft=false ;;
*) AC_MSG_ERROR([bad value ${enableval} for --enable-firewall-nft]) ;;
|
@vsai1990
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 16 changed files in this pull request and generated 3 comments.
Suppressed comments (8)
source/utapi/lib/utapi.c:7752
- Same as the TCP branch: in the UDP
fromip == nonepath, thepostrouting_tolanSNAT rule is currently still insideif (isNatReady)due to a missing closing brace before the SNAT section. That changes the original behavior for LAN hairpin NAT when WAN IP isn’t ready.
if ( isNatReady )
{
#ifdef NFT_ENABLE
if (atoi(nft_enable) == 0)
{
source/firewall_nft/Makefile.am:30
- With
FIREWALL_NFTenabled, bothsource/firewallandsource/firewall_nftbuild/install a binary namednfq_handler(seesource/firewall/Makefile.amand this file). This will cause an install collision (one overwrites the other or install fails) and makes it ambiguous which implementation runs at runtime.
bin_PROGRAMS = firewall_nft nfq_handler
firewall_nft_SOURCES = firewall_nft.c firewall_ipv6_nft.c firewall_priv_nft.c firewall_interface_nft.c firewall_ext_nft.c
if CPC_FIREWALL_ENABLE
firewall_nft_SOURCES += firewall_lib.c firewall_dsl.c rabid.c
AM_LDFLAGS += -lrdkconfig
endif
nfq_handler_SOURCES = raw_socket_send.c nfq_handler_nft.c
source/firewall_nft/nfq_handler_nft.c:490
main()usesargv[1]unconditionally to determine the address family. If the program is started without arguments (or with too few), it will segfault. Add a basicargcguard and usage message before readingargv[1].
int main(int argc, char *argv[])
{
struct nfq_handle *nfqHandle;
struct nfq_q_handle *queueHandle;
int fd, rv;
char buf[4096];
unsigned char i, j;
u_int16_t family = atoi(argv[1]) == 4 ? AF_INET : AF_INET6;
source/firewall_nft/nfq_handler_nft.c:432
ackNumis computed using IPv4 header fields (ipHdr->tot_len,ipHdr->ihl) even when the packet is IPv6. In the IPv6 pathipHdrpoints to an IPv6 header, so this reads the wrong offsets and produces an invalid ACK (and can cause memory issues). Compute the payload length usingip6_plenfor IPv6.
unsigned long ackNum = ntohs(ipHdr->tot_len) - tcpHdr->doff * 4 - ipHdr->ihl * 4 + ntohl(tcpHdr->seq);
source/firewall_nft/raw_socket_send.c:393
gwIpis read withfgets(), which retains the trailing newline. That newline becomes part of the redirect payload (and affects length/checksum calculations). Strip\r/\nafter reading.
FILE *fp = fopen("/var/.gwip", "r");
if(fp != NULL){
fgets(gwIp, sizeof("255.255.255.255"), fp);
fclose(fp);
}
source/firewall_nft/raw_socket_send.c:414
dataSizedoes not account for the extra[and]characters thatCreateData()adds for IPv6, so IPv6 packets are built 2 bytes short (bad payload/checksum).
dataSize = HDRLEN + strlen(gwIp) + sizeof(http_redirect_payload2) + strlen(url) + sizeof(http_redirect_payload_bottom);
source/utapi/lib/utapi.c:7648
- In the
fromip == none(hairpin NAT) path, the new#ifdef NFT_ENABLErefactor accidentally nests thepostrouting_tolanSNAT rule insideif (isNatReady). In the previous flow the SNAT was applied regardless of WAN/NAT readiness once the LAN-side prerouting DNAT was installed. This changes behavior and can break LAN hairpin access when WAN IP isn’t ready.
This issue also appears on line 7748 of the same file.
if ( isNatReady )
{
#ifdef NFT_ENABLE
if (atoi(nft_enable) == 0)
{
configure.ac:57
--enable-firewall-nfthelp text says the value istrue/false, but the parser only acceptsyes/no. Passing--enable-firewall-nft=truewill currently error even though it’s advertised as valid. Either update the help text or accepttrue|falseas synonyms.
#Enable NFT compile time flags if firewall-nft is set as yes
AC_ARG_ENABLE([firewall-nft],
[ --enable-firewall-nft=val Turn on nft Feature, val=true or false],
[case "${enableval}" in
yes) firewall_nft=true ;;
|
@vsai1990 |
|
Hi @AkhilaReddyK7, Please start build. Build is working from our side. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 16 changed files in this pull request and generated 6 comments.
Suppressed comments (3)
source/utapi/lib/utapi.c:7769
- Same scoping issue as the TCP block: the closing brace for
if ( isNatReady )comes after the SNAT (postrouting_tolan) section, making SNAT conditional onisNatReadyunintentionally.
#endif
#ifdef NFT_ENABLE
if (atoi(nft_enable) == 0)
{
source/utapi/lib/utapi.c:7664
- The SNAT (postrouting_tolan) block is currently inside the
if ( isNatReady )scope because the closing brace for thatifcomes after the SNAT section. This changes behavior compared to the surrounding logic and makes the SNAT rule conditional onisNatReadyunintentionally.
#endif
#ifdef NFT_ENABLE
if (atoi(nft_enable) == 0)
{
configure.ac:60
- The
--enable-firewall-nfthelp text says the value istrue/false, but the parser only acceptsyes/no. This makes the documented invocation fail with a configure-time error.
AC_ARG_ENABLE([firewall-nft],
[ --enable-firewall-nft=val Turn on nft Feature, val=true or false],
[case "${enableval}" in
yes) firewall_nft=true ;;
no) firewall_nft=false ;;
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 16 changed files in this pull request and generated 5 comments.
Suppressed comments (4)
source/firewall_nft/firewall_ext_nft.c:132
- This writes the mangle table declaration into filter_fp. In extender mode the final rules file is assembled in raw->mangle->nat->filter order; table declarations should be emitted into the corresponding section file to preserve ordering.
/*
* mangle
*/
fprintf(filter_fp, "add table ip mangle\n");
source/firewall_nft/firewall_ext_nft.c:138
- This writes the nat table declaration into filter_fp. In extender mode the final rules file is assembled in raw->mangle->nat->filter order; nat rules in nat_fp can be emitted before the nat table exists, producing an invalid ruleset.
/*
* nat
*/
fprintf(filter_fp, "add table ip nat\n");
source/firewall_nft/Makefile.am:31
- This directory builds a binary named
nfq_handler, but source/firewall/Makefile.am also builds and installsnfq_handler. Installing two different programs with the same name will typically overwrite one of them in bindir and make the result build-order dependent.
AM_CPPFLAGS = -I$(top_srcdir)/source/include -I$(top_srcdir)/source/util/utils $(DBUS_CFLAGS)
AM_LDFLAGS = -lccsp_common -lsecure_wrapper -lnetfilter_queue -lnfnetlink $(DBUS_LIBS) -pthread -lrt
bin_PROGRAMS = firewall_nft nfq_handler
firewall_nft_SOURCES = firewall_nft.c firewall_ipv6_nft.c firewall_priv_nft.c firewall_interface_nft.c firewall_ext_nft.c
if CPC_FIREWALL_ENABLE
firewall_nft_SOURCES += firewall_lib.c firewall_dsl.c rabid.c
AM_LDFLAGS += -lrdkconfig
endif
nfq_handler_SOURCES = raw_socket_send.c nfq_handler_nft.c
firewall_nft_LDADD = $(top_builddir)/source/syscfg/lib/libsyscfg.la \
configure.ac:60
- The configure flag help text says the value should be "true or false", but the parser only accepts "yes"/"no". This makes "--enable-firewall-nft=true" fail despite what the help advertises.
#Enable NFT compile time flags if firewall-nft is set as yes
AC_ARG_ENABLE([firewall-nft],
[ --enable-firewall-nft=val Turn on nft Feature, val=true or false],
[case "${enableval}" in
yes) firewall_nft=true ;;
no) firewall_nft=false ;;
*) AC_MSG_ERROR([bad value ${enableval} for --enable-firewall-nft]) ;;
esac],[firewall_nft=false])
| else | ||
| if [ -x /usr/bin/firewall_nft ];then | ||
| /usr/bin/firewall_nft "$@" | ||
| fi | ||
| fi |
| //nft rules added | ||
| fprintf(fp, "add chain ip6 filter %s\n", IPOE_HEALTHCHECK); | ||
| fprintf(fp, "insert INPUT count %s\n", IPOE_HEALTHCHECK); | ||
| #endif //_RDKB_GLOBAL_PRODUCT_REQ_ |
| fprintf(fp, "add rule ip6 filter DOS iifname lo return\n"); | ||
| fprintf(fp, "add rule ip6 filter DOS tcp dport 80 tcp flags syn jump DOS_TC\n"); | ||
| fprintf(fp, "add rule ip6 filter DOS udp state new jump DOS_UDP\n"); |
| /* | ||
| * raw | ||
| */ | ||
| fprintf(filter_fp, "add table ip raw\n"); | ||
|
|
| prepare_ipv6_firewall(filename2); | ||
| v_secure_system("nft -f /tmp/.nft_v6 2> /tmp/.nftv6table_ext_error"); | ||
|
|
Reason for change:
Test Procedure: RDKB Firewall functionality
Risks: Medium