* utmpx support @ 2012-03-04 17:41 finkler 2012-03-04 18:18 ` Rich Felker 0 siblings, 1 reply; 6+ messages in thread From: finkler @ 2012-03-04 17:41 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: utmpx support 2012-03-04 17:41 utmpx support finkler @ 2012-03-04 18:18 ` Rich Felker 2012-03-04 19:10 ` finkler 0 siblings, 1 reply; 6+ messages in thread From: Rich Felker @ 2012-03-04 18:18 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: utmpx support 2012-03-04 18:18 ` Rich Felker @ 2012-03-04 19:10 ` finkler 2012-03-06 1:30 ` Rich Felker 2012-03-06 1:39 ` Rich Felker 0 siblings, 2 replies; 6+ messages in thread From: finkler @ 2012-03-04 19:10 UTC (permalink / raw) To: musl [-- 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); ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Re: utmpx support 2012-03-04 19:10 ` finkler @ 2012-03-06 1:30 ` Rich Felker 2012-03-16 14:05 ` finkler 2012-03-06 1:39 ` Rich Felker 1 sibling, 1 reply; 6+ messages in thread From: Rich Felker @ 2012-03-06 1:30 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: utmpx support 2012-03-06 1:30 ` Rich Felker @ 2012-03-16 14:05 ` finkler 0 siblings, 0 replies; 6+ messages in thread From: finkler @ 2012-03-16 14:05 UTC (permalink / raw) To: musl [-- 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); ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Re: utmpx support 2012-03-04 19:10 ` finkler 2012-03-06 1:30 ` Rich Felker @ 2012-03-06 1:39 ` Rich Felker 1 sibling, 0 replies; 6+ messages in thread From: Rich Felker @ 2012-03-06 1:39 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-03-16 14:05 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-03-04 17:41 utmpx support finkler 2012-03-04 18:18 ` Rich Felker 2012-03-04 19:10 ` finkler 2012-03-06 1:30 ` Rich Felker 2012-03-16 14:05 ` finkler 2012-03-06 1:39 ` Rich Felker
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/musl/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).