From 3bdf9c3c2776fd34beacb8d39c91d8fdeef4f704 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Fri, 15 May 2020 00:19:00 +0200 Subject: [PATCH 01/18] Process cached SRV records Signed-off-by: DL6ER --- src/dnsmasq/rfc1035.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/dnsmasq/rfc1035.c b/src/dnsmasq/rfc1035.c index fddf13fc..7cf06626 100644 --- a/src/dnsmasq/rfc1035.c +++ b/src/dnsmasq/rfc1035.c @@ -1941,12 +1941,16 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen, if (crecp->flags & F_NXDOMAIN) nxdomain = 1; if (!dryrun) + { log_query(crecp->flags, name, NULL, NULL); + FTL_cache(crecp->flags, name, NULL, NULL, daemon->log_display_id); + } } else if (!dryrun) { char *target = blockdata_retrieve(crecp->addr.srv.target, crecp->addr.srv.targetlen, NULL); log_query(crecp->flags, name, NULL, 0); + FTL_cache(crecp->flags, name, NULL, NULL, daemon->log_display_id); if (add_resource_record(header, limit, &trunc, nameoffset, &ansp, crec_ttl(crecp, now), NULL, T_SRV, C_IN, "sssd", From eb4b35e7f10e0ed63fb384af34686b79a5b1f283 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Sun, 24 May 2020 22:39:33 +0200 Subject: [PATCH 02/18] Add NAMES_FROM_NETDB option. Signed-off-by: DL6ER --- src/config.c | 19 +++++++++++++++++++ src/config.h | 1 + src/resolve.c | 3 ++- 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/src/config.c b/src/config.c index 9d529c2b..bf7383e1 100644 --- a/src/config.c +++ b/src/config.c @@ -372,6 +372,25 @@ void read_FTLconf(void) else logg(" BLOCK_ESNI: Disabled"); + // NAMES_FROM_NETDB + // Should we use the fallback option to try to obtain client names from + // checking the network table? Assume this is an IPv6 client without a + // host names itself but the network table tells us that this is the same + // device where we have a host names for its IPv4 address. In this case, + // we use the host name associated to the other address as this is the same + // device. This behavior can be disabled using NAMES_FROM_NETDB=false + // defaults to: true + config.names_from_netdb = true; + buffer = parse_FTLconf(fp, "NAMES_FROM_NETDB"); + + if(buffer != NULL && strcasecmp(buffer, "false") == 0) + config.names_from_netdb = false; + + if(config.names_from_netdb) + logg(" NAMES_FROM_NETDB: Enabled, trying to get names from network database"); + else + logg(" NAMES_FROM_NETDB: Disabled"); + // Read DEBUG_... setting from pihole-FTL.conf read_debuging_settings(fp); diff --git a/src/config.h b/src/config.h index e7ba0f81..8b978a72 100644 --- a/src/config.h +++ b/src/config.h @@ -39,6 +39,7 @@ typedef struct { bool parse_arp_cache; bool cname_inspection; bool block_esni; + bool names_from_netdb; } ConfigStruct; typedef struct { diff --git a/src/resolve.c b/src/resolve.c index b02ae411..ea4df92d 100644 --- a/src/resolve.c +++ b/src/resolve.c @@ -256,7 +256,8 @@ static size_t resolveAndAddHostname(size_t ippos, size_t oldnamepos) char* newname = resolveHostname(ipaddr); // If no hostname was found, try to obtain hostname from the network table - if(strlen(newname) == 0) + // This may be disabled due to a user setting + if(strlen(newname) == 0 && config.names_from_netdb) { free(newname); newname = getDatabaseHostname(ipaddr); From efee05115b4153c94f02ff34bf241ee8ab4d0aa4 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Mon, 25 May 2020 18:40:16 +0200 Subject: [PATCH 03/18] Store client group information in shared memory. Signed-off-by: DL6ER --- src/database/gravity-db.c | 35 ++++++++++++++--------------------- src/datastructure.c | 2 +- src/datastructure.h | 2 +- 3 files changed, 16 insertions(+), 23 deletions(-) diff --git a/src/database/gravity-db.c b/src/database/gravity-db.c index 86d7f5d7..79756013 100644 --- a/src/database/gravity-db.c +++ b/src/database/gravity-db.c @@ -203,7 +203,7 @@ static bool get_client_groupids(clientsData* client) { char *querystr = NULL; const char *ip = getstr(client->ippos); - client->groups = NULL; + client->groupspos = 0u; // Do not proceed when database is not available if(!gravityDB_opened && !gravityDB_open()) @@ -257,7 +257,7 @@ static bool get_client_groupids(clientsData* client) { // Found no record for this client in the database // This makes this client qualify for the special "all" group - client->groups = strdup("0"); + client->groupspos = addstr("0"); } else { @@ -273,7 +273,7 @@ static bool get_client_groupids(clientsData* client) free(querystr); querystr = NULL; - if(client->groups != NULL) + if(client->groupspos != 0u) { // The client is not configured through the client table, return early return true; @@ -327,15 +327,15 @@ static bool get_client_groupids(clientsData* client) // There is a record for this client in the database const char* result = (const char*)sqlite3_column_text(table_stmt, 0); if(result != NULL) - client->groups = strdup(result); + client->groupspos = addstr(result); else - client->groups = strdup(""); + client->groupspos = addstr(""); } else if(rc == SQLITE_DONE) { // Found no record for this client in the database // -> No associated groups - client->groups = strdup(""); + client->groupspos = addstr(""); } else { @@ -421,7 +421,7 @@ bool gravityDB_prepare_client_statements(const int clientID, clientsData *client // Get associated groups for this client (if defined) char *querystr = NULL; - if(client->groups == NULL && !get_client_groupids(client)) + if(client->groupspos == 0u && !get_client_groupids(client)) return false; // Prepare whitelist statement @@ -432,7 +432,7 @@ bool gravityDB_prepare_client_statements(const int clientID, clientsData *client // of EXISTS(). if(config.debug & DEBUG_DATABASE) logg("gravityDB_open(): Preparing vw_whitelist statement for client %s", clientip); - querystr = get_client_querystr("vw_whitelist", client->groups); + querystr = get_client_querystr("vw_whitelist", getstr(client->groupspos)); sqlite3_stmt* stmt = NULL; int rc = sqlite3_prepare_v2(gravity_db, querystr, -1, &stmt, NULL); if( rc != SQLITE_OK ) @@ -447,7 +447,7 @@ bool gravityDB_prepare_client_statements(const int clientID, clientsData *client // Prepare gravity statement if(config.debug & DEBUG_DATABASE) logg("gravityDB_open(): Preparing vw_gravity statement for client %s", clientip); - querystr = get_client_querystr("vw_gravity", client->groups); + querystr = get_client_querystr("vw_gravity", getstr(client->groupspos)); rc = sqlite3_prepare_v2(gravity_db, querystr, -1, &stmt, NULL); if( rc != SQLITE_OK ) { @@ -461,7 +461,7 @@ bool gravityDB_prepare_client_statements(const int clientID, clientsData *client // Prepare blacklist statement if(config.debug & DEBUG_DATABASE) logg("gravityDB_open(): Preparing vw_blacklist statement for client %s", clientip); - querystr = get_client_querystr("vw_blacklist", client->groups); + querystr = get_client_querystr("vw_blacklist", getstr(client->groupspos)); rc = sqlite3_prepare_v2(gravity_db, querystr, -1, &stmt, NULL); if( rc != SQLITE_OK ) { @@ -496,14 +496,6 @@ static inline void gravityDB_finalize_client_statements(const int clientID) sqlite3_finalize(gravity_stmt->get(gravity_stmt, clientID)); gravity_stmt->set(gravity_stmt, clientID, NULL); } - - // Free group memory - clientsData* client = getClient(clientID, true); - if(client != NULL && client->groups != NULL) - { - free(client->groups); - client->groups = NULL; - } } // Close gravity database connection @@ -888,13 +880,14 @@ bool gravityDB_get_regex_client_groups(clientsData* client, const int numregex, gravityDB_check_fork(); char *querystr = NULL; - if(client->groups == NULL && !get_client_groupids(client)) + if(client->groupspos == 0u && !get_client_groupids(client)) return false; // Group filtering - if(asprintf(&querystr, "SELECT id from %s WHERE group_id IN (%s);", table, client->groups) < 1) + const char *groups = getstr(client->groupspos); + if(asprintf(&querystr, "SELECT id from %s WHERE group_id IN (%s);", table, groups) < 1) { - logg("gravityDB_get_regex_client_groups(%s, %s) - asprintf() error", table, client->groups); + logg("gravityDB_get_regex_client_groups(%s, %s) - asprintf() error", table, groups); return false; } diff --git a/src/datastructure.c b/src/datastructure.c index eb3adf08..fe1fb280 100644 --- a/src/datastructure.c +++ b/src/datastructure.c @@ -198,7 +198,7 @@ int findClientID(const char *clientIP, const bool count) client->lastQuery = 0; client->numQueriesARP = client->count; // Coonfigured groups are yet unknown - client->groups = NULL; + client->groupspos = 0u; // Initialize client-specific overTime data for(int i = 0; i < OVERTIME_SLOTS; i++) diff --git a/src/datastructure.h b/src/datastructure.h index b573f4b5..b40484b1 100644 --- a/src/datastructure.h +++ b/src/datastructure.h @@ -60,7 +60,7 @@ typedef struct { int blockedcount; int overTime[OVERTIME_SLOTS]; unsigned int numQueriesARP; - char *groups; + size_t groupspos; size_t ippos; size_t namepos; time_t lastQuery; From ed4e690245544eddf8f9ff0013905d781339f7ef Mon Sep 17 00:00:00 2001 From: DL6ER Date: Tue, 26 May 2020 00:15:36 +0200 Subject: [PATCH 04/18] Add boolean to be able to store if we decided which groups to be used (an empty string can actually mean no groups as a special case) Signed-off-by: DL6ER --- src/database/gravity-db.c | 22 +++++++++++++++++----- src/datastructure.c | 3 ++- src/datastructure.h | 1 + 3 files changed, 20 insertions(+), 6 deletions(-) diff --git a/src/database/gravity-db.c b/src/database/gravity-db.c index 79756013..fc874f63 100644 --- a/src/database/gravity-db.c +++ b/src/database/gravity-db.c @@ -203,6 +203,7 @@ static bool get_client_groupids(clientsData* client) { char *querystr = NULL; const char *ip = getstr(client->ippos); + client->found_group = false; client->groupspos = 0u; // Do not proceed when database is not available @@ -258,6 +259,7 @@ static bool get_client_groupids(clientsData* client) // Found no record for this client in the database // This makes this client qualify for the special "all" group client->groupspos = addstr("0"); + client->found_group = true; } else { @@ -273,7 +275,7 @@ static bool get_client_groupids(clientsData* client) free(querystr); querystr = NULL; - if(client->groupspos != 0u) + if(client->found_group) { // The client is not configured through the client table, return early return true; @@ -327,15 +329,17 @@ static bool get_client_groupids(clientsData* client) // There is a record for this client in the database const char* result = (const char*)sqlite3_column_text(table_stmt, 0); if(result != NULL) + { client->groupspos = addstr(result); - else - client->groupspos = addstr(""); + client->found_group = true; + } } else if(rc == SQLITE_DONE) { // Found no record for this client in the database // -> No associated groups client->groupspos = addstr(""); + client->found_group = true; } else { @@ -421,7 +425,7 @@ bool gravityDB_prepare_client_statements(const int clientID, clientsData *client // Get associated groups for this client (if defined) char *querystr = NULL; - if(client->groupspos == 0u && !get_client_groupids(client)) + if(!client->found_group && !get_client_groupids(client)) return false; // Prepare whitelist statement @@ -496,6 +500,14 @@ static inline void gravityDB_finalize_client_statements(const int clientID) sqlite3_finalize(gravity_stmt->get(gravity_stmt, clientID)); gravity_stmt->set(gravity_stmt, clientID, NULL); } + + // Unset group found property to trigger a check next time the + // client sends a query + clientsData* client = getClient(clientID, true); + if(client != NULL) + { + client->found_group = false; + } } // Close gravity database connection @@ -880,7 +892,7 @@ bool gravityDB_get_regex_client_groups(clientsData* client, const int numregex, gravityDB_check_fork(); char *querystr = NULL; - if(client->groupspos == 0u && !get_client_groupids(client)) + if(!client->found_group && !get_client_groupids(client)) return false; // Group filtering diff --git a/src/datastructure.c b/src/datastructure.c index fe1fb280..71bac34b 100644 --- a/src/datastructure.c +++ b/src/datastructure.c @@ -197,7 +197,8 @@ int findClientID(const char *clientIP, const bool count) // No query seen so far client->lastQuery = 0; client->numQueriesARP = client->count; - // Coonfigured groups are yet unknown + // Configured groups are yet unknown + client->found_group = false; client->groupspos = 0u; // Initialize client-specific overTime data diff --git a/src/datastructure.h b/src/datastructure.h index b40484b1..0cb068b1 100644 --- a/src/datastructure.h +++ b/src/datastructure.h @@ -56,6 +56,7 @@ typedef struct { typedef struct { unsigned char magic; bool new; + bool found_group; int count; int blockedcount; int overTime[OVERTIME_SLOTS]; From e11343bb3375e35971ac99a0b86b4664417d9b5f Mon Sep 17 00:00:00 2001 From: DL6ER Date: Tue, 26 May 2020 19:07:19 +0200 Subject: [PATCH 05/18] Fix broken RESOLVE_IPV{4,6} setting. Signed-off-by: DL6ER --- src/resolve.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/resolve.c b/src/resolve.c index b02ae411..390277a5 100644 --- a/src/resolve.c +++ b/src/resolve.c @@ -107,8 +107,8 @@ static char *resolveHostname(const char *addr) { logg(" ---> \"\" (configured to not resolve %s host names)", IPv6 ? "IPv6" : "IPv4"); - return strdup(""); } + return strdup(""); } // Initialize resolver subroutines if trying to resolve for the first time From 21f46bd45d3a79e241dafd179987d3ed4badec0e Mon Sep 17 00:00:00 2001 From: DL6ER Date: Tue, 26 May 2020 23:28:20 +0200 Subject: [PATCH 06/18] Clarify comment Signed-off-by: DL6ER --- src/database/gravity-db.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/database/gravity-db.c b/src/database/gravity-db.c index fc874f63..e95a40f4 100644 --- a/src/database/gravity-db.c +++ b/src/database/gravity-db.c @@ -277,7 +277,8 @@ static bool get_client_groupids(clientsData* client) if(client->found_group) { - // The client is not configured through the client table, return early + // The client is not configured through the client table, we + // substituted the default group. Return early here. return true; } From a0e0c4b31de9adfc6c54222a0a924f3c018c42df Mon Sep 17 00:00:00 2001 From: DL6ER Date: Wed, 27 May 2020 07:11:53 +0200 Subject: [PATCH 07/18] Also check REVOLCE_IPV{4,6} setting when trying to derive a host name from the FTL database. Signed-off-by: DL6ER --- src/database/network-table.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/database/network-table.c b/src/database/network-table.c index 9fca10b5..4b8def1c 100644 --- a/src/database/network-table.c +++ b/src/database/network-table.c @@ -895,6 +895,25 @@ void updateMACVendorRecords(void) char* __attribute__((malloc)) getDatabaseHostname(const char* ipaddr) { + // Test if this is an IPv6 address + bool IPv6 = false; + if(ipaddr != NULL && strstr(ipaddr,":") != NULL) + { + IPv6 = true; + } + + // Do we want to resolve IPv4/IPv6 names at all? + if( (IPv6 && !config.resolveIPv6) || + (!IPv6 && !config.resolveIPv4)) + { + if(config.debug & DEBUG_RESOLVER) + { + logg(" ---> \"\" (configured to not resolve %s host names)", + IPv6 ? "IPv6" : "IPv4"); + } + return strdup(""); + } + // Open pihole-FTL.db database file if(!dbopen()) { From 041f93f006376a00d787d1570583ffcf7ca6fc9e Mon Sep 17 00:00:00 2001 From: DL6ER Date: Wed, 27 May 2020 19:02:25 +0200 Subject: [PATCH 08/18] Improve build.sh script. Add "install" and "clean" targets. Also ensure successive builds are possible to speed up the entire process. Signed-off-by: DL6ER --- build.sh | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/build.sh b/build.sh index 43716660..00e379a7 100755 --- a/build.sh +++ b/build.sh @@ -9,9 +9,27 @@ # This file is copyright under the latest version of the EUPL. # Please see LICENSE file for your rights under this license. -rm -rf cmake/ && \ -mkdir cmake && \ -cd cmake && \ -cmake .. && \ -cmake --build . -- -j $(nproc) && \ -cp pihole-FTL ../ +# Abort script if one command returns a non-zero value +set -e + +# Prepare build environment +if [[ "${1}" == "clean" ]]; then + rm -rf cmake/ + exit 0 +fi + +# Configure build +mkdir -p cmake +cd cmake +cmake .. + +# Build the sources +cmake --build . -- -j $(nproc) + +# If we are asked to install, we do this here +# Otherwise, we simply copy the binary one level up +if [[ "${1}" == "install" ]]; then + sudo make install +else + cp pihole-FTL ../ +fi From dadc1cc98184d9eea0197a4f782f20350197f034 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Wed, 27 May 2020 20:12:24 +0200 Subject: [PATCH 09/18] Allow no/false and yes/true for all config options. Signed-off-by: DL6ER --- src/config.c | 60 +++++++++++++++++++++++++--------------------------- 1 file changed, 29 insertions(+), 31 deletions(-) diff --git a/src/config.c b/src/config.c index e4b64ab6..24fd92ea 100644 --- a/src/config.c +++ b/src/config.c @@ -39,6 +39,7 @@ static size_t size = 0; static char *parse_FTLconf(FILE *fp, const char * key); static void release_config_memory(void); static void getpath(FILE* fp, const char *option, const char *defaultloc, char **pointer); +static bool read_bool(const char *option, const bool fallback); void getLogFilePath(void) { @@ -113,11 +114,8 @@ void read_FTLconf(void) // AAAA_QUERY_ANALYSIS // defaults to: Yes - config.analyze_AAAA = true; buffer = parse_FTLconf(fp, "AAAA_QUERY_ANALYSIS"); - - if(buffer != NULL && strcasecmp(buffer, "no") == 0) - config.analyze_AAAA = false; + config.analyze_AAAA = read_bool(buffer, true); if(config.analyze_AAAA) logg(" AAAA_QUERY_ANALYSIS: Show AAAA queries"); @@ -141,11 +139,8 @@ void read_FTLconf(void) // RESOLVE_IPV6 // defaults to: Yes - config.resolveIPv6 = true; buffer = parse_FTLconf(fp, "RESOLVE_IPV6"); - - if(buffer != NULL && strcasecmp(buffer, "no") == 0) - config.resolveIPv6 = false; + config.resolveIPv6 = read_bool(buffer, true); if(config.resolveIPv6) logg(" RESOLVE_IPV6: Resolve IPv6 addresses"); @@ -154,10 +149,9 @@ void read_FTLconf(void) // RESOLVE_IPV4 // defaults to: Yes - config.resolveIPv4 = true; buffer = parse_FTLconf(fp, "RESOLVE_IPV4"); - if(buffer != NULL && strcasecmp(buffer, "no") == 0) - config.resolveIPv4 = false; + config.resolveIPv4 = read_bool(buffer, true); + if(config.resolveIPv4) logg(" RESOLVE_IPV4: Resolve IPv4 addresses"); else @@ -245,8 +239,8 @@ void read_FTLconf(void) // IGNORE_LOCALHOST // defaults to: false - config.ignore_localhost = false; buffer = parse_FTLconf(fp, "IGNORE_LOCALHOST"); + config.ignore_localhost = read_bool(buffer, false); if(buffer != NULL && strcasecmp(buffer, "yes") == 0) config.ignore_localhost = true; @@ -280,8 +274,8 @@ void read_FTLconf(void) // ANALYZE_ONLY_A_AND_AAAA // defaults to: false - config.analyze_only_A_AAAA = false; buffer = parse_FTLconf(fp, "ANALYZE_ONLY_A_AND_AAAA"); + config.analyze_only_A_AAAA = read_bool(buffer, false); if(buffer != NULL && strcasecmp(buffer, "true") == 0) config.analyze_only_A_AAAA = true; @@ -293,10 +287,9 @@ void read_FTLconf(void) // DBIMPORT // defaults to: Yes - config.DBimport = true; buffer = parse_FTLconf(fp, "DBIMPORT"); - if(buffer != NULL && strcasecmp(buffer, "no") == 0) - config.DBimport = false; + config.DBimport = read_bool(buffer, true); + if(config.DBimport) logg(" DBIMPORT: Importing history from database"); else @@ -322,11 +315,8 @@ void read_FTLconf(void) // PARSE_ARP_CACHE // defaults to: true - config.parse_arp_cache = true; buffer = parse_FTLconf(fp, "PARSE_ARP_CACHE"); - - if(buffer != NULL && strcasecmp(buffer, "false") == 0) - config.parse_arp_cache = false; + config.parse_arp_cache = read_bool(buffer, true); if(config.parse_arp_cache) logg(" PARSE_ARP_CACHE: Active"); @@ -335,11 +325,8 @@ void read_FTLconf(void) // CNAME_DEEP_INSPECT // defaults to: true - config.cname_inspection = true; buffer = parse_FTLconf(fp, "CNAME_DEEP_INSPECT"); - - if(buffer != NULL && strcasecmp(buffer, "false") == 0) - config.cname_inspection = false; + config.cname_inspection = read_bool(buffer, true); if(config.cname_inspection) logg(" CNAME_DEEP_INSPECT: Active"); @@ -353,19 +340,14 @@ void read_FTLconf(void) config.delay_startup = 0; if(buffer != NULL && sscanf(buffer, "%u", &config.delay_startup) && (config.delay_startup > 0 && config.delay_startup <= 300)) - { logg(" DELAY_STARTUP: Requested to wait %u seconds during startup.", config.delay_startup); - } else logg(" DELAY_STARTUP: No delay requested."); // BLOCK_ESNI // defaults to: true - config.block_esni = true; buffer = parse_FTLconf(fp, "BLOCK_ESNI"); - - if(buffer != NULL && strcasecmp(buffer, "false") == 0) - config.block_esni = false; + config.block_esni = read_bool(buffer, true); if(config.block_esni) logg(" BLOCK_ESNI: Enabled, blocking _esni.{blocked domain}"); @@ -562,7 +544,7 @@ static void setDebugOption(FILE* fp, const char* option, int16_t bitmask) return; // Set bit if value equals "true", clear bit otherwise - if(strcasecmp(buffer, "true") == 0) + if(read_bool(buffer, false)) config.debug |= bitmask; else config.debug &= ~bitmask; @@ -689,3 +671,19 @@ void read_debuging_settings(FILE *fp) release_config_memory(); } } + +static bool read_bool(const char *option, const bool fallback) +{ + if(option == NULL) + return fallback; + + else if(strcasecmp(option, "false") == 0 || + strcasecmp(option, "no") == 0) + return false; + + else if(strcasecmp(option, "true") == 0 || + strcasecmp(option, "yes") == 0) + return true; + + return fallback; +} From ee0790ad65acb0d52cf5923459ae2b9c0021d936 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Wed, 27 May 2020 20:30:28 +0200 Subject: [PATCH 10/18] Re-aquire client and upstream pointers after a name resolution. As we're leaving the locked area for the resolve, we cannot control if the shared memory object changed meanwhile. If it did, then the pointers will point into nowhere, leading to a SEGV_MAPERR. Signed-off-by: DL6ER --- src/resolve.c | 38 +++++++++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 7 deletions(-) diff --git a/src/resolve.c b/src/resolve.c index b02ae411..efe19dfc 100644 --- a/src/resolve.c +++ b/src/resolve.c @@ -302,17 +302,17 @@ void resolveClients(const bool onlynew) int skipped = 0; for(int clientID = 0; clientID < clientscount; clientID++) { - // Get client pointer + // Memory access needs to get locked + lock_shm(); + // Get client pointer for the first time (reading data) clientsData* client = getClient(clientID, true); if(client == NULL) { - logg("ERROR: Unable to get client pointer with ID %i, skipping...", clientID); + logg("ERROR: Unable to get client pointer (1) with ID %i, skipping...", clientID); skipped++; continue; } - // Memory access needs to get locked - lock_shm(); bool newflag = client->new; size_t ippos = client->ippos; size_t oldnamepos = client->namepos; @@ -330,6 +330,18 @@ void resolveClients(const bool onlynew) size_t newnamepos = resolveAndAddHostname(ippos, oldnamepos); lock_shm(); + // Get client pointer for the second time (writing data) + // We cannot use the same pointer again as we released + // the lock in between so we cannot know if something + // happened to the shared memory object (resize event) + client = getClient(clientID, true); + if(client == NULL) + { + logg("ERROR: Unable to get client pointer (2) with ID %i, skipping...", clientID); + skipped++; + continue; + } + // Store obtained host name (may be unchanged) client->namepos = newnamepos; // Mark entry as not new @@ -355,7 +367,9 @@ void resolveForwardDestinations(const bool onlynew) int skipped = 0; for(int upstreamID = 0; upstreamID < upstreams; upstreamID++) { - // Get upstream pointer + // Memory access needs to get locked + lock_shm(); + // Get upstream pointer for the first time (reading data) upstreamsData* upstream = getUpstream(upstreamID, true); if(upstream == NULL) { @@ -364,8 +378,6 @@ void resolveForwardDestinations(const bool onlynew) continue; } - // Memory access needs to get locked - lock_shm(); bool newflag = upstream->new; size_t ippos = upstream->ippos; size_t oldnamepos = upstream->namepos; @@ -383,6 +395,18 @@ void resolveForwardDestinations(const bool onlynew) size_t newnamepos = resolveAndAddHostname(ippos, oldnamepos); lock_shm(); + // Get upstream pointer for the second time (writing data) + // We cannot use the same pointer again as we released + // the lock in between so we cannot know if something + // happened to the shared memory object (resize event) + upstream = getUpstream(upstreamID, true); + if(upstream == NULL) + { + logg("ERROR: Unable to get upstream pointer with ID %i, skipping...", upstreamID); + skipped++; + continue; + } + // Store obtained host name (may be unchanged) upstream->namepos = newnamepos; // Mark entry as not new From c6b9f1d0d327958743d39727de00017e7c106961 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Thu, 28 May 2020 20:19:20 +0200 Subject: [PATCH 11/18] Set nice value of pihole-FTL (configurable) to increase DNS server performance. Signed-off-by: DL6ER --- src/capabilities.c | 8 +++++++ src/config.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+) diff --git a/src/capabilities.c b/src/capabilities.c index 609a1d24..6478526c 100644 --- a/src/capabilities.c +++ b/src/capabilities.c @@ -100,6 +100,14 @@ bool check_capabilities(void) logg("*************************************************************************"); capabilities_okay = false; } + if (!(data->permitted & (1 << CAP_SYS_NICE))) + { + // Necessary for dynamic port binding + logg("*************************************************************************"); + logg("* WARNING: Required Linux capability CAP_SYS_NICE not available *"); + logg("*************************************************************************"); + capabilities_okay = false; + } // Free allocated memory free(hdr); diff --git a/src/config.c b/src/config.c index e4b64ab6..65103a07 100644 --- a/src/config.c +++ b/src/config.c @@ -13,6 +13,8 @@ #include "memory.h" #include "setupVars.h" #include "log.h" +// nice() +#include ConfigStruct config; FTLFileNamesStruct FTLfiles = { @@ -39,6 +41,7 @@ static size_t size = 0; static char *parse_FTLconf(FILE *fp, const char * key); static void release_config_memory(void); static void getpath(FILE* fp, const char *option, const char *defaultloc, char **pointer); +static void set_nice(const char *buffer, int fallback); void getLogFilePath(void) { @@ -367,6 +370,20 @@ void read_FTLconf(void) if(buffer != NULL && strcasecmp(buffer, "false") == 0) config.block_esni = false; + // NICE + // Shall we change the nice of the current process? + // defaults to: -10 (can be disabled by setting value to -999) + // + // The nice value is an attribute that can be used to influence the CPU + // scheduler to favor or disfavor a process in scheduling decisions. + // + // The range of the nice value varies across UNIX systems. On modern Linux, + // the range is -20 (high priority) to +19 (low priority). On some other + // systems, the range is -20..20. Very early Linux kernels (Before Linux + // 2.0) had the range -infinity..15. + buffer = parse_FTLconf(fp, "NICE"); + set_nice(buffer, -10); + if(config.block_esni) logg(" BLOCK_ESNI: Enabled, blocking _esni.{blocked domain}"); else @@ -689,3 +706,45 @@ void read_debuging_settings(FILE *fp) release_config_memory(); } } + +static void set_nice(const char *buffer, const int fallback) +{ + int value, nice_set, nice_target = fallback; + + // Try to read niceness value + // Attempts to set a nice value outside the range are clamped to the range. + if(buffer != NULL && sscanf(buffer, "%i", &value) == 1) + nice_target = value; + + // Skip setting niceness if set to -999 + if(nice_target == -999) + { + logg(" NICE: Not changing nice value"); + return; + } + + // Adjust if != -999 + errno = 0; + if((nice_set = nice(nice_target)) == -1 && + errno == EPERM) + { + // ERROR EPERM: The calling process attempted to increase its priority + // by supplying a negative value but has insufficient privileges. + // On Linux, the RLIMIT_NICE resource limit can be used to define a limit to + // which an unprivileged process's nice value can be raised. We are not + // affected by this limit when pihole-FTL is running with CAP_SYS_NICE + logg(" NICE: Cannot change niceness to %d (permission denied)", + nice_target); + return; + } + if(nice_set == nice_target) + { + logg(" NICE: Set process niceness to %d%s", + nice_set, (nice_set == fallback) ? " (default)" : ""); + } + else + { + logg(" NICE: Set process niceness to %d (asked for %d)", + nice_set, nice_target); + } +} From 0c1ab0f7aa500e292d56c485057cea744e7f1062 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Thu, 28 May 2020 20:50:18 +0200 Subject: [PATCH 12/18] Add CAP_SYS_NICE for the tests Signed-off-by: DL6ER --- test/run.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/run.sh b/test/run.sh index bf64b9f5..747edf20 100755 --- a/test/run.sh +++ b/test/run.sh @@ -23,7 +23,7 @@ chown pihole:pihole /etc/pihole /run/pihole /var/log/pihole.log /var/log/pihole- cp ./pihole-FTL /home/pihole chmod +x /home/pihole/pihole-FTL # Note: We cannot add CAP_NET_RAW and CAP_NET_ADMIN at this point -setcap CAP_NET_BIND_SERVICE+eip /home/pihole/pihole-FTL +setcap CAP_NET_BIND_SERVICE,CAP_SYS_NICE+eip /home/pihole/pihole-FTL # Prepare gravity database sqlite3 /etc/pihole/gravity.db < test/gravity.db.sql From 5970c0577f3d1085734831d17be434a31bcae99e Mon Sep 17 00:00:00 2001 From: DL6ER Date: Thu, 28 May 2020 20:55:10 +0200 Subject: [PATCH 13/18] Ignore missing CAP_SYS_NICE in the CI tests as we are not allowed to change the nicencess. Signed-off-by: DL6ER --- test/run.sh | 2 +- test/test_suite.bats | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/run.sh b/test/run.sh index 747edf20..bf64b9f5 100755 --- a/test/run.sh +++ b/test/run.sh @@ -23,7 +23,7 @@ chown pihole:pihole /etc/pihole /run/pihole /var/log/pihole.log /var/log/pihole- cp ./pihole-FTL /home/pihole chmod +x /home/pihole/pihole-FTL # Note: We cannot add CAP_NET_RAW and CAP_NET_ADMIN at this point -setcap CAP_NET_BIND_SERVICE,CAP_SYS_NICE+eip /home/pihole/pihole-FTL +setcap CAP_NET_BIND_SERVICE+eip /home/pihole/pihole-FTL # Prepare gravity database sqlite3 /etc/pihole/gravity.db < test/gravity.db.sql diff --git a/test/test_suite.bats b/test/test_suite.bats index ff662ab8..4285c80b 100644 --- a/test/test_suite.bats +++ b/test/test_suite.bats @@ -358,7 +358,7 @@ } @test "No WARNING messages in pihole-FTL.log (besides known capability issues)" { - run bash -c 'grep "WARNING:" /var/log/pihole-FTL.log | grep -c -v -E "CAP_NET_ADMIN|CAP_NET_RAW"' + run bash -c 'grep "WARNING:" /var/log/pihole-FTL.log | grep -c -v -E "CAP_NET_ADMIN|CAP_NET_RAW|CAP_SYS_NICE"' printf "%s\n" "${lines[@]}" [[ ${lines[0]} == "0" ]] } From 3f739f82f7986ae29cf937cf366a3de8e0c387ab Mon Sep 17 00:00:00 2001 From: DL6ER Date: Sat, 23 May 2020 10:25:11 +0200 Subject: [PATCH 14/18] Add warning for invalid hostnames to FTL message table. Signed-off-by: DL6ER --- src/database/message-table.c | 21 +++++++++++++++++++-- src/database/message-table.h | 3 ++- src/resolve.c | 9 ++++++--- 3 files changed, 27 insertions(+), 6 deletions(-) diff --git a/src/database/message-table.c b/src/database/message-table.c index e0a783bd..c4251ea9 100644 --- a/src/database/message-table.c +++ b/src/database/message-table.c @@ -16,7 +16,7 @@ #include "database/gravity-db.h" static const char *message_types[MAX_MESSAGE] = - { "REGEX", "SUBNET" }; + { "REGEX", "SUBNET", "HOSTNAME" }; static unsigned char message_blob_types[MAX_MESSAGE][5] = { @@ -33,6 +33,13 @@ static unsigned char message_blob_types[MAX_MESSAGE][5] = SQLITE_TEXT, // comma-separated list of matching subnets (database IDs) SQLITE_TEXT, // chosen subnet (text representation) SQLITE_INTEGER // chosen subnet (database ID) + }, + { // HOSTNAME_MESSAGE: The message column contains the IP address of the device + SQLITE_TEXT, // Obtained host name + SQLITE_INTEGER, // Position of error in string + SQLITE_NULL, // not used + SQLITE_NULL, // not used + SQLITE_NULL // not used } }; // Create message table in the database @@ -186,4 +193,14 @@ void logg_subnet_warning(const char *ip, const int matching_count, const char *m char *names = get_group_names(matching_ids); add_message(SUBNET_MESSAGE, ip, 5, matching_count, names, matching_ids, chosen_match_text, chosen_match_id); free(names); -} \ No newline at end of file +} + +void logg_hostname_warning(const char *ip, const char *name, const unsigned int pos) +{ + // Log to pihole-FTL.log + logg("HOSTNAME WARNING: Host name of client \"%s\" => \"%s\" contains invalid character at position %d", + ip, name, pos); + + // Log to database + add_message(HOSTNAME_MESSAGE, ip, 2, name, (const int)pos); +} diff --git a/src/database/message-table.h b/src/database/message-table.h index bcd3a986..e3c32feb 100644 --- a/src/database/message-table.h +++ b/src/database/message-table.h @@ -16,7 +16,8 @@ void logg_regex_warning(const char *type, const char *warning, const int dbindex void logg_subnet_warning(const char *ip, const int matching_count, const char *matching_ids, const int matching_bits, const char *chosen_match_text, const int chosen_match_id); +void logg_hostname_warning(const char *ip, const char *name, const unsigned int pos); -enum message_type { REGEX_MESSAGE, SUBNET_MESSAGE, MAX_MESSAGE }; +enum message_type { REGEX_MESSAGE, SUBNET_MESSAGE, HOSTNAME_MESSAGE, MAX_MESSAGE }; #endif //MESSAGETABLE_H diff --git a/src/resolve.c b/src/resolve.c index 58d850e7..03051880 100644 --- a/src/resolve.c +++ b/src/resolve.c @@ -22,6 +22,8 @@ #include "database/network-table.h" // struct _res #include +// logg_hostname_warning() +#include "database/message-table.h" static bool res_initialized = false; @@ -45,8 +47,10 @@ static bool valid_hostname(char* name, const char* clientip) // Iterate over characters in hostname // to check for legal char: A-Z a-z 0-9 - _ . - for (char c; (c = *name); name++) + unsigned int len = strlen(name); + for (unsigned int i = 0; i < len; i++) { + const char c = name[i]; if ((c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z') || (c >= '0' && c <= '9') || @@ -56,8 +60,7 @@ static bool valid_hostname(char* name, const char* clientip) continue; // Invalid character found, log and return hostname being invalid - logg("WARN: Hostname of client %s contains invalid character: %c (char code %d)", - clientip, (unsigned char)c, (unsigned char)c); + logg_hostname_warning(clientip, name, i); return false; } From 2fcb2bdf534bf7d95627d00b9ac8d44cf15756e0 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Sat, 23 May 2020 23:57:37 +0200 Subject: [PATCH 15/18] Only open FTL database for storing a message when there is not already an open connection. Signed-off-by: DL6ER --- src/database/common.c | 10 ++++++++++ src/database/common.h | 1 + src/database/message-table.c | 15 +++++++++++---- 3 files changed, 22 insertions(+), 4 deletions(-) diff --git a/src/database/common.c b/src/database/common.c index af6cac18..ddffa2e5 100644 --- a/src/database/common.c +++ b/src/database/common.c @@ -24,9 +24,15 @@ sqlite3 *FTL_db = NULL; bool database = true; bool DBdeleteoldqueries = false; long int lastdbindex = 0; +static bool db_avail = false; static pthread_mutex_t dblock; +__attribute__ ((pure)) bool FTL_DB_avail(void) +{ + return db_avail; +} + void dbclose(void) { // Only try to close an existing database connection @@ -37,6 +43,8 @@ void dbclose(void) FTL_db = NULL; } + db_avail = false; + // Report any error if( rc != SQLITE_OK ) { @@ -86,6 +94,8 @@ bool dbopen(void) return false; } + db_avail = true; + return true; } diff --git a/src/database/common.h b/src/database/common.h index 73273b33..64d9d967 100644 --- a/src/database/common.h +++ b/src/database/common.h @@ -19,6 +19,7 @@ bool db_set_FTL_property(const unsigned int ID, const int value); /// Execute a formatted SQL query and get the return code int dbquery(const char *format, ...); +bool FTL_DB_avail(void) __attribute__ ((pure)); bool dbopen(void); void dbclose(void); int db_query_int(const char*); diff --git a/src/database/message-table.c b/src/database/message-table.c index c4251ea9..0f0a84d6 100644 --- a/src/database/message-table.c +++ b/src/database/message-table.c @@ -84,8 +84,14 @@ bool flush_message_table(void) static bool add_message(enum message_type type, const char *message, const int count,...) { - // Open database connection - dbopen(); + bool opened_database = false; + // Open database connection (if not already open) + if(!FTL_DB_avail()) + { + if(!dbopen()) + return false; + opened_database = true; + } // Prepare SQLite statement sqlite3_stmt* stmt = NULL; @@ -163,8 +169,9 @@ static bool add_message(enum message_type type, const char *message, return false; } - // Close database connection - dbclose(); + // Close database connection (if we opened it) + if(opened_database) + dbclose(); return true; } From 6ae4bb999b7c01f886cd408b69be8a37ef2423e7 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Sun, 24 May 2020 11:01:49 +0200 Subject: [PATCH 16/18] Clarify warning that the check found AT LEAST one invalid character. Signed-off-by: DL6ER --- src/database/message-table.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/database/message-table.c b/src/database/message-table.c index 0f0a84d6..20db4120 100644 --- a/src/database/message-table.c +++ b/src/database/message-table.c @@ -205,7 +205,7 @@ void logg_subnet_warning(const char *ip, const int matching_count, const char *m void logg_hostname_warning(const char *ip, const char *name, const unsigned int pos) { // Log to pihole-FTL.log - logg("HOSTNAME WARNING: Host name of client \"%s\" => \"%s\" contains invalid character at position %d", + logg("HOSTNAME WARNING: Host name of client \"%s\" => \"%s\" contains (at least) one invalid character at position %d", ip, name, pos); // Log to database From 0b92f53dde544ab6069b298ce819e140433323a1 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Tue, 26 May 2020 18:54:11 +0200 Subject: [PATCH 17/18] Ensure host name errors do not accumulate. Signed-off-by: DL6ER --- src/database/message-table.c | 59 ++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/src/database/message-table.c b/src/database/message-table.c index 20db4120..13be7ac5 100644 --- a/src/database/message-table.c +++ b/src/database/message-table.c @@ -93,6 +93,65 @@ static bool add_message(enum message_type type, const char *message, opened_database = true; } + // Ensure there are no duplicates when adding host name messages + if(type == HOSTNAME_MESSAGE) + { + sqlite3_stmt* stmt = NULL; + const char *querystr = "DELETE FROM message WHERE type = ?1 AND message = ?2"; + int rc = sqlite3_prepare_v2(FTL_db, querystr, -1, &stmt, NULL); + if( rc != SQLITE_OK ){ + logg("add_message(type=%u, message=%s) - SQL error prepare DELETE: %s", + type, message, sqlite3_errstr(rc)); + return false; + } + + // Bind type to prepared statement + if((rc = sqlite3_bind_text(stmt, 1, message_types[type], -1, SQLITE_STATIC)) != SQLITE_OK) + { + logg("add_message(type=%u, message=%s) - Failed to bind type DELETE: %s", + type, message, sqlite3_errstr(rc)); + sqlite3_reset(stmt); + sqlite3_finalize(stmt); + return false; + } + + // Bind message to prepared statement + if((rc = sqlite3_bind_text(stmt, 2, message, -1, SQLITE_STATIC)) != SQLITE_OK) + { + logg("add_message(type=%u, message=%s) - Failed to bind message DELETE: %s", + type, message, sqlite3_errstr(rc)); + sqlite3_reset(stmt); + sqlite3_finalize(stmt); + return false; + } + + // Execute and finalize + if((rc = sqlite3_step(stmt)) != SQLITE_OK && rc != SQLITE_DONE) + { + logg("add_message(type=%u, message=%s) - SQL error step DELETE: %s", + type, message, sqlite3_errstr(rc)); + return false; + } + if((rc = sqlite3_clear_bindings(stmt)) != SQLITE_OK) + { + logg("add_message(type=%u, message=%s) - SQL error clear DELETE: %s", + type, message, sqlite3_errstr(rc)); + return false; + } + if((rc = sqlite3_reset(stmt)) != SQLITE_OK) + { + logg("add_message(type=%u, message=%s) - SQL error reset DELETE: %s", + type, message, sqlite3_errstr(rc)); + return false; + } + if((rc = sqlite3_finalize(stmt)) != SQLITE_OK) + { + logg("add_message(type=%u, message=%s) - SQL error finalize DELETE: %s", + type, message, sqlite3_errstr(rc)); + return false; + } + } + // Prepare SQLite statement sqlite3_stmt* stmt = NULL; const char *querystr = "INSERT INTO message (timestamp,type,message,blob1,blob2,blob3,blob4,blob5) " From 5ccc18fbc1a87f20e2df2b13111cc5d96374012d Mon Sep 17 00:00:00 2001 From: DL6ER Date: Tue, 26 May 2020 20:18:44 +0200 Subject: [PATCH 18/18] Close database on any erros to ensure nothing stays locked. Signed-off-by: DL6ER --- src/database/message-table.c | 40 +++++++++++++++++++----------------- 1 file changed, 21 insertions(+), 19 deletions(-) diff --git a/src/database/message-table.c b/src/database/message-table.c index 13be7ac5..04d7a2cc 100644 --- a/src/database/message-table.c +++ b/src/database/message-table.c @@ -102,6 +102,8 @@ static bool add_message(enum message_type type, const char *message, if( rc != SQLITE_OK ){ logg("add_message(type=%u, message=%s) - SQL error prepare DELETE: %s", type, message, sqlite3_errstr(rc)); + if(opened_database) + dbclose(); return false; } @@ -112,6 +114,8 @@ static bool add_message(enum message_type type, const char *message, type, message, sqlite3_errstr(rc)); sqlite3_reset(stmt); sqlite3_finalize(stmt); + if(opened_database) + dbclose(); return false; } @@ -122,6 +126,8 @@ static bool add_message(enum message_type type, const char *message, type, message, sqlite3_errstr(rc)); sqlite3_reset(stmt); sqlite3_finalize(stmt); + if(opened_database) + dbclose(); return false; } @@ -130,26 +136,13 @@ static bool add_message(enum message_type type, const char *message, { logg("add_message(type=%u, message=%s) - SQL error step DELETE: %s", type, message, sqlite3_errstr(rc)); + if(opened_database) + dbclose(); return false; } - if((rc = sqlite3_clear_bindings(stmt)) != SQLITE_OK) - { - logg("add_message(type=%u, message=%s) - SQL error clear DELETE: %s", - type, message, sqlite3_errstr(rc)); - return false; - } - if((rc = sqlite3_reset(stmt)) != SQLITE_OK) - { - logg("add_message(type=%u, message=%s) - SQL error reset DELETE: %s", - type, message, sqlite3_errstr(rc)); - return false; - } - if((rc = sqlite3_finalize(stmt)) != SQLITE_OK) - { - logg("add_message(type=%u, message=%s) - SQL error finalize DELETE: %s", - type, message, sqlite3_errstr(rc)); - return false; - } + sqlite3_clear_bindings(stmt); + sqlite3_reset(stmt); + sqlite3_finalize(stmt); } // Prepare SQLite statement @@ -157,9 +150,12 @@ static bool add_message(enum message_type type, const char *message, const char *querystr = "INSERT INTO message (timestamp,type,message,blob1,blob2,blob3,blob4,blob5) " "VALUES ((cast(strftime('%s', 'now') as int)),?,?,?,?,?,?,?);"; int rc = sqlite3_prepare_v2(FTL_db, querystr, -1, &stmt, NULL); - if( rc != SQLITE_OK ){ + if( rc != SQLITE_OK ) + { logg("add_message(type=%u, message=%s) - SQL error prepare: %s", type, message, sqlite3_errstr(rc)); + if(opened_database) + dbclose(); return false; } @@ -170,6 +166,8 @@ static bool add_message(enum message_type type, const char *message, type, message, sqlite3_errstr(rc)); sqlite3_reset(stmt); sqlite3_finalize(stmt); + if(opened_database) + dbclose(); return false; } @@ -180,6 +178,8 @@ static bool add_message(enum message_type type, const char *message, type, message, sqlite3_errstr(rc)); sqlite3_reset(stmt); sqlite3_finalize(stmt); + if(opened_database) + dbclose(); return false; } @@ -211,6 +211,8 @@ static bool add_message(enum message_type type, const char *message, type, message, 3 + j, datatype, sqlite3_errstr(rc)); sqlite3_reset(stmt); sqlite3_finalize(stmt); + if(opened_database) + dbclose(); return false; } }