From: s902113@minyos.xx.rmit.oz.au (Luke Mewburn)
Newsgroups: rec.games.mud.lp
Subject: Suggested security fix for 2.4.5 (and 3.x?) mudlibs (REPOST WITH FIX)
Date: 10 Nov 1992 15:26:38 GMT
Organization: RMIT Computer Centre
NNTP-Posting-Host: minyos.xx.rmit.oz.au
Keywords: bugfix, amazing, wow, not_driver_flame_war

Greetings!
    If you use a 2.4.5 mudlib (or even a 3.x version - I've not
checked), and you are partially security concious, read on.
If not, or you like to flame people about coding standards and
which mudlibs they should be using, etc, crawl back to alt.flame...


    Disclaimer aside, whilst wading through the player object
on the mud I noticed that there was a security hole that could be
a pain. I'd actually fixed a related problem with shout which helped
highlight the problem.

    Basically, if your player object does logins in the following
manner:
    ...
    read name
    if (!restore..(.. + name))
	write("new character");
    ...	/* setup crap here ... */
    if newchar
	input_to("new_passwd");
    else
	input_to("check_passwd");
    ...
	
(i.e, the restore occurs BEFORE the password validation, read on! -
you have the potential problem).

    So, any code that traverses users() (shout, wiztell, etc), that
doesn't check for environment(this_player()) being not null (which,
BTW, probably still isn't as secure as it could be) will either barf
it it tries to access the env of the logging in player, or incorrectly
send a message to the player as query_guild_name() or query_level()
will succeed as the info is there after the restore...

    My initial idea for a solution was to fix all code that used
users(), then I thought - 'I can do that, but it would be better if
I solved the problem, not the symptoms'. What follows is my solution.

    First, have a 'trusted' object to validate characters and compares
passwords, etc. The code for that object follows. This code _could_ be
better, but, hey, it works :)

    Now, depending on how your login sequence looks, you have to do
some hacking... Lets say it does this:
    logon()		print mesgs, call logon2
    logon2()		passed the user name
			<do the restore / input_to bit as above>
    new_password()	for a new player
    check_password()	validate password, dest(this_obj()) if false
    move_p_to_start()	called by new_pwd() or chk_pwd() after validation.

    In the time that the user types in the name and hits return, and
the timeout for the password occurs, code which doesn't fully error
check (and there's shitloads of code out there like that:) could stuff
up for this player...

    Ok, now, lets reorganize the above abit to look something like:
(Note, my comments added for this note look like
/* XXX: look like this really :-) */
)
    logon2()		- setup init values (will be overridden by
			restore for old player)
			- check for illegal players, etc
			- have code like:
-- begin code seg --
         /* Zak 921107 - first part of password check */
    level = call_other("/obj/player/check_pw", "valid_character", str);
    if (!level)      /* level will be -1 for newbie, 0 for err, 1 for ok */
    {
        destruct(this_object());
        return 1;
    }
    if (level == -1)
        write("New character.\n");

/* XXX: more shit here for checking - deleted for clarity???? :) */

    name = str;                        /* Must be here for a new player. */
    myself = this_player();
    if (level != -1)
        input_to("check_password", 1);
    else
        input_to("new_password", 1);
    write("Password: "); 
    if (str == "guest")
        write("(just CR) ");
}
-- end code seg --

    Inside check_password(), have something like:

-- begin code seg --
check_password(p) {
    int res = call_other("/obj/player/check_pw", "validate_password", p, name);
    if (!res)
    {
        write("res is 0.\n");
        destruct(myself);
        return;
    }
    restore_object("characters/" + name);
/* XXX: Now, do the other stuff like logging, etc. */
    after_password();
}
-- end code seg --
    

    From both the end of new_password() and check_password(), remove
the instances of 'move_player_to_start()' and replace with
'after_password()'. after_password() looks like:


-- begin code seg --
after_password()
{
/* XXX: stuff you do after the restore, like: */
    armor_class = 0;
    name_of_weapon = 0;
    weapon_class = 0;
    time_on = ctime(time());
    attacker_ob = 0;
    alt_attacker_ob = 0;
/* XXX: etc */
    move_player_to_start(0);
}
-- end code seg --



    Now, I kinda helps _someone_ out there, as it's taken me about
20 minutes to compose this net.bandwidth.waste (tm) :) I might have
missed something, but who knows. If you have any queries or valid
comments, feel free to follow-up or mail. I'll mail_bomb_to_hell (tm)
any shithead who flames for the sake of flaming (guess what - I've had
a /real good day/ and the thought that this might help someone calms
me down a lot.


Zak.
[ Zak on Ivory Towers (itchy.macc.wisc.edu 2000) ]

PS: I have other stuff I might put on r.g.m.lp that might be useful
than flames about drivers :)...

--- CODE FOR /obj/player/check_pw.c FOLLOWS (unlike 1st post :) --- 
/*
 *	Password Validation Code
 *
 * Written 921107 by Zak
 *
 */
 
string password;	/* filled in from restore. */

nomask query_no_shadow() { return 1; }

/*
 *     valid_character
 *
 * Call with real_name of character to check, and returns:
 * -1 : new character
 *  0 : illegal name, etc
 *  1 : old character
 *
 */
nomask
valid_character(str)
{
    if (!previous_ok())
        return 0;
    if (!str || str == "")
        return 0;
    if (restore_object("characters/" + str))
        return 1;
    else
        return -1;	/* new character */
} /* valid_character */


nomask static
previous_ok()
{
    object prev;
    string pname, path;
    int onum;
    
    prev = previous_object();
    if (!prev)
    {
        log_file("ILL_PWD", ctime() + " no previous object.\n");
        return 0;
    }
    pname = file_name(prev);
    if (sscanf(pname, "%s#%d", path, onum) != 2)
    {
        log_file("ILL_PWD", ctime() + " bad path: " +pname + ".\n");
        return 0;
    }
    if (path != "obj/player/player")
    {
        log_file("ILL_PWD", ctime() + " bad path: " +pname + ".\n");
        return 0;
    }
    return 1;
} /* previous_ok */


/*
 *     validate_password
 *
 * Validates that the password 'p' for player 'name' is correct.
 *
 * returns with 0 if not correct, 1 if so. Note, it's
 * assumed that you DON'T call this from a new character.
 * If you do, 0 will be returned.
 *
 */

nomask
validate_password(p, name)
{
    int res;
    
    res=valid_character(name);
    if (res <= 0)    /* new or invalid character */
        return 0;
    if (name == "guest")
    {
        write("Welcome, Guest! Enjoy your tour of Ivory Towers.\n");
        return 1;
    }
    if (!password || password == "")
    {
        write("You have no password ! Set it with the 'password' cmd.\n");
        return 1;
    }
    if (crypt(p, password) != password)
    {
        write("Wrong password!\n");
        return 0;
    }
    return 1;
} /* validate_password */
--- CODE ---

--
                         Luke Mewburn [Zak]
           s902113@minyos.xx.rmit.oz.au   zak@rmit.edu.au
   "Nobody dies on the Discworld, they just become dimensionally
    disadvantaged."         Terry Pratchett in alt.fan.pratchett