Re: [PATCH] SSL: Add ENGINE_init() calls before using engines.

[Anderson Sasaki]

Hello,

Thanks again for the feedback.

> In no particular order:
> 
> - Should be "SSL: added ..." (no capital letter after a semicolon,
>   prefer past tense).
> 
> - An empty line after the summary.
> 
> - Please prefer double spacing.
> 
> - "uniNItialized"

The proposed changes were applied in the new patch version.

> > diff -r 46c0c7ef4913 -r f5b0a7910922 src/event/ngx_event_openssl.c
> > --- a/src/event/ngx_event_openssl.c Wed Apr 25 14:57:24 2018 +0300
> > +++ b/src/event/ngx_event_openssl.c Fri Apr 27 16:58:16 2018 +0200
> > @@ -527,6 +527,13 @@
> >  return NGX_ERROR;
> >  }
> >  
> > +if (!ENGINE_init(engine)) {
> 
> As previously noted at [1], this may need ENGINE_finish() to avoid
> leaking loaded engines.

Added ENGINE_finish() calls.

> 
> > +ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0,
> > +  "ENGINE_init(engine) failed");
> 
> There is no reason to log static string "engine" here.  Consider
> using engine name here.

Logging the engine name instead.

The patch follows:

# HG changeset patch
# User Anderson Toshiyuki Sasaki 
# Date 1525954250 -7200
#  Thu May 10 14:10:50 2018 +0200
# Node ID e7d0387ad229ca47514f346c8b692e6f388d72e1
# Parent  ceab908790c4eb7cd01c40bd16581ef794222ca5
SSL: added ENGINE_init() call before loading key.

It is necessary to call ENGINE_init() before using an OpenSSL engine
to get the engine functional reference.  Without this, when
ENGINE_load_private_key() is called, the engine is still uninitialized.

diff -r ceab908790c4 -r e7d0387ad229 src/event/ngx_event_openssl.c
--- a/src/event/ngx_event_openssl.c Tue Apr 24 14:04:59 2018 +0300
+++ b/src/event/ngx_event_openssl.c Thu May 10 14:10:50 2018 +0200
@@ -527,6 +527,13 @@
 return NGX_ERROR;
 }
 
+if (!ENGINE_init(engine)) {
+ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0,
+  "ENGINE_init(\"%s\") failed", p);
+ENGINE_free(engine);
+return NGX_ERROR;
+}
+
 *last++ = ':';
 
 pkey = ENGINE_load_private_key(engine, (char *) last, 0, 0);
@@ -534,10 +541,12 @@
 if (pkey == NULL) {
 ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0,
   "ENGINE_load_private_key(\"%s\") failed", last);
+ENGINE_finish(engine);
 ENGINE_free(engine);
 return NGX_ERROR;
 }
 
+ENGINE_finish(engine);
 ENGINE_free(engine);
 
 if (SSL_CTX_use_PrivateKey(ssl->ctx, pkey) == 0) {
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: Is there a particular reason --with-compat isn't enabled by default?

[Maxim Dounin]

Hello!

On Thu, May 10, 2018 at 01:17:41PM +0300, Ruslan Ermilov wrote:

> On Wed, May 09, 2018 at 12:29:06PM -0400, Thomas Ward wrote:
> > In regards to several off-lists inquiries downstream about people trying
> > to add additional third party modules, I've gone and started seeking
> > justification for enabling --with-compat.
> > 
> > Downstream in Ubuntu, I'm getting pushback in that the question of "Why
> > do we need to enable this, what does it add?".  I'm trying to find that
> > justification for it, and the best I can find is Maxim's statements on a
> > 2016 email/forum thread about how it actually makes dynamic module
> > support truly work (in a nutshell).  [1]
> > 
> > Further, there's pushback about "Will package security updates and
> > patches change the module ABI on security fixes or bug fixes?".  I don't
> > have a clear answer on this, and I had this question back when dynamic
> > module support was introduced, but never got a clear answer on this
> > point.  It does beg consideration with regards to dynamic module support
> > whether a simple patch applied to the same exact NGINX version will
> > break ABI.  The way we handle security patches and such downstream is we
> > apply patches to the existing NGINX version via `quilt`, which applies
> > the patch at build time.  Whether this makes an ABI change or not I
> > couldn't say, so I'm hunting a response from you, the devs, to give me a
> > clear answer on this.
> > 
> > So, for those who didn't read everything there's two questions here:
> > 
> >  (A) Other than making dynamic module support "work better", what does
> > --with-compat actually do behind the scenes (In a nutshell)?
> 
> It enables some macros and alters some structures in a way that's
> compatible with NGINX Plus, built with the same option.  Practically
> this means that checksums of module loadable objects will be identical
> between when using F/OSS sources and when using NGINX Plus sources.
> Searching for "NGX_COMPAT" throughout the F/OSS source code will
> give enough details.

While compatibily with nginx-plus is indeed provided by --with-compat
as well, I doubt that this aspect is interesting to Thomas.

More importantly, "--with-compat" provides compatibility between 
builds with different configure options.  

Without the "--with-compat" argument, one have to use exactly the 
same  "./configure" line to build both nginx and a module.  The 
"--with-compat" option relaxes this restriction, and it is more or 
less enough to build both nginx and a module with "--with-compat".

This generally simplifies building dynamic modules, and also 
allows one to load the same module into different nginx variants 
built with different flags.

> >  (B) Will a simple patch that patches security issues or adds fixes to
> > something later on but doesn't change the core NGINX version numbering
> > change the module ABI in such a way that it'll break modules built
> > against nginx without that patch (assuming that --with-compat was added,
> > since it's apparently needed to make dynamic modules 'actually work')
> 
> If a patch is simple, this is highly unlikely.  For a patch to
> break the ABI, at least some externally visible structures should
> be changed in some backwards incompatible ways.

While it is unlikely, it is possible.  Care should be taken when 
applying patches without changing the version number.  Whether 
"--with-compat" is specified or not is irrelevant.

-- 
Maxim Dounin
http://mdounin.ru/
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: nginx log uid/gid

[Maxim Dounin]

Hello!

On Wed, May 09, 2018 at 11:00:54AM -0400, Lubos Uhliarik wrote:

> Hello nginx devel list,
> 
> I'm experiencing following situation. When nginx is started, it creates logs 
> in its log directory with following permissions:
> 
> # ls -la /var/log/nginx
> total 12
> drwxrwx---. 2 nginx root 4096 May  9 09:59 .
> drwxr-xr-x. 9 root  root 4096 May  9 07:01 ..
> -rw-r--r--. 1 root  root0 May  9 09:59 access.log
> -rw-r--r--. 1 root  root  374 May  9 09:59 error.log
> 
> But when I send USR1 signal to nginx master process (for log rotation), it 
> creates files with different owner (user specified
> in nginx configuration - in this case "nginx" user).
> 
> # rm /var/log/nginx/*.log
> # systemctl kill --signal=USR1 nginx
> # ls -la /var/log/nginx
> total 8
> drwxrwx---. 2 nginx root 4096 May  9 10:02 .
> drwxr-xr-x. 9 root  root 4096 May  9 07:01 ..
> -rw-r--r--. 1 nginx root0 May  9 10:02 access.log
> -rw-r--r--. 1 nginx root0 May  9 10:02 error.log
> 
> Is this behavior desired? I guess so, since in 
> /src/os/unix/ngx_process_cycle.c is:
> 
> if (ngx_reopen) {
> ngx_reopen = 0;
> ngx_log_error(NGX_LOG_NOTICE, cycle->log, 0, "reopening logs");
> ngx_reopen_files(cycle, ccf->user);
> ngx_signal_worker_processes(cycle,
> ngx_signal_value(NGX_REOPEN_SIGNAL));
> }
> 
> ngx_reopen_files function call has second param set (ccf->user), which is in 
> all other
> cases -1. Why do you change owner only after processing USR1 signal? This 
> causes problem,
> when nginx is restarted:

After the USR1 signal nginx have to ensure that already running 
worker processess will be able to open new log files for writing.  
To do so, it ensures that files are owned by the nginx user, and 
have at least 0600 access mode.

(Note well that by using nginx:root on /var/log/nginx you are 
allowing privilage escalation similar to one previously seen in 
Debian packages, see CVE-2016-1247.)

> # systemctl restart nginx
> Job for nginx.service failed because the control process exited with error 
> code.
> See "systemctl status nginx.service" and "journalctl -xe" for details.
> 
> # systemctl status nginx.service
> ● nginx.service - The nginx HTTP and reverse proxy server
>Loaded: loaded (/usr/lib/systemd/system/nginx.service; disabled; vendor 
> preset: disabled)
>Active: failed (Result: exit-code) since Wed 2018-05-09 10:12:21 EDT; 5s 
> ago
>   Process: 1805 ExecStart=/usr/sbin/nginx (code=exited, status=0/SUCCESS)
>   Process: 1817 ExecStartPre=/usr/sbin/nginx -t (code=exited, 
> status=1/FAILURE)
>   Process: 1816 ExecStartPre=/usr/bin/rm -f /run/nginx.pid (code=exited, 
> status=0/SUCCESS)
>  Main PID: 1806 (code=exited, status=0/SUCCESS)
> 
> May 09 10:12:21 host-172-16-36-25 systemd[1]: Starting The nginx HTTP and 
> reverse proxy server...
> May 09 10:12:21 host-172-16-36-25 nginx[1817]: nginx: [alert] could not open 
> error log file: open() "/var/log/nginx/error.log" failed (13: Permission 
> denied)
> May 09 10:12:21 host-172-16-36-25 nginx[1817]: 2018/05/09 10:12:21 [warn] 
> 1817#0: could not build optimal types_hash, you should increase either 
> types_hash_max_size: 2048 o>
> May 09 10:12:21 host-172-16-36-25 nginx[1817]: nginx: the configuration file 
> /etc/nginx/nginx.conf syntax is ok
> May 09 10:12:21 host-172-16-36-25 nginx[1817]: 2018/05/09 10:12:21 [emerg] 
> 1817#0: open() "/var/log/nginx/error.log" failed (13: Permission denied)
> May 09 10:12:21 host-172-16-36-25 nginx[1817]: nginx: configuration file 
> /etc/nginx/nginx.conf test failed
> May 09 10:12:21 host-172-16-36-25 systemd[1]: nginx.service: Control process 
> exited, code=exited status=1
> May 09 10:12:21 host-172-16-36-25 systemd[1]: nginx.service: Failed with 
> result 'exit-code'.
> May 09 10:12:21 host-172-16-36-25 systemd[1]: Failed to start The nginx HTTP 
> and reverse proxy server.
> 
> This is a problem with SELinux (dac_override). Since master process runs as 
> root, /var/log/nginx has ownership nginx:root,
> permissions 770 and NGX_FILE_DEFAULT_ACCESS is 644 for newly created logs.
> 
> One possible solution is to set different permission mode for newly created 
> logs (664 with nginx:root ownership) or do not set
> owner of log files to nginx user (which had probably some reason in past 
> because of extra param in ngx_reopen_files).

If needed in a particular setup, log files can be pre-created with 
desired permissions before instructing nginx to reopen them via 
USR1.

-- 
Maxim Dounin
http://mdounin.ru/
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: Is there a particular reason --with-compat isn't enabled by default?

[Ruslan Ermilov]

On Wed, May 09, 2018 at 12:29:06PM -0400, Thomas Ward wrote:
> In regards to several off-lists inquiries downstream about people trying
> to add additional third party modules, I've gone and started seeking
> justification for enabling --with-compat.
> 
> Downstream in Ubuntu, I'm getting pushback in that the question of "Why
> do we need to enable this, what does it add?".  I'm trying to find that
> justification for it, and the best I can find is Maxim's statements on a
> 2016 email/forum thread about how it actually makes dynamic module
> support truly work (in a nutshell).  [1]
> 
> Further, there's pushback about "Will package security updates and
> patches change the module ABI on security fixes or bug fixes?".  I don't
> have a clear answer on this, and I had this question back when dynamic
> module support was introduced, but never got a clear answer on this
> point.  It does beg consideration with regards to dynamic module support
> whether a simple patch applied to the same exact NGINX version will
> break ABI.  The way we handle security patches and such downstream is we
> apply patches to the existing NGINX version via `quilt`, which applies
> the patch at build time.  Whether this makes an ABI change or not I
> couldn't say, so I'm hunting a response from you, the devs, to give me a
> clear answer on this.
> 
> So, for those who didn't read everything there's two questions here:
> 
>  (A) Other than making dynamic module support "work better", what does
> --with-compat actually do behind the scenes (In a nutshell)?

It enables some macros and alters some structures in a way that's
compatible with NGINX Plus, built with the same option.  Practically
this means that checksums of module loadable objects will be identical
between when using F/OSS sources and when using NGINX Plus sources.
Searching for "NGX_COMPAT" throughout the F/OSS source code will
give enough details.

>  (B) Will a simple patch that patches security issues or adds fixes to
> something later on but doesn't change the core NGINX version numbering
> change the module ABI in such a way that it'll break modules built
> against nginx without that patch (assuming that --with-compat was added,
> since it's apparently needed to make dynamic modules 'actually work')

If a patch is simple, this is highly unlikely.  For a patch to
break the ABI, at least some externally visible structures should
be changed in some backwards incompatible ways.
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

[nginx] Configure: fixed clang version detection (closes #1539).

[Ruslan Ermilov]

details:   http://hg.nginx.org/nginx/rev/ceab908790c4
branches:  
changeset: 7273:ceab908790c4
user:  Ruslan Ermilov 
date:  Tue Apr 24 14:04:59 2018 +0300
description:
Configure: fixed clang version detection (closes #1539).

While 325b3042edd6 fixed it on MINIX, it broke it on systems
that output the word "version" on several lines with "cc -v".
The fix is to only consider "clang version" or "LLVM version"
as clang version, but this time only using sed(1).

diffstat:

 auto/cc/clang |  3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diffs (13 lines):

diff -r fa0e093b64d7 -r ceab908790c4 auto/cc/clang
--- a/auto/cc/clang Tue May 08 19:35:56 2018 +0300
+++ b/auto/cc/clang Tue Apr 24 14:04:59 2018 +0300
@@ -6,7 +6,8 @@
 
 
 NGX_CLANG_VER=`$CC -v 2>&1 | grep 'version' 2>&1 \
-   | sed -e 's/^.* version \(.*\)/\1/'`
+   | sed -n -e 's/^.*clang version \(.*\)/\1/p' \
+-e 's/^.*LLVM version \(.*\)/\1/p'`
 
 echo " + clang version: $NGX_CLANG_VER"
 
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel