Reject profiles with (some) malformed tag data
If we know how to parse a tag, and it fails to parse, then treat the
profile as invalid. Also remove some overly paranoid/redundant error
checking from the static parse helpers.
Change-Id: Ie6c3fff015b512aafd8e6e49620d913d866bf3af
Reviewed-on: https://skia-review.googlesource.com/110240
Commit-Queue: Mike Klein <mtklein@chromium.org>
Reviewed-by: Mike Klein <mtklein@chromium.org>
diff --git a/src/ICCProfile.c b/src/ICCProfile.c
index e467903..cf0cc5b 100644
--- a/src/ICCProfile.c
+++ b/src/ICCProfile.c
@@ -118,35 +118,23 @@
} XYZ_Layout;
static bool read_tag_xyz(const skcms_ICCTag* tag, float* x, float* y, float* z) {
- const XYZ_Layout* xyzTag = NULL;
- if (tag &&
- tag->type == make_signature('X','Y','Z',' ') &&
- tag->size >= SAFE_SIZEOF(XYZ_Layout)) {
- xyzTag = (const XYZ_Layout*)tag->buf;
- }
-
- if (!xyzTag || !x || !y || !z) {
+ if (tag->type != make_signature('X','Y','Z',' ') || tag->size < SAFE_SIZEOF(XYZ_Layout)) {
return false;
}
+ const XYZ_Layout* xyzTag = (const XYZ_Layout*)tag->buf;
+
*x = read_big_fixed(xyzTag->X);
*y = read_big_fixed(xyzTag->Y);
*z = read_big_fixed(xyzTag->Z);
return true;
}
-static bool read_to_XYZD50(const skcms_ICCProfile* profile, skcms_Matrix3x3* toXYZ) {
- if (!profile || !toXYZ) { return false; }
- skcms_ICCTag rXYZ, gXYZ, bXYZ;
- if (!skcms_GetTagBySignature(profile, make_signature('r', 'X', 'Y', 'Z'), &rXYZ) ||
- !skcms_GetTagBySignature(profile, make_signature('g', 'X', 'Y', 'Z'), &gXYZ) ||
- !skcms_GetTagBySignature(profile, make_signature('b', 'X', 'Y', 'Z'), &bXYZ)) {
- return false;
- }
-
- return read_tag_xyz(&rXYZ, &toXYZ->vals[0][0], &toXYZ->vals[1][0], &toXYZ->vals[2][0]) &&
- read_tag_xyz(&gXYZ, &toXYZ->vals[0][1], &toXYZ->vals[1][1], &toXYZ->vals[2][1]) &&
- read_tag_xyz(&bXYZ, &toXYZ->vals[0][2], &toXYZ->vals[1][2], &toXYZ->vals[2][2]);
+static bool read_to_XYZD50(const skcms_ICCTag* rXYZ, const skcms_ICCTag* gXYZ,
+ const skcms_ICCTag* bXYZ, skcms_Matrix3x3* toXYZ) {
+ return read_tag_xyz(rXYZ, &toXYZ->vals[0][0], &toXYZ->vals[1][0], &toXYZ->vals[2][0]) &&
+ read_tag_xyz(gXYZ, &toXYZ->vals[0][1], &toXYZ->vals[1][1], &toXYZ->vals[2][1]) &&
+ read_tag_xyz(bXYZ, &toXYZ->vals[0][2], &toXYZ->vals[1][2], &toXYZ->vals[2][2]);
}
typedef struct {
@@ -299,7 +287,7 @@
static bool get_transfer_function(const skcms_ICCProfile* profile,
skcms_TransferFunction* transferFunction) {
- if (!profile || !profile->has_trc || !transferFunction) {
+ if (!profile->has_trc) {
return false;
}
@@ -683,22 +671,13 @@
return true;
}
-static bool read_a2b(const skcms_ICCProfile* profile, skcms_A2B* a2b) {
- if (!profile || !a2b) {
- return false;
- }
-
- skcms_ICCTag tag;
- if (!skcms_GetTagBySignature(profile, make_signature('A', '2', 'B', '0'), &tag)) {
- return false;
- }
-
- if (tag.type == make_signature('m', 'f', 't', '1')) {
- return read_tag_mft1(&tag, a2b);
- } else if (tag.type == make_signature('m', 'f', 't', '2')) {
- return read_tag_mft2(&tag, a2b);
- } else if (tag.type == make_signature('m', 'A', 'B', ' ')) {
- return read_tag_mab(&tag, a2b);
+static bool read_a2b(const skcms_ICCTag* tag, skcms_A2B* a2b) {
+ if (tag->type == make_signature('m', 'f', 't', '1')) {
+ return read_tag_mft1(tag, a2b);
+ } else if (tag->type == make_signature('m', 'f', 't', '2')) {
+ return read_tag_mft2(tag, a2b);
+ } else if (tag->type == make_signature('m', 'A', 'B', ' ')) {
+ return read_tag_mab(tag, a2b);
}
return false;
@@ -797,17 +776,39 @@
// Pre-parse commonly used tags.
skcms_ICCTag rTRC, gTRC, bTRC;
- profile->has_trc =
- skcms_GetTagBySignature(profile, make_signature('r', 'T', 'R', 'C'), &rTRC) &&
+ if (skcms_GetTagBySignature(profile, make_signature('r', 'T', 'R', 'C'), &rTRC) &&
skcms_GetTagBySignature(profile, make_signature('g', 'T', 'R', 'C'), &gTRC) &&
- skcms_GetTagBySignature(profile, make_signature('b', 'T', 'R', 'C'), &bTRC) &&
- read_curve(rTRC.buf, rTRC.size, &profile->trc[0], NULL) &&
- read_curve(gTRC.buf, gTRC.size, &profile->trc[1], NULL) &&
- read_curve(bTRC.buf, bTRC.size, &profile->trc[2], NULL);
+ skcms_GetTagBySignature(profile, make_signature('b', 'T', 'R', 'C'), &bTRC)) {
+ if (!read_curve(rTRC.buf, rTRC.size, &profile->trc[0], NULL) ||
+ !read_curve(gTRC.buf, gTRC.size, &profile->trc[1], NULL) ||
+ !read_curve(bTRC.buf, bTRC.size, &profile->trc[2], NULL)) {
+ // Malformed TRC tags
+ return false;
+ }
+ profile->has_trc = true;
+ }
profile->has_tf = get_transfer_function(profile, &profile->tf);
- profile->has_toXYZD50 = read_to_XYZD50 (profile, &profile->toXYZD50);
- profile->has_A2B = read_a2b (profile, &profile->A2B);
+
+ skcms_ICCTag rXYZ, gXYZ, bXYZ;
+ if (skcms_GetTagBySignature(profile, make_signature('r', 'X', 'Y', 'Z'), &rXYZ) &&
+ skcms_GetTagBySignature(profile, make_signature('g', 'X', 'Y', 'Z'), &gXYZ) &&
+ skcms_GetTagBySignature(profile, make_signature('b', 'X', 'Y', 'Z'), &bXYZ)) {
+ if (!read_to_XYZD50(&rXYZ, &gXYZ, &bXYZ, &profile->toXYZD50)) {
+ // Malformed XYZ tags
+ return false;
+ }
+ profile->has_toXYZD50 = true;
+ }
+
+ skcms_ICCTag a2b_tag;
+ if (skcms_GetTagBySignature(profile, make_signature('A', '2', 'B', '0'), &a2b_tag)) {
+ if (!read_a2b(&a2b_tag, &profile->A2B)) {
+ // Malformed A2B tag
+ return false;
+ }
+ profile->has_A2B = true;
+ }
return true;
}
diff --git a/tests.c b/tests.c
index ef20677..b1dce57 100644
--- a/tests.c
+++ b/tests.c
@@ -510,7 +510,6 @@
{ "profiles/color.org/Upper_Right.icc", true, NULL, NULL, NULL, &Upper_Right_A2B },
{ "profiles/misc/Apple_Wide_Color.icc", true, NULL, NULL, NULL, &Apple_Wide_Color_A2B },
{ "profiles/misc/Coated_FOGRA39_CMYK.icc", true, NULL, NULL, NULL, &Coated_FOGRA39_CMYK_A2B },
- { "profiles/misc/ColorGATE_Sihl_PhotoPaper.icc", true, NULL, NULL, NULL, NULL }, // Has kTRC. Broken tag table, and A2B0 fails to parse
{ "profiles/misc/ColorLogic_ISO_Coated_CMYK.icc", true, NULL, NULL, NULL, &ColorLogic_ISO_Coated_CMYK_A2B }, // Has kTRC.
{ "profiles/misc/Japan_Color_2001_Coated.icc", true, NULL, NULL, NULL, &Japan_Color_2001_Coated_A2B },
{ "profiles/misc/Lexmark_X110.icc", true, NULL, NULL, NULL, &Lexmark_X110_A2B },
@@ -562,15 +561,18 @@
// fuzzer generated profiles that found parsing bugs
+ // Bad profiles found inn the wild
+ { "profiles/misc/ColorGATE_Sihl_PhotoPaper.icc", false, NULL, NULL, NULL, NULL }, // Broken tag table, and A2B0 fails to parse
+
// Bad tag table data - these should not parse
{ "profiles/fuzz/last_tag_too_small.icc", false, NULL, NULL, NULL, NULL }, // skia:7592
{ "profiles/fuzz/named_tag_too_small.icc", false, NULL, NULL, NULL, NULL }, // skia:7592
- // These parse but have trouble afterward.
- { "profiles/fuzz/curv_size_overflow.icc", true, NULL, NULL, NULL, NULL }, // skia:7593
- { "profiles/fuzz/truncated_curv_tag.icc", true, NULL, NULL, NULL, NULL }, // oss-fuzz:6103
- { "profiles/fuzz/zero_a.icc", true, NULL, NULL, NULL, NULL }, // oss-fuzz:????
- { "profiles/fuzz/a2b_too_many_input_channels.icc", true, NULL, NULL, NULL, NULL }, // oss-fuzz:6521
+ // Bad tag data - these should not parse
+ { "profiles/fuzz/curv_size_overflow.icc", false, NULL, NULL, NULL, NULL }, // skia:7593
+ { "profiles/fuzz/truncated_curv_tag.icc", false, NULL, NULL, NULL, NULL }, // oss-fuzz:6103
+ { "profiles/fuzz/zero_a.icc", false, NULL, NULL, NULL, NULL }, // oss-fuzz:????
+ { "profiles/fuzz/a2b_too_many_input_channels.icc", false, NULL, NULL, NULL, NULL }, // oss-fuzz:6521
};
static void load_file(const char* filename, void** buf, size_t* len) {
