[PATCH 0/4] Added captive portal possibility

[Craig McQueen]

The following patches are related to the captive portal code originally
provided by Alexandre Chataignon. The first patch is a copy of the
original patch, provided again as a reference. The 3 following patches
are improvements I made following a review of Alexandre's patch, to
improve the following:

* Check that the query first question is for an A record, and only
send an A record if so.
* Handle the case of a query containing additional records (don't
send a corrupt response).
* For queries from a loopack IP address, don't return the captive
address, but do the normal DNS handling.

Alexandre Chataignon (1):
dnsproxy: Added captive portal possibility

Craig McQueen (3):
dnsproxy: Fixes to captive portal implementation.
Add functions to check if an IP address is loopback or link-local.
dnsproxy: don't return captive address for queries from loopback IP

src/connman.h | 6 ++-
src/dnsproxy.c | 142 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
src/inet.c | 58 ++++++++++++++++-------
src/technology.c | 7 ++-
src/tethering.c | 22 ++++++++-
5 files changed, 215 insertions(+), 20 deletions(-)

[Craig McQueen]

From: Alexandre Chataignon <***@mobirider.com>

---

This is Alexandre's original patch. I have only changed the title
slightly.

src/connman.h | 4 ++-
src/dnsproxy.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
src/technology.c | 7 ++++-
src/tethering.c | 22 ++++++++++++++--
4 files changed, 108 insertions(+), 4 deletions(-)

diff --git a/src/connman.h b/src/connman.h
index 35eb3f5..7b3265f 100644
--- a/src/connman.h
+++ b/src/connman.h
@@ -619,7 +619,7 @@ int __connman_tethering_init(void);
void __connman_tethering_cleanup(void);

const char *__connman_tethering_get_bridge(void);
-void __connman_tethering_set_enabled(void);
+void __connman_tethering_set_enabled(bool captive_dns);
void __connman_tethering_set_disabled(void);

int __connman_private_network_request(DBusMessage *msg, const char *owner);
@@ -930,6 +930,8 @@ int __connman_dnsproxy_add_listener(int index);
void __connman_dnsproxy_remove_listener(int index);
int __connman_dnsproxy_append(int index, const char *domain, const char *server);
int __connman_dnsproxy_remove(int index, const char *domain, const char *server);
+int __connman_dnsproxy_enable_captive(uint32_t ip) ;
+int __connman_dnsproxy_disable_captive(void) ;

int __connman_6to4_probe(struct connman_service *service);
void __connman_6to4_remove(struct connman_ipconfig *ipconfig);
diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index a1eda55..f72b5cc 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -76,6 +76,14 @@ struct domain_hdr {
#else
#error "Unknown byte order"
#endif
+struct a_dns_answer {
+ uint16_t ptr;
+ uint16_t type;
+ uint16_t class;
+ uint32_t ttl;
+ uint16_t rdlength;
+ uint32_t ip;
+} __attribute__ ((packed)) ;

struct partial_reply {
uint16_t len;
@@ -216,6 +224,7 @@ static GHashTable *listener_table = NULL;
static time_t next_refresh;
static GHashTable *partial_tcp_req_table;
static guint cache_timer = 0;
+static uint32_t captive_ip = 0;

static guint16 get_id(void)
{
@@ -502,6 +511,58 @@ static void send_response(int sk, unsigned char *buf, int len,
}
}

+static void send_response_A(int sk, unsigned char *req, int len,
+ uint32_t ip,
+ const struct sockaddr *to, socklen_t tolen,
+ int protocol)
+{
+ struct domain_hdr *hdr;
+ struct a_dns_answer* ans;
+ int err, offset = protocol_offset(protocol);
+
+ DBG("sk %d", sk);
+
+ if (offset < 0)
+ return;
+
+ if (len < 12)
+ return;
+
+ size_t new_len = len+sizeof(struct a_dns_answer) ;
+ char* new_buf ;
+ new_buf = g_malloc(new_len) ;
+ memcpy(new_buf, req, len) ;
+
+ hdr = (void *) (new_buf + offset);
+
+ DBG("id 0x%04x qr %d opcode %d", hdr->id, hdr->qr, hdr->opcode);
+
+ hdr->qr = 1;
+ hdr->rcode = ns_r_noerror;
+
+ hdr->ancount = htons(1);
+ hdr->nscount = 0;
+ hdr->arcount = 0;
+
+ /* Create A answer */
+ /* Other attributes */
+ ans = (void *) (new_buf + len);
+ ans->ptr = htons(0xc00c) ;/* 11....1100 -> ptr to 12th octet = query */
+ ans->type = htons(ns_t_a);
+ ans->class = htons(ns_c_in);
+ ans->ttl = 0;
+ ans->rdlength = htons(4);
+ ans->ip = htonl(ip);
+
+ err = sendto(sk, new_buf, new_len, MSG_NOSIGNAL, to, tolen);
+ g_free(new_buf) ;
+ if (err < 0) {
+ connman_error("Failed to send DNS response to %d: %s",
+ sk, strerror(errno));
+ return;
+ }
+}
+
static int get_req_udp_socket(struct request_data *req)
{
GIOChannel *channel;
@@ -3429,6 +3490,14 @@ static bool udp_listener_event(GIOChannel *channel, GIOCondition condition,

DBG("Received %d bytes (id 0x%04x)", len, buf[0] | buf[1] << 8);

+ /* If captive mode, all is redirected to tether ip */
+ if (captive_ip) {
+ send_response_A(sk, buf, len,
+ captive_ip,
+ client_addr, *client_addr_len, IPPROTO_UDP);
+ return true ;
+ }
+
err = parse_request(buf, len, query, sizeof(query));
if (err < 0 || (g_slist_length(server_list) == 0)) {
send_response(sk, buf, len, client_addr,
@@ -3885,3 +3954,13 @@ void __connman_dnsproxy_cleanup(void)

g_hash_table_destroy(partial_tcp_req_table);
}
+
+int __connman_dnsproxy_enable_captive(uint32_t ip) {
+ captive_ip = ip ;
+ return 0;
+}
+
+int __connman_dnsproxy_disable_captive(void) {
+ captive_ip = 0 ;
+ return 0;
+}
diff --git a/src/technology.c b/src/technology.c
index 55303a0..c6475b8 100644
--- a/src/technology.c
+++ b/src/technology.c
@@ -66,6 +66,7 @@ struct connman_technology {
*/
char *tethering_ident;
char *tethering_passphrase;
+ bool tethering_captive;

bool enable_persistent; /* Save the tech state */

@@ -224,7 +225,7 @@ void connman_technology_tethering_notify(struct connman_technology *technology,
tethering_changed(technology);

if (enabled)
- __connman_tethering_set_enabled();
+ __connman_tethering_set_enabled(technology->tethering_captive);
else
__connman_tethering_set_disabled();
}
@@ -428,6 +429,10 @@ static void technology_load(struct connman_technology *technology)

technology->tethering_passphrase = g_key_file_get_string(keyfile,
identifier, "Tethering.Passphrase", NULL);
+
+ technology->tethering_captive = g_key_file_get_boolean(keyfile,
+ identifier, "Tethering.Captive", false);
+
done:
g_free(identifier);

diff --git a/src/tethering.c b/src/tethering.c
index ceeec74..9187178 100644
--- a/src/tethering.c
+++ b/src/tethering.c
@@ -56,6 +56,7 @@ static char *private_network_primary_dns = NULL;
static char *private_network_secondary_dns = NULL;

static volatile int tethering_enabled;
+static volatile bool captive_enabled;
static GDHCPServer *tethering_dhcp_server = NULL;
static struct connman_ippool *dhcp_ippool = NULL;
static DBusConnection *connection;
@@ -178,10 +179,10 @@ static void tethering_restart(struct connman_ippool *pool, void *user_data)
{
DBG("pool %p", pool);
__connman_tethering_set_disabled();
- __connman_tethering_set_enabled();
+ __connman_tethering_set_enabled(captive_enabled);
}

-void __connman_tethering_set_enabled(void)
+void __connman_tethering_set_enabled(bool captive_dns)
{
int index;
int err;
@@ -193,6 +194,7 @@ void __connman_tethering_set_enabled(void)
const char *dns;
unsigned char prefixlen;
char **ns;
+ captive_enabled = captive_dns;

DBG("enabled %d", tethering_enabled + 1);

@@ -279,6 +281,19 @@ void __connman_tethering_set_enabled(void)
return;
}

+ if (captive_dns) {
+ err = __connman_dnsproxy_enable_captive(htonl(inet_addr(gateway)));
+ if (err < 0) {
+ connman_error("Cannot enable captive DNS");
+ dhcp_server_stop(tethering_dhcp_server);
+ __connman_bridge_disable(BRIDGE_NAME);
+ __connman_ippool_unref(dhcp_ippool);
+ __connman_bridge_remove(BRIDGE_NAME);
+ __sync_fetch_and_sub(&tethering_enabled, 1);
+ return;
+ }
+ }
+
err = __connman_ipv6pd_setup(BRIDGE_NAME);
if (err < 0 && err != -EINPROGRESS)
DBG("Cannot setup IPv6 prefix delegation %d/%s", err,
@@ -302,6 +317,7 @@ void __connman_tethering_set_disabled(void)
__connman_dnsproxy_remove_listener(index);

__connman_nat_disable(BRIDGE_NAME);
+ __connman_dnsproxy_disable_captive();

dhcp_server_stop(tethering_dhcp_server);

@@ -398,6 +414,7 @@ static void remove_private_network(gpointer user_data)
__connman_nat_disable(BRIDGE_NAME);
connman_rtnl_remove_watch(pn->iface_watch);
__connman_ippool_unref(pn->pool);
+ __connman_dnsproxy_disable_captive();

if (pn->watch > 0) {
g_dbus_remove_watch(connection, pn->watch);
@@ -548,6 +565,7 @@ void __connman_tethering_cleanup(void)
__connman_bridge_disable(BRIDGE_NAME);
__connman_bridge_remove(BRIDGE_NAME);
__connman_nat_disable(BRIDGE_NAME);
+ __connman_dnsproxy_disable_captive();
}

if (!connection)

[Craig McQueen]

* Avoid corruption of response to a query that includes "additional"
records (e.g. OPT).
* Provide an A record response to only an A record query.
---
src/dnsproxy.c | 115 ++++++++++++++++++++++++++++++++++++++++++-------------
src/technology.c | 2 +-
2 files changed, 90 insertions(+), 27 deletions(-)

diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index f72b5cc..45ecd69 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -76,6 +76,10 @@ struct domain_hdr {
#else
#error "Unknown byte order"
#endif
+struct dns_query_part {
+ uint16_t qtype;
+ uint16_t qclass;
+} __attribute__ ((packed));
struct a_dns_answer {
uint16_t ptr;
uint16_t type;
@@ -367,6 +371,39 @@ static int dns_name_length(unsigned char *buf)
return strlen((char *)buf);
}

+static int dns_name_field_length(unsigned char *buf, int max_len)
+{
+ int len = 0;
+
+ for (;;) {
+ if (len == max_len)
+ return -EINVAL;
+ if ((buf[0] & NS_CMPRSFLGS) == NS_CMPRSFLGS) {
+ /* compressed name */
+ len += 2;
+ if (len > max_len)
+ return -EINVAL;
+ return len;
+ }
+ if (buf[0] == 0)
+ return len + 1;
+ len += buf[0] + 1;
+ buf += buf[0] + 1;
+ }
+}
+
+static int dns_query_length(unsigned char *buf, int max_len)
+{
+ int len = 0;
+ len = dns_name_field_length(buf, max_len);
+ if (len < 0)
+ return len;
+ len += sizeof(struct dns_query_part);
+ if (len > max_len)
+ return -EINVAL;
+ return len;
+}
+
static void update_cached_ttl(unsigned char *buf, int len, int new_ttl)
{
unsigned char *c;
@@ -516,8 +553,13 @@ static void send_response_A(int sk, unsigned char *req, int len,
const struct sockaddr *to, socklen_t tolen,
int protocol)
{
- struct domain_hdr *hdr;
- struct a_dns_answer* ans;
+ struct domain_hdr *req_hdr;
+ struct domain_hdr *new_hdr;
+ struct a_dns_answer *ans;
+ char *new_buf;
+ unsigned char *req_query;
+ struct dns_query_part *req_query_part;
+ int query_len, response_len;
int err, offset = protocol_offset(protocol);

DBG("sk %d", sk);
@@ -525,36 +567,57 @@ static void send_response_A(int sk, unsigned char *req, int len,
if (offset < 0)
return;

- if (len < 12)
+ if (len < offset + sizeof(struct domain_hdr))
return;
-
- size_t new_len = len+sizeof(struct a_dns_answer) ;
- char* new_buf ;
- new_buf = g_malloc(new_len) ;
- memcpy(new_buf, req, len) ;

- hdr = (void *) (new_buf + offset);
+ req_hdr = (void *) (req + offset);

- DBG("id 0x%04x qr %d opcode %d", hdr->id, hdr->qr, hdr->opcode);
+ DBG("id 0x%04x qr %d opcode %d", req_hdr->id, req_hdr->qr, req_hdr->opcode);

- hdr->qr = 1;
- hdr->rcode = ns_r_noerror;
+ if (ntohs(req_hdr->qdcount) == 0)
+ return;

- hdr->ancount = htons(1);
- hdr->nscount = 0;
- hdr->arcount = 0;
+ req_query = req + offset + sizeof(struct domain_hdr);
+ query_len = dns_query_length(req_query, len - offset - sizeof(struct domain_hdr));
+ if (query_len < 0)
+ return;
+ req_query_part = (void *) (req_query + query_len - sizeof(struct dns_query_part));
+
+ size_t new_len = len + sizeof(struct a_dns_answer);
+ new_buf = g_malloc(new_len);
+
+ /* Copy header */
+ response_len = offset + sizeof(struct domain_hdr);
+ memcpy(new_buf, req, response_len);
+
+ new_hdr = (void *) (new_buf + offset);
+
+ new_hdr->qr = 1;
+ new_hdr->rcode = ns_r_noerror;
+
+ new_hdr->ancount = 0;
+ new_hdr->nscount = 0;
+ new_hdr->arcount = 0;
+
+ /* Copy query */
+ memcpy(new_buf + response_len, req_query, query_len);
+ response_len += query_len;
+
+ if (ntohs(req_query_part->qtype) == ns_t_a &&
+ ntohs(req_query_part->qclass) == ns_c_in) {
+ /* Add an A answer */
+ new_hdr->ancount = htons(1);
+ ans = (void *) (new_buf + response_len);
+ ans->ptr = htons(0xc00c) ;/* 11....1100 -> ptr to 12th octet = query */
+ ans->type = htons(ns_t_a);
+ ans->class = htons(ns_c_in);
+ ans->ttl = 0;
+ ans->rdlength = htons(4);
+ ans->ip = htonl(ip);
+ response_len += sizeof(struct a_dns_answer);
+ }

- /* Create A answer */
- /* Other attributes */
- ans = (void *) (new_buf + len);
- ans->ptr = htons(0xc00c) ;/* 11....1100 -> ptr to 12th octet = query */
- ans->type = htons(ns_t_a);
- ans->class = htons(ns_c_in);
- ans->ttl = 0;
- ans->rdlength = htons(4);
- ans->ip = htonl(ip);
-
- err = sendto(sk, new_buf, new_len, MSG_NOSIGNAL, to, tolen);
+ err = sendto(sk, new_buf, response_len, MSG_NOSIGNAL, to, tolen);
g_free(new_buf) ;
if (err < 0) {
connman_error("Failed to send DNS response to %d: %s",
diff --git a/src/technology.c b/src/technology.c
index c6475b8..1197df5 100644
--- a/src/technology.c
+++ b/src/technology.c
@@ -431,7 +431,7 @@ static void technology_load(struct connman_technology *technology)
identifier, "Tethering.Passphrase", NULL);

technology->tethering_captive = g_key_file_get_boolean(keyfile,
- identifier, "Tethering.Captive", false);
+ identifier, "Tethering.Captive", NULL);

done:
g_free(identifier);

[Craig McQueen]

Use these functions in the code to simplify the logic.
---

This is a possibly generically useful change.

Note that the code in __connman_inet_get_running_interfaces() was a bit
confusing -- the code for the IPv6 case checked for a link-local
address, not a loopback address as the comment said. Is that
intentional or accidental? It's not obvious.

src/connman.h | 2 ++
src/inet.c | 58 ++++++++++++++++++++++++++++++++++++++++++----------------
2 files changed, 44 insertions(+), 16 deletions(-)

diff --git a/src/connman.h b/src/connman.h
index 7b3265f..6214431 100644
--- a/src/connman.h
+++ b/src/connman.h
@@ -151,6 +151,7 @@ void __connman_task_cleanup(void);

#include <connman/inet.h>

+bool __connman_inet_is_addr_loopback(const struct sockaddr *addr);
char **__connman_inet_get_running_interfaces(void);
int __connman_inet_modify_address(int cmd, int flags, int index, int family,
const char *address,
@@ -166,6 +167,7 @@ int __connman_inet_get_interface_ll_address(int index, int family, void *address
typedef void (*__connman_inet_rs_cb_t) (struct nd_router_advert *reply,
unsigned int length, void *user_data);

+bool __connman_inet_ipv6_is_addr_linklocal(const struct sockaddr *addr);
int __connman_inet_ipv6_send_rs(int index, int timeout,
__connman_inet_rs_cb_t callback, void *user_data);
int __connman_inet_ipv6_send_ra(int index, struct in6_addr *src_addr,
diff --git a/src/inet.c b/src/inet.c
index e06d9c8..38d7230 100644
--- a/src/inet.c
+++ b/src/inet.c
@@ -2582,6 +2582,44 @@ bool connman_inet_check_hostname(const char *ptr, size_t len)
return true;
}

+bool __connman_inet_ipv6_is_addr_linklocal(const struct sockaddr *addr)
+{
+ const struct in6_addr *addr6;
+
+ switch (addr->sa_family) {
+ case AF_INET6:
+ addr6 = &((const struct sockaddr_in6 *)addr)->sin6_addr;
+ if (IN6_IS_ADDR_LINKLOCAL(addr6))
+ return true;
+ break;
+ default:
+ break;
+ }
+ return false;
+}
+
+bool __connman_inet_is_addr_loopback(const struct sockaddr *addr)
+{
+ const struct in6_addr *addr6;
+ in_addr_t addr4;
+
+ switch (addr->sa_family) {
+ case AF_INET:
+ addr4 = ntohl(((const struct sockaddr_in *)addr)->sin_addr.s_addr);
+ if (((addr4 & 0xff000000) >> 24) == 127)
+ return true;
+ break;
+ case AF_INET6:
+ addr6 = &((const struct sockaddr_in6 *)addr)->sin6_addr;
+ if (IN6_IS_ADDR_LOOPBACK(addr6))
+ return true;
+ break;
+ default:
+ break;
+ }
+ return false;
+}
+
char **__connman_inet_get_running_interfaces(void)
{
char **result;
@@ -2622,27 +2660,15 @@ char **__connman_inet_get_running_interfaces(void)

for (i = 0; i < numif; i++) {
struct ifreq *r = &ifr[i];
- struct in6_addr *addr6;
- in_addr_t addr4;

/*
* Note that we do not return loopback interfaces here as they
* are not needed for our purposes.
*/
- switch (r->ifr_addr.sa_family) {
- case AF_INET:
- addr4 = ntohl(((struct sockaddr_in *)
- &r->ifr_addr)->sin_addr.s_addr);
- if (((addr4 & 0xff000000) >> 24) == 127)
- continue;
- break;
- case AF_INET6:
- addr6 = &((struct sockaddr_in6 *)
- &r->ifr_addr)->sin6_addr;
- if (IN6_IS_ADDR_LINKLOCAL(addr6))
- continue;
- break;
- }
+ if (__connman_inet_is_addr_loopback(&r->ifr_addr))
+ continue;
+ if (__connman_inet_ipv6_is_addr_linklocal(&r->ifr_addr))
+ continue;

result[count++] = g_strdup(r->ifr_name);
}

[Craig McQueen]

---
src/dnsproxy.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index 45ecd69..37e9605 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -3554,7 +3554,7 @@ static bool udp_listener_event(GIOChannel *channel, GIOCondition condition,
DBG("Received %d bytes (id 0x%04x)", len, buf[0] | buf[1] << 8);

/* If captive mode, all is redirected to tether ip */
- if (captive_ip) {
+ if (captive_ip && !__connman_inet_is_addr_loopback(client_addr)) {
send_response_A(sk, buf, len,
captive_ip,
client_addr, *client_addr_len, IPPROTO_UDP);

[Patrik Flykt]

Post by Craig McQueen
The following patches are related to the captive portal code originally
provided by Alexandre Chataignon. The first patch is a copy of the
original patch, provided again as a reference. The 3 following patches
are improvements I made following a review of Alexandre's patch, to
* Check that the query first question is for an A record, and only
send an A record if so.
* Handle the case of a query containing additional records (don't
send a corrupt response).
* For queries from a loopack IP address, don't return the captive
address, but do the normal DNS handling.

So this is a special case of a special case.

The general case to solve is to add local entries to ConnMan's resolver,
i.e. entries in /etc/hosts should be supported. As should the hostname
handled by systemd-hostnamed. A and AAAA records as well as the IP4.ARPA
and IP6.ARPA domains need to be added automatically.

After this comes the first special case: what, if any, should be
configured for hosts in the tethered network. The second special case is
the wildcard catch-all for the portal case specified above. This seems
to point to a wildcard entry in the file mapping all other names. I'm
not very sure how both of these cases will work out in the end.

Plan B for handling this special case is to set up unbound or similar,
start ConnMan with --nodnsproxy while adding the IP address of the
tethering interface to FallbackNameservers. The nameservers in
FallbackNameservers will be sent out via DHCP if ConnMan is not proxying
DNS requests. In this particular special case there were no uplink
connections so the DHCP server IP address is always the same - minus any
bugs of course.

Plan C is to use mDNS from Avahi, but that seems to be outside of the
original solution unless short names are always used.

Cheers,

Patrik