Hello and thank you for the code review,

On 2018-10-15 16:27, Maxim Dounin wrote:
> Hello!
>
> On Fri, Oct 12, 2018 at 09:54:04PM +0200, Paul Pawlowski via
> nginx-devel wrote:
>
>> # HG changeset patch
>> # User Paul Pawlowski <paul@mrarm.io>
>> # Date 1539371172 -7200
>> # Fri Oct 12 21:06:12 2018 +0200
>> # Node ID 67cc676dbfaf56332bb6c61e635c0073c1e49907
>> # Parent 8b68d50090e4f134a35da60146fefd5e63770759
>> Add client_body_temp_access configuration directive
>
> Style: there should be a trailing dot and a reference to ticket
> you've created, and please use past tense. Also, I think that
> "client_body_access" might be a better name:
>
> Added client_body_access directive (ticket #1653).
>
>> Adds client_body_temp_access configuration directive, which sets the
>> unix permissions of the temporary files holding client request bodies.
>>
>> This makes it possible for a process running as another user to access
>> the files stored using client_body_temp_path and
>> client_body_in_file_only.
>> This is useful when using the mentioned directives in order to have
>> nginx store the request body to file and forward the file path to
>> another server running on the same machine as another user.
>
> Style: please wrap lines at 80 characters or less.
>
I've addressed the style concerns, as well as changed the name to the
suggested one.
>>
>> diff -r 8b68d50090e4 -r 67cc676dbfaf contrib/vim/syntax/nginx.vim
>> --- a/contrib/vim/syntax/nginx.vim Wed Oct 03 14:08:51 2018 +0300
>> +++ b/contrib/vim/syntax/nginx.vim Fri Oct 12 21:06:12 2018 +0200
>> @@ -155,6 +155,7 @@
>> syn keyword ngxDirective contained chunked_transfer_encoding
>> syn keyword ngxDirective contained client_body_buffer_size
>> syn keyword ngxDirective contained client_body_in_file_only
>> +syn keyword ngxDirective contained client_body_temp_access
>> syn keyword ngxDirective contained client_body_in_single_buffer
>> syn keyword ngxDirective contained client_body_temp_path
>> syn keyword ngxDirective contained client_body_timeout
>
> This list is expected to be sorted alphabetically.
>
>> diff -r 8b68d50090e4 -r 67cc676dbfaf src/http/ngx_http_core_module.c
>> --- a/src/http/ngx_http_core_module.c Wed Oct 03 14:08:51 2018 +0300
>> +++ b/src/http/ngx_http_core_module.c Fri Oct 12 21:06:12 2018 +0200
>> @@ -370,6 +370,13 @@
>> offsetof(ngx_http_core_loc_conf_t, client_body_temp_path),
>> NULL },
>>
>> + { ngx_string("client_body_temp_access"),
>> +
>> NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE123,
>> + ngx_conf_set_access_slot,
>> + NGX_HTTP_LOC_CONF_OFFSET,
>> + offsetof(ngx_http_core_loc_conf_t, client_body_temp_access),
>> + NULL },
>> +
>> { ngx_string("client_body_in_file_only"),
>>
>> NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE1,
>> ngx_conf_set_enum_slot,
>> @@ -3373,6 +3380,7 @@
>> clcf->if_modified_since = NGX_CONF_UNSET_UINT;
>> clcf->max_ranges = NGX_CONF_UNSET_UINT;
>> clcf->client_body_in_file_only = NGX_CONF_UNSET_UINT;
>> + clcf->client_body_temp_access = NGX_CONF_UNSET_UINT;
>> clcf->client_body_in_single_buffer = NGX_CONF_UNSET;
>> clcf->internal = NGX_CONF_UNSET;
>> clcf->sendfile = NGX_CONF_UNSET;
>> @@ -3594,6 +3602,8 @@
>> ngx_conf_merge_uint_value(conf->client_body_in_file_only,
>> prev->client_body_in_file_only,
>> NGX_HTTP_REQUEST_BODY_FILE_OFF);
>> + ngx_conf_merge_uint_value(conf->client_body_temp_access,
>> + prev->client_body_temp_access, 0);
>
> Explicitly using 0600 as the default should be a better option.
I have changed the default value to 0600.
>
>> ngx_conf_merge_value(conf->client_body_in_single_buffer,
>> prev->client_body_in_single_buffer, 0);
>> ngx_conf_merge_value(conf->internal, prev->internal, 0);
>> diff -r 8b68d50090e4 -r 67cc676dbfaf src/http/ngx_http_core_module.h
>> --- a/src/http/ngx_http_core_module.h Wed Oct 03 14:08:51 2018 +0300
>> +++ b/src/http/ngx_http_core_module.h Fri Oct 12 21:06:12 2018 +0200
>> @@ -375,6 +375,7 @@
>> ngx_uint_t if_modified_since; /* if_modified_since */
>> ngx_uint_t max_ranges; /* max_ranges */
>> ngx_uint_t client_body_in_file_only; /*
>> client_body_in_file_only */
>> + ngx_uint_t client_body_temp_access; /* client_body_temp_access
>> */
>>
>> ngx_flag_t client_body_in_single_buffer;
>> /*
>> client_body_in_singe_buffer */
>> diff -r 8b68d50090e4 -r 67cc676dbfaf src/http/ngx_http_request_body.c
>> --- a/src/http/ngx_http_request_body.c Wed Oct 03 14:08:51 2018 +0300
>> +++ b/src/http/ngx_http_request_body.c Fri Oct 12 21:06:12 2018 +0200
>> @@ -450,6 +450,7 @@
>> tf->file.fd = NGX_INVALID_FILE;
>> tf->file.log = r->connection->log;
>> tf->path = clcf->client_body_temp_path;
>> + tf->access = clcf->client_body_temp_access;
>> tf->pool = r->pool;
>> tf->warn = "a client request body is buffered to a temporary
>> file";
>> tf->log_level = r->request_body_file_log_level;
>
> The following block:
>
> if (r->request_body_file_group_access) {
> tf->access = 0660;
> }
>
> raises the question on how it is expected to interact with
> r->request_body_file_group_access. I suspect that
> r->request_body_file_group_access is not currently needed and can
> be removed altogether, but this needs an additional investigation.
I've checked where is it used - the only place is the
ngx_http_dav_module, where it is used in order to set the permissions
for the temporary uploaded file in the PUT handler - however, as the
file is later moved for storage using ngx_ext_rename_file function (in
ngx_http_dav_put_handler) with permissions taken from the configuration
file (with a default value of 0600) I do not believe that the temporary
file permission matter at that stage.

The usage by third party modules also seem limited:
I've tried to do a GitHub search for it:
https://github.com/search?q=request_body_file_group_access+-filename%3Angx_http_request_body.c+-filename%3Angx_http_request.h+-filename%3Angx_http_dav_module.c&type=Code
which results in 1,123 code results. However most of these are simply
slightly altered versions of ngx_http_request_body.c, and if we exclude
the top three results we end up with 264 results:
https://github.com/search?q=request_body_file_group_access+-filename%3Angx_http_request_body.c+-filename%3Angx_http_request.h+-filename%3Angx_http_dav_module.c+-filename%3Angx_http_lua_req_body.c+-filename%3Angx_http_waf_filter_module.c+-filename%3Angx_http_spdy.c&type=Code
with most of them still largely being nginx codebase - however a few
usages can still be found including:
-
https://github.com/oscar810429/graphicsmagick_nginx_module/blob/6f171579ba7638f62f885d3d8e0687d7977f2f33/ngx_http_graphicsmagick_module.c
[I don't believe it is needed here - the file is not forwarded to other
users I believe after taking a quick look at the code]
-
https://github.com/heapsource/BlackLinks/blob/8173ba44270d65cbe50aa2865eab91af3d44f3d4/nginx-hello/ngx_http_hello_world_module.c
-
https://github.com/alacner/nginx_lua_module/blob/e194c26f96e50504540fd5830283e1c01670d170/src/ngx_http_lua_module.c
-
https://github.com/AntonRiab/ngx_pgcopy/blob/ca32432024d0677754faddbe4d8b45c5d896c79b/ngx_http_pgcopy_module.c
Even though I believe they should be replaceable, is this something
I should look into more?

I'm making the deletion of the request_body_file_group_access a separate
commit, as this touches code not directly related to the core added
functionality.
>
> Another question to consider is how this is expected to interact
> with directories created for various temp file levels. These
> directories, much like the client_body_temp_path diretory itself,
> are unconditionally created with 0700 access, and will prevent
> access to temporary files from non-nginx user as well. These can
> be pre-created with appropriate permissions - but as long as it is
> a recommended approach, this probably needs to be mentioned in the
> commit log.
This is an issue I have completely missed while writing this patch.
I have checked how do the other nginx module solve this particular
issue and figured out that probably the best option would be to use
the specified file permissions as the ones to use (using the
ngx_dir_access helper function), and this is what I've implemented.




I'm unfortunately unsure what is the proper way to E-Mail the patches in
a response mail so I'm simply attaching them to the E-Mail below
(there are two separate commits below):

# HG changeset patch
# User Paul Pawlowski <paul@mrarm.io>
# Date 1539621300 -7200
# Mon Oct 15 18:35:00 2018 +0200
# Node ID c94787463c982dab370b695e2f3ddcbc7655ead5
# Parent 8b68d50090e4f134a35da60146fefd5e63770759
Added client_body_access configuration directive (ticket #1651).

Added client_body_access configuration directive, which sets the unix
permissions of the temporary files and directories holding client
request
bodies.

This makes it possible for a process running as another user to access
the files stored using client_body_temp_path and
client_body_in_file_only.
This is useful when using the mentioned directives in order to have
nginx store
the request body to file and forward the file path to another server
running on
the same machine as another user.

diff -r 8b68d50090e4 -r c94787463c98 contrib/vim/syntax/nginx.vim
--- a/contrib/vim/syntax/nginx.vim Wed Oct 03 14:08:51 2018 +0300
+++ b/contrib/vim/syntax/nginx.vim Mon Oct 15 18:35:00 2018 +0200
@@ -153,6 +153,7 @@
syn keyword ngxDirective contained charset
syn keyword ngxDirective contained charset_types
syn keyword ngxDirective contained chunked_transfer_encoding
+syn keyword ngxDirective contained client_body_access
syn keyword ngxDirective contained client_body_buffer_size
syn keyword ngxDirective contained client_body_in_file_only
syn keyword ngxDirective contained client_body_in_single_buffer
diff -r 8b68d50090e4 -r c94787463c98 src/core/ngx_file.c
--- a/src/core/ngx_file.c Wed Oct 03 14:08:51 2018 +0300
+++ b/src/core/ngx_file.c Mon Oct 15 18:35:00 2018 +0200
@@ -146,10 +146,17 @@
uint32_t n;
ngx_err_t err;
ngx_str_t name;
+ ngx_uint_t path_access;
ngx_uint_t prefix;
ngx_pool_cleanup_t *cln;
ngx_pool_cleanup_file_t *clnf;

+ if (access != 0) {
+ path_access = ngx_dir_access(access);
+ } else {
+ path_access = 0700;
+ }
+
if (file->name.len) {
name = file->name;
levels = 0;
@@ -230,7 +237,7 @@
return NGX_ERROR;
}

- if (ngx_create_path(file, path) == NGX_ERROR) {
+ if (ngx_create_path(file, path, path_access) == NGX_ERROR) {
return NGX_ERROR;
}
}
@@ -263,7 +270,7 @@


ngx_int_t
-ngx_create_path(ngx_file_t *file, ngx_path_t *path)
+ngx_create_path(ngx_file_t *file, ngx_path_t *path, ngx_uint_t access)
{
size_t pos;
ngx_err_t err;
@@ -283,7 +290,7 @@
ngx_log_debug1(NGX_LOG_DEBUG_CORE, file->log, 0,
"temp file: \"%s\"", file->name.data);

- if (ngx_create_dir(file->name.data, 0700) == NGX_FILE_ERROR) {
+ if (ngx_create_dir(file->name.data, access) == NGX_FILE_ERROR)
{
err = ngx_errno;
if (err != NGX_EEXIST) {
ngx_log_error(NGX_LOG_CRIT, file->log, err,
diff -r 8b68d50090e4 -r c94787463c98 src/core/ngx_file.h
--- a/src/core/ngx_file.h Wed Oct 03 14:08:51 2018 +0300
+++ b/src/core/ngx_file.h Mon Oct 15 18:35:00 2018 +0200
@@ -140,7 +140,8 @@
ngx_pool_t *pool, ngx_uint_t persistent, ngx_uint_t clean,
ngx_uint_t access);
void ngx_create_hashed_filename(ngx_path_t *path, u_char *file, size_t
len);
-ngx_int_t ngx_create_path(ngx_file_t *file, ngx_path_t *path);
+ngx_int_t ngx_create_path(ngx_file_t *file, ngx_path_t *path,
+ ngx_uint_t access);
ngx_err_t ngx_create_full_path(u_char *dir, ngx_uint_t access);
ngx_int_t ngx_add_path(ngx_conf_t *cf, ngx_path_t **slot);
ngx_int_t ngx_create_paths(ngx_cycle_t *cycle, ngx_uid_t user);
diff -r 8b68d50090e4 -r c94787463c98 src/http/ngx_http_core_module.c
--- a/src/http/ngx_http_core_module.c Wed Oct 03 14:08:51 2018 +0300
+++ b/src/http/ngx_http_core_module.c Mon Oct 15 18:35:00 2018 +0200
@@ -370,6 +370,13 @@
offsetof(ngx_http_core_loc_conf_t, client_body_temp_path),
NULL },

+ { ngx_string("client_body_access"),
+
NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE123,
+ ngx_conf_set_access_slot,
+ NGX_HTTP_LOC_CONF_OFFSET,
+ offsetof(ngx_http_core_loc_conf_t, client_body_access),
+ NULL },
+
{ ngx_string("client_body_in_file_only"),

NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE1,
ngx_conf_set_enum_slot,
@@ -3373,6 +3380,7 @@
clcf->if_modified_since = NGX_CONF_UNSET_UINT;
clcf->max_ranges = NGX_CONF_UNSET_UINT;
clcf->client_body_in_file_only = NGX_CONF_UNSET_UINT;
+ clcf->client_body_access = NGX_CONF_UNSET_UINT;
clcf->client_body_in_single_buffer = NGX_CONF_UNSET;
clcf->internal = NGX_CONF_UNSET;
clcf->sendfile = NGX_CONF_UNSET;
@@ -3594,6 +3602,8 @@
ngx_conf_merge_uint_value(conf->client_body_in_file_only,
prev->client_body_in_file_only,
NGX_HTTP_REQUEST_BODY_FILE_OFF);
+ ngx_conf_merge_uint_value(conf->client_body_access,
+ prev->client_body_access, 0600);
ngx_conf_merge_value(conf->client_body_in_single_buffer,
prev->client_body_in_single_buffer, 0);
ngx_conf_merge_value(conf->internal, prev->internal, 0);
diff -r 8b68d50090e4 -r c94787463c98 src/http/ngx_http_core_module.h
--- a/src/http/ngx_http_core_module.h Wed Oct 03 14:08:51 2018 +0300
+++ b/src/http/ngx_http_core_module.h Mon Oct 15 18:35:00 2018 +0200
@@ -375,6 +375,7 @@
ngx_uint_t if_modified_since; /* if_modified_since */
ngx_uint_t max_ranges; /* max_ranges */
ngx_uint_t client_body_in_file_only; /* client_body_in_file_only
*/
+ ngx_uint_t client_body_access; /* client_body_access */

ngx_flag_t client_body_in_single_buffer;
/*
client_body_in_singe_buffer */
diff -r 8b68d50090e4 -r c94787463c98 src/http/ngx_http_request_body.c
--- a/src/http/ngx_http_request_body.c Wed Oct 03 14:08:51 2018 +0300
+++ b/src/http/ngx_http_request_body.c Mon Oct 15 18:35:00 2018 +0200
@@ -450,6 +450,7 @@
tf->file.fd = NGX_INVALID_FILE;
tf->file.log = r->connection->log;
tf->path = clcf->client_body_temp_path;
+ tf->access = clcf->client_body_access;
tf->pool = r->pool;
tf->warn = "a client request body is buffered to a temporary
file";
tf->log_level = r->request_body_file_log_level;






# HG changeset patch
# User Paul Pawlowski <paul@mrarm.io>
# Date 1539623192 -7200
# Mon Oct 15 19:06:32 2018 +0200
# Node ID e6f5c3e20646abe8708db185e5acdd861c13ce35
# Parent c94787463c982dab370b695e2f3ddcbc7655ead5
Removed request_body_file_group_access from ngx_http_request_s.

request_body_file_group_access's only usage is in the
ngx_http_dav_module.
However the temporary file created by that module after a successful
upload is
moved into another directory, and the file permissions are changed to
user
specified ones. Therefore, it doesn't matter what the initial file
permissions
were and the code can be simplified.

diff -r c94787463c98 -r e6f5c3e20646
src/http/modules/ngx_http_dav_module.c
--- a/src/http/modules/ngx_http_dav_module.c Mon Oct 15 18:35:00 2018
+0200
+++ b/src/http/modules/ngx_http_dav_module.c Mon Oct 15 19:06:32 2018
+0200
@@ -170,7 +170,6 @@
r->request_body_in_file_only = 1;
r->request_body_in_persistent_file = 1;
r->request_body_in_clean_file = 1;
- r->request_body_file_group_access = 1;
r->request_body_file_log_level = 0;

rc = ngx_http_read_client_request_body(r,
ngx_http_dav_put_handler);
diff -r c94787463c98 -r e6f5c3e20646 src/http/ngx_http_request.h
--- a/src/http/ngx_http_request.h Mon Oct 15 18:35:00 2018 +0200
+++ b/src/http/ngx_http_request.h Mon Oct 15 19:06:32 2018 +0200
@@ -482,7 +482,6 @@
unsigned request_body_in_file_only:1;
unsigned
request_body_in_persistent_file:1;
unsigned request_body_in_clean_file:1;
- unsigned request_body_file_group_access:1;
unsigned request_body_file_log_level:3;
unsigned request_body_no_buffering:1;

diff -r c94787463c98 -r e6f5c3e20646 src/http/ngx_http_request_body.c
--- a/src/http/ngx_http_request_body.c Mon Oct 15 18:35:00 2018 +0200
+++ b/src/http/ngx_http_request_body.c Mon Oct 15 19:06:32 2018 +0200
@@ -457,10 +457,6 @@
tf->persistent = r->request_body_in_persistent_file;
tf->clean = r->request_body_in_clean_file;

- if (r->request_body_file_group_access) {
- tf->access = 0660;
- }
-
rb->temp_file = tf;

if (rb->bufs == NULL) {
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel