Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: Cherokee: users

issue 43: Problem in the range port cherokee-admin

 

 

Cherokee users RSS feed   Index | Next | Previous | View Threaded


perki_pat3 at yahoo

Apr 22, 2008, 4:59 PM

Post #1 of 5 (372 views)
Permalink
issue 43: Problem in the range port cherokee-admin

Hi!, last monday there was a meeting by Alo at the university of
Salamanca, and a few of the attendees decided to take a look on cherokee.

I found several issues in the Google Code's page, and this one looked
simple enough to go for it.
The user reported that:
"""
The cherokee-admin doesn't check the range port when you use the option
-p. For example if you use cherokee-admin -p 100000, the program open
aleatory port. The solution is checking the range of ports ( 0 - 65000~)
when you use the option -p.
"""

golgi:/home/pablo/cherokee/cherokee/cherokee$ diff -u main_admin.c
main_admin.c.new
--- main_admin.c 2008-04-23 01:46:52.000000000 +0200
+++ main_admin.c.new 2008-04-23 01:43:42.000000000 +0200
@@ -143,6 +143,11 @@
break;
case 'p':
port = atoi(optarg);
+ if(port>65535 || port <=0) {
+ fprintf(stderr,
+ "Bad port number, port range is between 1 and 65535\n");
+ exit(1);
+ }
break;
case 'd':
document_root = strdup(optarg);
golgi:/home/pablo/cherokee/cherokee/cherokee$

I don't remember to have read about coding style at Cherokee's web. I
haven't colaborated in any software projects, every help is welcomed.

bye.



______________________________________________
LLama Gratis a cualquier PC del Mundo.
Llamadas a fijos y m髒iles desde 1 c閚timo por minuto.
http://es.voice.yahoo.com

_______________________________________________
Cherokee mailing list
Cherokee[at]cherokee-project.com
http://cherokee-project.com/cgi-bin/mailman/listinfo/cherokee


alvaro at sun

Apr 22, 2008, 10:21 PM

Post #2 of 5 (355 views)
Permalink
Re: issue 43: Problem in the range port cherokee-admin [In reply to]

On 23 Apr 2008, at 01:59, Perki Pat wrote:
> Hi!, last monday there was a meeting by Alo at the university of
> Salamanca, and a few of the attendees decided to take a look on
> cherokee.

Hello Perki! I'm delighted you have finally come along.. :-)

> The cherokee-admin doesn't check the range port when you use the
> option
> -p. For example if you use cherokee-admin -p 100000, the program open
> aleatory port. The solution is checking the range of ports ( 0 -
> 65000~)
> when you use the option -p.
>
> --- main_admin.c 2008-04-23 01:46:52.000000000 +0200
> +++ main_admin.c.new 2008-04-23 01:43:42.000000000 +0200
> @@ -143,6 +143,11 @@
> break;
> case 'p':
> port = atoi(optarg);
> + if(port>65535 || port <=0) {
> + fprintf(stderr,
> + "Bad port number, port range is between 1 and 65535\n");
> + exit(1);
> + }
> break;
> case 'd':
> document_root = strdup(optarg);

The patch is good, congrats! However, there is a couple of changes
that would improve the patch before we apply it:

- I would move the checking code to the initialize_server_socket()
function in cherokee/server.c file. In that way the port would always
be checked, not only when the -p parameter is used; for instance, it
would ensure that a port read from a configuration file is correct as
well.

- There are a few macros from printing errors:

PRINT_ERROR_S: Print en error with no parameters.
PRINT_ERROR: Print an error with parameters (needs to be formated).
PRINT_MSG_S: Print a message with no parameters.
PRINT_MSG: Print a message with parameters (needs to be formated).

In this case, I would use PRINT_ERROR_S.

- Ah, and finally, instead of calling exit() it would be better if you
used: return ret_error; That is our equivalent of raising and
exception (well, sort of).

> I don't remember to have read about coding style at Cherokee's web. I
> haven't colaborated in any software projects, every help is welcomed.

There isn't any document defining the coding style, although it would
very interesting to have one. For now the best practice is to look at
the code to see how things are written.

--
Greetings, alo.

_______________________________________________
Cherokee mailing list
Cherokee[at]cherokee-project.com
http://cherokee-project.com/cgi-bin/mailman/listinfo/cherokee


perki_pat3 at yahoo

Apr 23, 2008, 7:33 AM

Post #3 of 5 (353 views)
Permalink
Re: issue 43: Problem in the range port cherokee-admin [In reply to]

Perki Pat escribi贸:
> Alvaro Lopez Ortega escribi贸:
>> On 23 Apr 2008, at 01:59, Perki Pat wrote:
>> The patch is good, congrats! However, there is a couple of changes
>> that would improve the patch before we apply it:
>>
>> - I would move the checking code to the initialize_server_socket()
>> function in cherokee/server.c file. In that way the port would always
>> be checked, not only when the -p parameter is used; for instance, it
>> would ensure that a port read from a configuration file is correct as
>> well.
>
> initialize_server_socket() receives an unsigned short argument for the
> port: it's always in range (gcc told me).
>
> It's called from a couple places (server.c:940 and server.c:1070), and
> in both places it takes its value from a cherokee_server_t struct, which
> has a couple field (port and port_tls) which are also unsigned short.
>
> I'm looking where that fields are assigned. I've found where they're
> filled from config file. Now I'll look for more places.

ok, I think this choice is done. I'll paste the diffs below.

We can check that if I'm root the port or port_tls is greater than 1024.
(Windows also?) Or maybe it's better to "outsource" it to bind()+perror()??

I've changed severals cuint_t port; with cushort_t port;

I've seen several atoi()s with no boundaries checking
(server.c:configure_server_property(), lines circa 1480). is right?

Also, when I do cherokee -p 67, if the config file says server!port=81,
cherokee listens in port 81. Maybe command line arguments should
"overwrite" config file statements.

In the practicals I do at the university I'm used to always assert
pointer arguments received by a function:
#include <assert.h> /*and define DEBUG elsewhere*/
/*...*/
static ret_t
configure_server_property (cherokee_config_node_t *conf, void *data)
{
/*variable definitions*/
assert(conf && data);
/*real work...*/

The problem with paranoical error checking is that a 200 lines program
becomes a 700 one, but maybe it helps hunting several bugs.


ps: cc to list?
pss: the diffs:
pablo[at]golgi:~$ for i in cherokee-old/cherokee/*.[ch]; do diff -u $i
`echo $i | sed "s/cherokee-old/cherokee-new/" -`; done;
--- cherokee-old/cherokee/main_admin.c 2008-04-23 13:23:06.000000000 +0200
+++ cherokee-new/cherokee/main_admin.c 2008-04-23 13:29:28.000000000 +0200
@@ -46,7 +46,7 @@
#define DEFAULT_BIND "127.0.0.1"
#define RULE "vserver!default!rule!"

-static int port = DEFAULT_PORT;
+static cushort_t port = DEFAULT_PORT;
static char *document_root = DEFAULT_DOCUMENTROOT;
static char *config_file = DEFAULT_CONFIG_FILE;
static char *bind_to = DEFAULT_BIND;
@@ -121,7 +121,7 @@
static void
process_parameters (int argc, char **argv)
{
- int c;
+ int c,tmp_port;

struct option long_options[] = {
{"help", no_argument, NULL, 'h'},
@@ -142,7 +142,12 @@
bind_to = NULL;
break;
case 'p':
- port = atoi(optarg);
+ tmp_port = atoi(optarg);
+ if(tmp_port<1 || tmp_port>65535) {
+ PRINT_MSG("Port %d is not between 1 and 65535\n",tmp_port);
+ exit(1);
+ }
+ port=tmp_port;
break;
case 'd':
document_root = strdup(optarg);
--- cherokee-old/cherokee/main.c 2008-04-23 13:23:06.000000000 +0200
+++ cherokee-new/cherokee/main.c 2008-04-23 12:03:03.000000000 +0200
@@ -69,7 +69,7 @@
static char *document_root = NULL;
static cherokee_boolean_t daemon_mode = false;
static cherokee_boolean_t just_test = false;
-static cuint_t port = 80;
+static cushort_t port = 80;

static ret_t common_server_initialization (cherokee_server_t *srv);

@@ -202,7 +202,7 @@
static void
process_parameters (int argc, char **argv)
{
- int c;
+ int c,tmp_port;

struct option long_options[] = {
{"help", no_argument, NULL, 'h'},
@@ -227,7 +227,12 @@
document_root = strdup(optarg);
break;
case 'p':
- port = atoi(optarg);
+ tmp_port = atoi(optarg);
+ if(tmp_port<1 || tmp_port>65535) {
+ PRINT_MSG("Port %d is not between 1 and 65535\n",tmp_port);
+ exit(1);
+ }
+ port=tmp_port;
break;
case 't':
just_test = true;
--- cherokee-old/cherokee/server.c 2008-04-23 13:23:06.000000000 +0200
+++ cherokee-new/cherokee/server.c 2008-04-23 13:00:02.000000000 +0200
@@ -1462,13 +1462,22 @@
ret_t ret;
char *key = conf->key.buf;
cherokee_server_t *srv = SRV(data);
+ int tmp;

if (equal_buf_str (&conf->key, "port")) {
- srv->port = atoi(conf->val.buf);
-
+ tmp = atoi(conf->val.buf);
+ if(tmp<1 || tmp>65535) {
+ PRINT_MSG("Port %d is not between 1 and 65535\n",tmp);
+ return ret_error;
+ }
+ srv->port=tmp;
} else if (equal_buf_str (&conf->key, "port_tls")) {
- srv->port_tls = atoi(conf->val.buf);
-
+ tmp = atoi(conf->val.buf);
+ if(tmp<1 || tmp>65535) {
+ PRINT_MSG("TLS port %d is not between 1 and 65535\n",tmp);
+ return ret_error;
+ }
+ srv->port_tls=tmp;
} else if (equal_buf_str (&conf->key, "fdlimit")) {
srv->fdlimit_custom = atoi (conf->val.buf);



______________________________________________
LLama Gratis a cualquier PC del Mundo.
Llamadas a fijos y m髒iles desde 1 c閚timo por minuto.
http://es.voice.yahoo.com


perki_pat3 at yahoo

Apr 23, 2008, 9:55 AM

Post #4 of 5 (352 views)
Permalink
Re: issue 43: Problem in the range port cherokee-admin [In reply to]

Alvaro Lopez Ortega escribi贸:
> On 23 Apr 2008, at 01:59, Perki Pat wrote:
> The patch is good, congrats! However, there is a couple of changes that
> would improve the patch before we apply it:
>
> - I would move the checking code to the initialize_server_socket()
> function in cherokee/server.c file. In that way the port would always be
> checked, not only when the -p parameter is used; for instance, it would
> ensure that a port read from a configuration file is correct as well.

initialize_server_socket() receives an unsigned short argument for the
port: it's always in range (gcc told me).

It's called from a couple places (server.c:940 and server.c:1070), and
in both places it takes its value from a cherokee_server_t struct, which
has a couple field (port and port_tls) which are also unsigned short.

I'm looking where that fields are assigned. I've found where they're
filled from config file. Now I'll look for more places.

Other choice is to receive an int type, and check it into
initialize_server_socket()

> - There are a few macros from printing errors:
>
> PRINT_ERROR_S: Print en error with no parameters.
> PRINT_ERROR: Print an error with parameters (needs to be formated).
> PRINT_MSG_S: Print a message with no parameters.
> PRINT_MSG: Print a message with parameters (needs to be formated).
>
> In this case, I would use PRINT_ERROR_S.
>
> - Ah, and finally, instead of calling exit() it would be better if you
> used: return ret_error; That is our equivalent of raising and exception
> (well, sort of).
ok, thanks for the advices

>> I don't remember to have read about coding style at Cherokee's web. I
>> haven't colaborated in any software projects, every help is welcomed.
>
> There isn't any document defining the coding style, although it would
> very interesting to have one. For now the best practice is to look at
> the code to see how things are written.
BlueZ uses Linux coding style: http://www.bluez.org/development.html
http://www.kroah.com/linux/talks/ols_2002_kernel_codingstyle_paper/codingstyle.ps


______________________________________________
LLama Gratis a cualquier PC del Mundo.
Llamadas a fijos y m髒iles desde 1 c閚timo por minuto.
http://es.voice.yahoo.com


alvaro at sun

Apr 23, 2008, 12:35 PM

Post #5 of 5 (352 views)
Permalink
Re: issue 43: Problem in the range port cherokee-admin [In reply to]

On 23 Apr 2008, at 16:33, Perki Pat wrote:
> Perki Pat escribi贸:
>> Alvaro Lopez Ortega escribi贸:
>>> On 23 Apr 2008, at 01:59, Perki Pat wrote:
>>> The patch is good, congrats! However, there is a couple of changes
>>> that would improve the patch before we apply it:
>>>
>>> - I would move the checking code to the initialize_server_socket()
>>> function in cherokee/server.c file. In that way the port would
>>> always be checked, not only when the -p parameter is used; for
>>> instance, it would ensure that a port read from a configuration
>>> file is correct as well.
>> initialize_server_socket() receives an unsigned short argument for
>> the port: it's always in range (gcc told me).
>> It's called from a couple places (server.c:940 and server.c:1070),
>> and in both places it takes its value from a cherokee_server_t
>> struct, which has a couple field (port and port_tls) which are also
>> unsigned short.
>> I'm looking where that fields are assigned. I've found where
>> they're filled from config file. Now I'll look for more places.
>
> ok, I think this choice is done. I'll paste the diffs below.
>
> We can check that if I'm root the port or port_tls is greater than
> 1024.
> (Windows also?) Or maybe it's better to "outsource" it to bind()
> +perror()??

No, that is not a good idea. We have been discussing something similar
these days, actually. The idea is that we must let the operating
system to do its jobs instead of trying to do it twice in the server
side.

As you said, at the first look checking that the port is higher than
1024 when it isn't run as root may look like a good idea. However,
there are systems that will allow the server to bind to a low port
even if it is not run as root. For instance, Windows or Solaris with
RBAC would do it.

So, the underneath idea here is that we must not assume anything; the
best approach is to let the operating system make the decision on what
is allowed and what is not. In this case, whether to open a low port
is permitted.

> I've changed severals cuint_t port; with cushort_t port;
>
> I've seen several atoi()s with no boundaries checking
> (server.c:configure_server_property(), lines circa 1480). is right?

It depends on each case. It might be something that we should let the
operating system to validate.

> Also, when I do cherokee -p 67, if the config file says server!
> port=81,
> cherokee listens in port 81. Maybe command line arguments should
> "overwrite" config file statements.

No, that is the expected behavior actually. The -p parameter is only
meant to be used in combination with -r, otherwise it should not
affect the 'server!port' configuration entry.

> ps: cc to list?

Yeah, please sent everything to the list. Public revision is one of
the most interesting advantages of F/OSS.

--
Greetings, alo.

_______________________________________________
Cherokee mailing list
Cherokee[at]cherokee-project.com
http://cherokee-project.com/cgi-bin/mailman/listinfo/cherokee

Cherokee users RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact lists@gossamer-threads.com
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.