4
\$\begingroup\$

I wrote this code to handle logged in users. The session IDs will be stored in cookies. I would like to know if it's usable or if there are security problems.

It uses a 64bit id and another 64bit validation ID. Perhaps this is a bad idea, but here's what I thought: if someone tries to carry a brute force attack and happens to find the first value, he will most likely not have found the second value, so I can destroy that session before it's compromised. Would using a single huge id be better? What size would be considered safe?

I know the ideal would be to write a custom tree to avoid calling bt_find() and bt_remove() in some places and I plan to do that next.

session.h

#ifndef SESSION_H
#define SESSION_H

#include <stdlib.h>
#include <time.h>

#define ID_SIZE 8

typedef struct Session_Manager Session_Manager;
typedef struct {
    char id0[ID_SIZE];
    char id1[ID_SIZE];
} Session;

//Every manager has its own set of sessions
Session_Manager *session_manager_new(void);
void session_manager_delete(Session_Manager *sm);

//Expiry is how long it takes for a session to be deleted
void session_manager_set_expiry(Session_Manager *sm, time_t seconds);
time_t session_manager_get_expiry(Session_Manager *sm);

//It's possible to execute a custom function to clean up user_data
void session_manager_set_on_delete(Session_Manager *sm, void (*delete_cb)(void *));
void (*session_manager_get_on_delete(Session_Manager *sm))(void *);

//Delete all expired sessions
void sess_clean_old_sessions(Session_Manager *sm);

//Create new session and associate user_data to it
Session *sess_new_session(Session_Manager *sm, void *user_data);

//Validate session, increase expiry time and return user_data
void *sess_get_data(Session_Manager *sm, Session *session);

//Delete session
void sess_delete_session(Session_Manager *sm, Session *session);

#endif

session.c

#include <unistd.h>
#include <fcntl.h>
#include <stdint.h>
#include "session.h"
#include "binary_tree.h"

#define EXPIRY_DEFAULT 3600
#define allocate malloc
#define deallocate free

char *table = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVXYZ0123456789";

struct Session_Manager {
    Binary_Tree sessions;
    time_t expiry;
    void (*on_delete)(void *);
};

/* If id0 is correct but id1 is wrong it will fail and the session will be
deleted in order to make bruteforce unlikely to work. id1 is completely ignored
until a session with id0 is found */
typedef struct {
    uint64_t id0;
    uint64_t id1;
    time_t expiry;
    void *user_data;
} _Session;

//Callback to compare two sessions
static int compare_sessions(const void *s0_ptr, const void *s1_ptr)
{
    const _Session *s0 = s0_ptr;
    const _Session *s1 = s1_ptr;

    return s0->id0 - s1->id0;
}

//Callback to clean old sessions
static void clean_sessions(void *s_ptr, void *sm_ptr)
{
    _Session *s = s_ptr;
    Session_Manager *sm = sm_ptr;

    if(s->expiry <= time(NULL)){
        if(sm->on_delete != NULL)
            sm->on_delete(s->user_data);

        bt_remove(&sm->sessions, s);
        deallocate(s);
    }
}

static void delete_all_content(void *s_ptr, void *sm_ptr)
{
    _Session *s = s_ptr;
    Session_Manager *sm = sm_ptr;

    if(sm->on_delete != NULL)
        sm->on_delete(s->user_data);

    deallocate(s);
}

//Every manager has its own set of sessions
Session_Manager *session_manager_new(void)
{
    Session_Manager *sm = allocate(sizeof(Session_Manager));
    if(sm == NULL)
        return NULL;

    bt_init(&sm->sessions, compare_sessions);
    sm->expiry = EXPIRY_DEFAULT;
    sm->on_delete = NULL;

    return sm;
}

void session_manager_delete(Session_Manager *sm)
{
    bt_iterate_arg(&sm->sessions, delete_all_content, sm);      
    bt_free(&sm->sessions);
    deallocate(sm);
}

//Expiry is how long it takes for a session to be deleted
void session_manager_set_expiry(Session_Manager *sm, time_t seconds)
{
    sm->expiry = seconds;
}

time_t session_manager_get_expiry(Session_Manager *sm)
{
    return sm->expiry;
}

void session_manager_set_on_delete(Session_Manager *sm, void (*delete_cb)(void *))
{
    sm->on_delete = delete_cb;
}

void (*session_manager_get_on_delete(Session_Manager *sm))(void *)
{
    return sm->on_delete;
}

//Delete all expired sessions
void sess_clean_old_sessions(Session_Manager *sm)
{
    bt_iterate_arg(&sm->sessions, clean_sessions, sm);
}

//Create new session
Session *sess_new_session(Session_Manager *sm, void *user_data)
{
    int file_descriptor, ret;
    _Session *new_session = allocate(sizeof(_Session));
    unsigned int random;
    unsigned char *ids = (unsigned char *)new_session;

    if(new_session == NULL)
        return NULL;

    if((file_descriptor = open("/dev/urandom", O_RDONLY)) == -1){
        deallocate(new_session);
        return NULL;
    }

    generate_random:
    for(size_t i = 0; i < ID_SIZE * 2; ++i){
        if(read(file_descriptor, &random, sizeof(int)) == -1){
            deallocate(new_session);
            close(file_descriptor);
            return NULL;
        }

        random = random % 62;
        ids[i] = table[random];     
    }

    if((ret = bt_insert(&sm->sessions, new_session)) != BT_SUCCESS){
        if(ret == BT_DUPLICATE)
            goto generate_random;

        deallocate(new_session);
        close(file_descriptor);
        return NULL;
    }

    close(file_descriptor);
    new_session->expiry = time(NULL) + sm->expiry;
    new_session->user_data = user_data;

    return (Session *) new_session;
}

//Validate session, increase expiry time and return user_data
void *sess_get_data(Session_Manager *sm, Session *session)
{
    _Session *real_session = bt_find(&sm->sessions, session);
    if(real_session == NULL)
        return NULL;

    //Check id1
    if(real_session->id1 != *(uint64_t *)session->id1){
        if(sm->on_delete != NULL)
            sm->on_delete(real_session->user_data);

        bt_remove(&sm->sessions, real_session);
        deallocate(real_session);
        return NULL;
    }

    real_session->expiry = time(NULL) + sm->expiry;
    return real_session->user_data;
}

//Delete session
void sess_delete_session(Session_Manager *sm, Session *session)
{
    _Session *real_session = bt_find(&sm->sessions, session);

    //There's no session with this id
    if(real_session == NULL)
        return;

    if(sm->on_delete != NULL)
        sm->on_delete(real_session->user_data);

    bt_remove(&sm->sessions, real_session);
    deallocate(real_session);
}
\$\endgroup\$
2
\$\begingroup\$

I am going to be very urandom here.

  1. ids[i] = table[random] badly decreases the entropy. Instead of 64 random bits, your ID has less than 48.

  2. Can you elaborate on a use case, especially how the validation ID is used.

  3. As much as I am against global variables here is one exception I am ready to accept: /dev/urandom can well be opened once when the process starts. There's no real difference between a global file descriptor and a global literal. Besides, its randomness doesn't degrade.

  4. sess_new_session is overcomplicated. Factor out getting random number; reuse same new_session in case of BT_DUPLICATE. See also note 2.

\$\endgroup\$
  • \$\begingroup\$ Thank you for answering. I'm using that table because I need to set cookies with the id and just copying the random bytes directly produces invalid characters. The id1 is used in sess_get_data(), if id1 does not match, it will destroy the session. I don't understand how to reuse same new_session since the id is already associated with another session? What length would you consider safe? \$\endgroup\$ – 2013Asker Apr 25 '14 at 7:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.