HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Session manager for logged in users

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
forsessionmanagerloggedusers

Problem

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 
#include 

#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
#include
#include
#include "session.h"
#include "binary_tree.h"

#define EXPIRY_DEFAULT 3600
#define allocate malloc
#def

Solution

I am going to be very urandom here.

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

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

-
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.

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

Context

StackExchange Code Review Q#48123, answer score: 2

Revisions (0)

No revisions yet.