From 080b7574cfb1d0e455ca381825215eefc7765650 Mon Sep 17 00:00:00 2001 From: Gregory Oschwald Date: Thu, 29 Oct 2015 14:54:29 -0700 Subject: [PATCH 1/2] Do not follow invalid pointers in the search tree --- libGeoIP/GeoIP.c | 25 ++++++++++++++++++------- test/Makefile.am | 10 ++++++---- test/test-geoip-invalid-file.c | 30 ++++++++++++++++++++++++++++++ 3 files changed, 54 insertions(+), 11 deletions(-) create mode 100644 test/test-geoip-invalid-file.c diff --git a/libGeoIP/GeoIP.c b/libGeoIP/GeoIP.c index 279161a..cc659ba 100644 --- a/libGeoIP/GeoIP.c +++ b/libGeoIP/GeoIP.c @@ -1241,15 +1241,20 @@ unsigned int _GeoIP_seek_record_v6_gl(GeoIP *gi, geoipv6_t ipnum, __GEOIP_PREPARE_TEREDO(&ipnum); } for (depth = 127; depth >= 0; depth--) { + unsigned int byte_offset = gi->record_length * 2 * offset; + if (byte_offset > gi->size - gi->record_length * 2) { + // The pointer is invalid + break; + } if (gi->cache == NULL && gi->index_cache == NULL) { /* read from disk */ silence = pread(fno, stack_buffer, gi->record_length * 2, - (long)gi->record_length * 2 * offset ); + (long)byte_offset ); } else if (gi->index_cache == NULL) { /* simply point to record in memory */ - buf = gi->cache + (long)gi->record_length * 2 * offset; + buf = gi->cache + (long)byte_offset; } else { - buf = gi->index_cache + (long)gi->record_length * 2 * offset; + buf = gi->index_cache + (long)byte_offset; } if (GEOIP_CHKBIT_V6(depth, ipnum.s6_addr )) { @@ -1332,15 +1337,20 @@ unsigned int _GeoIP_seek_record_gl(GeoIP *gi, unsigned long ipnum, int fno = fileno(gi->GeoIPDatabase); _check_mtime(gi); for (depth = 31; depth >= 0; depth--) { + unsigned int byte_offset = gi->record_length * 2 * offset; + if (byte_offset > gi->size - gi->record_length * 2) { + // The pointer is invalid + break; + } if (gi->cache == NULL && gi->index_cache == NULL) { /* read from disk */ silence = pread(fno, stack_buffer, gi->record_length * 2, - gi->record_length * 2 * offset); + byte_offset); } else if (gi->index_cache == NULL) { /* simply point to record in memory */ - buf = gi->cache + (long)gi->record_length * 2 * offset; + buf = gi->cache + (long)byte_offset; } else { - buf = gi->index_cache + (long)gi->record_length * 2 * offset; + buf = gi->index_cache + (long)byte_offset; } if (ipnum & (1 << depth)) { @@ -1508,9 +1518,10 @@ GeoIP * GeoIP_open(const char * filename, int flags) free(gi); return NULL; } + + gi->size = buf.st_size; if (flags & (GEOIP_MEMORY_CACHE | GEOIP_MMAP_CACHE) ) { gi->mtime = buf.st_mtime; - gi->size = buf.st_size; /* MMAP added my Peter Shipley */ if (flags & GEOIP_MMAP_CACHE) { diff --git a/test/Makefile.am b/test/Makefile.am index 73743b4..8c38178 100644 --- a/test/Makefile.am +++ b/test/Makefile.am @@ -1,4 +1,4 @@ -check_PROGRAMS = test-geoip +check_PROGRAMS = test-geoip test-geoip-invalid-file noinst_PROGRAMS = benchmark test-geoip-region test-geoip-city test-geoip-org test-geoip-domain test-geoip-asnum test-geoip-isp test-geoip-netspeed @@ -9,13 +9,15 @@ EXTRA_PROGRAMS = benchmark \ test-geoip-domain \ test-geoip-asnum \ test-geoip-isp \ - test-geoip-netspeed + test-geoip-netspeed LDADD = $(top_builddir)/libGeoIP/libGeoIP.la AM_CPPFLAGS = -I$(top_srcdir)/libGeoIP -DSRCDIR=\"$(top_srcdir)\" -Wall test_geoip_SOURCES = test-geoip.c +test_geoip_invalid_file_SOURCES = test-geoip-invalid-file.c + test_geoip_region_SOURCES = test-geoip-region.c test_geoip_org_SOURCES = test-geoip-org.c @@ -32,5 +34,5 @@ test_geoip_city_SOURCES = test-geoip-city.c benchmark_SOURCES = benchmark.c -EXTRA_DIST = Makefile.vc city_test.txt country_test.txt country_test2.txt country_test_name.txt region_test.txt -TESTS = test-geoip +EXTRA_DIST = Makefile.vc city_test.txt country_test.txt country_test2.txt country_test_name.txt region_test.txt +TESTS = test-geoip test-geoip-invalid-file diff --git a/test/test-geoip-invalid-file.c b/test/test-geoip-invalid-file.c new file mode 100644 index 0000000..4c8c116 --- /dev/null +++ b/test/test-geoip-invalid-file.c @@ -0,0 +1,30 @@ +#include "GeoIP.h" + +int main() +{ + GeoIP *gi = GeoIP_open(SRCDIR "/README.md", GEOIP_MEMORY_CACHE); + + // We don't detect invalid files at load, unfortunately. + if (gi == NULL) { + fprintf(stderr, "Error opening database\n"); + return 1; + } + + const char *country = GeoIP_country_code_by_addr(gi, "24.24.24.24"); + if (country != NULL) { + fprintf( + stderr, + "Received a non-NULL value on an invalid database from GeoIP_country_code_by_addr\n"); + return 1; + } + + country = GeoIP_country_code_by_addr_v6(gi, "24.24.24.24"); + if (country != NULL) { + fprintf( + stderr, + "Received a non-NULL value on an invalid database from GeoIP_country_code_by_addr_v6\n"); + return 1; + } + + return 0; +} From fda002b0bbcdf4663d3e05249f8b5a0930abcaf7 Mon Sep 17 00:00:00 2001 From: Gregory Oschwald Date: Fri, 30 Oct 2015 07:00:48 -0700 Subject: [PATCH 2/2] Check pread return values. Misc cleanup --- .travis.yml | 2 + ChangeLog | 7 +++ apps/geoiplookup6.c | 1 - libGeoIP/GeoIP.c | 83 ++++++++++++++++++++++------------ libGeoIP/GeoIPCity.c | 2 +- test/test-geoip-invalid-file.c | 2 +- 6 files changed, 64 insertions(+), 33 deletions(-) diff --git a/.travis.yml b/.travis.yml index 8d5670c..4572e46 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,3 +1,5 @@ +sudo: false + language: c compiler: diff --git a/ChangeLog b/ChangeLog index ab4726f..4d4732d 100644 --- a/ChangeLog +++ b/ChangeLog @@ -3,6 +3,13 @@ * Fixed a MSVC parser stack overflow when parsing `regionName.c` and `timeZone.c`. Fix by elliotlo. GitHub #64. * Updated region codes and timezones. +* When using `GEOIP_MEMORY_CACHE` with an invalid database file, the search + tree traversal could attempt to read memory outside of the memory allocated + for the memory cache, resulting in a segmentation fault. A check was added + to ensure that the traversal code does not try to read beyond the end of the + file, whether in memory, memory mapped, or on disk. +* Previously the return values from file reads were ignored. We now check + these values to ensure that there were no errors. 1.6.6 2015-07-28 diff --git a/apps/geoiplookup6.c b/apps/geoiplookup6.c index 801803f..f460503 100644 --- a/apps/geoiplookup6.c +++ b/apps/geoiplookup6.c @@ -150,7 +150,6 @@ geoiplookup(GeoIP * gi, char *hostname, int i) printf("%s: IP Address not found\n", GeoIPDBDescription[i]); }else { printf("%s: %s\n", GeoIPDBDescription[i], asnum_name); - // _say_range_by_ip(gi, ipnum); } }else if (GEOIP_CITY_EDITION_REV0_V6 == i) { gir = GeoIP_record_by_ipnum_v6(gi, ipnum); diff --git a/libGeoIP/GeoIP.c b/libGeoIP/GeoIP.c index cc659ba..2a0f970 100644 --- a/libGeoIP/GeoIP.c +++ b/libGeoIP/GeoIP.c @@ -872,7 +872,7 @@ char * _GeoIP_iso_8859_1__utf8(const char * iso) if (p) { while ( ( c = *iso++ ) ) { if (c < 0) { - k = 0xc2; + k = (char) 0xc2; if (c >= -64) { k++; } @@ -959,7 +959,7 @@ static void _setup_segments(GeoIP * gi) int i, j, segment_record_length; unsigned char delim[3]; unsigned char buf[LARGE_SEGMENT_RECORD_LENGTH]; - ssize_t silence _UNUSED; + int fno = fileno(gi->GeoIPDatabase); gi->databaseSegments = NULL; @@ -971,9 +971,13 @@ static void _setup_segments(GeoIP * gi) return; } for (i = 0; i < STRUCTURE_INFO_MAX_SIZE; i++) { - silence = read(fno, delim, 3); + if (read(fno, delim, 3) != 3) { + return; + } if (delim[0] == 255 && delim[1] == 255 && delim[2] == 255) { - silence = read(fno, &gi->databaseType, 1 ); + if (read(fno, &gi->databaseType, 1) != 1) { + return; + } if (gi->databaseType >= 106) { /* backwards compatibility with databases from April 2003 and earlier */ gi->databaseType -= 105; @@ -1027,7 +1031,12 @@ static void _setup_segments(GeoIP * gi) gi->databaseSegments[0] = 0; segment_record_length = SEGMENT_RECORD_LENGTH; - silence = read(fno, buf, segment_record_length ); + if (read(fno, buf, + segment_record_length) != segment_record_length) { + free(gi->databaseSegments); + gi->databaseSegments = NULL; + return; + } for (j = 0; j < segment_record_length; j++) { gi->databaseSegments[0] += (buf[j] << (j * 8)); } @@ -1206,7 +1215,7 @@ void _check_mtime(GeoIP *gi) gi->index_cache, sizeof(unsigned char) * idx_size ); if (gi->index_cache != NULL) { if (pread(fileno(gi->GeoIPDatabase), gi->index_cache, - idx_size, 0 ) != idx_size) { + idx_size, 0) != idx_size) { DEBUG_MSGF( gi->flags, "Error reading file %s where reloading\n", @@ -1234,22 +1243,26 @@ unsigned int _GeoIP_seek_record_v6_gl(GeoIP *gi, geoipv6_t ipnum, const unsigned char * p; int j; - ssize_t silence _UNUSED; int fno = fileno(gi->GeoIPDatabase); + + unsigned int record_pair_length = gi->record_length * 2; + _check_mtime(gi); if (GeoIP_teredo(gi) ) { __GEOIP_PREPARE_TEREDO(&ipnum); } for (depth = 127; depth >= 0; depth--) { - unsigned int byte_offset = gi->record_length * 2 * offset; - if (byte_offset > gi->size - gi->record_length * 2) { - // The pointer is invalid + unsigned int byte_offset = record_pair_length * offset; + if (byte_offset > gi->size - record_pair_length) { + /* The pointer is invalid */ break; } if (gi->cache == NULL && gi->index_cache == NULL) { /* read from disk */ - silence = pread(fno, stack_buffer, gi->record_length * 2, - (long)byte_offset ); + if (pread(fno, stack_buffer, record_pair_length, + (long)byte_offset) != record_pair_length) { + break; + } } else if (gi->index_cache == NULL) { /* simply point to record in memory */ buf = gi->cache + (long)byte_offset; @@ -1330,22 +1343,26 @@ unsigned int _GeoIP_seek_record_gl(GeoIP *gi, unsigned long ipnum, unsigned char stack_buffer[2 * MAX_RECORD_LENGTH]; const unsigned char *buf = (gi->cache == NULL) ? stack_buffer : NULL; unsigned int offset = 0; - ssize_t silence _UNUSED; const unsigned char * p; int j; int fno = fileno(gi->GeoIPDatabase); + + unsigned int record_pair_length = gi->record_length * 2; + _check_mtime(gi); for (depth = 31; depth >= 0; depth--) { - unsigned int byte_offset = gi->record_length * 2 * offset; - if (byte_offset > gi->size - gi->record_length * 2) { - // The pointer is invalid + unsigned int byte_offset = record_pair_length * offset; + if (byte_offset > gi->size - record_pair_length) { + /* The pointer is invalid */ break; } if (gi->cache == NULL && gi->index_cache == NULL) { /* read from disk */ - silence = pread(fno, stack_buffer, gi->record_length * 2, - byte_offset); + if (pread(fno, stack_buffer, record_pair_length, + byte_offset) != record_pair_length) { + break; + } } else if (gi->index_cache == NULL) { /* simply point to record in memory */ buf = gi->cache + (long)byte_offset; @@ -2010,7 +2027,7 @@ char *GeoIP_database_info(GeoIP * gi) unsigned char buf[3]; char *retval; int hasStructureInfo = 0; - ssize_t silence _UNUSED; + int fno; if (gi == NULL) { @@ -2026,7 +2043,9 @@ char *GeoIP_database_info(GeoIP * gi) /* first get past the database structure information */ for (i = 0; i < STRUCTURE_INFO_MAX_SIZE; i++) { - silence = read(fno, buf, 3 ); + if (read(fno, buf, 3 ) != 3) { + return NULL; + } if (buf[0] == 255 && buf[1] == 255 && buf[2] == 255) { hasStructureInfo = 1; break; @@ -2047,13 +2066,17 @@ char *GeoIP_database_info(GeoIP * gi) } for (i = 0; i < DATABASE_INFO_MAX_SIZE; i++) { - silence = read(fno, buf, 3 ); + if (read(fno, buf, 3 ) != 3) { + return NULL; + } if (buf[0] == 0 && buf[1] == 0 && buf[2] == 0) { retval = malloc(sizeof(char) * (i + 1)); if (retval == NULL) { return NULL; } - silence = read(fno, retval, i); + if (read(fno, retval, i) != i) { + return NULL; + } retval[i] = '\0'; return retval; } @@ -2321,7 +2344,6 @@ char *_get_name_gl(GeoIP * gi, unsigned long ipnum, GeoIPLookup * gl) char * org_buf, * buf_pointer; int record_pointer; size_t len; - ssize_t silence _UNUSED; if (gi->databaseType != GEOIP_ORG_EDITION && gi->databaseType != GEOIP_ISP_EDITION && @@ -2352,9 +2374,10 @@ char *_get_name_gl(GeoIP * gi, unsigned long ipnum, GeoIPLookup * gl) (2 * gi->record_length - 1) * gi->databaseSegments[0]; if (gi->cache == NULL) { - silence = pread(fileno( - gi->GeoIPDatabase), buf, MAX_ORG_RECORD_LENGTH, - record_pointer); + if (pread(fileno(gi->GeoIPDatabase), buf, MAX_ORG_RECORD_LENGTH, + record_pointer) == -1) { + return NULL; + } if (gi->charset == GEOIP_CHARSET_UTF8) { org_buf = _GeoIP_iso_8859_1__utf8( (const char * )buf ); } else { @@ -2383,7 +2406,6 @@ char *_get_name_v6_gl(GeoIP * gi, geoipv6_t ipnum, GeoIPLookup * gl) char * org_buf, * buf_pointer; int record_pointer; size_t len; - ssize_t silence _UNUSED; if ( gi->databaseType != GEOIP_ORG_EDITION_V6 && @@ -2411,9 +2433,10 @@ char *_get_name_v6_gl(GeoIP * gi, geoipv6_t ipnum, GeoIPLookup * gl) (2 * gi->record_length - 1) * gi->databaseSegments[0]; if (gi->cache == NULL) { - silence = pread(fileno( - gi->GeoIPDatabase), buf, MAX_ORG_RECORD_LENGTH, - record_pointer); + if (pread(fileno(gi->GeoIPDatabase), buf, MAX_ORG_RECORD_LENGTH, + record_pointer) == -1) { + return NULL; + } buf[MAX_ORG_RECORD_LENGTH] = 0; if (gi->charset == GEOIP_CHARSET_UTF8) { org_buf = _GeoIP_iso_8859_1__utf8( (const char * )buf ); diff --git a/libGeoIP/GeoIPCity.c b/libGeoIP/GeoIPCity.c index a4cfb96..13c273b 100644 --- a/libGeoIP/GeoIPCity.c +++ b/libGeoIP/GeoIPCity.c @@ -89,7 +89,7 @@ _extract_record(GeoIP * gi, unsigned int seek_record, int *next_record_ptr) bytes_read = pread(fileno( gi->GeoIPDatabase), record_buf, FULL_RECORD_LENGTH, record_pointer); - if (bytes_read == 0) { + if (bytes_read <= 0) { /* eof or other error */ free(begin_record_buf); free(record); diff --git a/test/test-geoip-invalid-file.c b/test/test-geoip-invalid-file.c index 4c8c116..089a770 100644 --- a/test/test-geoip-invalid-file.c +++ b/test/test-geoip-invalid-file.c @@ -4,7 +4,7 @@ int main() { GeoIP *gi = GeoIP_open(SRCDIR "/README.md", GEOIP_MEMORY_CACHE); - // We don't detect invalid files at load, unfortunately. + /* We don't detect invalid files at load, unfortunately. */ if (gi == NULL) { fprintf(stderr, "Error opening database\n"); return 1;