Hi all, Here are some patches for HSTS.
- 0001: checks the HSTS database file is not world-writable, and refuses to read it if it is, and disables HSTS. This was in my original GSoC proposal, but did not get through in the end. - 0002: Correct the comments in the top of the HSTS file. - 0003: strictly comply with RFC 6797 (see commit description). Regards, - AJ
From e161919eb3aa3174ee38084b787cdbf4b9965bd6 Mon Sep 17 00:00:00 2001 From: Ander Juaristi <[email protected]> Date: Wed, 6 Apr 2016 12:55:17 +0200 Subject: [PATCH 1/3] Check the HSTS file is not world-writable * hsts.c (hsts_file_access_valid): check that the file is a regular file, and that it's not world-writable. (hsts_store_open): if the HSTS database file does not meet the above requirements, disable HSTS at all. --- src/hsts.c | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/src/hsts.c b/src/hsts.c index af7ade1..7c68182 100644 --- a/src/hsts.c +++ b/src/hsts.c @@ -334,6 +334,22 @@ hsts_store_dump (hsts_store_t store, FILE *fp) } } +/* + * Test: + * - The file is a regular file (ie. not a symlink), and + * - The file is not world-writable. + */ +static bool +hsts_file_access_valid (const char *filename) +{ + struct_stat st; + + if (stat (filename, &st) == -1) + return false; + + return !(st.st_mode & S_IWOTH) && S_ISREG (st.st_mode); +} + /* HSTS API */ /* @@ -471,7 +487,7 @@ hsts_store_open (const char *filename) store->table = hash_table_new (0, hsts_hash_func, hsts_cmp_func); store->last_mtime = 0; - if (file_exists_p (filename)) + if (file_exists_p (filename) && hsts_file_access_valid (filename)) { fp = fopen (filename, "r"); @@ -486,6 +502,18 @@ hsts_store_open (const char *filename) if (fstat (fileno (fp), &st) == 0) store->last_mtime = st.st_mtime; } + else if (file_exists_p (filename)) + { + /* + * If we're not reading the HSTS database, + * then by all means act as if HSTS was disabled. + */ + hsts_store_close (store); + xfree (store); + + logprintf (LOG_NOTQUIET, "Will not apply HSTS. " + "The HSTS database must be a regular and non-world-writable file.\n"); + } out: if (fp) -- 2.1.4
From b6193f4f9c0f3de54fe3c1837a1a08c87e80fb24 Mon Sep 17 00:00:00 2001 From: Ander Juaristi <[email protected]> Date: Wed, 6 Apr 2016 13:13:52 +0200 Subject: [PATCH 2/3] Correct HSTS database file description * src/hsts.c (hsts_store_dump): s/[:port]/<port>/ --- src/hsts.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/hsts.c b/src/hsts.c index af7ade1..6f6e1fe 100644 --- a/src/hsts.c +++ b/src/hsts.c @@ -316,7 +316,7 @@ hsts_store_dump (hsts_store_t store, FILE *fp) /* Print preliminary comments. We don't care if any of these fail. */ fputs ("# HSTS 1.0 Known Hosts database for GNU Wget.\n", fp); fputs ("# Edit at your own risk.\n", fp); - fputs ("# <hostname>[:<port>]\t<incl. subdomains>\t<created>\t<max-age>\n", fp); + fputs ("# <hostname>\t<port>\t<incl. subdomains>\t<created>\t<max-age>\n", fp); /* Now cycle through the HSTS store in memory and dump the entries */ for (hash_table_iterate (store->table, &it); hash_table_iter_next (&it);) -- 2.1.4
From 779f320b96245df03a86576ba4cf56750d0e30f6 Mon Sep 17 00:00:00 2001 From: Ander Juaristi <[email protected]> Date: Wed, 6 Apr 2016 13:31:41 +0200 Subject: [PATCH 3/3] Strictly comply with RFC 6797 * src/hsts.c (hsts_store_entry): strictly comply with RFC 6797. RFC 6797 states in section 8.1 that the UA's cached information should only be updated if: "either or both of the max-age and includeSubDomains header field value tokens are conveying information different than that already maintained by the UA." --- src/hsts.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/hsts.c b/src/hsts.c index af7ade1..ef6d444 100644 --- a/src/hsts.c +++ b/src/hsts.c @@ -426,9 +426,8 @@ hsts_store_entry (hsts_store_t store, hsts_remove_entry (store, kh); else if (max_age > 0) { - entry->include_subdomains = include_subdomains; - - if (entry->max_age != max_age) + if (entry->max_age != max_age || + entry->include_subdomains != include_subdomains) { /* RFC 6797 states that 'max_age' is a TTL relative to the reception of the STS header so we have to update the 'created' field too */ @@ -436,6 +435,7 @@ hsts_store_entry (hsts_store_t store, if (t != -1) entry->created = t; entry->max_age = max_age; + entry->include_subdomains = include_subdomains; } } /* we ignore negative max_ages */ -- 2.1.4
