Re: svn commit: r1826279 - /httpd/httpd/branches/2.4.x/STATUS

[Luca Toscano]

2018-03-24 17:29 GMT+01:00 Luca Toscano :

>
>
> 2018-03-23 19:01 GMT+01:00 Joe Orton :
>
>> On Fri, Mar 16, 2018 at 11:50:17AM +0100, Luca Toscano wrote:
>> > From my point of view, adding a comment nearby a directive (except in
>> some
>> > cases like you explained above) should be totally safe and transparent
>> to
>> > the user. I haven't ever thought about the possibility that having a
>> inline
>> > comment could be dangerous, and in my opinion we should enforce this
>> vision
>> > and explicitly document when it is not possible it and why.
>> >
>> > The above is my naive view though (after working on this project for a
>> very
>> > short time) so I'd really like to know what's your angle about not
>> > encouraging inline comments (pretty sure that there are use cases that I
>> > didn't think of, and that might be good to be documented).
>>
>> I'd be fine with making in-line comments 100% safe (stripped)
>> everywhere.  I'd think I'd also be fine with making inline comments a
>> config error in all cases, or increasing the X% of cases where that's an
>> error already.
>>
>> I'm not happy about increasing (but to still below 100%) the places
>> where comments are silently stripped, leaving the remaining places where
>> comments might be a security issue (as in Require host foo#bar).  I'm
>> worried this will *increase* the risk of security issues as users become
>> accustomed to using in-line comments.
>>
>
> Thanks a lot for the explanation, now I completely got what you mean. I
> was convinced that inline comments were safe for all directives, and I
> think that a lot of our users already think this too (I was pinged by a
> colleague that was puzzled about the ServerAlias behavior). I came up with
> the following code patch that introduces a new function in utils.c, heavily
> inspired by your patches (I felt bad to say copy/pasted :) that could help:
>
> http://people.apache.org/~elukey/httpd-trunk-core_
> server_alias_comment.patch
>
> Still not 100% tested, but it seems to work for ServerAlias (might need
> more development time, comments welcome!). Reviewing all the directives to
> apply this though might become a big burden, and potentially introduce new
> bugs in 2.4 that we don't want. On the other side, the simplest solution of
> raising an error when inline comments are used might be better, but we'd
> risk to break existing working configurations when users upgrade to the new
> release..
>
> I don't have a strong position on this, I am dumping thoughts more than
> giving solutions, really interested in what others think.
>

If anybody still has patience and wants to review some code, I tried to
update my patch to swap Joe's changes for forward_dns_check_authorization
with a call to ap_getword_conf_nocomment:

http://people.apache.org/~elukey/httpd-trunk-core_server_alias_comment.patch

Tried to test it with 'Require forward-dns' and it seems working fine..

Luca

Re: svn commit: r1826279 - /httpd/httpd/branches/2.4.x/STATUS

[Luca Toscano]

2018-03-23 19:01 GMT+01:00 Joe Orton :

> On Fri, Mar 16, 2018 at 11:50:17AM +0100, Luca Toscano wrote:
> > From my point of view, adding a comment nearby a directive (except in
> some
> > cases like you explained above) should be totally safe and transparent to
> > the user. I haven't ever thought about the possibility that having a
> inline
> > comment could be dangerous, and in my opinion we should enforce this
> vision
> > and explicitly document when it is not possible it and why.
> >
> > The above is my naive view though (after working on this project for a
> very
> > short time) so I'd really like to know what's your angle about not
> > encouraging inline comments (pretty sure that there are use cases that I
> > didn't think of, and that might be good to be documented).
>
> I'd be fine with making in-line comments 100% safe (stripped)
> everywhere.  I'd think I'd also be fine with making inline comments a
> config error in all cases, or increasing the X% of cases where that's an
> error already.
>
> I'm not happy about increasing (but to still below 100%) the places
> where comments are silently stripped, leaving the remaining places where
> comments might be a security issue (as in Require host foo#bar).  I'm
> worried this will *increase* the risk of security issues as users become
> accustomed to using in-line comments.
>

Thanks a lot for the explanation, now I completely got what you mean. I was
convinced that inline comments were safe for all directives, and I think
that a lot of our users already think this too (I was pinged by a colleague
that was puzzled about the ServerAlias behavior). I came up with the
following code patch that introduces a new function in utils.c, heavily
inspired by your patches (I felt bad to say copy/pasted :) that could help:

http://people.apache.org/~elukey/httpd-trunk-core_server_alias_comment.patch

Still not 100% tested, but it seems to work for ServerAlias (might need
more development time, comments welcome!). Reviewing all the directives to
apply this though might become a big burden, and potentially introduce new
bugs in 2.4 that we don't want. On the other side, the simplest solution of
raising an error when inline comments are used might be better, but we'd
risk to break existing working configurations when users upgrade to the new
release..

I don't have a strong position on this, I am dumping thoughts more than
giving solutions, really interested in what others think.

Thanks!

Luca

Re: svn commit: r1826279 - /httpd/httpd/branches/2.4.x/STATUS

[Joe Orton]

On Fri, Mar 16, 2018 at 11:50:17AM +0100, Luca Toscano wrote:
> From my point of view, adding a comment nearby a directive (except in some
> cases like you explained above) should be totally safe and transparent to
> the user. I haven't ever thought about the possibility that having a inline
> comment could be dangerous, and in my opinion we should enforce this vision
> and explicitly document when it is not possible it and why.
> 
> The above is my naive view though (after working on this project for a very
> short time) so I'd really like to know what's your angle about not
> encouraging inline comments (pretty sure that there are use cases that I
> didn't think of, and that might be good to be documented).

I'd be fine with making in-line comments 100% safe (stripped) 
everywhere.  I'd think I'd also be fine with making inline comments a 
config error in all cases, or increasing the X% of cases where that's an 
error already.

I'm not happy about increasing (but to still below 100%) the places 
where comments are silently stripped, leaving the remaining places where 
comments might be a security issue (as in Require host foo#bar).  I'm 
worried this will *increase* the risk of security issues as users become 
accustomed to using in-line comments.

Regards, Joe

Re: svn commit: r1826279 - /httpd/httpd/branches/2.4.x/STATUS

[Luca Toscano]

Hi Joe,

2018-03-16 10:38 GMT+01:00 Joe Orton :

> On Thu, Mar 08, 2018 at 11:05:29PM +0100, Yann Ylavic wrote:
> > On Thu, Mar 8, 2018 at 11:00 PM,   wrote:
> > >
> > >*) mod_access_compat, mod_authz_host: Prevent access control
> misconfiguration
> > >   due to interpretation of #comments in Require host or Allow/Deny
> directives.
> > >   trunk patch: http://svn.apache.org/r1667676
> > >http://svn.apache.org/r1826207
> > >   2.4.x patch: trunk works, svn merge -c 1667676,1826207
> ^/httpd/httpd/trunk .
> > > - +1: jorton,  jim,
> > > + +1: jorton, jim, ylavic
> >
> > This one possibly/later could be addressed at
> > ap_getword_conf[_nocomment)() level, many/most directives should stop
> > on #comments no?
>
> I'm not confident about changing ap_getword_conf() itself because it's
> not that remote a possibility that someone is using config lines with #
> for some other reason, e.g. # is legitimately used in URIs. httpd
> currently almost everywhere doesn't treat in-line # as special, even if
> it's documented to be not permitted.
>

Makes sense!


> With an ap_getword_conf_nocomment() which silently strips comments I'd
> worry we'd almost be *encouraging* use of in-line comments by making
> them safe in some cases. For authz cases like the one in this merge,
> there's a strong argument for comments to be errors rather than silently
> ignored because previous behaviour was really quite horrible.
>

>From my point of view, adding a comment nearby a directive (except in some
cases like you explained above) should be totally safe and transparent to
the user. I haven't ever thought about the possibility that having a inline
comment could be dangerous, and in my opinion we should enforce this vision
and explicitly document when it is not possible it and why.

The above is my naive view though (after working on this project for a very
short time) so I'd really like to know what's your angle about not
encouraging inline comments (pretty sure that there are use cases that I
didn't think of, and that might be good to be documented).

Thanks!

Luca

Re: svn commit: r1826279 - /httpd/httpd/branches/2.4.x/STATUS

[Joe Orton]

On Thu, Mar 08, 2018 at 11:05:29PM +0100, Yann Ylavic wrote:
> On Thu, Mar 8, 2018 at 11:00 PM,   wrote:
> >
> >*) mod_access_compat, mod_authz_host: Prevent access control 
> > misconfiguration
> >   due to interpretation of #comments in Require host or Allow/Deny 
> > directives.
> >   trunk patch: http://svn.apache.org/r1667676
> >http://svn.apache.org/r1826207
> >   2.4.x patch: trunk works, svn merge -c 1667676,1826207 
> > ^/httpd/httpd/trunk .
> > - +1: jorton,  jim,
> > + +1: jorton, jim, ylavic
> 
> This one possibly/later could be addressed at
> ap_getword_conf[_nocomment)() level, many/most directives should stop
> on #comments no?

I'm not confident about changing ap_getword_conf() itself because it's 
not that remote a possibility that someone is using config lines with # 
for some other reason, e.g. # is legitimately used in URIs. httpd 
currently almost everywhere doesn't treat in-line # as special, even if 
it's documented to be not permitted.

With an ap_getword_conf_nocomment() which silently strips comments I'd 
worry we'd almost be *encouraging* use of in-line comments by making 
them safe in some cases. For authz cases like the one in this merge, 
there's a strong argument for comments to be errors rather than silently 
ignored because previous behaviour was really quite horrible.

So... tl;dr, I'm not really sure.

Regards, Joe

Re: svn commit: r1826279 - /httpd/httpd/branches/2.4.x/STATUS

[Luca Toscano]

Hi Yann,

2018-03-08 23:05 GMT+01:00 Yann Ylavic :

> On Thu, Mar 8, 2018 at 11:00 PM,   wrote:
> >
> >*) mod_access_compat, mod_authz_host: Prevent access control
> misconfiguration
> >   due to interpretation of #comments in Require host or Allow/Deny
> directives.
> >   trunk patch: http://svn.apache.org/r1667676
> >http://svn.apache.org/r1826207
> >   2.4.x patch: trunk works, svn merge -c 1667676,1826207
> ^/httpd/httpd/trunk .
> > - +1: jorton,  jim,
> > + +1: jorton, jim, ylavic
>
> This one possibly/later could be addressed at
> ap_getword_conf[_nocomment)() level, many/most directives should stop
> on #comments no?
>

+1, I just came across:

~/httpd-trunk$ sudo /usr/local/apache2/bin/apachectl -S
VirtualHost configuration:
*:80   is a NameVirtualHost
 default server test1.com (/usr/local/apache2/conf/httpd.conf:186)
 port 80 namevhost test1.com
(/usr/local/apache2/conf/httpd.conf:186)
 alias test2.com
 alias #This
 alias is
 alias a
 alias comment

Generated by:

ServerAlias test2.com #This is a comment

Luca

Re: svn commit: r1826279 - /httpd/httpd/branches/2.4.x/STATUS

[William A Rowe Jr]

Doesn't our crazy old unquoted ErrorDocument directive have this issue too?

On Mar 8, 2018 16:05, "Yann Ylavic"  wrote:

> On Thu, Mar 8, 2018 at 11:00 PM,   wrote:
> >
> >*) mod_access_compat, mod_authz_host: Prevent access control
> misconfiguration
> >   due to interpretation of #comments in Require host or Allow/Deny
> directives.
> >   trunk patch: http://svn.apache.org/r1667676
> >http://svn.apache.org/r1826207
> >   2.4.x patch: trunk works, svn merge -c 1667676,1826207
> ^/httpd/httpd/trunk .
> > - +1: jorton,  jim,
> > + +1: jorton, jim, ylavic
>
> This one possibly/later could be addressed at
> ap_getword_conf[_nocomment)() level, many/most directives should stop
> on #comments no?
>

Re: svn commit: r1826279 - /httpd/httpd/branches/2.4.x/STATUS

[Yann Ylavic]

On Thu, Mar 8, 2018 at 11:00 PM,   wrote:
>
>*) mod_access_compat, mod_authz_host: Prevent access control 
> misconfiguration
>   due to interpretation of #comments in Require host or Allow/Deny 
> directives.
>   trunk patch: http://svn.apache.org/r1667676
>http://svn.apache.org/r1826207
>   2.4.x patch: trunk works, svn merge -c 1667676,1826207 
> ^/httpd/httpd/trunk .
> - +1: jorton,  jim,
> + +1: jorton, jim, ylavic

This one possibly/later could be addressed at
ap_getword_conf[_nocomment)() level, many/most directives should stop
on #comments no?