From c779de7e99c1e113c657576d9b403e8bb0cb70e7 Mon Sep 17 00:00:00 2001 From: Kenichi Ishibashi Date: Mon, 4 Jun 2012 14:29:42 +0900 Subject: [PATCH] Adding checks (part 2/2) and minor changes - Added further checks - Removed commenting-out code - Fixed style ("type &variable" -> "type& variable") - In RecunstructTransformed(), the 6th argument of RecunstructGlyf() call should be |loca_table->dst_length|, not |loca_table->dst_offset|. BUG=none TEST=compiled Review URL: https://codereview.appspot.com/6160043 --- cpp/woff2.cc | 218 +++++++++++++++++++++++++++++++-------------------- 1 file changed, 135 insertions(+), 83 deletions(-) diff --git a/cpp/woff2.cc b/cpp/woff2.cc index 1d51e55..cc64185 100644 --- a/cpp/woff2.cc +++ b/cpp/woff2.cc @@ -9,12 +9,14 @@ // that gets all sorted out, we can get rid of these ifdefs. #define USE_LZMA +#include + #ifdef USE_LZMA #include "third_party/lzma_sdk/LzmaLib.h" #endif #include -#include + #include "opentype-sanitiser.h" #include "ots-memory-stream.h" @@ -68,6 +70,7 @@ const uint32_t kCompressionTypeMask = 0xf; const uint32_t kCompressionTypeNone = 0; const uint32_t kCompressionTypeGzip = 1; const uint32_t kCompressionTypeLzma = 2; +// agl: This doesn't seem to match the value in the spec, which is 16. const uint32_t kShortFlagsContinue = 3; struct Point { @@ -88,12 +91,22 @@ struct Table { uint32_t dst_length; }; +// TODO: copied from ots.cc, probably shouldn't be duplicated. +// Round a value up to the nearest multiple of 4. Don't round the value in the +// case that rounding up overflows. +template T Round4(T value) { + if (std::numeric_limits::max() - value < 3) { + return value; + } + return (value + 3) & ~3; +} + // Based on section 6.1.1 of MicroType Express draft spec bool Read255UShort(ots::Buffer* buf, unsigned int* value) { - const int kWordCode = 253; - const int kOneMoreByteCode2 = 254; - const int kOneMoreByteCode1 = 255; - const int kLowestUCode = 253; + static const int kWordCode = 253; + static const int kOneMoreByteCode2 = 254; + static const int kOneMoreByteCode1 = 255; + static const int kLowestUCode = 253; uint8_t code = 0; if (!buf->ReadU8(&code)) { return OTS_FAILURE(); @@ -132,6 +145,10 @@ bool ReadBase128(ots::Buffer* buf, uint32_t* value) { if (!buf->ReadU8(&code)) { return OTS_FAILURE(); } + // If any of the top seven bits are set then we're about to overflow. + if (result & 0xe0000000) { + return OTS_FAILURE(); + } result = (result << 7) | (code & 0x7f); if ((code & 0x80) == 0) { *value = result; @@ -144,19 +161,23 @@ bool ReadBase128(ots::Buffer* buf, uint32_t* value) { size_t StoreU32(uint8_t* dst, size_t offset, uint32_t x) { dst[offset] = x >> 24; - dst[offset + 1] = (x >> 16) & 0xff; - dst[offset + 2] = (x >> 8) & 0xff; - dst[offset + 3] = x & 0xff; + dst[offset + 1] = x >> 16; + dst[offset + 2] = x >> 8; + dst[offset + 3] = x; return offset + 4; } size_t Store16(uint8_t* dst, size_t offset, int x) { dst[offset] = x >> 8; - dst[offset + 1] = x & 0xff; + dst[offset + 1] = x; return offset + 2; } int WithSign(int flag, int baseval) { + // agl: it isn't generally possible to negate an arbitary integer. (For + // example, -2137483648 cannot be negated.) However, I believe that the range + // is 0 <= baseval < 65536 in which case it's safe. If you agree then there + // should be a comment documenting the precondition of this function. return (flag & 1) ? baseval : -baseval; } @@ -167,7 +188,7 @@ bool TripletDecode(const uint8_t* flags_in, const uint8_t* in, size_t in_size, int y = 0; if (n_points > in_size) { - return false; + return OTS_FAILURE(); } unsigned int triplet_index = 0; @@ -175,7 +196,7 @@ bool TripletDecode(const uint8_t* flags_in, const uint8_t* in, size_t in_size, uint8_t flag = flags_in[i]; bool on_curve = !(flag >> 7); flag &= 0x7f; - int n_data_bytes; + unsigned int n_data_bytes; if (flag < 84) { n_data_bytes = 1; } else if (flag < 120) { @@ -185,13 +206,6 @@ bool TripletDecode(const uint8_t* flags_in, const uint8_t* in, size_t in_size, } else { n_data_bytes = 4; } -#if 0 - fprintf(stderr, "flag = %d:", flag); - for (int j = 0; j < n_data_bytes; ++j) { - fprintf(stderr, " %d", in[triplet_index + j]); - } - fprintf(stderr, "\n"); -#endif if (triplet_index + n_data_bytes > in_size || triplet_index + n_data_bytes < triplet_index) { return OTS_FAILURE(); @@ -223,15 +237,14 @@ bool TripletDecode(const uint8_t* flags_in, const uint8_t* in, size_t in_size, (in[triplet_index + 2] << 8) + in[triplet_index + 3]); } triplet_index += n_data_bytes; + // agl: these additions can overflow, although I'm not sure that we care. x += dx; y += dy; result->push_back(Point()); - Point &back = result->back(); + Point& back = result->back(); back.x = x; back.y = y; back.on_curve = on_curve; - // fprintf(stderr, "point %d: %d %d %s (delta %d %d)\n", - // i, x, y, on_curve ? "on" : "off", dx, dy); } *in_bytes_consumed = triplet_index; return true; @@ -239,20 +252,22 @@ bool TripletDecode(const uint8_t* flags_in, const uint8_t* in, size_t in_size, // This function stores just the point data. On entry, dst points to the // beginning of a simple glyph. Returns true on success. -bool StorePoints(const std::vector &points, +bool StorePoints(const std::vector& points, unsigned int n_contours, unsigned int instruction_length, uint8_t* dst, size_t dst_size, size_t* glyph_size) { + // I believe that n_contours < 65536, in which case this is safe. However, a + // comment and/or an assert would be good. unsigned int flag_offset = kEndPtsOfContoursOffset + 2 * n_contours + 2 + instruction_length; int last_flag = -1; int repeat_count = 0; int last_x = 0; int last_y = 0; - int x_bytes = 0; - int y_bytes = 0; + unsigned int x_bytes = 0; + unsigned int y_bytes = 0; for (unsigned int i = 0; i < points.size(); ++i) { - const Point &point = points[i]; + const Point& point = points[i]; int flag = point.on_curve ? kGlyfOnCurve : 0; int dx = point.x - last_x; int dy = point.y - last_y; @@ -272,16 +287,20 @@ bool StorePoints(const std::vector &points, } else { y_bytes += 2; } - // fprintf(stderr, "nominal flag = %d\n", flag); + if (flag == last_flag && repeat_count != 255) { dst[flag_offset - 1] |= kGlyfRepeat; repeat_count++; } else { if (repeat_count != 0) { - if (flag_offset >= dst_size) return OTS_FAILURE(); + if (flag_offset >= dst_size) { + return OTS_FAILURE(); + } dst[flag_offset++] = repeat_count; } - if (flag_offset >= dst_size) return OTS_FAILURE(); + if (flag_offset >= dst_size) { + return OTS_FAILURE(); + } dst[flag_offset++] = flag; repeat_count = 0; } @@ -291,11 +310,15 @@ bool StorePoints(const std::vector &points, } if (repeat_count != 0) { - if (flag_offset >= dst_size) return OTS_FAILURE(); + if (flag_offset >= dst_size) { + return OTS_FAILURE(); + } dst[flag_offset++] = repeat_count; } - if (flag_offset + x_bytes + y_bytes > dst_size || - flag_offset + x_bytes + y_bytes < flag_offset) { + // agl: overflow checks can't have multiple additions in them. + if (flag_offset + x_bytes < flag_offset || + flag_offset + x_bytes + y_bytes < flag_offset + x_bytes || + flag_offset + x_bytes + y_bytes > dst_size) { return OTS_FAILURE(); } @@ -310,6 +333,8 @@ bool StorePoints(const std::vector &points, } else if (dx > -256 && dx < 256) { dst[x_offset++] = std::abs(dx); } else { + // agl: this can go wrong if dx doesn't fit in an int16_t, but it's not + // clear if that matters. x_offset = Store16(dst, x_offset, dx); } last_x += dx; @@ -329,7 +354,7 @@ bool StorePoints(const std::vector &points, // Compute the bounding box of the coordinates, and store into a glyf buffer. // A precondition is that there are at least 10 bytes available. -void ComputeBbox(const std::vector &points, uint8_t* dst) { +void ComputeBbox(const std::vector& points, uint8_t* dst) { int x_min = 0; int y_min = 0; int x_max = 0; @@ -353,20 +378,30 @@ void ComputeBbox(const std::vector &points, uint8_t* dst) { // Process entire bbox stream. This is done as a separate pass to allow for // composite bbox computations (an optional more aggressive transform). bool ProcessBboxStream(ots::Buffer* bbox_stream, unsigned int n_glyphs, - const std::vector &loca_values, uint8_t* glyf_buf) { + const std::vector& loca_values, uint8_t* glyf_buf, + size_t glyf_buf_length) { const uint8_t* buf = bbox_stream->buffer(); + // agl: I belive that n_glyphs is < 65536, making the next line safe. But a + // comment and/or assert is needed to document that. unsigned int bitmap_length = ((n_glyphs + 31) >> 5) << 2; - if (bbox_stream->length() < bitmap_length) { + if (!bbox_stream->Skip(bitmap_length)) { return OTS_FAILURE(); } - bbox_stream->Skip(bitmap_length); for (unsigned int i = 0; i < n_glyphs; ++i) { if (buf[i >> 3] & (0x80 >> (i & 7))) { uint32_t loca_offset = loca_values[i]; + // agl: again, I believe that this is safe, but the fact that loca_values + // has n_glyphs+1 elements should be documented. if (loca_values[i + 1] - loca_offset < kEndPtsOfContoursOffset) { return OTS_FAILURE(); } - bbox_stream->Read(glyf_buf + loca_offset + 2, 8); + if (glyf_buf_length < 2 + 10 || + loca_offset > glyf_buf_length - 2 - 10) { + return OTS_FAILURE(); + } + if (!bbox_stream->Read(glyf_buf + loca_offset + 2, 8)) { + return OTS_FAILURE(); + } } } return true; @@ -399,7 +434,6 @@ bool ProcessComposite(ots::Buffer* composite_stream, uint8_t* dst, if (!composite_stream->Skip(arg_size)) { return OTS_FAILURE(); } - //fprintf(stderr, "flags = %04x, arg_size = %d\n", flags, arg_size); } size_t composite_glyph_size = composite_stream->offset() - start_offset; if (composite_glyph_size + kCompositeGlyphBegin > dst_size) { @@ -415,16 +449,19 @@ bool ProcessComposite(ots::Buffer* composite_stream, uint8_t* dst, } // Build TrueType loca table -bool StoreLoca(const std::vector &loca_values, int index_format, +bool StoreLoca(const std::vector& loca_values, int index_format, uint8_t* dst, size_t dst_size) { - size_t loca_size = loca_values.size(); - size_t offset_size = index_format ? 4 : 2; + const uint64_t loca_size = loca_values.size(); + const uint64_t offset_size = index_format ? 4 : 2; + if ((loca_size << 2) >> 2 != loca_size) { + return OTS_FAILURE(); + } if (offset_size * loca_size > dst_size) { return OTS_FAILURE(); } size_t offset = 0; for (size_t i = 0; i < loca_values.size(); ++i) { - int value = loca_values[i]; + uint32_t value = loca_values[i]; if (index_format) { offset = StoreU32(dst, offset, value); } else { @@ -438,31 +475,34 @@ bool StoreLoca(const std::vector &loca_values, int index_format, bool ReconstructGlyf(const uint8_t* data, size_t data_size, uint8_t* dst, size_t dst_size, uint8_t* loca_buf, size_t loca_size) { + static const int kNumSubStreams = 7; ots::Buffer file(data, data_size); uint32_t version; - const int kNumSubStreams = 7; std::vector > substreams(kNumSubStreams); if (!file.ReadU32(&version)) { return OTS_FAILURE(); } - uint16_t num_glyphs = 0; - uint16_t index_format = 0; + uint16_t num_glyphs; + uint16_t index_format; if (!file.ReadU16(&num_glyphs) || !file.ReadU16(&index_format)) { return OTS_FAILURE(); } - // fprintf(stderr, "num_glyphs = %d\n", num_glyphs); unsigned int offset = (2 + kNumSubStreams) * 4; + // agl: data_size must be >= offset, but this condition isn't established + // initially. + if (offset > data_size) { + return OTS_FAILURE(); + } for (int i = 0; i < kNumSubStreams; ++i) { - uint32_t substream_size = 0; + uint32_t substream_size; if (!file.ReadU32(&substream_size)) { return OTS_FAILURE(); } if (substream_size > data_size - offset) { return OTS_FAILURE(); } - // fprintf(stderr, "substream size = %d\n", substream_size); substreams[i] = std::make_pair(data + offset, substream_size); offset += substream_size; } @@ -484,7 +524,6 @@ bool ReconstructGlyf(const uint8_t* data, size_t data_size, if (!n_contour_stream.ReadU16(&n_contours)) { return OTS_FAILURE(); } - // fprintf(stderr, "n_contours[%d] = %d\n", i, n_contours); uint8_t* glyf_dst = dst + loca_offset; size_t glyf_dst_size = dst_size - loca_offset; if (n_contours == 0xffff) { @@ -520,6 +559,9 @@ bool ReconstructGlyf(const uint8_t* data, size_t data_size, return OTS_FAILURE(); } n_points_vec.push_back(n_points_contour); + if (total_n_points + n_points_contour < total_n_points) { + return OTS_FAILURE(); + } total_n_points += n_points_contour; } unsigned int flag_size = total_n_points; @@ -535,7 +577,9 @@ bool ReconstructGlyf(const uint8_t* data, size_t data_size, &points, &triplet_bytes_consumed)) { return OTS_FAILURE(); } - if (glyf_dst_size < kEndPtsOfContoursOffset + 2 * n_contours) { + const uint32_t header_and_endpts_contours_size = + kEndPtsOfContoursOffset + 2 * n_contours; + if (glyf_dst_size < header_and_endpts_contours_size) { return OTS_FAILURE(); } Store16(glyf_dst, 0, n_contours); @@ -544,17 +588,24 @@ bool ReconstructGlyf(const uint8_t* data, size_t data_size, int end_point = -1; for (unsigned int contour_ix = 0; contour_ix < n_contours; ++contour_ix) { end_point += n_points_vec[contour_ix]; + // agl: this fails if end_point > 65535, does that matter? offset = Store16(glyf_dst, offset, end_point); } - flag_stream.Skip(flag_size); - glyph_stream.Skip(triplet_bytes_consumed); + if (!flag_stream.Skip(flag_size)) { + return OTS_FAILURE(); + } + if (!glyph_stream.Skip(triplet_bytes_consumed)) { + return OTS_FAILURE(); + } unsigned int instruction_size; if (!Read255UShort(&glyph_stream, &instruction_size)) { return OTS_FAILURE(); } - // fprintf(stderr, "%d: instruction size = %d\n", i, instruction_size); - uint8_t* instruction_dst = glyf_dst + kEndPtsOfContoursOffset + - 2 * n_contours; + if (glyf_dst_size - header_and_endpts_contours_size < + instruction_size + 2) { + return OTS_FAILURE(); + } + uint8_t* instruction_dst = glyf_dst + header_and_endpts_contours_size; Store16(instruction_dst, 0, instruction_size); if (!instruction_stream.Read(instruction_dst + 2, instruction_size)) { return OTS_FAILURE(); @@ -570,8 +621,7 @@ bool ReconstructGlyf(const uint8_t* data, size_t data_size, if (glyph_size + 3 < glyph_size) { return OTS_FAILURE(); } - // Round up to 4-byte alignment - glyph_size = (glyph_size + 3) & -4; + glyph_size = Round4(glyph_size); if (glyph_size > dst_size - loca_offset) { // This shouldn't happen, but this test defensively maintains the // invariant that loca_offset <= dst_size. @@ -580,7 +630,8 @@ bool ReconstructGlyf(const uint8_t* data, size_t data_size, loca_offset += glyph_size; } loca_values[num_glyphs] = loca_offset; - if (!ProcessBboxStream(&bbox_stream, num_glyphs, loca_values, dst)) { + if (!ProcessBboxStream(&bbox_stream, num_glyphs, loca_values, + dst, dst_size)) { return OTS_FAILURE(); } return StoreLoca(loca_values, index_format, loca_buf, loca_size); @@ -589,7 +640,7 @@ bool ReconstructGlyf(const uint8_t* data, size_t data_size, // This is linear search, but could be changed to binary because we // do have a guarantee that the tables are sorted by tag. But the total // cpu time is expected to be very small in any case. -const Table* FindTable(const std::vector &tables, uint32_t tag) { +const Table* FindTable(const std::vector
& tables, uint32_t tag) { size_t n_tables = tables.size(); for (size_t i = 0; i < n_tables; ++i) { if (tables[i].tag == tag) { @@ -599,18 +650,26 @@ const Table* FindTable(const std::vector
&tables, uint32_t tag) { return NULL; } -bool ReconstructTransformed(const std::vector
&tables, uint32_t tag, +bool ReconstructTransformed(const std::vector
& tables, uint32_t tag, const uint8_t* transformed_buf, size_t transformed_size, - uint8_t* dst) { + uint8_t* dst, size_t dst_length) { if (tag == TAG('g', 'l', 'y', 'f')) { const Table* glyf_table = FindTable(tables, tag); const Table* loca_table = FindTable(tables, TAG('l', 'o', 'c', 'a')); if (glyf_table == NULL || loca_table == NULL) { return OTS_FAILURE(); } + if (static_cast(glyf_table->dst_offset + glyf_table->dst_length) > + dst_length) { + return OTS_FAILURE(); + } + if (static_cast(loca_table->dst_offset + loca_table->dst_length) > + dst_length) { + return OTS_FAILURE(); + } return ReconstructGlyf(transformed_buf, transformed_size, dst + glyf_table->dst_offset, glyf_table->dst_length, - dst + loca_table->dst_offset, loca_table->dst_offset); + dst + loca_table->dst_offset, loca_table->dst_length); } else if (tag == TAG('l', 'o', 'c', 'a')) { // processing was already done by glyf table, but validate if (!FindTable(tables, TAG('g', 'l', 'y', 'f'))) { @@ -623,28 +682,20 @@ bool ReconstructTransformed(const std::vector
&tables, uint32_t tag, return true; } -// TODO: copied from ots.cc, probably shouldn't be duplicated. -// Round a value up to the nearest multiple of 4. Don't round the value in the -// case that rounding up overflows. -template T Round4(T value) { - if (std::numeric_limits::max() - value < 3) { - return value; - } - return (value + 3) & ~3; -} - uint32_t ComputeChecksum(const uint8_t* buf, size_t size) { uint32_t checksum = 0; for (size_t i = 0; i < size; i += 4) { // We assume the addition is mod 2^32. This is a pretty safe assumption, // but technically it's undefined behavior. + // agl: in reference to the comment above: when using uint32_t, the fact + // that addition is mod 2^32 is well defined. checksum += (buf[i] << 24) | (buf[i + 1] << 16) | (buf[i + 2] << 8) | buf[i + 3]; } return checksum; } -bool FixChecksums(const std::vector
&tables, uint8_t* dst) { +bool FixChecksums(const std::vector
& tables, uint8_t* dst) { const Table* head_table = FindTable(tables, TAG('h', 'e', 'a', 'd')); if (head_table == NULL || head_table->dst_length < kCheckSumAdjustmentOffset + 4) { @@ -675,6 +726,7 @@ bool Woff2Uncompress(uint8_t* dst_buf, size_t dst_size, uLongf uncompressed_length = dst_size; int r = uncompress(reinterpret_cast(dst_buf), &uncompressed_length, src_buf, src_size); + // agl: I think src_size should be dst_size in the next line. if (r != Z_OK || uncompressed_length != src_size) { return OTS_FAILURE(); } @@ -753,11 +805,11 @@ bool ReadShortDirectory(ots::Buffer* file, std::vector
* tables, uint32_t last_compression_type = 0; for (size_t i = 0; i < num_tables; ++i) { Table* table = &(*tables)[i]; - uint8_t flag_byte = 0; + uint8_t flag_byte; if (!file->ReadU8(&flag_byte)) { return OTS_FAILURE(); } - uint32_t tag = 0; + uint32_t tag; if ((flag_byte & 0x1f) == 0x1f) { if (!file->ReadU32(&tag)) { return OTS_FAILURE(); @@ -783,7 +835,7 @@ bool ReadShortDirectory(ots::Buffer* file, std::vector
* tables, if ((flag_byte & 0x20) != 0) { flags |= kWoff2FlagsTransform; } - uint32_t dst_length = 0; + uint32_t dst_length; if (!ReadBase128(file, &dst_length)) { return OTS_FAILURE(); } @@ -814,10 +866,10 @@ namespace ots { size_t ComputeWOFF2FinalSize(const uint8_t* data, size_t length) { ots::Buffer file(data, length); + uint32_t total_length; - file.Skip(16); - uint32_t total_length = 0; - if (!file.ReadU32(&total_length)) { + if (!file.Skip(16) || + !file.ReadU32(&total_length)) { return 0; } return total_length; @@ -828,8 +880,8 @@ bool ConvertWOFF2ToTTF(uint8_t* result, size_t result_length, static const uint32_t kWoff2Signature = 0x774f4632; // "wOF2" ots::Buffer file(data, length); - uint32_t signature = 0; - uint32_t flavor = 0; + uint32_t signature; + uint32_t flavor; if (!file.ReadU32(&signature) || signature != kWoff2Signature || !file.ReadU32(&flavor)) { return OTS_FAILURE(); @@ -837,11 +889,11 @@ bool ConvertWOFF2ToTTF(uint8_t* result, size_t result_length, // TODO(bashi): Should call IsValidVersionTag() here. - uint32_t reported_length = 0; + uint32_t reported_length; if (!file.ReadU32(&reported_length) || length != reported_length) { return OTS_FAILURE(); } - uint16_t num_tables = 0; + uint16_t num_tables; if (!file.ReadU16(&num_tables) || !num_tables) { return OTS_FAILURE(); } @@ -969,7 +1021,7 @@ bool ConvertWOFF2ToTTF(uint8_t* result, size_t result_length, transform_length); } else { if (!ReconstructTransformed(tables, table->tag, - transform_buf, transform_length, result)) { + transform_buf, transform_length, result, result_length)) { return OTS_FAILURE(); } }