bb login.c

[Denys Vlasenko]

On Sunday 01 February 2009 20:47, walter harms wrote:
> Denys Vlasenko schrieb:
> > On Saturday 31 January 2009 18:37, walter harms wrote:
> >> Hi Denis,
> >> i have broken down my patch to login.c into several pieces (4).
> >> Shall i send the diff or the .c ?
> > 
> > Please send diffs
> 
> attached all patches should apply on top of each other. The patches are based
> on the login.c i send to the list end of last year.
> 
> v0-v1: basicly moves SELINUX and LOGIN_SCRIPTS out of the way

-#if ENABLE_SELINUX
-       if (is_selinux_enabled()) {
-               security_context_t old_tty_sid, new_tty_sid;
+       USE_SELINUX(initselinux(username, full_tty, &user_sid));

-               if (get_default_context(username, NULL, &user_sid)) {

but then

+static void initselinux(char *username, char *full_tty,
+                                               security_context_t * user_sid)
+{
+       security_context_t old_tty_sid, new_tty_sid;
+
+       if (get_default_context(username, NULL, user_sid)) {
+               bb_error_msg_and_die("cannot get SID for %s", username);
+       }

You lost "if (is_selinux_enabled()) ..." part


> v1-v2: rework the get ttynam() part, move from array to pointer

+       full_tty = ttyname(STDIN_FILENO);
+       if (!full_tty || strncmp(full_tty, "/dev/", 5)) {
+               full_tty = (char *) "UNKNOWN";
+               short_tty = full_tty;
+       } else {
+               short_tty = full_tty + 5;

If ttyname does not start with "/dev/", you set it to "UNKNOWN".
This is wrong.


> v2-v3: move pswd check for PAM and !PAM into check_pswd()
> 	- fix getpwnam_r buffer size

+static
+struct passwd *bb_getpwnam_r(char *username)
+{
+       struct passwd *pwdstruct,*ptr;
+       struct passwd *pwd1;
+       size_t len = 1024;
+       long int initlen = sysconf(_SC_GETPW_R_SIZE_MAX);
+
+       if (initlen > 0)
+               len = (size_t) initlen;

what if sysconf() return a value which doesn't fit in size_t?

+
+       /*
+          pwdstruct is a list of pointers to

no, it isn't

+          data stored in pwdbuf

stored where?

+        */
+
+       pwdstruct= xmalloc(len+sizeof(*pwdstruct));
+       ptr=pwdstruct+len;

Bug. pwdstruct + n moves ptr n * sizeof(struct passwd) bytes!



+       /*
+         pwd1 will become NULL on not found
+          getpwnam_r will become 0 on eror

getpwnam does not "become", it "returns".



+/*
+  this is a special case
+*/
+       if (!pamuser || !pamuser[0])
+               goto auth_failed;

Does this comment actually explain something? If it says
'NULL or "" username is considered a failure', well,
that's obvious from the code.


        if (option_mask32 & LOGIN_OPT_f)
...
...
...
                opt &= ~LOGIN_OPT_f;

You forgot to copy updated flags to option_mask32.



I think check_pswd() can be better implemented as:

struct passwd *check_pswd(const char *username)

instead of

static int check_pswd(char *username, int userlen, struct passwd **pw2)



When these issues are addressed:

function                                             old     new   delta
login_main                                          1552    1584     +32

:(

I applied patches 1 and 2, thanks! Please look at attached patch.
It's a cleaned up version of patch 3. I hesitate to apply it
to svn, there might be more unexploded ordnance there.

If it would be at least not bigger with the patch according
to "make bloatcheck", then maybe...
--
vda
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 5.patch
Type: text/x-diff
Size: 7731 bytes
Desc: not available
Url : http://lists.busybox.net/pipermail/busybox/attachments/20090202/82b9a001/attachment-0001.bin