Simple note taking program

Question

This is similar to the program in the book Hacking: The Art of Exploitation. It is rewritten to fit my needs.

There are two files, takenote and readnote. To write a note simply use takenote with your note as a command line argument: ./takenote "Buy eggs". The note gets added to the file /tmp/notes with User ID (non readable form) and timestamp, seperated by colons (:).

To read your notes simply use ./readnote. This only displays notes that were written by the current user.

To avoid being able to just cat /tmp/notes, change the user of both takenote and readnote to root and then use the setuid permission:

sudo chown root takenote readnote
sudo chmod u+s takenote readnote

This should be done before using takenote for the first time.

takenote.c

#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <time.h>
#include <unistd.h>

#define FILENAME "/tmp/notes"

void usage(char*);
void fatal(char*);

int main(int argc, char* argv[]) {
    int fd; // file descriptor
    uid_t uid;
    time_t raw_time;
    struct tm* date_time;

    if (argc < 2)
        usage(argv[0]);

    // Open file
    fd = open(FILENAME, O_WRONLY | O_APPEND | O_CREAT, S_IRUSR | S_IWUSR);
    if (fd == -1)
        fatal("opening file");

    // Write UID
    uid = getuid();
    if (write(fd, &uid, sizeof(uid)) == -1)
        fatal("writing UID");
    if (write(fd, ":", 1) == -1) // Adding seperator
        fatal("writing seperator");

    // Write timestamp
    time(&raw_time);
    date_time = localtime(&raw_time);
    if (write(fd, asctime(date_time), 24) == -1)
        fatal("writing timestamp");
    if (write(fd, ":", 1) == -1) // Adding seperator
        fatal("writing seperator");

    // Write note
    if (write(fd, argv[1], strlen(argv[1])) == -1)
        fatal("writing note");
    if (write(fd, "\n", 1) == -1) // Adding newline
        fatal("writing newline");

    // Close file
    if (close(fd) == -1)
        fatal("closing file");

    return 0;
}

void usage(char* prog_name) {
    printf("Usage: %s <note to be saved in %s>\n", prog_name, FILENAME);
    exit(-1);
}

void fatal(char* msg) {
    fprintf(stderr, "Error %s.\n", msg);
    exit(-1);
}

readnote.c

#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <unistd.h>

#define FILENAME "/tmp/notes"

void fatal(char*);
int print_notes(int, uid_t);
int find_user_note(int, uid_t);

int main() {
    int fd; // file descriptor
    uid_t uid;

    // Open file
    fd = open(FILENAME, O_RDONLY);
    if (fd == -1)
        fatal("opening file");

    // Read notes for UserID uid
    uid = getuid();
    while (print_notes(fd, uid) != -1) // Read until EOF.
        ;

    // Close file
    if (close(fd) == -1)
        fatal("closing file");

    return 0;
}

void fatal(char* msg) {
    fprintf(stderr, "Error %s.\n", msg);
    exit(-1);
}

// Function to read all user notes with corresponding UID.
// Returns -1 if EOF, 0 otherwise.
int print_notes(int fd, uid_t uid) {
    int note_length;
    char* buffer;

    note_length = find_user_note(fd, uid);
    if (note_length == -1) // If EOF
        return -1;

    // Copy note into buffer and print it.
    buffer = (char*) malloc(note_length + 1);
    if (buffer == NULL)
        fatal("allocating memory");
    if (read(fd, buffer, note_length) == -1)
        fatal("reading note");
    buffer[note_length] = 0;
    printf("%s", buffer);
    free(buffer);

    return 0;
}

// Function to find the next user note. Returns -1
// if EOF, note length otherwise.
int find_user_note(int fd, uid_t uid) {
    uid_t note_uid;
    unsigned char byte;
    size_t note_length;

    note_uid = -1;

    while (note_uid != uid) {
        // If UID is not fully read return EOF.
        if (read(fd, &note_uid, sizeof(uid)) != sizeof(uid))
            return -1;
        // If seperator (:) is not read return EOF.
        if (read(fd, &byte, 1) != 1)
            return -1;

        byte = 0;
        note_length = 0;
        // Read date, time and note
        while (byte != '\n') {
            // If we can't read another byte return EOF.
            if (read(fd, &byte, 1) != 1)
                return -1;
            ++note_length;
        }
    }
    // Rewind read pointer to beginning of note (excluding UID).
    lseek(fd, note_length * -1, SEEK_CUR);
    return note_length;
}

Compiled with

gcc takenote.c -o takenote
gcc readnote.c -o readnote

Answer 1

I am not well versed in c and will defer to others for language particular concerns, but I hope I can say something helpful.

I am very impressed with your diligence in checking for failures, whether from malloc, filesystem interaction, or anything else that could fail. That said, you should probably take more care about how you handle failures when they occur. You should do your best to clear any open resources and bad persistent state before you panic and exit. That's not always possible: if someone's using the program on a flash drive and they pull it out half way through writing then it's really their fault that things get corrupted and you can't fix the file from half way across the room! Even so, making sure that you close the file when you can and such is worth doing.

Similarly, when reading your file you should assume (especially if you are trying to think through the security implications of this all) that the file could be corrupted (e.g. flash drive pulled out) or otherwise messed with.

On a similar note, it is very important to validate any input that comes from the user, and check that it isn't going to cause problems. For example, if you have magic characters that are acting as delimiters (such as colons, newlines, etc) then you'll need to make sure that the user doesn't include them in any of their notes. In the best case scenario such input will break your program, and in the worst case scenario it will allow the user to break through the security guarantees of your program and mess with stuff they're not meant to mess with.

Code wise, as a general rule in this family of languages, it is recommended that if statements and loops and such always have a bracketed block of code after them. The code will work if you only have one line after them, but it's easier to reason about and harder to accidentally introduce bugs with if you wrap up that line in brackets anyway.

if(test)
    {
        fun();
    }

(I do appreciate that when your while loop is meant to be entirely self contained, the semicolon sits on the next line. Even so, {} would perhaps be clearer, and other ways of writing the loop perhaps clearer still.)

Two other little points that are slightly out of scope for a code review: If you're writing something thinking about security (which well done for thinking about!) don't leave it to chance. If say your user has to change the permissions on the programs, it's better to provide some sort of installation script that does it all for them, rather than telling them what to do.

In this particular case, I would be very wary of the "It must be root" approach to security. It means that if you do have a bug which lets people do something nasty (and I'll assume "something nasty" means "run arbitrary code with a buffer overflow or something") they're doing something nasty with full root privileges. The general approach in security is that everything should be done with the lowest level of privilege it can possibly do its job with.