cvs commit: src/usr.sbin/jexec jexec.8 jexec.c

[Bjoern A. Zeeb]

On Mon, 26 May 2008, Michael Reifenberger wrote:

Hi,

> mr          2008-05-26 11:57:49 UTC
>
>  FreeBSD src repository
>
>  Modified files:
>    usr.sbin/jexec       jexec.8 jexec.c
>  Log:
>  Extend jexec to accept hostname or ip-number besides jail-id.
>
>  MFC after:      2 weeks
>
>  Revision  Changes    Path
>  1.5       +7 -2      src/usr.sbin/jexec/jexec.8
>  1.5       +53 -2     src/usr.sbin/jexec/jexec.c

It seems you decided to leave this in (for now).

So here are my problems with the code - could you please fix them?

1. you are accepting an IP address or a hostname instead of a jail ID
    without an option letter like -i or -h. Why is this a problem?
    I have hostnames like 127.
    127 is a valid IP Address.
    127 could be a Jail-ID I am not refereing to.

2. When going through the list of IPs you got from the kernel:
+       for (i = 0; i < len / sizeof(*xp); i++) {
+               in.s_addr = ntohl(xp->pr_ip);
    it should be htonl. (at least for correct documentation purposes)

3. You are comparing strings of IP addresses:
+               if ((strncmp(inet_ntoa(in), addr, slen) == 0) ||
    Now, would that match 127 or 0x7f000000? No. Convert the string
    to an address and compare the numbers.

4. You are taking the length og the input address:
+       slen = strlen(addr);
    Assume I gave 192.0.2.1 as input, the length would be 9.
    Assume the kernel gives you back 192.0.2.111.
    strncmp on those two would give you a match, which is wrong.
    Same is true for hostnames, like input "foo", kernel "foobar".

5. addr2jid should be static

6. On match you are doing:
+                       free(sxp);
+                       return (xp->pr_id);
    freeing sxp but returning xp->pr_id but
    wherever xp points to is within the malloced area of sxp.
    Use after free.


Greetings
Bjoern

-- 
Bjoern A. Zeeb              Stop bit received. Insert coin for new game.