Re: [PATCH] apr_poll, take 2
On Wed, Jul 10, 2002 at 01:43:20AM -0400, [EMAIL PROTECTED] wrote: > Index: modules/experimental/mod_ext_filter.c > === > RCS file: /home/cvs/httpd-2.0/modules/experimental/mod_ext_filter.c,v > retrieving revision 1.31 > diff -u -d -b -w -u -r1.31 mod_ext_filter.c > --- modules/experimental/mod_ext_filter.c 28 Jun 2002 08:40:24 - 1.31 > +++ modules/experimental/mod_ext_filter.c 10 Jul 2002 05:36:02 - > @@ -71,6 +71,7 @@ > #include "apr_strings.h" > #include "apr_hash.h" > #include "apr_lib.h" > +#include "apr_poll.h" > #define APR_WANT_STRFUNC > #include "apr_want.h" > > @@ -626,7 +627,7 @@ > #if APR_FILES_AS_SOCKETS > int num_events; > > -rv = apr_poll(ctx->pollset, > +rv = apr_poll(ctx->pollset, 2 Need a comma here. -- justin
[PATCH] apr_poll, take 2
FirstBill found a bug in my poll implementation today. It was working for
sockets most of the time, but it always failed with pipes. It also wasn't
correctly returning APR_TIMEUP. This new patch should fix the problems.
Same as last time, apply the patch, then in srclib/apr, untar the tarball.
Ryan
Index: modules/experimental/mod_ext_filter.c
===
RCS file: /home/cvs/httpd-2.0/modules/experimental/mod_ext_filter.c,v
retrieving revision 1.31
diff -u -d -b -w -u -r1.31 mod_ext_filter.c
--- modules/experimental/mod_ext_filter.c 28 Jun 2002 08:40:24 - 1.31
+++ modules/experimental/mod_ext_filter.c 10 Jul 2002 05:36:02 -
@@ -71,6 +71,7 @@
#include "apr_strings.h"
#include "apr_hash.h"
#include "apr_lib.h"
+#include "apr_poll.h"
#define APR_WANT_STRFUNC
#include "apr_want.h"
@@ -626,7 +627,7 @@
#if APR_FILES_AS_SOCKETS
int num_events;
-rv = apr_poll(ctx->pollset,
+rv = apr_poll(ctx->pollset, 2
&num_events,
f->r->server->timeout);
if (rv || dc->debug >= DBGLVL_GORY) {
Index: modules/proxy/proxy_connect.c
===
RCS file: /home/cvs/httpd-2.0/modules/proxy/proxy_connect.c,v
retrieving revision 1.59
diff -u -d -b -w -u -r1.59 proxy_connect.c
--- modules/proxy/proxy_connect.c 17 May 2002 11:24:16 - 1.59
+++ modules/proxy/proxy_connect.c 10 Jul 2002 05:36:02 -
@@ -61,6 +61,7 @@
#define CORE_PRIVATE
#include "mod_proxy.h"
+#include "apr_poll.h"
module AP_MODULE_DECLARE_DATA proxy_connect_module;
@@ -320,7 +321,7 @@
while (1) { /* Infinite loop until error (one side closes the connection) */
/* ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server, "proxy: CONNECT: going to
sleep (poll)");*/
-if ((rv = apr_poll(pollfd, &pollcnt, -1)) != APR_SUCCESS)
+if ((rv = apr_poll(pollfd, 2, &pollcnt, -1)) != APR_SUCCESS)
{
apr_socket_close(sock);
ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, "proxy: CONNECT: error
apr_poll()");
Index: server/mpm/beos/beos.c
===
RCS file: /home/cvs/httpd-2.0/server/mpm/beos/beos.c,v
retrieving revision 1.97
diff -u -d -b -w -u -r1.97 beos.c
--- server/mpm/beos/beos.c 4 Jul 2002 15:20:52 - 1.97
+++ server/mpm/beos/beos.c 10 Jul 2002 05:36:05 -
@@ -87,6 +87,7 @@
#include "mpm.h"
#include "mpm_default.h"
#include "apr_thread_mutex.h"
+#include "apr_poll.h"
extern int _kset_fd_limit_(int num);
@@ -421,7 +422,7 @@
apr_int16_t event;
apr_status_t ret;
-ret = apr_poll(pollset, &srv, -1);
+ret = apr_poll(pollset, num_listening_sockets, &srv, -1);
if (ret != APR_SUCCESS) {
if (APR_STATUS_IS_EINTR(ret)) {
Index: server/mpm/experimental/leader/leader.c
===
RCS file: /home/cvs/httpd-2.0/server/mpm/experimental/leader/leader.c,v
retrieving revision 1.22
diff -u -d -b -w -u -r1.22 leader.c
--- server/mpm/experimental/leader/leader.c 4 Jul 2002 15:20:53 - 1.22
+++ server/mpm/experimental/leader/leader.c 10 Jul 2002 05:36:05 -
@@ -106,6 +106,7 @@
#include "ap_listen.h"
#include "scoreboard.h"
#include "mpm_default.h"
+#include "apr_poll.h"
#include
#include /* for INT_MAX */
@@ -888,7 +889,7 @@
apr_status_t ret;
apr_int16_t event;
-ret = apr_poll(pollset, &n, -1);
+ret = apr_poll(pollset, num_listensocks, &n, -1);
if (ret != APR_SUCCESS) {
if (APR_STATUS_IS_EINTR(ret)) {
continue;
Index: server/mpm/experimental/perchild/perchild.c
===
RCS file: /home/cvs/httpd-2.0/server/mpm/experimental/perchild/perchild.c,v
retrieving revision 1.128
diff -u -d -b -w -u -r1.128 perchild.c
--- server/mpm/experimental/perchild/perchild.c 30 Jun 2002 21:59:50 - 1.128
+++ server/mpm/experimental/perchild/perchild.c 10 Jul 2002 05:36:09 -
@@ -95,6 +95,7 @@
#include "mpm.h"
#include "scoreboard.h"
#include "util_filter.h"
+#include "apr_poll.h"
/* ### should be APR-ized */
#include
@@ -748,7 +749,7 @@
while (!workers_may_exit) {
apr_int16_t event;
-srv = apr_poll(pollset, &n, -1);
+srv = apr_poll(pollset, num_listensocks, &n, -1);
if (srv != APR_SUCCESS) {
if (APR_STATUS_IS_EINTR(srv)) {
Index: server/mpm/experimental/threadpool/threadpool.c
===
RCS file: /home/cvs/httpd-2.0/server/mpm/experimental/threa
Re: Books about apache mods dev.?
For 1.3, get Doug MacEachern O'reilly book http://www.amazon.com/exec/obidos/ASIN/156592567X/ For 2.0, I think Ryan Bloom's covers some (though I havent seen it yet) http://www.amazon.com/exec/obidos/ASIN/0072223448/ > Hi Apache Gurus! > Just wanted to ask if someone can recommend any books about developing > apache modules? > TIA > Jim. -- Teach Yourself Apache 2 -- http://apacheworld.org/ty24/
RE: [PATCH] mpm/winnt service permissions
Just one thought :-)
I think that at least Administrator privileges are needed to start the
services.
The ApacheMonitor will definitely need that once when async behavior
will be used, so that calls for starting services gets serialized with
LockServiceDatabase that needs Admin privileges.
So I'm for the GENERIC_READ/GENERIC_WRITE/GENERIC_EXECUTE generic access
types, and not for finding security holes. Neither AM nor Apache
shouldn't brake that allowing starting or stopping something that cannot
be done through Service Manager itself, and should report that as access
violation errors.
MT.
> -Original Message-
> From: David Shane Holden [mailto:[EMAIL PROTECTED]]
> Sent: Wednesday, July 10, 2002 2:28 AM
> To: [EMAIL PROTECTED]
> Subject: Re: [PATCH] mpm/winnt service permissions
>
>
> Correct me if I'm wrong, but it sounds like you think this is for
> ApacheMonitor. This is for the winnt mpm itself.
> I thought your patch this morning was for the mpm just as I
> believe you
> think this is for the monitor.
>
> Shane
>
>
> William A. Rowe, Jr. wrote:
>
> > At 01:40 PM 7/9/2002, you wrote:
> >
> >> This patch sets the calls to OpenSCManager and OpenService
> to use the
> >> minimum required privileges.
> >
> >
> > Cool. Could you cvs up to grab the latest version with Mladen's
> > patch, compare your suggested changes to his latest changes for
> > requested privileges, and provide an updated patch to discuss?
> >
> > Bill
> >
> >> - SC_MANAGER_ALL_ACCESS);
> >> + SC_MANAGER_CONNECT);
> >> if (!schSCManager) {
> >> rv = apr_get_os_error();
> >> ap_log_error(APLOG_MARK, APLOG_ERR |
> APLOG_STARTUP, rv,
> >> NULL,
> >> @@ -1265,7 +1262,7 @@
> >> SC_HANDLE schSCManager;
> >>
> >> schSCManager = OpenSCManager(NULL, NULL, //
> default machine
> >> & database
> >> - SC_MANAGER_ALL_ACCESS);
> >> + SC_MANAGER_CONNECT);
> >>
> >> if (!schSCManager) {
> >> ap_log_error(APLOG_MARK, APLOG_ERR | APLOG_STARTUP,
> >> apr_get_os_error(), NULL,
> >> @@ -1275,7 +1272,8 @@
> >>
> >> /* ###: utf-ize */
> >> schService = OpenService(schSCManager, mpm_service_name,
> >> - SERVICE_ALL_ACCESS);
> >> + SERVICE_INTERROGATE |
> >> SERVICE_QUERY_STATUS |
> >> + SERVICE_START | SERVICE_STOP);
> >>
> >> if (schService == NULL) {
> >> /* Could not open the service */
> >
> >
> >
>
>
>
>
Re: [PATCH] mpm/winnt service permissions
At 07:27 PM 7/9/2002, you wrote: >Correct me if I'm wrong, but it sounds like you think this is for >ApacheMonitor. This is for the winnt mpm itself. >I thought your patch this morning was for the mpm just as I believe you >think this is for the monitor. Yup... That would be it. Thanks for the patch, I'll review. Bill
recent chunked encoding fix -vs- mod_proxy...
I have a situation where I have an external-facing apache server proxying to another apache server inside a firewall. I've updated the proxying one to Apache 1.3.26 so that it won't get hacked due to the chunked encoding bug, but I'm not able to upgrade the other one behind the firewall for quite some time (a few months since it's integrated with another product). I've been trying to figure out if I'm vulnerable externally or not in this situation. It appears to me that I'm not, because it looks to me like the mod_proxy handler calls the same core chunked reading functionality that the rest of Apache uses (i.e. from main/http_protocol.c) and that appears to be where all the fixes were made. However, I thought I'd run this by you good folks here since you're a lot more experienced with the Apache code than I am (just 2 days for me so far) Dave
Re: [PATCH] mpm/winnt service permissions
Correct me if I'm wrong, but it sounds like you think this is for
ApacheMonitor. This is for the winnt mpm itself.
I thought your patch this morning was for the mpm just as I believe you
think this is for the monitor.
Shane
William A. Rowe, Jr. wrote:
> At 01:40 PM 7/9/2002, you wrote:
>
>> This patch sets the calls to OpenSCManager and OpenService to use the
>> minimum required privileges.
>
>
> Cool. Could you cvs up to grab the latest version with Mladen's patch,
> compare your suggested changes to his latest changes for requested
> privileges, and provide an updated patch to discuss?
>
> Bill
>
>
>> Index: service.c
>> ===
>> RCS file: /home/cvspublic/httpd-2.0/server/mpm/winnt/service.c,v
>> retrieving revision 1.56
>> diff -u -3 -r1.56 service.c
>> --- service.c 2 Jul 2002 19:03:15 - 1.56
>> +++ service.c 9 Jul 2002 18:02:38 -
>> @@ -483,10 +483,10 @@
>> if ((osver.dwPlatformId == VER_PLATFORM_WIN32_NT)
>>&& (osver.dwMajorVersion > 4)
>>&& (ChangeServiceConfig2)
>> - && (schSCManager = OpenSCManager(NULL, NULL,
>> SC_MANAGER_ALL_ACCESS)))
>> + && (schSCManager = OpenSCManager(NULL, NULL,
>> SC_MANAGER_CONNECT)))
>> {
>> SC_HANDLE schService = OpenService(schSCManager,
>> mpm_service_name,
>> - SERVICE_ALL_ACCESS);
>> + SERVICE_CHANGE_CONFIG);
>> if (schService) {
>> /* Cast is necessary, ChangeServiceConfig2 handles multiple
>> * object types, some volatile, some not.
>> @@ -854,10 +854,9 @@
>> {
>> SC_HANDLE schService;
>> SC_HANDLE schSCManager;
>> -
>> -// TODO: Determine the minimum permissions required for
>> security
>> +
>> schSCManager = OpenSCManager(NULL, NULL, /* local, default
>> database */
>> - SC_MANAGER_ALL_ACCESS);
>> + SC_MANAGER_CREATE_SERVICE);
>> if (!schSCManager) {
>> rv = apr_get_os_error();
>> ap_log_error(APLOG_MARK, APLOG_ERR | APLOG_STARTUP, rv,
>> NULL,
>> @@ -870,7 +869,7 @@
>> if (reconfig) {
>> /* ###: utf-ize */
>> schService = OpenService(schSCManager, mpm_service_name,
>> - SERVICE_ALL_ACCESS);
>> + SERVICE_CHANGE_CONFIG);
>> if (!schService) {
>> ap_log_error(APLOG_MARK, APLOG_ERR|APLOG_ERR,
>> apr_get_os_error(), NULL,
>> @@ -1008,9 +1007,8 @@
>>
>> fprintf(stderr,"Removing the %s service\n", mpm_display_name);
>>
>> -// TODO: Determine the minimum permissions required for
>> security
>> schSCManager = OpenSCManager(NULL, NULL, /* local, default
>> database */
>> - SC_MANAGER_ALL_ACCESS);
>> + SC_MANAGER_CONNECT);
>> if (!schSCManager) {
>> rv = apr_get_os_error();
>> ap_log_error(APLOG_MARK, APLOG_ERR | APLOG_STARTUP, rv,
>> NULL,
>> @@ -1019,7 +1017,7 @@
>> }
>>
>> /* ###: utf-ize */
>> -schService = OpenService(schSCManager, mpm_service_name,
>> SERVICE_ALL_ACCESS);
>> +schService = OpenService(schSCManager, mpm_service_name,
>> DELETE);
>>
>> if (!schService) {
>> rv = apr_get_os_error();
>> @@ -1123,9 +1121,8 @@
>> SC_HANDLE schService;
>> SC_HANDLE schSCManager;
>>
>> -// TODO: Determine the minimum permissions required for
>> security
>> schSCManager = OpenSCManager(NULL, NULL, /* local, default
>> database */
>> - SC_MANAGER_ALL_ACCESS);
>> + SC_MANAGER_CONNECT);
>> if (!schSCManager) {
>> rv = apr_get_os_error();
>> ap_log_error(APLOG_MARK, APLOG_ERR | APLOG_STARTUP, rv,
>> NULL,
>> @@ -1265,7 +1262,7 @@
>> SC_HANDLE schSCManager;
>>
>> schSCManager = OpenSCManager(NULL, NULL, // default machine
>> & database
>> - SC_MANAGER_ALL_ACCESS);
>> + SC_MANAGER_CONNECT);
>>
>> if (!schSCManager) {
>> ap_log_error(APLOG_MARK, APLOG_ERR | APLOG_STARTUP,
>> apr_get_os_error(), NULL,
>> @@ -1275,7 +1272,8 @@
>>
>> /* ###: utf-ize */
>> schService = OpenService(schSCManager, mpm_service_name,
>> - SERVICE_ALL_ACCESS);
>> + SERVICE_INTERROGATE |
>> SERVICE_QUERY_STATUS |
>> + SERVICE_START | SERVICE_STOP);
>>
>> if (schService == NULL) {
>> /* Could not open the service */
>
>
>
[Fwd: Re: time for a 2.0.40 me thinks]
Bill Stoddard wrote: >>William A. Rowe, Jr. wrote: >> >> >> >>>At 12:18 PM 7/9/2002, Bill Stoddard wrote: >>> >>> >>> >I'm going to do a start the T&R process on thursday for a 2.0.40 > > release >so if you've got things which you want in there and are stable >(apr_poll) comes to mind please commit them. > >Ian > > I'll try to run a profile on the new apr_poll code posted by Ryan this afternoon or evening. It would be -really- good if we could make the apr_time changes before 2.0.40... >>>Agreed. They are in or out. BPane... any new news on profiling with >>>busec values? >>> >>> >>No news; I don't expect that I'll be able to work on this >>for a while. >> >>--Brian >> >> > >Where and when was the post that described this proposal? I'd like to take >a look. Is there a patch? > There's no patch that I know of, but here's the thread with wrowe's original proposal for the binary microseconds design: http://marc.theaimsgroup.com/?l=apr-dev&m=102376298728487&w=2 With the big batch of changes that I made last week, almost all of the time-related code in the httpd now uses the new macros-- apr_time_sec(), apr_time_usec(), etc--instead of multiplying and dividing by 1,000,000 directly. Hopefully that will make it easier to try out a new time representation. --Brian
Re: cvs commit: apache-1.3/src/main http_protocol.c
David Burry wrote: > > While we are debating the best way to accomplish this Content-Length fix for the >next release, I kind of need to have it working for me right now in the current >released version. Therefore I've implemented this partial fix against 1.3.26 on my >system: > The current CVS code is correct. Well, should be. I think Roy's comments were based on (1) the fact that we didn't check for negative results in the big patch (which is true, and was fixed very soon afterwards) and (2) didn't realize that the ap_strtol() does an inherent check of digits and we utilize that. Sure the code does a superfluous setting of r->remaining, but that gets optimized away anyway and it's clear :) *duck* -- === Jim Jagielski [|] [EMAIL PROTECTED] [|] http://www.jaguNET.com/ "A society that will trade a little liberty for a little order will lose both and deserve neither" - T.Jefferson
Re: cvs commit: apache-1.3/src/main http_protocol.c
Roy T. Fielding wrote:
>
> WTF? -1 Jim, that code is doing an error check prior to the
> strtol. It is not looking for the start of the number, but
> ensuring that the number is non-negative and all digits prior
> to calling the library routine. A simple check of *lenp would
> have been sufficient for the blank case.
>
> I need to go through the other changes as well, so I'll fix it,
> but don't release with this code.
Never said it was looking for the start... :)
The original code checked for all spaces and digits, which is well
and good. In this case, however, we just check for spaces, and if all
spaces, we don't bother with the ap_strtol() call. However, if
we *do* hit a non-space, we leverage the fact that ap_strtol() will
do the right thing. The first non-digit it sees (since we are
base 10) it will bail out and error, and we'll see the error with
the (endstr && *endstr) check. So we leverage the fact that ap_strtol()
knows all about digits and what are valid ones and we check for non-digits.
So the only other thing we need to check for is negative, which we do
(a later commit). With ap_strtol, the ap_isdigit() check is not needed
since we check for non-digits when we check the return values for
ap_strtol.
The below (with the follow-up commit) does exactly what we need.
>
> Roy
>
>
> On Tuesday, July 9, 2002, at 07:47 AM, [EMAIL PROTECTED] wrote:
>
> > jim 2002/07/09 07:47:24
> >
> > Modified:src CHANGES
> >src/main http_protocol.c
> > Log:
> > Allow for null/all-whitespace C-L fields as we did pre-1.3.26. However,
> > we do not allow for the total bogusness of values for C-L, just this
> > one special case. IMO a C-L field of "iloveyou" is bogus as is one
> > of "123yabbadabbado", which older versions appear to have allowed
> > (and in the 1st case, assume 0 and in the 2nd assume 123). Didn't
> > make sense to make this runtime, but a documented special case
> > instead.
> >
> > Revision ChangesPath
> > 1.1836+8 -0 apache-1.3/src/CHANGES
> >
> > Index: CHANGES
> > ===
> > RCS file: /home/cvs/apache-1.3/src/CHANGES,v
> > retrieving revision 1.1835
> > retrieving revision 1.1836
> > diff -u -r1.1835 -r1.1836
> > --- CHANGES 8 Jul 2002 18:06:54 - 1.1835
> > +++ CHANGES 9 Jul 2002 14:47:23 - 1.1836
> > @@ -1,5 +1,13 @@
> >Changes with Apache 1.3.27
> >
> > + *) In 1.3.26, a null or all blank Content-Length field would be
> > + triggered as an error; previous versions would silently ignore
> > + this and assume 0. As a special case, we now allow this and
> > + behave as we previously did. HOWEVER, previous versions would
> > + also silently accept bogus C-L values; We do NOT do that. That
> > + *is* an invalid value and we treat it as such.
> > + [Jim Jagielski]
> > +
> > *) Add ProtocolReqCheck directive, which determines if Apache will
> > check for a valid protocol string in the request (eg: HTTP/1.1)
> > and return HTTP_BAD_REQUEST if not valid. Versions of Apache
> >
> >
> >
> > 1.324 +8 -2 apache-1.3/src/main/http_protocol.c
> >
> > Index: http_protocol.c
> > ===
> > RCS file: /home/cvs/apache-1.3/src/main/http_protocol.c,v
> > retrieving revision 1.323
> > retrieving revision 1.324
> > diff -u -r1.323 -r1.324
> > --- http_protocol.c 8 Jul 2002 18:06:55 - 1.323
> > +++ http_protocol.c 9 Jul 2002 14:47:24 - 1.324
> > @@ -2011,10 +2011,16 @@
> >const char *pos = lenp;
> >int conversion_error = 0;
> >
> > -while (ap_isdigit(*pos) || ap_isspace(*pos))
> > +while (ap_isspace(*pos))
> >++pos;
> >
> >if (*pos == '\0') {
> > +/* special case test - a C-L field NULL or all blanks is
> > + * assumed OK and defaults to 0. Otherwise, we do a
> > + * strict check of the field */
> > +r->remaining = 0;
> > +}
> > +else {
> >char *endstr;
> >errno = 0;
> >r->remaining = ap_strtol(lenp, &endstr, 10);
> > @@ -2023,7 +2029,7 @@
> >}
> >}
> >
> > -if (*pos != '\0' || conversion_error) {
> > +if (conversion_error) {
> >ap_log_rerror(APLOG_MARK, APLOG_NOERRNO|APLOG_ERR, r,
> >"Invalid Content-Length");
> >return HTTP_BAD_REQUEST;
> >
> >
> >
> >
>
--
===
Jim Jagielski [|] [EMAIL PROTECTED] [|] http://www.jaguNET.com/
"A society that will trade a little liberty for a little order
will lose both and deserve neither" - T.Jefferson
Re: cvs commit: apache-1.3/src/main http_protocol.c
While we are debating the best way to accomplish this Content-Length fix for the next
release, I kind of need to have it working for me right now in the current released
version. Therefore I've implemented this partial fix against 1.3.26 on my system:
root@nueva[391] pwd
/usr/share/src/packages/apache_1.3.26/src/main
root@nueva[392] diff -c http_protocol.original.c http_protocol.c
*** http_protocol.original.cTue Jul 9 13:35:54 2002
--- http_protocol.c Tue Jul 9 12:35:59 2002
***
*** 1991,1997
r->read_chunked = 1;
}
! else if (lenp) {
const char *pos = lenp;
int conversion_error = 0;
--- 1991,1997
r->read_chunked = 1;
}
! else if (lenp && *lenp != '\0') {
const char *pos = lenp;
int conversion_error = 0;
Admittedly it only opens up empty string (blank) Content-Length values to default to
0, not white space ones, but I think that's all I really need to get me going for now
until the next release. I believe this may be the "simple check of *lenp" that Roy
was talking about. Since I'm brand new to the Apache source code in general, and not
really a C expert either, any comments or criticisms are welcome regarding this.
Dave
At 11:18 AM 7/9/2002 -0700, Roy T. Fielding wrote:
>WTF? -1 Jim, that code is doing an error check prior to the
>strtol. It is not looking for the start of the number, but
>ensuring that the number is non-negative and all digits prior
>to calling the library routine. A simple check of *lenp would
>have been sufficient for the blank case.
is this a bug??
while looking at another bug I stumbled onto us not setting the query
string here.. (where we set the unescaped one) I was wondering if this
was by design..
Index: mod_include.c
===
RCS file: /home/cvs/httpd-2.0/modules/filters/mod_include.c,v
retrieving revision 1.230
diff -u -r1.230 mod_include.c
--- mod_include.c 28 Jun 2002 08:40:24 - 1.230
+++ mod_include.c 9 Jul 2002 20:28:26 -
@@ -163,6 +163,7 @@
}
if (r->args) {
char *arg_copy = apr_pstrdup(r->pool, r->args);
+apr_table_setn(r->subprocess_env, "QUERY_STRING", r->args);
ap_unescape_url(arg_copy);
apr_table_setn(e, "QUERY_STRING_UNESCAPED",
Re: [PATCH] mpm/winnt service permissions
At 01:40 PM 7/9/2002, you wrote:
>This patch sets the calls to OpenSCManager and OpenService to use the
>minimum required privileges.
Cool. Could you cvs up to grab the latest version with Mladen's patch,
compare your suggested changes to his latest changes for requested
privileges, and provide an updated patch to discuss?
Bill
>Index: service.c
>===
>RCS file: /home/cvspublic/httpd-2.0/server/mpm/winnt/service.c,v
>retrieving revision 1.56
>diff -u -3 -r1.56 service.c
>--- service.c 2 Jul 2002 19:03:15 - 1.56
>+++ service.c 9 Jul 2002 18:02:38 -
>@@ -483,10 +483,10 @@
> if ((osver.dwPlatformId == VER_PLATFORM_WIN32_NT)
>&& (osver.dwMajorVersion > 4)
>&& (ChangeServiceConfig2)
>- && (schSCManager = OpenSCManager(NULL, NULL,
>SC_MANAGER_ALL_ACCESS)))
>+ && (schSCManager = OpenSCManager(NULL, NULL, SC_MANAGER_CONNECT)))
> {
> SC_HANDLE schService = OpenService(schSCManager, mpm_service_name,
>- SERVICE_ALL_ACCESS);
>+ SERVICE_CHANGE_CONFIG);
> if (schService) {
> /* Cast is necessary, ChangeServiceConfig2 handles multiple
> * object types, some volatile, some not.
>@@ -854,10 +854,9 @@
> {
> SC_HANDLE schService;
> SC_HANDLE schSCManager;
>-
>-// TODO: Determine the minimum permissions required for security
>+
> schSCManager = OpenSCManager(NULL, NULL, /* local, default
> database */
>- SC_MANAGER_ALL_ACCESS);
>+ SC_MANAGER_CREATE_SERVICE);
> if (!schSCManager) {
> rv = apr_get_os_error();
> ap_log_error(APLOG_MARK, APLOG_ERR | APLOG_STARTUP, rv, NULL,
>@@ -870,7 +869,7 @@
> if (reconfig) {
> /* ###: utf-ize */
> schService = OpenService(schSCManager, mpm_service_name,
>- SERVICE_ALL_ACCESS);
>+ SERVICE_CHANGE_CONFIG);
> if (!schService) {
> ap_log_error(APLOG_MARK, APLOG_ERR|APLOG_ERR,
> apr_get_os_error(), NULL,
>@@ -1008,9 +1007,8 @@
>
> fprintf(stderr,"Removing the %s service\n", mpm_display_name);
>
>-// TODO: Determine the minimum permissions required for security
> schSCManager = OpenSCManager(NULL, NULL, /* local, default
> database */
>- SC_MANAGER_ALL_ACCESS);
>+ SC_MANAGER_CONNECT);
> if (!schSCManager) {
> rv = apr_get_os_error();
> ap_log_error(APLOG_MARK, APLOG_ERR | APLOG_STARTUP, rv, NULL,
>@@ -1019,7 +1017,7 @@
> }
>
> /* ###: utf-ize */
>-schService = OpenService(schSCManager, mpm_service_name,
>SERVICE_ALL_ACCESS);
>+schService = OpenService(schSCManager, mpm_service_name, DELETE);
>
> if (!schService) {
> rv = apr_get_os_error();
>@@ -1123,9 +1121,8 @@
> SC_HANDLE schService;
> SC_HANDLE schSCManager;
>
>-// TODO: Determine the minimum permissions required for security
> schSCManager = OpenSCManager(NULL, NULL, /* local, default
> database */
>- SC_MANAGER_ALL_ACCESS);
>+ SC_MANAGER_CONNECT);
> if (!schSCManager) {
> rv = apr_get_os_error();
> ap_log_error(APLOG_MARK, APLOG_ERR | APLOG_STARTUP, rv, NULL,
>@@ -1265,7 +1262,7 @@
> SC_HANDLE schSCManager;
>
> schSCManager = OpenSCManager(NULL, NULL, // default machine &
> database
>- SC_MANAGER_ALL_ACCESS);
>+ SC_MANAGER_CONNECT);
>
> if (!schSCManager) {
> ap_log_error(APLOG_MARK, APLOG_ERR | APLOG_STARTUP,
> apr_get_os_error(), NULL,
>@@ -1275,7 +1272,8 @@
>
> /* ###: utf-ize */
> schService = OpenService(schSCManager, mpm_service_name,
>- SERVICE_ALL_ACCESS);
>+ SERVICE_INTERROGATE |
>SERVICE_QUERY_STATUS |
>+ SERVICE_START | SERVICE_STOP);
>
> if (schService == NULL) {
> /* Could not open the service */
[PATCH] mpm/winnt service permissions
This patch sets the calls to OpenSCManager and OpenService to use the
minimum required privileges.
Index: service.c
===
RCS file: /home/cvspublic/httpd-2.0/server/mpm/winnt/service.c,v
retrieving revision 1.56
diff -u -3 -r1.56 service.c
--- service.c 2 Jul 2002 19:03:15 - 1.56
+++ service.c 9 Jul 2002 18:02:38 -
@@ -483,10 +483,10 @@
if ((osver.dwPlatformId == VER_PLATFORM_WIN32_NT)
&& (osver.dwMajorVersion > 4)
&& (ChangeServiceConfig2)
- && (schSCManager = OpenSCManager(NULL, NULL, SC_MANAGER_ALL_ACCESS)))
+ && (schSCManager = OpenSCManager(NULL, NULL, SC_MANAGER_CONNECT)))
{
SC_HANDLE schService = OpenService(schSCManager, mpm_service_name,
- SERVICE_ALL_ACCESS);
+ SERVICE_CHANGE_CONFIG);
if (schService) {
/* Cast is necessary, ChangeServiceConfig2 handles multiple
* object types, some volatile, some not.
@@ -854,10 +854,9 @@
{
SC_HANDLE schService;
SC_HANDLE schSCManager;
-
-// TODO: Determine the minimum permissions required for security
+
schSCManager = OpenSCManager(NULL, NULL, /* local, default database */
- SC_MANAGER_ALL_ACCESS);
+ SC_MANAGER_CREATE_SERVICE);
if (!schSCManager) {
rv = apr_get_os_error();
ap_log_error(APLOG_MARK, APLOG_ERR | APLOG_STARTUP, rv, NULL,
@@ -870,7 +869,7 @@
if (reconfig) {
/* ###: utf-ize */
schService = OpenService(schSCManager, mpm_service_name,
- SERVICE_ALL_ACCESS);
+ SERVICE_CHANGE_CONFIG);
if (!schService) {
ap_log_error(APLOG_MARK, APLOG_ERR|APLOG_ERR,
apr_get_os_error(), NULL,
@@ -1008,9 +1007,8 @@
fprintf(stderr,"Removing the %s service\n", mpm_display_name);
-// TODO: Determine the minimum permissions required for security
schSCManager = OpenSCManager(NULL, NULL, /* local, default database */
- SC_MANAGER_ALL_ACCESS);
+ SC_MANAGER_CONNECT);
if (!schSCManager) {
rv = apr_get_os_error();
ap_log_error(APLOG_MARK, APLOG_ERR | APLOG_STARTUP, rv, NULL,
@@ -1019,7 +1017,7 @@
}
/* ###: utf-ize */
-schService = OpenService(schSCManager, mpm_service_name, SERVICE_ALL_ACCESS);
+schService = OpenService(schSCManager, mpm_service_name, DELETE);
if (!schService) {
rv = apr_get_os_error();
@@ -1123,9 +1121,8 @@
SC_HANDLE schService;
SC_HANDLE schSCManager;
-// TODO: Determine the minimum permissions required for security
schSCManager = OpenSCManager(NULL, NULL, /* local, default database */
- SC_MANAGER_ALL_ACCESS);
+ SC_MANAGER_CONNECT);
if (!schSCManager) {
rv = apr_get_os_error();
ap_log_error(APLOG_MARK, APLOG_ERR | APLOG_STARTUP, rv, NULL,
@@ -1265,7 +1262,7 @@
SC_HANDLE schSCManager;
schSCManager = OpenSCManager(NULL, NULL, // default machine & database
- SC_MANAGER_ALL_ACCESS);
+ SC_MANAGER_CONNECT);
if (!schSCManager) {
ap_log_error(APLOG_MARK, APLOG_ERR | APLOG_STARTUP, apr_get_os_error(),
NULL,
@@ -1275,7 +1272,8 @@
/* ###: utf-ize */
schService = OpenService(schSCManager, mpm_service_name,
- SERVICE_ALL_ACCESS);
+ SERVICE_INTERROGATE | SERVICE_QUERY_STATUS |
+ SERVICE_START | SERVICE_STOP);
if (schService == NULL) {
/* Could not open the service */
Re: cvs commit: apache-1.3/src/main http_protocol.c
WTF? -1 Jim, that code is doing an error check prior to the
strtol. It is not looking for the start of the number, but
ensuring that the number is non-negative and all digits prior
to calling the library routine. A simple check of *lenp would
have been sufficient for the blank case.
I need to go through the other changes as well, so I'll fix it,
but don't release with this code.
Roy
On Tuesday, July 9, 2002, at 07:47 AM, [EMAIL PROTECTED] wrote:
> jim 2002/07/09 07:47:24
>
> Modified:src CHANGES
>src/main http_protocol.c
> Log:
> Allow for null/all-whitespace C-L fields as we did pre-1.3.26. However,
> we do not allow for the total bogusness of values for C-L, just this
> one special case. IMO a C-L field of "iloveyou" is bogus as is one
> of "123yabbadabbado", which older versions appear to have allowed
> (and in the 1st case, assume 0 and in the 2nd assume 123). Didn't
> make sense to make this runtime, but a documented special case
> instead.
>
> Revision ChangesPath
> 1.1836+8 -0 apache-1.3/src/CHANGES
>
> Index: CHANGES
> ===
> RCS file: /home/cvs/apache-1.3/src/CHANGES,v
> retrieving revision 1.1835
> retrieving revision 1.1836
> diff -u -r1.1835 -r1.1836
> --- CHANGES 8 Jul 2002 18:06:54 - 1.1835
> +++ CHANGES 9 Jul 2002 14:47:23 - 1.1836
> @@ -1,5 +1,13 @@
>Changes with Apache 1.3.27
>
> + *) In 1.3.26, a null or all blank Content-Length field would be
> + triggered as an error; previous versions would silently ignore
> + this and assume 0. As a special case, we now allow this and
> + behave as we previously did. HOWEVER, previous versions would
> + also silently accept bogus C-L values; We do NOT do that. That
> + *is* an invalid value and we treat it as such.
> + [Jim Jagielski]
> +
> *) Add ProtocolReqCheck directive, which determines if Apache will
> check for a valid protocol string in the request (eg: HTTP/1.1)
> and return HTTP_BAD_REQUEST if not valid. Versions of Apache
>
>
>
> 1.324 +8 -2 apache-1.3/src/main/http_protocol.c
>
> Index: http_protocol.c
> ===
> RCS file: /home/cvs/apache-1.3/src/main/http_protocol.c,v
> retrieving revision 1.323
> retrieving revision 1.324
> diff -u -r1.323 -r1.324
> --- http_protocol.c 8 Jul 2002 18:06:55 - 1.323
> +++ http_protocol.c 9 Jul 2002 14:47:24 - 1.324
> @@ -2011,10 +2011,16 @@
>const char *pos = lenp;
>int conversion_error = 0;
>
> -while (ap_isdigit(*pos) || ap_isspace(*pos))
> +while (ap_isspace(*pos))
>++pos;
>
>if (*pos == '\0') {
> +/* special case test - a C-L field NULL or all blanks is
> + * assumed OK and defaults to 0. Otherwise, we do a
> + * strict check of the field */
> +r->remaining = 0;
> +}
> +else {
>char *endstr;
>errno = 0;
>r->remaining = ap_strtol(lenp, &endstr, 10);
> @@ -2023,7 +2029,7 @@
>}
>}
>
> -if (*pos != '\0' || conversion_error) {
> +if (conversion_error) {
>ap_log_rerror(APLOG_MARK, APLOG_NOERRNO|APLOG_ERR, r,
>"Invalid Content-Length");
>return HTTP_BAD_REQUEST;
>
>
>
>
RE: time for a 2.0.40 me thinks
> William A. Rowe, Jr. wrote: > > > At 12:18 PM 7/9/2002, Bill Stoddard wrote: > > > >> > I'm going to do a start the T&R process on thursday for a 2.0.40 > >> release > >> > so if you've got things which you want in there and are stable > >> > (apr_poll) comes to mind please commit them. > >> > > >> > Ian > >> > >> I'll try to run a profile on the new apr_poll code posted by Ryan this > >> afternoon or evening. It would be -really- good if we could make the > >> apr_time changes before 2.0.40... > > > > > > Agreed. They are in or out. BPane... any new news on profiling with > > busec values? > > > No news; I don't expect that I'll be able to work on this > for a while. > > --Brian Where and when was the post that described this proposal? I'd like to take a look. Is there a patch? Bill
Re: time for a 2.0.40 me thinks
William A. Rowe, Jr. wrote: > At 12:18 PM 7/9/2002, Bill Stoddard wrote: > >> > I'm going to do a start the T&R process on thursday for a 2.0.40 >> release >> > so if you've got things which you want in there and are stable >> > (apr_poll) comes to mind please commit them. >> > >> > Ian >> >> I'll try to run a profile on the new apr_poll code posted by Ryan this >> afternoon or evening. It would be -really- good if we could make the >> apr_time changes before 2.0.40... > > > Agreed. They are in or out. BPane... any new news on profiling with > busec values? No news; I don't expect that I'll be able to work on this for a while. --Brian
RE: time for a 2.0.40 me thinks
At 12:18 PM 7/9/2002, Bill Stoddard wrote: > > I'm going to do a start the T&R process on thursday for a 2.0.40 release > > so if you've got things which you want in there and are stable > > (apr_poll) comes to mind please commit them. > > > > Ian > >I'll try to run a profile on the new apr_poll code posted by Ryan this >afternoon or evening. It would be -really- good if we could make the >apr_time changes before 2.0.40... Agreed. They are in or out. BPane... any new news on profiling with busec values? Bill
RE: time for a 2.0.40 me thinks
> I'm going to do a start the T&R process on thursday for a 2.0.40 release > > so if you've got things which you want in there and are stable > (apr_poll) comes to mind please commit them. > > If all goes well we should have a tarball out the door this time > next week. > > Regards > Ian I'll try to run a profile on the new apr_poll code posted by Ryan this afternoon or evening. It would be -really- good if we could make the apr_time changes before 2.0.40... Bill
Re: [patch] mod_negotiation and authorization
On Tue, Jul 02, 2002 at 10:31:53AM -0400, Rodent of Unusual Size wrote:
> Did anyone check this one out? (I haven't) It sounds
> as though it would scratch some itches..
Not many other itches, by the looks of things :-(
On the assumption that it would be convenient to make available a
version of the patch that actually applies to the current CVS HEAD,
I'm including an updated version below.
Background and docs are available at, for example,
http://marc.theaimsgroup.com/?l=apache-httpd-dev&m=102190232502173&q=raw
-- if reposting that would be useful, let me know and I'll get on to
it.
Possibly this should be adapted to include some of the other
400-series status codes -- 402, 407, or perhaps 411 or 412 might be
useful (longer term) to be handled similarly. I'll wait to copy from
the mod_autoindex code on that count, though. For now, it's 401 only.
Build and tested against 2.0.39, which appears to still be the current
version (1.102) of mod_negotiation.c
Comments and criticisms welcome.
f
--
Francis Daly[EMAIL PROTECTED]
--- modules/mappers/mod_negotiation.c.2039 Fri May 17 12:24:16 2002
+++ modules/mappers/mod_negotiation.c Mon Jul 8 22:27:45 2002
@@ -88,10 +88,17 @@
*/
typedef struct {
+int reveal_secret_url;
int forcelangpriority;
apr_array_header_t *language_priority;
} neg_dir_config;
+/* reveal_secret_url flags
+ */
+#define RSU_UNDEF2/* this means "no explicit config" */
+#define RSU_ON 1/* "config on" */
+#define RSU_OFF 0/* "config off" */
+
/* forcelangpriority flags
*/
#define FLP_UNDEF0/* Same as FLP_DEFAULT, but base overrides */
@@ -107,6 +114,7 @@
{
neg_dir_config *new = (neg_dir_config *) apr_palloc(p, sizeof(neg_dir_config));
+new->reveal_secret_url = RSU_UNDEF;
new->forcelangpriority = FLP_UNDEF;
new->language_priority = NULL;
return new;
@@ -119,6 +127,9 @@
neg_dir_config *new = (neg_dir_config *) apr_palloc(p, sizeof(neg_dir_config));
/* give priority to the config in the subdirectory */
+new->reveal_secret_url = (add->reveal_secret_url != RSU_UNDEF)
+ ? add->reveal_secret_url
+: base->reveal_secret_url;
new->forcelangpriority = (add->forcelangpriority != FLP_UNDEF)
? add->forcelangpriority
: base->forcelangpriority;
@@ -128,6 +139,22 @@
return new;
}
+static const char *reveal_secret_url(cmd_parms *cmd, void *n_, int arg)
+{
+neg_dir_config *n = n_;
+const char *err = ap_check_cmd_context(cmd, NOT_IN_FILES);
+
+if (err != NULL) {
+return err;
+}
+n->reveal_secret_url = arg == RSU_OFF ? RSU_OFF : RSU_ON;
+/* that is functionally equivalent to
+n->reveal_secret_url = arg != 0;
+ for the RSU_* values #defined'd above. Clarity vs efficiency?
+*/
+return NULL;
+}
+
static const char *set_language_priority(cmd_parms *cmd, void *n_,
const char *lang)
{
@@ -188,6 +215,8 @@
{
AP_INIT_FLAG("CacheNegotiatedDocs", cache_negotiated_docs, NULL, RSRC_CONF,
"Either 'on' or 'off' (default)"),
+AP_INIT_FLAG("MultiviewsRevealSecretURL", reveal_secret_url, NULL,
+RSRC_CONF|OR_AUTHCFG,
+ "Either 'on' or 'off' (default)"),
AP_INIT_ITERATE("LanguagePriority", set_language_priority, NULL, OR_FILEINFO,
"space-delimited list of MIME language abbreviations"),
AP_INIT_ITERATE("ForceLanguagePriority", set_force_priority, NULL, OR_FILEINFO,
@@ -1045,6 +1074,7 @@
struct accept_rec accept_info;
void *new_var;
int anymatch = 0;
+int secretmatch = 0;
clean_var_rec(&mime_info);
@@ -1110,6 +1140,13 @@
if (sub_req->finfo.filetype != APR_REG)
continue;
+/* Note if it failed UNAUTHORIZED. We may want to return this
+ * status, eventually
+ */
+if (sub_req->status == HTTP_UNAUTHORIZED) {
+secretmatch = 1;
+}
+
/* If it has a handler, we'll pretend it's a CGI script,
* since that's a good indication of the sort of thing it
* might be doing.
@@ -1232,6 +1269,9 @@
* request must die.
*/
if (anymatch && !neg->avail_vars->nelts) {
+if (secretmatch && neg->conf->reveal_secret_url == RSU_ON) {
+return HTTP_UNAUTHORIZED;
+}
ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
"Negotiation: discovered file(s) matching request: %s"
" (None could be negotiated).",
Re: 1.3.26: Content-Length header too strict now
On Tue, Jul 09, 2002 at 10:21:11AM -0400, Jim Jagielski wrote: > we would need to check errno just for ERANGE and then adjust the endstr > check as well... Or else just keep the test as is, and allow for > all blank beforehand. I think I'd prefer this method, since it > provides a specific solution to a specific problem. So the question > is runtime directive or simply allow blank by default?? > > Votes? +1 for just allowing blank as a special case for the Content-Length: -aaron
time for a 2.0.40 me thinks
I'm going to do a start the T&R process on thursday for a 2.0.40 release so if you've got things which you want in there and are stable (apr_poll) comes to mind please commit them. If all goes well we should have a tarball out the door this time next week. Regards Ian
Re: 1.3.26: Content-Length header too strict now
Hmmm... The problem is that an all whitespace content-length value is
noted as an error. I would agree that if that is the case, Apache
should default to setting it to 0. This is easy to work around.
Since I've already added the ProtocolReqCheck directive, would people
think it's a good idea to either provide a similar directive for this,
or simple have 1.3.27 allow for "blank" C-L ??
This is the code area:
r->remaining = ap_strtol(lenp, &endstr, 10);
if (errno || (endstr && *endstr)) {
conversion_error = 1;
}
we would need to check errno just for ERANGE and then adjust the endstr
check as well... Or else just keep the test as is, and allow for
all blank beforehand. I think I'd prefer this method, since it
provides a specific solution to a specific problem. So the question
is runtime directive or simply allow blank by default??
Votes?
David Burry wrote:
>
> Hi!
>
> I'm having a problem since upgrading from Apache 1.3.23 to 1.3.26. It appears that
>the "Content-Length" header field is much more strict in what it will accept from
>http clients than it was before, and this is causing me biiig problems.
>
> A certain http client (which shall remain nameless due to embarrassment) is
>generating a request header like this:
>
> GET /some/file HTTP/1.0
> Host: some.place.com
> Content-Type:
> Last-Modified:
> Accept: */*
> User-Agent: foo
> Content-Length:
>
> Technically this is a very big no-no to have some blank header fields like this, I
>know. Content-Length, for instance, should either specify 0 (zero) or not be listed
>there at all. But the client is already out there in millions of users' hands,
>embedded into several popular products (as part of an auto-update sort of
>mechanism).. so ideally, what I'd like to have, is an environment variable
>flag that disables some of the strictness of the Content-Length header field
>checking, to allow this aberrant behavior in some cases without producing errors.
>Perhaps this can be made part of Apache 1.3.27?
>
> Please let me know what you all think of this idea.
>
> Dave
>
--
===
Jim Jagielski [|] [EMAIL PROTECTED] [|] http://www.jaguNET.com/
"A society that will trade a little liberty for a little order
will lose both and deserve neither" - T.Jefferson
