Re: svn commit: r1826279 - /httpd/httpd/branches/2.4.x/STATUS
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
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
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
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
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
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
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
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