Hi,
On 24/05/16 21:10, Tim Rühsen wrote:
>
> Hi Ander,
>
> could you rearrange the code in hsts_store_open() a bit to
> - avoid double calling file_exists_p (filename)
> - reduce the scope of 'st' and 'fp'
>
> if (file_exists_p (filename)) {
> if (hsts_file_access_valid (filename)) {
> struct_stat st;
> FILE *fp = fopen (filename, "r");
> ...
> } else {
> ...
> }
>
> out:
> return store;
> }
How about this?
>
> Regards, Tim
>
Regards,
- AJ
>From 94bcb5d1c3928b76863783c7f4daee215870a959 Mon Sep 17 00:00:00 2001
From: Ander Juaristi <[email protected]>
Date: Wed, 6 Apr 2016 12:55:17 +0200
Subject: [PATCH] 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 | 54 ++++++++++++++++++++++++++++++++++++++++++------------
1 file changed, 42 insertions(+), 12 deletions(-)
diff --git a/src/hsts.c b/src/hsts.c
index d5e0bee..4be7f0d 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 */
/*
@@ -464,8 +480,6 @@ hsts_store_t
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);
@@ -473,24 +487,40 @@ hsts_store_open (const char *filename)
if (file_exists_p (filename))
{
- fp = fopen (filename, "r");
+ if (hsts_file_access_valid (filename))
+ {
+ struct_stat st;
+ FILE *fp = fopen (filename, "r");
- if (!fp || !hsts_read_database (store, fp, false))
+ if (!fp || !hsts_read_database (store, fp, false))
+ {
+ /* abort! */
+ hsts_store_close (store);
+ xfree (store);
+ fclose (fp);
+ goto out;
+ }
+
+ if (fstat (fileno (fp), &st) == 0)
+ store->last_mtime = st.st_mtime;
+
+ fclose (fp);
+ }
+ else
{
- /* abort! */
+ /*
+ * If we're not reading the HSTS database,
+ * then by all means act as if HSTS was disabled.
+ */
hsts_store_close (store);
xfree (store);
- goto out;
- }
- if (fstat (fileno (fp), &st) == 0)
- store->last_mtime = st.st_mtime;
+ logprintf (LOG_NOTQUIET, "Will not apply HSTS. "
+ "The HSTS database must be a regular and non-world-writable file.\n");
+ }
}
out:
- if (fp)
- fclose (fp);
-
return store;
}
--
2.1.4
