From a45b48c47e0c0a4acdb1dfe0db10df202e469cf8 Mon Sep 17 00:00:00 2001 From: Mason Simon Date: Mon, 14 May 2018 08:40:46 -0700 Subject: [PATCH 01/13] Note how to run tests --- README.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/README.md b/README.md index 827cc59..4692454 100644 --- a/README.md +++ b/README.md @@ -43,6 +43,10 @@ In your `nginx.conf`, 1. In your `log_format` directives, replace `$remote_addr` with `$remote_addr_ipscrub`. 1. Reload your nginx config. +### Running Tests + +`make test` + ## GDPR [GDPR](https://www.eugdpr.org) goes into effect on May 25, 2018. It legislates the handling of personal data about your users, including IP addresses. From afa00cac2c370a0343b779e1a6b3bd20cd7bd7e4 Mon Sep 17 00:00:00 2001 From: Mason Simon Date: Mon, 14 May 2018 08:50:00 -0700 Subject: [PATCH 02/13] Note about error log leaking IPs --- README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README.md b/README.md index 4692454..37ba4d1 100644 --- a/README.md +++ b/README.md @@ -43,6 +43,8 @@ In your `nginx.conf`, 1. In your `log_format` directives, replace `$remote_addr` with `$remote_addr_ipscrub`. 1. Reload your nginx config. +**NOTE**: nginx may still leak IP addresses in the error log. If this is a concern, disable error logging or wipe the log regularly. + ### Running Tests `make test` From b173ac97753acfc7738e7e95d6605679749b41da Mon Sep 17 00:00:00 2001 From: Mason Simon Date: Wed, 16 May 2018 15:51:06 +0900 Subject: [PATCH 03/13] Link to package for Arch Linux --- README.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/README.md b/README.md index 37ba4d1..7d15809 100644 --- a/README.md +++ b/README.md @@ -34,8 +34,18 @@ Scenario (2) is defended against because the server operator does not know the s ## Usage +### Installation + +#### Building From Source + `ipscrub` can be built statically with nginx or as a [dynamic module](https://www.nginx.com/blog/compiling-dynamic-modules-nginx-plus/). See the `Makefile` for examples of both ways. +#### Packages + +- [Arch Linux](https://aur.archlinux.org/packages/nginx-mod-ipscrub/) (thanks to https://github.com/deep-42-thought) + +### Configuration + In your `nginx.conf`, 1. At the top-level, load the module by adding the line `load_module ngx_ipscrub_module.so;` (NOTE: only if you built as a dynamic module). From 216cf21a05e9e62dc48666aa9894c5117b2e48ac Mon Sep 17 00:00:00 2001 From: Mason Simon Date: Wed, 16 May 2018 15:52:23 +0900 Subject: [PATCH 04/13] Properly linkify Arch packager's username --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 7d15809..b68bd4b 100644 --- a/README.md +++ b/README.md @@ -42,7 +42,7 @@ Scenario (2) is defended against because the server operator does not know the s #### Packages -- [Arch Linux](https://aur.archlinux.org/packages/nginx-mod-ipscrub/) (thanks to https://github.com/deep-42-thought) +- [Arch Linux](https://aur.archlinux.org/packages/nginx-mod-ipscrub/) (thanks to [deep-42-thought](https://github.com/deep-42-thought)) ### Configuration From 142c3099763ee5aff33f7365ad1948e944b92c3b Mon Sep 17 00:00:00 2001 From: Mason Simon Date: Fri, 25 May 2018 18:56:59 -0700 Subject: [PATCH 05/13] Use /dev/urandom --- ipscrub/src/ngx_ipscrub_module.c | 7 ++++++- ipscrub/src/ngx_ipscrub_support.c | 21 +++++++++++++++++++++ ipscrub/src/ngx_ipscrub_support.h | 1 + 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/ipscrub/src/ngx_ipscrub_module.c b/ipscrub/src/ngx_ipscrub_module.c index 09a2ef4..78361df 100644 --- a/ipscrub/src/ngx_ipscrub_module.c +++ b/ipscrub/src/ngx_ipscrub_module.c @@ -146,7 +146,12 @@ ngx_http_variable_remote_addr_ipscrub(ngx_http_request_t *r, ngx_http_variable_v // Regenerate salt if past end of period. time_t now = time(NULL); if (period_start == -1 || now - period_start > icf->period_seconds) { - nonce = ngx_random(); + // nonce = ngx_random(); + rc = randlong(&nonce); + if (rc != NGX_OK) { + return NGX_HTTP_INTERNAL_SERVER_ERROR; + } + // TODO: actually calculate when period_start should have been. period_start = now; } diff --git a/ipscrub/src/ngx_ipscrub_support.c b/ipscrub/src/ngx_ipscrub_support.c index c409d20..2f68691 100644 --- a/ipscrub/src/ngx_ipscrub_support.c +++ b/ipscrub/src/ngx_ipscrub_support.c @@ -36,3 +36,24 @@ ngx_int_t concat(ngx_pool_t *pool, ngx_str_t prefix, ngx_str_t suffix, u_char ** return NGX_OK; } + +// randlong fills out with secure random bytes and returns NGX_OK iff successful. +ngx_int_t randlong(long *out) { + #if !(NGX_DARWIN || NGX_SOLARIS || NGX_FREEBSD || NGX_LINUX) + // Windows not supported a.t.m. + // TODO: support Windows (https://msdn.microsoft.com/en-us/library/sxtz2fa8.aspx). + return -1; + #endif + + int rand = open("/dev/urandom", O_RDONLY); + if (rand < 0) { + return -1; + } + + ssize_t ret = read(rand, out, sizeof(long)); + if (ret != sizeof(long)) { + return -1; + } + + return NGX_OK; +} diff --git a/ipscrub/src/ngx_ipscrub_support.h b/ipscrub/src/ngx_ipscrub_support.h index e509f07..7509911 100644 --- a/ipscrub/src/ngx_ipscrub_support.h +++ b/ipscrub/src/ngx_ipscrub_support.h @@ -8,5 +8,6 @@ ngx_int_t null_terminate(ngx_pool_t *pool, ngx_str_t input, u_char **hashed); ngx_int_t concat(ngx_pool_t *pool, ngx_str_t prefix, ngx_str_t suffix, u_char **out); +ngx_int_t randlong(long *out); #endif /* _IPSCRUB_SUPPORT_H_INCLUDED_ */ From 44ccc14540da2e02e3ae7af035d33306da85700f Mon Sep 17 00:00:00 2001 From: Mason Simon Date: Fri, 25 May 2018 18:58:07 -0700 Subject: [PATCH 06/13] Remove dead code --- ipscrub/src/ngx_ipscrub_module.c | 1 - 1 file changed, 1 deletion(-) diff --git a/ipscrub/src/ngx_ipscrub_module.c b/ipscrub/src/ngx_ipscrub_module.c index 78361df..e01d8ef 100644 --- a/ipscrub/src/ngx_ipscrub_module.c +++ b/ipscrub/src/ngx_ipscrub_module.c @@ -146,7 +146,6 @@ ngx_http_variable_remote_addr_ipscrub(ngx_http_request_t *r, ngx_http_variable_v // Regenerate salt if past end of period. time_t now = time(NULL); if (period_start == -1 || now - period_start > icf->period_seconds) { - // nonce = ngx_random(); rc = randlong(&nonce); if (rc != NGX_OK) { return NGX_HTTP_INTERNAL_SERVER_ERROR; From fa8830b003833b7865d40d3c9b80b833eaf6e732 Mon Sep 17 00:00:00 2001 From: Mason Simon Date: Sun, 27 May 2018 19:19:09 -0700 Subject: [PATCH 07/13] Generate salt using 128bits from arc4random_buf --- Dockerfile | 2 +- README.md | 2 +- ipscrub/config | 8 ++++++- ipscrub/src/ngx_ipscrub_module.c | 7 +++--- ipscrub/src/ngx_ipscrub_support.c | 36 ++++++++++++++++++------------- ipscrub/src/ngx_ipscrub_support.h | 2 +- 6 files changed, 35 insertions(+), 22 deletions(-) diff --git a/Dockerfile b/Dockerfile index 2876c8d..e441598 100644 --- a/Dockerfile +++ b/Dockerfile @@ -2,7 +2,7 @@ FROM ubuntu:latest WORKDIR . -RUN apt-get update && apt-get install -y curl gcc make git libpcre3-dev zlib1g-dev +RUN apt-get update && apt-get install -y curl gcc make git libpcre3-dev zlib1g-dev libbsd-dev ADD ipscrub /ipscrub/ ADD Makefile / diff --git a/README.md b/README.md index b68bd4b..c01b5fe 100644 --- a/README.md +++ b/README.md @@ -15,7 +15,7 @@ ## Security Model -1. On initialization, and again every `PERIOD`, generate `salt` as `HASH(ngx_random() ++ timestamp)`. +1. On initialization, and again every `PERIOD`, generate `salt` using 128bits from `arc4random_buf()`. 2. On each request, generate masked IP address as `HASH(salt ++ IP address)`. 3. Log masked IP address. diff --git a/ipscrub/config b/ipscrub/config index d261e37..8d91d40 100644 --- a/ipscrub/config +++ b/ipscrub/config @@ -4,7 +4,13 @@ ngx_module_type=HTTP ngx_module_name=ngx_ipscrub_module ngx_module_deps="$ngx_addon_dir/src/ngx_ipscrub_support.h $ngx_addon_dir/src/ngx_ipscrub_debug.h" ngx_module_srcs="$ngx_addon_dir/src/ngx_ipscrub_module.c $ngx_addon_dir/src/ngx_ipscrub_support.c $ngx_addon_dir/src/ngx_ipscrub_debug.c" -ngx_module_libs=SHA1 + +# On Linux, link with libbsd, to get arc4random. +if [ `uname` = Linux ]; then + ngx_module_libs="SHA1 -lbsd" +else + ngx_module_libs="SHA1" +fi . auto/module diff --git a/ipscrub/src/ngx_ipscrub_module.c b/ipscrub/src/ngx_ipscrub_module.c index e01d8ef..8176403 100644 --- a/ipscrub/src/ngx_ipscrub_module.c +++ b/ipscrub/src/ngx_ipscrub_module.c @@ -42,7 +42,8 @@ static ngx_command_t ngx_ipscrub_commands[] = { // Globals. const int default_period_seconds = 10 * 60; // Period between salt changes. time_t period_start = -1; -long nonce = -1; // Input to salt generation. +#define num_nonce_bytes 16 +u_char nonce[num_nonce_bytes]; // Input to salt generation. static ngx_http_module_t ngx_ipscrub_module_ctx = { @@ -146,7 +147,7 @@ ngx_http_variable_remote_addr_ipscrub(ngx_http_request_t *r, ngx_http_variable_v // Regenerate salt if past end of period. time_t now = time(NULL); if (period_start == -1 || now - period_start > icf->period_seconds) { - rc = randlong(&nonce); + rc = randbytes((u_char *) &nonce, num_nonce_bytes); if (rc != NGX_OK) { return NGX_HTTP_INTERNAL_SERVER_ERROR; } @@ -156,7 +157,7 @@ ngx_http_variable_remote_addr_ipscrub(ngx_http_request_t *r, ngx_http_variable_v } salt.data = (u_char *) &nonce; - salt.len = sizeof(nonce); + salt.len = num_nonce_bytes; // Although ngx_crypt provides a salted SHA function, specified by a salt beginning with {SSHA}, that function exposes the salt in its result. For our security model, this is inappropriate. Instead, we use the regular nginx SHA function specified by {SHA}, and manually combine the nonce and plaintext. rc = concat(r->pool, r->connection->addr_text, salt, &combined); diff --git a/ipscrub/src/ngx_ipscrub_support.c b/ipscrub/src/ngx_ipscrub_support.c index 2f68691..fb3300d 100644 --- a/ipscrub/src/ngx_ipscrub_support.c +++ b/ipscrub/src/ngx_ipscrub_support.c @@ -3,6 +3,15 @@ #include "ngx_ipscrub_support.h" +#if (NGX_FREEBSD || NGX_SOLARIS || NGX_DARWIN) +// arc4random is built-in on these platforms. +#elif (NGX_LINUX) +#include +#else +// TODO: test using libbsd on Windows. +#error ipscrub requires arc4random_buf. +#endif + // null_terminate allocates a new, null-terminated string based on input. ngx_int_t null_terminate(ngx_pool_t *pool, ngx_str_t input, u_char **out) { @@ -37,23 +46,20 @@ ngx_int_t concat(ngx_pool_t *pool, ngx_str_t prefix, ngx_str_t suffix, u_char ** return NGX_OK; } -// randlong fills out with secure random bytes and returns NGX_OK iff successful. -ngx_int_t randlong(long *out) { - #if !(NGX_DARWIN || NGX_SOLARIS || NGX_FREEBSD || NGX_LINUX) - // Windows not supported a.t.m. - // TODO: support Windows (https://msdn.microsoft.com/en-us/library/sxtz2fa8.aspx). - return -1; - #endif - - int rand = open("/dev/urandom", O_RDONLY); - if (rand < 0) { - return -1; +// randbytes fills out with secure random bytes. +// Return value of NGX_OK indicates success. +// Return value of NGX_ERROR indicates error. +ngx_int_t randbytes(u_char *out, int num_bytes) { + if (out == NULL) { + return NGX_ERROR; + } + if (num_bytes < 1 || num_bytes > 64) { + // Values outside these bounds may indicate parameter usage mistake. + return NGX_ERROR; } - ssize_t ret = read(rand, out, sizeof(long)); - if (ret != sizeof(long)) { - return -1; - } + + arc4random_buf(out, num_bytes); return NGX_OK; } diff --git a/ipscrub/src/ngx_ipscrub_support.h b/ipscrub/src/ngx_ipscrub_support.h index 7509911..38ea3f0 100644 --- a/ipscrub/src/ngx_ipscrub_support.h +++ b/ipscrub/src/ngx_ipscrub_support.h @@ -8,6 +8,6 @@ ngx_int_t null_terminate(ngx_pool_t *pool, ngx_str_t input, u_char **hashed); ngx_int_t concat(ngx_pool_t *pool, ngx_str_t prefix, ngx_str_t suffix, u_char **out); -ngx_int_t randlong(long *out); +ngx_int_t randbytes(u_char *out, int num_bytes); #endif /* _IPSCRUB_SUPPORT_H_INCLUDED_ */ From 92c4cffaa6f37440aeb63c8b20976d8b625c2b3b Mon Sep 17 00:00:00 2001 From: Mason Simon Date: Sun, 27 May 2018 19:22:44 -0700 Subject: [PATCH 08/13] Fix whitespace --- ipscrub/src/ngx_ipscrub_support.c | 1 - 1 file changed, 1 deletion(-) diff --git a/ipscrub/src/ngx_ipscrub_support.c b/ipscrub/src/ngx_ipscrub_support.c index fb3300d..e83e0bf 100644 --- a/ipscrub/src/ngx_ipscrub_support.c +++ b/ipscrub/src/ngx_ipscrub_support.c @@ -58,7 +58,6 @@ ngx_int_t randbytes(u_char *out, int num_bytes) { return NGX_ERROR; } - arc4random_buf(out, num_bytes); return NGX_OK; From 9d8ab16c7151821a9925384dedc9d1f2865af90b Mon Sep 17 00:00:00 2001 From: Mason Simon Date: Sun, 27 May 2018 23:24:27 -0700 Subject: [PATCH 09/13] Fix README description of RNG --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index c01b5fe..1010e39 100644 --- a/README.md +++ b/README.md @@ -19,7 +19,7 @@ 2. On each request, generate masked IP address as `HASH(salt ++ IP address)`. 3. Log masked IP address. -`ipscrub` uses `ngx_random` to generate random nonces. `ngx_random` is defined as the C `random()` function on non-Windows platforms, and `rand()` on Windows. NOTE: this is not a cryptographically secure RNG, but for the following threat model, that is ok. +`ipscrub` uses `arc4random` to generate random nonces (see [Theo de Raat's talk on arc4random](https://www.youtube.com/watch?v=aWmLWx8ut20) for a great overview). On Linux this requires installing [libbsd](https://libbsd.freedesktop.org/wiki/) (package libbsd-dev on Ubuntu/Debian). ALSO NOTE: the generated hash WILL change on each `PERIOD` transition, so you will only have continuity within each `PERIOD`. But because users can transition between networks at any time (e.g. wifi -> cellular), you'd have this type of issue even if you were storing raw IPs. From 8a7b94ce157c1042a66f32005b3a6f285a962ec4 Mon Sep 17 00:00:00 2001 From: Mason Simon Date: Tue, 29 May 2018 09:50:50 -0700 Subject: [PATCH 10/13] Add changelog; remove Arch package link until updated --- README.md | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 1010e39..5893498 100644 --- a/README.md +++ b/README.md @@ -9,6 +9,7 @@ * [Security Model](#security-model) * [Threat Model](#threat-model) * [Usage](#usage) +* [Changelog](#changelog) * [GDPR](#gdpr) * [YAGNI](#yagni) * [License](#license) @@ -40,10 +41,6 @@ Scenario (2) is defended against because the server operator does not know the s `ipscrub` can be built statically with nginx or as a [dynamic module](https://www.nginx.com/blog/compiling-dynamic-modules-nginx-plus/). See the `Makefile` for examples of both ways. -#### Packages - -- [Arch Linux](https://aur.archlinux.org/packages/nginx-mod-ipscrub/) (thanks to [deep-42-thought](https://github.com/deep-42-thought)) - ### Configuration In your `nginx.conf`, @@ -59,6 +56,11 @@ In your `nginx.conf`, `make test` +### Changelog + +- 1.0.1 fixed vulnerability to unmasking hashed IPs (thanks to [@marcan](https://github.com/marcan)) +- 1.0.0 initial release + ## GDPR [GDPR](https://www.eugdpr.org) goes into effect on May 25, 2018. It legislates the handling of personal data about your users, including IP addresses. From faaee67199b7ac4d170fd8383166e9ce5ddc6da7 Mon Sep 17 00:00:00 2001 From: Mason Simon Date: Tue, 29 May 2018 10:22:10 -0700 Subject: [PATCH 11/13] Support for checking for updates --- Makefile | 3 +++ README.md | 10 ++++++++-- script/check-up-to-date.sh | 12 ++++++++++++ 3 files changed, 23 insertions(+), 2 deletions(-) create mode 100755 script/check-up-to-date.sh diff --git a/Makefile b/Makefile index 2899844..ebd4dfb 100644 --- a/Makefile +++ b/Makefile @@ -61,3 +61,6 @@ run-demo: docker build -t ipscrub . docker build -t ipscrub-demo-client demo/ docker-compose up --abort-on-container-exit + +check-up-to-date: script/check-up-to-date.sh + $< diff --git a/README.md b/README.md index 5893498..104f495 100644 --- a/README.md +++ b/README.md @@ -41,7 +41,7 @@ Scenario (2) is defended against because the server operator does not know the s `ipscrub` can be built statically with nginx or as a [dynamic module](https://www.nginx.com/blog/compiling-dynamic-modules-nginx-plus/). See the `Makefile` for examples of both ways. -### Configuration +#### Configuration In your `nginx.conf`, @@ -52,10 +52,16 @@ In your `nginx.conf`, **NOTE**: nginx may still leak IP addresses in the error log. If this is a concern, disable error logging or wipe the log regularly. -### Running Tests +#### Running Tests `make test` +#### Checking for Updates + +`make check-up-to-date` + +This will have a non-zero exit code if you aren't up-to-date, so you can automate regular checks. + ### Changelog - 1.0.1 fixed vulnerability to unmasking hashed IPs (thanks to [@marcan](https://github.com/marcan)) diff --git a/script/check-up-to-date.sh b/script/check-up-to-date.sh new file mode 100755 index 0000000..a15e09a --- /dev/null +++ b/script/check-up-to-date.sh @@ -0,0 +1,12 @@ +#!/bin/bash + +echo "Checking..." +latest=`curl "https://api.github.com/repos/masonicboom/ipscrub/releases/latest" 2>/dev/null | grep --extended-regexp -o "\"tag_name\":\s+\".*\"" | cut -d '"' -f 4` +current=`git tag | sort | tail -n 1` + +if [ $current != $latest ]; then + echo "Current version is $current; latest is $latest. Check https://github.com/masonicboom/ipscrub/releases to see if you should update." + exit 1 +else + echo "You are on the latest version of ipscrub ($current)." +fi \ No newline at end of file From daa1540e03031eb3bf24688128f557094415625f Mon Sep 17 00:00:00 2001 From: Mason Simon Date: Tue, 29 May 2018 10:25:18 -0700 Subject: [PATCH 12/13] Resurrect link to Arch Linux package --- README.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/README.md b/README.md index 104f495..c4d4565 100644 --- a/README.md +++ b/README.md @@ -41,6 +41,10 @@ Scenario (2) is defended against because the server operator does not know the s `ipscrub` can be built statically with nginx or as a [dynamic module](https://www.nginx.com/blog/compiling-dynamic-modules-nginx-plus/). See the `Makefile` for examples of both ways. +#### Packages + +- [Arch Linux](https://aur.archlinux.org/packages/nginx-mod-ipscrub/) (thanks to [deep-42-thought](https://github.com/deep-42-thought)) + #### Configuration In your `nginx.conf`, From ecfb7dbcf568cfb2aebbdfd7e7834abc451fc25b Mon Sep 17 00:00:00 2001 From: Mason Simon Date: Tue, 29 May 2018 10:26:41 -0700 Subject: [PATCH 13/13] Use @ signs consistently for GitHub users --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index c4d4565..fdd5c98 100644 --- a/README.md +++ b/README.md @@ -43,7 +43,7 @@ Scenario (2) is defended against because the server operator does not know the s #### Packages -- [Arch Linux](https://aur.archlinux.org/packages/nginx-mod-ipscrub/) (thanks to [deep-42-thought](https://github.com/deep-42-thought)) +- [Arch Linux](https://aur.archlinux.org/packages/nginx-mod-ipscrub/) (thanks to [@deep-42-thought](https://github.com/deep-42-thought)) #### Configuration