Hi,
I've written two patches for the HSTS code. Please review them when you have
time.
In august, dkg catched a potential race condition in Wget's HSTS code (replayed
message below). The first patch aims to solve that issue using flock(2). It
works well on my machine: I've tested it by spawning two Wget processes on top
of gdb.
The second patch fixes what's in my opinion a critical bug. It only triggered
when more than one Wget processes used the same HSTS database simultaneously,
and that's why it's gone unnoticed until now. I noticed it while I was working
on the first patch, so it could as well be part of it, but IMO this bug is more
related to the principles of our HSTS design itself than to the specific race
condition issue.
On 08/18/2015 09:12 PM, Ander Juaristi wrote:
On 08/10/2015 06:06 PM, Daniel Kahn Gillmor wrote:
This sounds like there might still be a race condition where one
process's changes get clobbered. can we make any atomicity guarantees
for users who might be concerned about this?
You're right. My fault not to take this into account. This could be fixed with
flock/fcntl. I think they're both in gnulib.
These last two issues require code changes. I'll take the responsibility to fix
them, but outside of GSoC. The first one requires a bit of consensus before
coding anything, but the second one seems a bit more straightforward. For now,
I attach the two previous patches. The first one (the HSTS docs) amended with
dkg's suggestions. If there're no more complaints, I think they can be pushed,
because Wget's behaviour has not changed yet. When we implement any of the
ideas proposed above, we'll update the docs.
Regards,
- AJ
>From 826d204421434a3c64d363b8b66c5bbc24657274 Mon Sep 17 00:00:00 2001
From: Ander Juaristi <[email protected]>
Date: Mon, 5 Oct 2015 23:03:45 +0200
Subject: [PATCH 1/2] Fix potential race condition
* src/hsts.c (hsts_read_database): get an open file handle
instead of a file name.
(hsts_store_dump): get an open file handle
instead of a file name.
(hsts_store_open): open the file and pass the open file handle.
(hsts_store_save): lock the file before the read-merge-dump
process.
Reported-by: Daniel Kahn Gillmor <[email protected]>
---
src/hsts.c | 125 +++++++++++++++++++++++++++++++++----------------------------
1 file changed, 68 insertions(+), 57 deletions(-)
diff --git a/src/hsts.c b/src/hsts.c
index 5c4ca35..c7339b8 100644
--- a/src/hsts.c
+++ b/src/hsts.c
@@ -39,13 +39,16 @@ as that of the covered work. */
#include "c-ctype.h"
#ifdef TESTING
#include "test.h"
-#include <unistd.h> /* for unlink(), used only in tests */
#endif
+#include <unistd.h>
+#include <sys/types.h>
#include <stdlib.h>
#include <time.h>
#include <sys/stat.h>
#include <string.h>
+#include <stdio.h>
+#include <sys/file.h>
struct hsts_store {
struct hash_table *table;
@@ -263,9 +266,8 @@ hsts_store_merge (hsts_store_t store,
}
static bool
-hsts_read_database (hsts_store_t store, const char *file, bool merge_with_existing_entries)
+hsts_read_database (hsts_store_t store, FILE *fp, bool merge_with_existing_entries)
{
- FILE *fp = NULL;
char *line = NULL, *p;
size_t len = 0;
int items_read;
@@ -279,67 +281,54 @@ hsts_read_database (hsts_store_t store, const char *file, bool merge_with_existi
func = (merge_with_existing_entries ? hsts_store_merge : hsts_new_entry);
- fp = fopen (file, "r");
- if (fp)
+ while (getline (&line, &len, fp) > 0)
{
- while (getline (&line, &len, fp) > 0)
- {
- for (p = line; c_isspace (*p); p++)
- ;
-
- if (*p == '#')
- continue;
+ for (p = line; c_isspace (*p); p++)
+ ;
- items_read = sscanf (p, "%255s %d %d %lu %lu",
- host,
- &port,
- &include_subdomains,
- (unsigned long *) &created,
- (unsigned long *) &max_age);
-
- if (items_read == 5)
- func (store, host, port, created, max_age, !!include_subdomains);
- }
+ if (*p == '#')
+ continue;
- xfree (line);
- fclose (fp);
+ items_read = sscanf (p, "%255s %d %d %lu %lu",
+ host,
+ &port,
+ &include_subdomains,
+ (unsigned long *) &created,
+ (unsigned long *) &max_age);
- result = true;
+ if (items_read == 5)
+ func (store, host, port, created, max_age, !!include_subdomains);
}
+ xfree (line);
+ result = true;
+
return result;
}
static void
-hsts_store_dump (hsts_store_t store, const char *filename)
+hsts_store_dump (hsts_store_t store, FILE *fp)
{
- FILE *fp = NULL;
hash_table_iterator it;
- fp = fopen (filename, "w");
- if (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);
+
+ /* Now cycle through the HSTS store in memory and dump the entries */
+ for (hash_table_iterate (store->table, &it); hash_table_iter_next (&it);)
{
- /* 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);
+ struct hsts_kh *kh = (struct hsts_kh *) it.key;
+ struct hsts_kh_info *khi = (struct hsts_kh_info *) it.value;
- /* Now cycle through the HSTS store in memory and dump the entries */
- for (hash_table_iterate (store->table, &it); hash_table_iter_next (&it);)
+ if (fprintf (fp, "%s\t%d\t%d\t%lu\t%lu\n",
+ kh->host, kh->explicit_port, khi->include_subdomains,
+ khi->created, khi->max_age) < 0)
{
- struct hsts_kh *kh = (struct hsts_kh *) it.key;
- struct hsts_kh_info *khi = (struct hsts_kh_info *) it.value;
-
- if (fprintf (fp, "%s\t%d\t%d\t%lu\t%lu\n",
- kh->host, kh->explicit_port, khi->include_subdomains,
- khi->created, khi->max_age) < 0)
- {
- logprintf (LOG_ALWAYS, "Could not write the HSTS database correctly.\n");
- break;
- }
+ logprintf (LOG_ALWAYS, "Could not write the HSTS database correctly.\n");
+ break;
}
-
- fclose (fp);
}
}
@@ -472,6 +461,7 @@ hsts_store_open (const char *filename)
{
hsts_store_t store = NULL;
struct stat st;
+ FILE *fp = NULL;
store = xnew0 (struct hsts_store);
store->table = hash_table_new (0, hsts_hash_func, hsts_cmp_func);
@@ -482,13 +472,16 @@ hsts_store_open (const char *filename)
if (stat (filename, &st) == 0)
store->last_mtime = st.st_mtime;
- if (!hsts_read_database (store, filename, false))
+ fp = fopen (filename, "r");
+ if (!fp || !hsts_read_database (store, fp, false))
{
/* abort! */
hsts_store_close (store);
xfree (store);
store = NULL;
}
+ if (fp)
+ fclose (fp);
}
return store;
@@ -498,18 +491,36 @@ void
hsts_store_save (hsts_store_t store, const char *filename)
{
struct stat st;
+ FILE *f = NULL;
+ int fd = 0;
if (filename && hash_table_count (store->table) > 0)
{
- /* If the file has changed, merge the changes with our in-memory data
- before dumping them to the file.
- Otherwise we could potentially overwrite the data stored by other Wget processes.
- */
- if (store->last_mtime && stat (filename, &st) == 0 && st.st_mtime > store->last_mtime)
- hsts_read_database (store, filename, true);
-
- /* now dump to the file */
- hsts_store_dump (store, filename);
+ f = fopen (filename, "a+");
+ if (f)
+ {
+ /* Lock the file to avoid potential race conditions */
+ fd = fileno (f);
+ flock (fd, LOCK_EX);
+
+ /* If the file has changed, merge the changes with our in-memory data
+ before dumping them to the file.
+ Otherwise we could potentially overwrite the data stored by other Wget processes.
+ */
+ if (store->last_mtime && stat (filename, &st) == 0 && st.st_mtime > store->last_mtime)
+ hsts_read_database (store, f, true);
+
+ /* We've merged the latest changes so we can now truncate the file
+ and dump everything. */
+ fseek (f, 0, SEEK_SET);
+ ftruncate (fd, 0);
+
+ /* now dump to the file */
+ hsts_store_dump (store, f);
+
+ flock (fd, LOCK_UN);
+ fclose (f);
+ }
}
}
--
1.9.1
>From f8ece3df88bdc3dad935f990f7de0f13db884d4d Mon Sep 17 00:00:00 2001
From: Ander Juaristi <[email protected]>
Date: Wed, 7 Oct 2015 21:25:41 +0200
Subject: [PATCH 2/2] Fix HSTS merge bug
* src/hsts.c (hsts_store_merge): call hsts_new_entry() if the entry
does not exist in the database.
When merging the existing HSTS database on disk with the one on memory,
the entries that were on disk but not on memory were ignored. Thus,
only the existing entries were merged. This behavior was only triggered
when more than one Wget processes were using the same HSTS database
simultaneously. This commit fixes the bug by adding the new entries
to the on-memory database if they were not found there.
---
src/hsts.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/hsts.c b/src/hsts.c
index c7339b8..a92ec67 100644
--- a/src/hsts.c
+++ b/src/hsts.c
@@ -261,6 +261,8 @@ hsts_store_merge (hsts_store_t store,
success = true;
}
+ else if (!khi)
+ success = hsts_new_entry (store, host, port, created, max_age, include_subdomains);
return success;
}
--
1.9.1
