From 04b1fb7e8ec2100a109973c966068e5ad220da15 Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Mon, 21 Dec 2015 22:32:32 +0100 Subject: [PATCH] Data integrity check for cache record Provide simple data validation - in the meaning of data integrity - check for the cache record. This should allow cache format upgrade in the future and mitigate potential segmentation faults (with the risk of an arbitrary code execution). Also, from now on, the cache file is endianness invariant. --- src/cache.c | 190 ++++++++++++++++++++++++++++++++++----------------- src/cache.h | 28 ++++++-- src/cmusfm.h | 1 + src/server.c | 8 --- src/utils.c | 8 +++ 5 files changed, 156 insertions(+), 79 deletions(-) diff --git a/src/cache.c b/src/cache.c index 688c5b8..52879cd 100644 --- a/src/cache.c +++ b/src/cache.c @@ -28,68 +28,91 @@ #include #include #include +#include #include "cmusfm.h" #include "debug.h" -/* Return the actual size of given cache record structure. */ +/* Return the actual size of the given cache record structure. */ static size_t get_cache_record_size(const struct cmusfm_cache_record *record) { - return sizeof(*record) + record->artist_len + record->album_len + - record->album_artist_len + record->track_len + record->mbid_len; + return sizeof(*record) + record->len_artist + record->len_album + + record->len_track + record->len_album_artist + record->len_mbid; +} + +/* Return the checksum for the length-invariant part of the cache record + * structure - segmentation-fault-safe checksum. */ +static uint8_t get_cache_record_checksum1(const struct cmusfm_cache_record *record) { + return make_data_hash((char *)&record->timestamp, + sizeof(*record) - ((void *)&record->timestamp - (void *)record)); +} + +/* Return the data checksum of the given cache record structure. If the + * overall record length is compromised, we might end up dead... */ +static uint8_t get_cache_record_checksum2(const struct cmusfm_cache_record *record) { + return make_data_hash((char *)&record->timestamp, + get_cache_record_size(record) - ((void *)&record->timestamp - (void *)record)); } /* Copy scrobbler track info into the cache record structure. Returned * pointer has to be freed by the `free` function. */ -struct cmusfm_cache_record *get_cache_record(const scrobbler_trackinfo_t *sb_tinf) { +static struct cmusfm_cache_record *get_cache_record(const scrobbler_trackinfo_t *sb_tinf) { - struct cmusfm_cache_record *cr; + struct cmusfm_cache_record *record; char *ptr; - /* allocate memory for initial record data (header) */ - cr = (struct cmusfm_cache_record *)calloc(1, sizeof(*cr)); + /* allocate memory for the initial record data (header) */ + record = (struct cmusfm_cache_record *)calloc(1, sizeof(*record)); + + record->signature = CMUSFM_CACHE_SIGNATURE; + record->timestamp = sb_tinf->timestamp; + record->track_number = sb_tinf->track_number; + record->duration = sb_tinf->duration; - cr->signature = CMUSFM_CACHE_SIGNATURE; - cr->timestamp = sb_tinf->timestamp; - cr->track_number = sb_tinf->track_number; - cr->duration = sb_tinf->duration; if (sb_tinf->artist) - cr->artist_len = strlen(sb_tinf->artist) + 1; + record->len_artist = strlen(sb_tinf->artist) + 1; if (sb_tinf->album) - cr->album_len = strlen(sb_tinf->album) + 1; -// if (sb_tinf->album_artist) -// cr->album_artist_len = strlen(sb_tinf->album_artist) + 1; + record->len_album = strlen(sb_tinf->album) + 1; if (sb_tinf->track) - cr->track_len = strlen(sb_tinf->track) + 1; -// if (sb_tinf->mbid) -// cr->mbid_len = strlen(sb_tinf->mbid) + 1; + record->len_track = strlen(sb_tinf->track) + 1; +#if 0 + if (sb_tinf->album_artist) + record->len_album_artist = strlen(sb_tinf->album_artist) + 1; + if (sb_tinf->mbid) + record->len_mbid = strlen(sb_tinf->mbid) + 1; +#endif /* enlarge allocated memory for string data payload */ - cr = (struct cmusfm_cache_record *)realloc(cr, get_cache_record_size(cr)); - ptr = (char *)&cr[1]; + record = (struct cmusfm_cache_record *)realloc(record, get_cache_record_size(record)); + ptr = (char *)&record[1]; - if (cr->artist_len) { + if (record->len_artist) { strcpy(ptr, sb_tinf->artist); - ptr += cr->artist_len; + ptr += record->len_artist; } - if (cr->album_len) { + if (record->len_album) { strcpy(ptr, sb_tinf->album); - ptr += cr->album_len; + ptr += record->len_album; } -// if (cr->album_artist_len) { -// strcpy(ptr, sb_tinf->album_artist); -// ptr += cr->album_artist_len; -// } - if (cr->track_len) { + if (record->len_track) { strcpy(ptr, sb_tinf->track); - ptr += cr->track_len; + ptr += record->len_track; + } +#if 0 + if (record->len_album_artist) { + strcpy(ptr, sb_tinf->album_artist); + ptr += record->len_album_artist; } -// if (cr->mbid_len) { -// strcpy(ptr, sb_tinf->mbid); -// ptr += cr->mbid_len; -// } + if (record->len_mbid) { + strcpy(ptr, sb_tinf->mbid); + ptr += record->len_mbid; + } +#endif - return cr; + record->checksum1 = get_cache_record_checksum1(record); + record->checksum2 = get_cache_record_checksum2(record); + + return record; } /* Write data, which should be submitted later, to the cache file. */ @@ -97,6 +120,7 @@ void cmusfm_cache_update(const scrobbler_trackinfo_t *sb_tinf) { FILE *f; struct cmusfm_cache_record *record; + size_t record_size; debug("cache update: %ld", sb_tinf->timestamp); debug("payload: %s - %s (%s) - %d. %s (%ds)", @@ -107,7 +131,20 @@ void cmusfm_cache_update(const scrobbler_trackinfo_t *sb_tinf) { return; record = get_cache_record(sb_tinf); - fwrite(record, get_cache_record_size(record), 1, f); + record_size = get_cache_record_size(record); + + /* convert host endianness to the "universal" one */ + record->signature = htons(record->signature); + record->timestamp = htonl(record->timestamp); + record->track_number = htons(record->track_number); + record->duration = htons(record->duration); + record->len_artist = htons(record->len_artist); + record->len_album = htons(record->len_album); + record->len_track = htons(record->len_track); + record->len_album_artist = htons(record->len_album_artist); + record->len_mbid = htons(record->len_mbid); + + fwrite(record, record_size, 1, f); free(record); fclose(f); @@ -131,51 +168,72 @@ void cmusfm_cache_submit(scrobbler_session_t *sbs) { /* read file until EOF */ while (!feof(f)) { rd_len = fread(rd_buff, 1, sizeof(rd_buff), f); - record = (struct cmusfm_cache_record*)rd_buff; + record = (struct cmusfm_cache_record *)rd_buff; /* iterate while there is no enough data for full cache record header */ - while ((void*)record - (void*)rd_buff + sizeof(*record) <= rd_len) { - - if (record->signature != CMUSFM_CACHE_SIGNATURE) { - debug("invalid cache record signature: %x", record->signature); - fclose(f); - return; + while ((void *)record - (void *)rd_buff + sizeof(*record) <= rd_len) { + + /* convert "universal" endianness to the host one */ + record->signature = ntohs(record->signature); + record->timestamp = ntohl(record->timestamp); + record->track_number = ntohs(record->track_number); + record->duration = ntohs(record->duration); + record->len_artist = ntohs(record->len_artist); + record->len_album = ntohs(record->len_album); + record->len_track = ntohs(record->len_track); + record->len_album_artist = ntohs(record->len_album_artist); + record->len_mbid = ntohs(record->len_mbid); + + /* validate record type and first-stage data integration */ + if (record->signature != CMUSFM_CACHE_SIGNATURE || + record->checksum1 != get_cache_record_checksum1(record)) { + fprintf(stderr, "error: invalid cache record signature\n"); + debug("signature: %x, checksum: %x", record->signature, record->checksum1); + goto return_failure; } record_size = get_cache_record_size(record); debug("record size: %ld", record_size); /* break if current record is truncated */ - if ((void*)record - (void*)rd_buff + record_size > rd_len) + if ((void *)record - (void *)rd_buff + record_size > rd_len) break; + /* check for second-stage data integration */ + if (record->checksum2 != get_cache_record_checksum2(record)) { + fprintf(stderr, "error: cache record data corrupted\n"); + goto return_failure; + } + /* restore scrobbler track info structure from cache */ memset(&sb_tinf, 0, sizeof(sb_tinf)); sb_tinf.timestamp = record->timestamp; sb_tinf.track_number = record->track_number; sb_tinf.duration = record->duration; - ptr = (char*)&record[1]; + ptr = (char *)&record[1]; - if (record->artist_len) { + if (record->len_artist) { sb_tinf.artist = ptr; - ptr += record->artist_len; + ptr += record->len_artist; } - if (record->album_len) { + if (record->len_album) { sb_tinf.album = ptr; - ptr += record->album_len; + ptr += record->len_album; } -// if (record->album_artist_len) { -// sb_tinf.album_artist = ptr; -// ptr += record->album_artist_len; -// } - if (record->track_len) { + if (record->len_track) { sb_tinf.track = ptr; - ptr += record->track_len; + ptr += record->len_track; + } +#if 0 + if (record->len_album_artist) { + sb_tinf.album_artist = ptr; + ptr += record->len_album_artist; + } + if (record->len_mbid) { + sb_tinf.mbid = ptr; + ptr += record->len_mbid; } -// if (record->mbid_len) { -// sb_tinf.mbid = ptr; -// ptr += record->mbid_len; -// } +#endif debug("cache: %s - %s (%s) - %d. %s (%ds)", sb_tinf.artist, sb_tinf.album, sb_tinf.album_artist, @@ -185,18 +243,22 @@ void cmusfm_cache_submit(scrobbler_session_t *sbs) { scrobbler_scrobble(sbs, &sb_tinf); /* point to next record */ - record = (struct cmusfm_cache_record*)((char*)record + record_size); + record = (struct cmusfm_cache_record *)((char *)record + record_size); } - if ((unsigned)((void*)record - (void*)rd_buff) != rd_len) - /* seek to the beginning of current record, because it is + if ((unsigned)((void *)record - (void *)rd_buff) != rd_len) + /* seek to the beginning of the current record, because it is * truncated, so we have to read it one more time */ - fseek(f, (void*)record - (void*)rd_buff - rd_len, SEEK_CUR); + fseek(f, (void *)record - (void *)rd_buff - rd_len, SEEK_CUR); } +return_failure: + fclose(f); - /* remove cache file */ + /* Remove the cache file, regardless of the submission status. Note, that + * keeping invalid file will result in an inability to submit tracks later + * - there is no validity check upon cache creation. */ unlink(cmusfm_cache_file); } diff --git a/src/cache.h b/src/cache.h index 560f29a..53b3476 100644 --- a/src/cache.h +++ b/src/cache.h @@ -1,6 +1,6 @@ /* * cmusfm - cache.h - * Copyright (c) 2014 Arkadiusz Bokowy + * Copyright (c) 2014-2015 Arkadiusz Bokowy * * This file is a part of a cmusfm. * @@ -25,14 +25,27 @@ #include "libscrobbler2.h" -#define CMUSFM_CACHE_SIGNATURE 0x6643 +/* "Cr" string (big-endian) at the beginning of the record */ +#define CMUSFM_CACHE_SIGNATURE 0x4372 /* cache record header structure */ struct __attribute__((__packed__)) cmusfm_cache_record { - uint32_t signature; - uint32_t timestamp, track_number, duration; - uint16_t artist_len, album_len, album_artist_len; - uint16_t track_len, mbid_len; + + /* record header */ + uint16_t signature; + uint8_t checksum1; + uint8_t checksum2; + + uint32_t timestamp; + uint16_t track_number; + uint16_t duration; + + uint16_t len_artist; + uint16_t len_album; + uint16_t len_track; + uint16_t len_album_artist; + uint16_t len_mbid; + /* NULL-terminated strings char artist[]; char album[]; @@ -40,11 +53,12 @@ struct __attribute__((__packed__)) cmusfm_cache_record { char track[]; char mbid[]; */ + }; -char *get_cmusfm_cache_file(void); void cmusfm_cache_update(const scrobbler_trackinfo_t *sb_tinf); void cmusfm_cache_submit(scrobbler_session_t *sbs); +char *get_cmusfm_cache_file(void); #endif diff --git a/src/cmusfm.h b/src/cmusfm.h index 0f52a7e..2e11786 100644 --- a/src/cmusfm.h +++ b/src/cmusfm.h @@ -75,6 +75,7 @@ struct format_match { char *get_cmus_home_dir(void); char *get_cmus_home_file(const char *file); +int make_data_hash(const unsigned char *data, int len); #if ENABLE_LIBNOTIFY char *get_album_cover_file(const char *location, const char *format); #endif diff --git a/src/server.c b/src/server.c index 5bc221e..01d18ad 100644 --- a/src/server.c +++ b/src/server.c @@ -77,14 +77,6 @@ static void set_trackinfo(scrobbler_trackinfo_t *sbt, struct sock_data_tag *dt) sbt->track = get_sock_data_track(dt); } -/* Simple and fast "hashing" function. */ -static int make_data_hash(const unsigned char *data, int len) { - int x, hash; - for (x = hash = 0; x < len; x++) - hash += data[x] * (x + 1); - return hash; -} - /* Process real server task - Last.fm submission. */ static void cmusfm_server_process_data(int fd, scrobbler_session_t *sbs) { diff --git a/src/utils.c b/src/utils.c index 5f545c8..3fb376a 100644 --- a/src/utils.c +++ b/src/utils.c @@ -75,6 +75,14 @@ char *get_cmus_home_file(const char *file) { return fullpath; } +/* Simple and fast "hashing" function. */ +int make_data_hash(const unsigned char *data, int len) { + int x, hash; + for (x = hash = 0; x < len; x++) + hash += data[x] * (x + 1); + return hash; +} + #if ENABLE_LIBNOTIFY /* Return an album cover file based on the current location. Location should * be either a local file name or an URL. When cover file can not be found,