patterncMinor
Session manager for logged in users
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
session.h
session.c
```
#include
#include
#include
#include "session.h"
#include "binary_tree.h"
#define EXPIRY_DEFAULT 3600
#define allocate malloc
#def
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);
#endifsession.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.
-
-
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:
-
sess_new_session is overcomplicated. Factor out getting random number; reuse same
-
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.