Changeset 3183c21

Timestamp:

2008-09-06T22:59:32Z (16 years ago)

Author:

Wilmer van der Gaast <wilmer@…>

Branches:

master

Children:

15d1469

Parents:

0a4f6f4

Message:

Completely reviewed all uses of irc->password, irc_setpass() and
USTATUS_IDENTIFIED after another account overwriting vulnerability was
found by Tero Marttila.

Files:

• irc.c (modified) (2 diffs)
• root_commands.c (modified) (5 diffs)
• storage.c (modified) (6 diffs)
• storage.h (modified) (2 diffs)
• storage_text.c (modified) (4 diffs)
• storage_xml.c (modified) (5 diffs)

irc.c

@@ -38 +38 @@
         irc_t *irc = set->data;
         
-        if( irc->status & USTATUS_IDENTIFIED )
+        if( irc->status & USTATUS_IDENTIFIED && value )
         {
                 irc_setpass( irc, value );
@@ -239 +239 @@
         
         if( irc->status & USTATUS_IDENTIFIED && set_getbool( &irc->set, "save_on_quit" ) ) 
-                if( storage_save( irc, TRUE ) != STORAGE_OK )
+                if( storage_save( irc, NULL, TRUE ) != STORAGE_OK )
                         irc_usermsg( irc, "Error while saving settings!" );
         

root_commands.c

@@ -131 +131 @@
 static void cmd_identify( irc_t *irc, char **cmd )
 {
-        storage_status_t status = storage_load( irc->nick, cmd[1], irc );
+        storage_status_t status = storage_load( irc, cmd[1] );
         char *account_on[] = { "account", "on", NULL };
         
@@ -143 +143 @@
         case STORAGE_OK:
                 irc_usermsg( irc, "Password accepted, settings and accounts loaded" );
+                irc_setpass( irc, cmd[1] );
+                irc->status |= USTATUS_IDENTIFIED;
                 irc_umode_set( irc, "+R", 1 );
                 if( set_getbool( &irc->set, "auto_connect" ) )
@@ -162 +164 @@
         }
 
-        irc_setpass( irc, cmd[1] );
-        switch( storage_save( irc, FALSE )) {
+        switch( storage_save( irc, cmd[1], FALSE ) ) {
                 case STORAGE_ALREADY_EXISTS:
                         irc_usermsg( irc, "Nick is already registered" );
@@ -170 +171 @@
                 case STORAGE_OK:
                         irc_usermsg( irc, "Account successfully created" );
+                        irc_setpass( irc, cmd[1] );
                         irc->status |= USTATUS_IDENTIFIED;
                         irc_umode_set( irc, "+R", 1 );
@@ -887 +889 @@
 static void cmd_save( irc_t *irc, char **cmd )
 {
-        if( storage_save( irc, TRUE ) == STORAGE_OK )
+        if( ( irc->status & USTATUS_IDENTIFIED ) == 0 )
+                irc_usermsg( irc, "Please create an account first" );
+        else if( storage_save( irc, NULL, TRUE ) == STORAGE_OK )
                 irc_usermsg( irc, "Configuration saved" );
         else

storage.c

@@ -103 +103 @@
 }
 
-storage_status_t storage_load (const char *nick, const char *password, irc_t * irc)
-{
-        GList *gl;
+storage_status_t storage_load (irc_t * irc, const char *password)
+{
+        GList *gl;
+        
+        if (irc && irc->status & USTATUS_IDENTIFIED)
+                return STORAGE_OTHER_ERROR;
         
         /* Loop until we don't get NO_SUCH_USER */
@@ -112 +115 @@
                 storage_status_t status;
 
-                status = st->load(nick, password, irc);
-                if (status == STORAGE_OK) {
-                        irc_setpass(irc, password);
+                status = st->load(irc, password);
+                if (status == STORAGE_OK)
                         return status;
-                }
                 
                 if (status != STORAGE_NO_SUCH_USER) 
@@ -125 +126 @@
 }
 
-storage_status_t storage_save (irc_t *irc, int overwrite)
-{
-        return ((storage_t *)global.storage->data)->save(irc, overwrite);
+storage_status_t storage_save (irc_t *irc, char *password, int overwrite)
+{
+        storage_status_t st;
+        
+        if (password != NULL) {
+                /* Should only use this in the "register" command. */
+                if (irc->password || overwrite)
+                        return STORAGE_OTHER_ERROR;
+                
+                irc_setpass(irc, password);
+        } else if ((irc->status & USTATUS_IDENTIFIED) == 0) {
+                return STORAGE_NO_SUCH_USER;
+        }
+        
+        st = ((storage_t *)global.storage->data)->save(irc, overwrite);
+        
+        if (password != NULL) {
+                irc_setpass(irc, NULL);
+        }
+        
+        return st;
 }
 
@@ -143 +162 @@
 
                 status = st->remove(nick, password);
-                if (status != STORAGE_NO_SUCH_USER && 
-                        status != STORAGE_OK)
+                if (status != STORAGE_NO_SUCH_USER && status != STORAGE_OK)
                         ret = status;
         }
@@ -150 +168 @@
         return ret;
 }
+
+#if 0
+Not using this yet. Test thoroughly before adding UI hooks to this function.
 
 storage_status_t storage_rename (const char *onick, const char *nnick, const char *password)
@@ -189 +210 @@
         return STORAGE_OK;
 }
+#endif

storage.h

@@ -45 +45 @@
         storage_status_t (*check_pass) (const char *nick, const char *password);
 
-        storage_status_t (*load) (const char *nick, const char *password, irc_t * irc);
+        storage_status_t (*load) (irc_t *irc, const char *password);
         storage_status_t (*save) (irc_t *irc, int overwrite);
         storage_status_t (*remove) (const char *nick, const char *password);
@@ -55 +55 @@
 storage_status_t storage_check_pass (const char *nick, const char *password);
 
-storage_status_t storage_load (const char *nick, const char *password, irc_t * irc);
-storage_status_t storage_save (irc_t *irc, int overwrite);
+storage_status_t storage_load (irc_t * irc, const char *password);
+storage_status_t storage_save (irc_t *irc, char *password, int overwrite);
 storage_status_t storage_remove (const char *nick, const char *password);
 
-storage_status_t storage_rename (const char *onick, const char *nnick, const char *password);
+/* storage_status_t storage_rename (const char *onick, const char *nnick, const char *password); */
 
 void register_storage_backend(storage_t *);

storage_text.c

@@ -44 +44 @@
 }
 
-static storage_status_t text_load ( const char *my_nick, const char* password, irc_t *irc )
+static storage_status_t text_load( irc_t *irc, const char* password )
 {
         char s[512];
@@ -54 +54 @@
         account_t *acc, *acc_lookup[9];
         
-        if( irc->status & USTATUS_IDENTIFIED )
-                return( 1 );
-        
-        g_snprintf( s, 511, "%s%s%s", global.conf->configdir, my_nick, ".accounts" );
+        g_snprintf( s, 511, "%s%s%s", global.conf->configdir, irc->nick, ".accounts" );
         fp = fopen( s, "r" );
         if( !fp ) return STORAGE_NO_SUCH_USER;
@@ -68 +65 @@
                 return STORAGE_INVALID_PASSWORD;
         }
-        
-        /* Do this now. If the user runs with AuthMode = Registered, the
-           account command will not work otherwise. */
-        irc->status |= USTATUS_IDENTIFIED;
         
         while( fscanf( fp, "%511[^\n]s", s ) > 0 )
@@ -101 +94 @@
         }
         
-        g_snprintf( s, 511, "%s%s%s", global.conf->configdir, my_nick, ".nicks" );
+        g_snprintf( s, 511, "%s%s%s", global.conf->configdir, irc->nick, ".nicks" );
         fp = fopen( s, "r" );
         if( !fp ) return STORAGE_NO_SUCH_USER;

storage_xml.c

@@ -259 +259 @@
 }
 
-static storage_status_t xml_load_real( const char *my_nick, const char *password, irc_t *irc, xml_pass_st action )
+static storage_status_t xml_load_real( irc_t *irc, const char *my_nick, const char *password, xml_pass_st action )
 {
         GMarkupParseContext *ctx;
@@ -266 +266 @@
         GError *gerr = NULL;
         int fd, st;
-        
-        if( irc && irc->status & USTATUS_IDENTIFIED )
-                return( 1 );
         
         xd = g_new0( struct xml_parsedata, 1 );
@@ -321 +318 @@
                 return STORAGE_OK;
         
-        irc->status |= USTATUS_IDENTIFIED;
-        
         return STORAGE_OK;
 }
 
-static storage_status_t xml_load( const char *my_nick, const char *password, irc_t *irc )
-{
-        return xml_load_real( my_nick, password, irc, XML_PASS_UNKNOWN );
+static storage_status_t xml_load( irc_t *irc, const char *password )
+{
+        return xml_load_real( irc, irc->nick, password, XML_PASS_UNKNOWN );
 }
 
@@ -335 +330 @@
         /* This is a little bit risky because we have to pass NULL for the
            irc_t argument. This *should* be fine, if I didn't miss anything... */
-        return xml_load_real( my_nick, password, NULL, XML_PASS_CHECK_ONLY );
+        return xml_load_real( NULL, my_nick, password, XML_PASS_CHECK_ONLY );
 }
 
@@ -370 +365 @@
         md5_byte_t pass_md5[21];
         md5_state_t md5_state;
-        
-        if( irc->password == NULL )
-        {
-                irc_usermsg( irc, "Please register yourself if you want to save your settings." );
-                return STORAGE_OTHER_ERROR;
-        }
         
         path2 = g_strdup( irc->nick );