utmpx support

utmpx support

[finkler]

Hi there,

I was wondering whether it is intentional or just due to more pressing 
tasks that utmpx is a stub?
If it is because of the latter I would gladly be of help, after all this 
seems kind of trivial, or am I missing something?

Thanks in advance,
finkler

Re: utmpx support

[Rich Felker]

On Sun, Mar 04, 2012 at 06:41:25PM +0100, finkler wrote:
> Hi there,
> 
> I was wondering whether it is intentional or just due to more
> pressing tasks that utmpx is a stub?

It's intentional, but if you have a real need for utmp support, I'd be
willing to hear about it.

My own view is that utmp is a major source of security risks due both
to the need for suid/sgid binaries to access it and the inherent
information leak of publicly publishing users' login status, and that
it has few if any legitimate purposes. It comes from a very different
era/culture, reminiscent of the days when putting a password on your
account was seen as offensive. :-)

> If it is because of the latter I would gladly be of help, after all
> this seems kind of trivial, or am I missing something?

Perhaps a better approach would be making a separate small static
libutmp.a that could be linked by people wanting real utmp support as
opposed to the stubs.

Rich

Re: utmpx support

[finkler]

[-- Attachment #1: Type: text/plain, Size: 1411 bytes --]

On 04.03.2012 19:18, Rich Felker wrote:
> On Sun, Mar 04, 2012 at 06:41:25PM +0100, finkler wrote:
>
> It's intentional, but if you have a real need for utmp support, I'd be
> willing to hear about it.

Well, I am currently trying to implement new (POSIX) user-space 
utilities for Linux, including my own init/login system.
I thought quite a bit about the purpose of utmp(x) myself I admit, 
however, I came to the conclusion that even though I know no one who 
still uses serial terminals to connect to a machine, connecting through 
LAN/Internet is very popular, and it couldn't hurt that an administrator 
has the means to easily see who is currently logged in (who).

I agree that utmpx has grown disproportionally and has lots of misuses 
which just bloat the code.
But as I said, some method of accessing login information seems sane to 
me. And if used with restrain utmpx is quite well suited for this ... I 
think.

> Perhaps a better approach would be making a separate small static
> libutmp.a that could be linked by people wanting real utmp support as
> opposed to the stubs.

Certainly, for what it's worth, I hacked up an utmpx implementation.
I hope I didn't make any mistakes (code and standard wise), but the code 
size is small, so it should be easily verifiable for another set of eyes 
:-).

Regards, and thank you very much for the good work with this lib, it is 
really great.

Finkler


[-- Attachment #2: utmpx.c --]
[-- Type: text/plain, Size: 1774 bytes --]

#include <utmpx.h>
#include <stddef.h>
#include "libc.h"

#define _PATH_UTMPX "/etc/utmp"

static FILE *f;
static struct utmpx ut;

void endutxent(void)
{
  memset(&ut, 0, sizeof ut);
  if (f == NULL) return;
  fclose(f);
  f == NULL;
}

void setutxent(void)
{
  memset(&ut, 0, sizeof ut);
  if (f == NULL) return;
  rewind(f);
}

struct utmpx *getutxent(void)
{
	if (f == NULL) {
    f = fopen(_PATH_UTMPX, "a+");
    if (f == NULL) {
      f = fopen(_PATH_UTMPX, "r");
      if (f == NULL) return NULL;
    }
  }
  if (fread(&ut, sizeof ut, 1, f)) return &ut;
  return NULL;
}

struct utmpx *getutxid(const struct utmpx *id)
{  
  while(getutxent()) {
    switch (id->ut_type) {
      case BOOT_TIME:
      case OLD_TIME:
      case NEW_TIME:
        if (id->ut_type == ut.ut_type) return &ut;
        break;
      case INIT_PROCESS:
      case LOGIN_PROCESS:
      case USER_PROCESS:
      case DEAD_PROCESS:
        if (id->type == ut.ut_type && !strcmp(id->ut_id, ut.ut_id)) return &ut;
        break;
    }
  }    
  return NULL;
}

struct utmpx *getutxline(const struct utmpx *line)
{
	while(getutxent()) {
    switch (ut.ut_type) {
      case LOGIN_PROCESS:
      case USER_PROCESS:
        if (!strcmp(line->ut_line, ut.ut_line)) return &ut;
        break;
    }
  }
  return NULL;
}

struct utmpx *pututxline(const struct utmpx *utmpx)
{
  setutxent();
  if (getutxid(utmpx)) fseek(f, -(sizeof ut), SEEK_CUR);
  if (fwrite(&ut, sizeof ut, 1, f)) return &ut;
  return NULL;
}

void updwtmpx(const char *f, const struct utmpx *u)
{
}

weak_alias(endutxent, endutent);
weak_alias(setutxent, setutent);
weak_alias(getutxent, getutent);
weak_alias(getutxid, getutid);
weak_alias(getutxline, getutline);
weak_alias(pututxline, pututline);
weak_alias(updwtmpx, updwtmp);

Re: Re: utmpx support

[Rich Felker]

On Sun, Mar 04, 2012 at 08:10:36PM +0100, finkler wrote:
> void endutxent(void)
> {
>   memset(&ut, 0, sizeof ut);

These memsets don't seem to be doing anything necessary.

> struct utmpx *getutxent(void)
> {
> 	if (f == NULL) {
>     f = fopen(_PATH_UTMPX, "a+");

Append mode is definitely not correct if you want to be able to
overwrite existing entries.

> struct utmpx *pututxline(const struct utmpx *utmpx)
> {
>   setutxent();
>   if (getutxid(utmpx)) fseek(f, -(sizeof ut), SEEK_CUR);

I'm confused about the intent of this code.. Presumably it's supposed
to modify the entry it just found (relying on getutxid leaving the
file at the right position), but that's not going to work because the
file is opened in append mode.

Also, -(sizeof ut) is not a valid offset; it's almost SIZE_MAX. If you
were using fseeko, this would result in a gigantic offset (rather than
a negative one) after the conversion; with fseek (which takes a long),
it should convert back to the desired negative value, but for safety
you should cast to a signed type *before* negating the result of
sizeof.

There's also the issue that there's absolutely no locking on updates,
so if multiple apps try to add/update utmp entries at the same time,
the file would be corrupted. This could be fixed by using an fcntl
lock on fileno(f) for the duration of the function.

Rich

Re: utmpx support

[finkler]

[-- Attachment #1: Type: text/plain, Size: 2094 bytes --]

On 06.03.2012 02:30, Rich Felker wrote:
> On Sun, Mar 04, 2012 at 08:10:36PM +0100, finkler wrote:
> Append mode is definitely not correct if you want to be able to
> overwrite existing entries.
Changed it to r+ and r

> I'm confused about the intent of this code.. Presumably it's supposed
> to modify the entry it just found (relying on getutxid leaving the
> file at the right position), but that's not going to work because the
> file is opened in append mode.
>
> Also, -(sizeof ut) is not a valid offset; it's almost SIZE_MAX. If you
> were using fseeko, this would result in a gigantic offset (rather than
> a negative one) after the conversion; with fseek (which takes a long),
> it should convert back to the desired negative value, but for safety
> you should cast to a signed type *before* negating the result of
> sizeof
Made the cast inside the function call.

> There's also the issue that there's absolutely no locking on updates,
> so if multiple apps try to add/update utmp entries at the same time,
> the file would be corrupted. This could be fixed by using an fcntl
> lock on fileno(f) for the duration of the function.
I added a lock, however, I am unsure whether I should use a lock that 
waits until it is free or not, and whether I should lock the whole file, 
or just the portion which is about to be written.
Right now I use a waiting lock, and lock the whole file.

 > Here strcmp is being called on data that was read from a file with no
 > validation. This is potentially a security issue (DoS); if the file
 > does not contain a null-terminated string, strcmp could run past the
 > end of the buffer and eventually segfault and crash the calling
 > program. It's probably hard to trigger the issue since the string for
 > comparison is also located in a utmp structure, but I think there
 > should be some validation, probably just fixing invalid data right
 > after the fread call so it never leaks out.
I changed it to use strncmp with the sizeof the respective struct field, 
is this enough?

Thank you very much for your time, I attached the reworked version.

[-- Attachment #2: utmpx.c --]
[-- Type: text/plain, Size: 2052 bytes --]

#include <utmpx.h>
#include <stddef.h>
#include "libc.h"

#define _PATH_UTMPX "/etc/utmp"

static FILE *f;
static struct utmpx ut;

void endutxent(void)
{
  if (f == NULL) return;
  fclose(f);
  f == NULL;
}

void setutxent(void)
{
  if (f == NULL) return;
  rewind(f);
}

struct utmpx *getutxent(void)
{
  if (f == NULL) {
    f = fopen(_PATH_UTMPX, "r+");
    if (f == NULL) {
      f = fopen(_PATH_UTMPX, "r");
      if (f == NULL) return NULL;
    }
  }
  if (fread(&ut, sizeof ut, 1, f)) return &ut;
  return NULL;
}

struct utmpx *getutxid(const struct utmpx *id)
{  
  while(getutxent()) {
    switch (id->ut_type) {
      case BOOT_TIME:
      case OLD_TIME:
      case NEW_TIME:
        if (id->ut_type == ut.ut_type) return &ut;
        break;
      case INIT_PROCESS:
      case LOGIN_PROCESS:
      case USER_PROCESS:
      case DEAD_PROCESS:
        if (id->type == ut.ut_type && !strncmp(id->ut_id, ut.ut_id, sizeof ut.ut_id)) return &ut;
        break;
    }
  }    
  return NULL;
}

struct utmpx *getutxline(const struct utmpx *line)
{
	while(getutxent()) {
    switch (ut.ut_type) {
      case LOGIN_PROCESS:
      case USER_PROCESS:
        if (!strncmp(line->ut_line, ut.ut_line, sizeof ut.ut_line)) return &ut;
        break;
    }
  }
  return NULL;
}

struct utmpx *pututxline(const struct utmpx *utmpx)
{
  struct flock fl;
  size_t n;
  
  fl.l_start = 0;
  fl.l_len = 0;
  fl.l_pid = getpid();
  fl.l_type = F_WRLCK;
  fl.l_whence = SEEK_SET;
  
  if (fcntl(fileno(f), F_SETLKW, &fl) < 0) return NULL;
  
  setutxent();
  
  if (getutxid(utmpx)) fseek(f, -((long)(sizeof ut)), SEEK_CUR);
  
  n = fwrite(&ut, sizeof ut, 1, f);
  
  fl.l_type = F_UNLCK;
  fcntl(fileno(f), F_SETLK, &fl);
  
  if (n == 1)
    return &ut;
  return NULL;
}

void updwtmpx(const char *f, const struct utmpx *u)
{
}

weak_alias(endutxent, endutent);
weak_alias(setutxent, setutent);
weak_alias(getutxent, getutent);
weak_alias(getutxid, getutid);
weak_alias(getutxline, getutline);
weak_alias(pututxline, pututline);
weak_alias(updwtmpx, updwtmp);

Re: Re: utmpx support

[Rich Felker]

On Sun, Mar 04, 2012 at 08:10:36PM +0100, finkler wrote:
> struct utmpx *getutxid(const struct utmpx *id)
> {  
>   while(getutxent()) {
>     switch (id->ut_type) {
>       case BOOT_TIME:
>       case OLD_TIME:
>       case NEW_TIME:
>         if (id->ut_type == ut.ut_type) return &ut;
>         break;
>       case INIT_PROCESS:
>       case LOGIN_PROCESS:
>       case USER_PROCESS:
>       case DEAD_PROCESS:
>         if (id->type == ut.ut_type && !strcmp(id->ut_id, ut.ut_id)) return &ut;

Here strcmp is being called on data that was read from a file with no
validation. This is potentially a security issue (DoS); if the file
does not contain a null-terminated string, strcmp could run past the
end of the buffer and eventually segfault and crash the calling
program. It's probably hard to trigger the issue since the string for
comparison is also located in a utmp structure, but I think there
should be some validation, probably just fixing invalid data right
after the fread call so it never leaks out.

> struct utmpx *getutxline(const struct utmpx *line)
> {
> 	while(getutxent()) {
>     switch (ut.ut_type) {
>       case LOGIN_PROCESS:
>       case USER_PROCESS:
>         if (!strcmp(line->ut_line, ut.ut_line)) return &ut;

Here too.

Rich