From 0dad7d5ad91870047244e54c4bab6dea350b87ca Mon Sep 17 00:00:00 2001 From: Markus Kuhn Date: Thu, 1 Jun 2017 16:30:50 +0100 Subject: [PATCH 1/5] add ability to load AACookieKey from file --- mod_ucam_webauth.c | 51 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 49 insertions(+), 2 deletions(-) diff --git a/mod_ucam_webauth.c b/mod_ucam_webauth.c index 53a5fce..9651173 100644 --- a/mod_ucam_webauth.c +++ b/mod_ucam_webauth.c @@ -1879,7 +1879,7 @@ dump_config(request_rec *r, apr_pool_t *p, /* --- */ /* Note that most string and flag parameters are processed by the generic - ap_set_string_slot and ap_set flag_slot routines */ + ap_set_string_slot and ap_set_flag_slot routines */ static const char * set_response_timeout(cmd_parms *cmd, @@ -1996,6 +1996,53 @@ set_cache_control(cmd_parms *cmd, /* --- */ +static const char * +set_cookie_key(cmd_parms *cmd, + void *mconfig, + const char *arg) + +{ + + mod_ucam_webauth_cfg *cfg = (mod_ucam_webauth_cfg *)mconfig; + const char *path; + apr_file_t *file; + apr_size_t len = 20; /* maximum number of bytes to read from file */ + apr_status_t status; + int i; + + if (strncmp(arg, "file:", 5) == 0) { + if (ap_check_cmd_context(cmd, NOT_IN_HTACCESS)) + return "AACookieKey: 'file:' access not permitted in .htaccess"; + if (!arg[5]) return "AACookieKey: empty filename"; + path = ap_server_root_relative(cmd->pool, arg + 5); + status = apr_file_open(&file, path, APR_FOPEN_READ | APR_FOPEN_BINARY, + 0, cmd->pool); + if (status != APR_SUCCESS) return "AACookieKey: cannot open file"; + cfg->cookie_key = apr_pcalloc(cmd->pool, len+1); + if (!cfg->cookie_key) return "AACookieKey: apr_pcalloc() == NULL"; + apr_file_read(file, cfg->cookie_key, &len); + if (len < 16) return "AACookieKey: file too short (16-20 bytes required)"; + for (i = 0; i < len; i++) { + /* Supress \0 in binary input, so result can be handled as string. */ + if (cfg->cookie_key[i] == 0) + cfg->cookie_key[i] = '@'; + } + cfg->cookie_key[len] = '\0'; + apr_file_close(file); + } else { + cfg->cookie_key = (char *) arg; + } + + ap_log_error(APLOG_MARK, APLOG_DEBUG|APLOG_STARTUP, 0, cmd->server, + "setting cookie_key = '%s' (%lu bytes)", + cfg->cookie_key, strlen(cfg->cookie_key)); + + return NULL; + +} + +/* --- */ + static const char * set_log_level(cmd_parms *cmd, void *mconfig, @@ -3182,7 +3229,7 @@ static const command_rec webauth_commands[] = { "re-use of cached content"), AP_INIT_TAKE1("AACookieKey", - ap_set_string_slot, + set_cookie_key, (void *)APR_OFFSETOF (mod_ucam_webauth_cfg,cookie_key), RSRC_CONF | OR_AUTHCFG, From cff657b667ea7583c2d408ddf50cd0c0630c4323 Mon Sep 17 00:00:00 2001 From: Markus Kuhn Date: Fri, 2 Jun 2017 14:15:33 +0100 Subject: [PATCH 2/5] extend file: support to AAHeaderKey, improved error messages also extended maximum key length to be read from file to 64 bytes = 512 bit = SHA-1 block length --- mod_ucam_webauth.c | 73 +++++++++++++++++++++++++++++----------------- 1 file changed, 47 insertions(+), 26 deletions(-) diff --git a/mod_ucam_webauth.c b/mod_ucam_webauth.c index 9651173..ca00309 100644 --- a/mod_ucam_webauth.c +++ b/mod_ucam_webauth.c @@ -1996,46 +1996,67 @@ set_cache_control(cmd_parms *cmd, /* --- */ +/* process argument of AACookieKey and AAHeaderKey */ static const char * -set_cookie_key(cmd_parms *cmd, - void *mconfig, - const char *arg) +set_key(cmd_parms *cmd, + void *mconfig, + const char *arg) { - - mod_ucam_webauth_cfg *cfg = (mod_ucam_webauth_cfg *)mconfig; + int offset = (int)(long)cmd->info; + char **key = (char **)((char *)mconfig + offset); + const char *directive = + cmd->directive->directive; /* "AACookieKey" or "AAHeaderKey" */ const char *path; apr_file_t *file; - apr_size_t len = 20; /* maximum number of bytes to read from file */ + apr_size_t len = 64; /* maximum number of bytes to read from file */ apr_status_t status; + char buf[256]; int i; if (strncmp(arg, "file:", 5) == 0) { - if (ap_check_cmd_context(cmd, NOT_IN_HTACCESS)) - return "AACookieKey: 'file:' access not permitted in .htaccess"; - if (!arg[5]) return "AACookieKey: empty filename"; - path = ap_server_root_relative(cmd->pool, arg + 5); + /* load HMAC key bytes from file */ + if (ap_check_cmd_context(cmd, NOT_IN_HTACCESS)) { + /* Reasons for not allowing 'file:' access in .htaccess: + * - file would be read for each request (performance) + * - could be abused by someone with control over + * a .htaccess file to extract confidential information + * from a file only readable to the Apache process. + * - cmd->temp_pool no longer available + */ + return "AACookieKey/AAHeaderKey: 'file:' key not permitted in .htaccess"; + } + if (!arg[5] || !(path = ap_server_root_relative(cmd->temp_pool, arg + 5))) { + return apr_pstrcat(cmd->pool, directive, " 'file:", arg+5, + "': invalid file path", NULL); + } status = apr_file_open(&file, path, APR_FOPEN_READ | APR_FOPEN_BINARY, - 0, cmd->pool); - if (status != APR_SUCCESS) return "AACookieKey: cannot open file"; - cfg->cookie_key = apr_pcalloc(cmd->pool, len+1); - if (!cfg->cookie_key) return "AACookieKey: apr_pcalloc() == NULL"; - apr_file_read(file, cfg->cookie_key, &len); - if (len < 16) return "AACookieKey: file too short (16-20 bytes required)"; + 0, cmd->temp_pool); + if (status != APR_SUCCESS) + return apr_pstrcat(cmd->pool, directive, " 'file:", + path, "': ", apr_strerror(status, buf, sizeof(buf)), + NULL); + *key = apr_pcalloc(cmd->pool, len+1); + if (!*key) return "apr_pcalloc() == NULL"; + apr_file_read(file, *key, &len); + apr_file_close(file); + if (len < 16) + return apr_pstrcat(cmd->pool, directive, " 'file:", arg+5, + "': key file too short (16 bytes required)", NULL); + /* Substitute \0 in binary input, so key can be handled as string. */ for (i = 0; i < len; i++) { - /* Supress \0 in binary input, so result can be handled as string. */ - if (cfg->cookie_key[i] == 0) - cfg->cookie_key[i] = '@'; + if ((*key)[i] == 0) + (*key)[i] = '@'; } - cfg->cookie_key[len] = '\0'; - apr_file_close(file); + (*key)[len] = '\0'; } else { - cfg->cookie_key = (char *) arg; + /* use string argument directly as HMAC key */ + *key = (char *) arg; } ap_log_error(APLOG_MARK, APLOG_DEBUG|APLOG_STARTUP, 0, cmd->server, - "setting cookie_key = '%s' (%lu bytes)", - cfg->cookie_key, strlen(cfg->cookie_key)); + "setting %s '%-.4s...' (%lu bytes)", + directive, *key, strlen(*key)); return NULL; @@ -3229,7 +3250,7 @@ static const command_rec webauth_commands[] = { "re-use of cached content"), AP_INIT_TAKE1("AACookieKey", - set_cookie_key, + set_key, (void *)APR_OFFSETOF (mod_ucam_webauth_cfg,cookie_key), RSRC_CONF | OR_AUTHCFG, @@ -3343,7 +3364,7 @@ static const command_rec webauth_commands[] = { "a list of additional headers to include in the request"), AP_INIT_TAKE1("AAHeaderKey", - ap_set_string_slot, + set_key, (void *)APR_OFFSETOF (mod_ucam_webauth_cfg,header_key), RSRC_CONF | OR_AUTHCFG, From bf32ae2d6466e2bcd528ff365991566ba66b4274 Mon Sep 17 00:00:00 2001 From: Markus Kuhn Date: Fri, 2 Jun 2017 15:18:20 +0100 Subject: [PATCH 3/5] updated documentation: AACookieKey/AAHeaderKey file:... --- README.Config | 65 ++++++++++++++++++++++++++++++++++----------------- 1 file changed, 43 insertions(+), 22 deletions(-) diff --git a/README.Config b/README.Config index ff2cbf0..50c5667 100644 --- a/README.Config +++ b/README.Config @@ -377,7 +377,7 @@ AACacheControl AACookieKey - Syntax: AACookieKey "text" + Syntax: AACookieKey "text" | "file:filename" Context: all Override: AuthConfig Module: mod_ucam_webauth @@ -389,16 +389,44 @@ AACookieKey cookies (see AACookieName, AACookiePath, AACookieDomain below). This parameter has no default and must be set. + If the parameter starts with "file:", the key will instead be read + from the file named in the rest of the string. Relative paths will + be interpreted from the server root. The key will be formed from the + first 64 bytes of the named file, or the entire file, whichever is + shorter. If the file does not provide at least 16 bytes, the server + will abort with an error. Using the "file:" prefix is not permitted + in .htaccess files. + + On Linux, create a separate cookie.key file with e.g. + + $ sudo bash + # ( umask 0077 ; head -c 64 /dev/random >/etc/apache2/cookie.key ) + + and then use that with + + AACookieKey file:cookie.key + + Alternatively, + + AACookieKey file:/dev/urandom + + will request from the kernel's random-byte generator a new cookie + key each time the server rereads its configuration, and avoids a + secret on the file system. + The key must not disclosed, since with knowledge of the key an - attacker can forge authentication. For preference, AACookieKey - should be placed in a file that is only readable by the user used to - start Apache, commonly root. One way to achieve this, while leaving - the main configuration file mainly readable, would be to put the - AACookieKey directive in a file only readable by root (or whoever) - that is incorporated into the main configuration using the 'Include' - directive. It may also be necessary to disable the standard mod_info - module since this can be use to display the Apache configuration and - this will include the value assigned to AACookieKey. + attacker can forge authentication. Putting the key directly into + a main configuration file can cause two problems: + + - Read access to this Apache configuration file must be + restricted. + + - Access to the server information page generated by the standard + mod_info module must also be restricted, since that will also + reveal the parameter of the AACookieKey directive. + + Storing the key in a separate cookie.key file, only readable to + root, avoids both these risks. AACookieName @@ -683,7 +711,7 @@ AAHeaders AAHeaderKey - Syntax: AAHeaderKey "text" | none + Syntax: AAHeaderKey "text" | "file:filename" | none Context: all Override: AuthConfig Module: mod_ucam_webauth @@ -697,17 +725,10 @@ AAHeaderKey string 'none' in which case no hash generation will take place. Knowledge of this key could allow an attacker to forge a request - containing what appear to be header values set by this module. As - with AACookieKey, for preference this directive should be placed in - a file that is only readable by the user used to start Apache, - commonly root. One way to achieve this, while leaving the main - configuration file mainly readable, would be to put the AACookieKey - directive in a file only readable by root (or whoever) that is - incorporated into the main configuration using the 'Include' - directive. It may also be necessary to disable the standard mod_info - module since this can be use to display the Apache configuration and - this will include the value assigned to AAHeaderKey. - + containing what appear to be header values set by this module. See + the description of AACookieKey for more information on how to read + the key from a file only readable by the user used to start Apache, + commonly root. AAIgnoreResponseLife From 7d2f1c0e2482d3e13438e8d60ed50c703cf99891 Mon Sep 17 00:00:00 2001 From: Markus Kuhn Date: Fri, 2 Jun 2017 15:33:52 +0100 Subject: [PATCH 4/5] update examples to Apache 2.4 authorization directives --- README.Config | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/README.Config b/README.Config index 50c5667..0f6a299 100644 --- a/README.Config +++ b/README.Config @@ -146,26 +146,29 @@ a particular directory to anybody with a Ucam-WebAuth login is, e.g. An alternative simple configuration will allow access to the resources -to users with a Ucam-WebAuth login or to client computers with -hostnames ending .cam.ac.uk, but not otherwise: +to users with a Ucam-WebAuth login, to client computers with hostnames +ending .cam.ac.uk, or to those using a CUDN IPv6 address, but not +otherwise: AACookieKey "some random string" - Order allow,deny - Allow from .cam.ac.uk + Require host .cam.ac.uk + Require ip 2001:630:210::/44 AuthType Ucam-WebAuth Require valid-user - Satisfy any In these examples the resources to be protected are selected by a directive; resources specified by or directives can also be protected in the same way. 'AuthType' must be -set to 'Ucam-WebAuth'. A 'Require' directive must appear before -authentication will take place. - -See standard Apache documentation for more details of the 'Order', -'Allow', 'Require', and 'Satisfy' directives. +set to 'Ucam-WebAuth'. A directive 'Require user', 'Require group' or +'Require valid-user' must appear before authentication will take +place. + +See standard Apache 2.4 documentation for more details of the +'Require' directive. (Apache 2.2 and earlier use a more complictated +authorization language involving 'Order', 'Allow', 'Require', and +'Satisfy' directives.) The AACookieKey directive is required, though it can appear either outside the block, in which case it provides a default for From f8bd0846fc42e34637d2ce81468f5c612d41f85e Mon Sep 17 00:00:00 2001 From: Markus Kuhn Date: Fri, 2 Jun 2017 19:01:46 +0100 Subject: [PATCH 5/5] add two documentation notes on file:/dev/urandom suggested by jw35 --- README.Config | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/README.Config b/README.Config index 0f6a299..6435528 100644 --- a/README.Config +++ b/README.Config @@ -415,7 +415,12 @@ AACookieKey will request from the kernel's random-byte generator a new cookie key each time the server rereads its configuration, and avoids a - secret on the file system. + secret on the file system. However, this will trigger a round trip + to Raven for all currently-authenticated users every time Apache is + restarted/reloaded. While not a major issue, this will, for example, + result in the loss of POST data. (On the other hand, using + "AAHeaderKey file:/dev/urandom" would be pointless: the whole point + of AAHeaderKey is to establish a shared secret with other systems.) The key must not disclosed, since with knowledge of the key an attacker can forge authentication. Putting the key directly into