Skip to content

Commit 33e4aac

Browse files
Harden iccviz GamutVolume/RoundTripDE against untrusted input
Six defensive/correctness fixes in iccviz/IccVizModel.cpp, all in the analysis features; no behaviour change for well-formed profiles: - RoundTripDE: require xB source-sample count == nPcs so Apply cannot over-read the PCS buffer on a crafted B2A declaring >nPcs input channels. - GamutVolume: reject non-finite/zero voxelSize (fall back to auto) and floor it to 0.5 dE; clamp dilate to [0,4]; add a 256M-cell ceiling in voxelEnclosedVolume (uint64) that guards the 32-bit/wasm size_t product. - voxelEnclosedVolume: drop Lab points outside the box instead of clamping their voxel onto the exterior face (also makes the float->int cast safe); flood-fill / erosion stacks use size_t so a >INT_MAX grid cannot truncate. - Neutral-axis: verify transform src/dst counts match the LUT channel counts. - Correct the GamutVolume dilate/erode and 2-skeleton comments (honest about the convex-corner over-estimate and the N>=4 boundary under-capture); note the degeneracy limitation. - Add <cstdint>/<cstddef> (std::uint64_t was previously only transitive). Verified: g++ -Wall -Wextra clean; linked and ran on sRGB v4 (normal result correct, NaN voxelSize and dilate=100000 both handled without hang/crash). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent b225761 commit 33e4aac

1 file changed

Lines changed: 76 additions & 20 deletions

File tree

iccviz/IccVizModel.cpp

Lines changed: 76 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@
2121

2222
#include <algorithm>
2323
#include <cmath>
24+
#include <cstddef> // std::size_t
25+
#include <cstdint> // std::uint32_t / std::uint64_t (used below; previously only transitive)
2426
#include <cstdio>
2527
#include <cstring>
2628
#include <limits>
@@ -1185,6 +1187,13 @@ bool buildNeutralAxisGraph(CIccProfile* pIcc, icTagSignature sig, Graph& out,
11851187
CIccApplyXform* apply = xform->GetNewApply(st);
11861188
if (!apply || st != icCmmStatOk) { delete apply; delete xform; return skip("transform apply init failed"); }
11871189

1190+
// Apply reads GetNumSrcSamples()/writes GetNumDstSamples(); require they match the
1191+
// raw LUT channel counts so the src/dst buffers below (sized inCh/outCh) cannot be
1192+
// over-read/over-written by a multi-element transform (cf. iccDEV #1633).
1193+
if (xform->GetNumSrcSamples() != inCh || xform->GetNumDstSamples() != outCh) {
1194+
delete apply; delete xform; return skip("transform channel-count mismatch");
1195+
}
1196+
11881197
// One series per device colorant; sample L* from 100 (white) down to 0 (black).
11891198
std::vector<Series> series(outCh);
11901199
for (int c = 0; c < outCh; ++c) {
@@ -1263,21 +1272,41 @@ bool buildNeutralAxisGraph(CIccProfile* pIcc, icTagSignature sig, Graph& out,
12631272
// ── Gamut volume: boundary voxelisation + flood-fill (see IccVizModel.hpp) ────
12641273
// Pure Lab-space geometry, ported verbatim from chardata's gamut-wasm
12651274
// `gamutVolumeIcc` (the ICC-specific device→PCS boundary eval lives in the public
1266-
// GamutVolume() below). Why this and not signed-tetra / convex hull / star-|tetra|
1267-
// on the boundary: the device 2-skeleton tiles the gamut surface but self-overlaps
1268-
// and isn't consistently wound, so those all mis-measure; voxel occupancy of the
1275+
// GamutVolume() below). Why voxel occupancy and not signed-tetra / convex hull /
1276+
// star-|tetra| on the boundary: the sampled device boundary self-overlaps and
1277+
// isn't consistently wound, so those all mis-measure; voxel occupancy of the
12691278
// enclosed solid is robust. Dilate seals sampling gaps against flood-fill leaks;
1270-
// the matching erosion removes the dilation's outward bias.
1279+
// the erosion removes MOST of the dilation's outward bias but not all of it — the
1280+
// box (Chebyshev) dilation and the 6-neighbour (city-block) erosion cancel on
1281+
// axis-aligned faces yet leave a residual outward bias at convex corners/edges,
1282+
// so `volume` is a slight over-estimate there. Combined with the 2-skeleton
1283+
// sampling caveat in boundaryDeviceSamples() (the true boundary is under-captured
1284+
// for N≥4), treat the result as a robust ESTIMATE, not an exact measure.
1285+
1286+
// Bounds for the untrusted / caller-supplied gamut-volume geometry (see the
1287+
// GamutVolume() argument clamping and the cell ceiling below).
1288+
const int kMaxGamutDilate = 4; // structuring-element radius clamp (DoS guard)
1289+
const double kMinVoxelSize = 0.5; // ΔE*ab floor: caps the Lab grid resolution
1290+
const std::uint64_t kMaxVoxelCells = 256000000ull; // total voxel ceiling (~256 MB; CWE-190/400)
12711291

12721292
// Voxelise flat Lab boundary points (x3), dilate by `dilate`, flood-fill the
12731293
// exterior, erode the dilation back, return the enclosed volume (ΔE*ab³). Fixed
1274-
// generous Lab box so real gamuts sit strictly interior to it.
1294+
// generous Lab box so real gamuts sit strictly interior to it; points outside the
1295+
// box are dropped (not clamped onto its face). Returns -1 (and enclosedCells = -1)
1296+
// if the grid would exceed kMaxVoxelCells.
12751297
double voxelEnclosedVolume(const std::vector<float>& lab, double vs,
12761298
int dilate, long long& enclosedCells) {
12771299
const double Lmin = -20, Lmax = 120, ABmin = -150, ABmax = 150;
12781300
const int nL = std::max(1, (int)std::ceil((Lmax - Lmin) / vs));
12791301
const int nA = std::max(1, (int)std::ceil((ABmax - ABmin) / vs));
12801302
const int nB = nA;
1303+
// Reject an oversized/overflowing grid before allocating (a tiny caller voxelSize
1304+
// could otherwise wrap the size_t product on 32-bit targets — CWE-190). Signalled
1305+
// back through enclosedCells = -1 so GamutVolume() can fail cleanly.
1306+
const std::uint64_t cells64 = static_cast<std::uint64_t>(nL) *
1307+
static_cast<std::uint64_t>(nA) *
1308+
static_cast<std::uint64_t>(nB);
1309+
if (cells64 == 0 || cells64 > kMaxVoxelCells) { enclosedCells = -1; return -1.0; }
12811310
auto IDX = [&](int l, int a, int b) -> std::size_t {
12821311
return ((std::size_t)l * nA + a) * nB + b;
12831312
};
@@ -1286,26 +1315,34 @@ double voxelEnclosedVolume(const std::vector<float>& lab, double vs,
12861315

12871316
const int nPts = (int)(lab.size() / 3);
12881317
for (int i = 0; i < nPts; ++i) {
1289-
const int li = (int)std::floor((lab[i*3] - Lmin) / vs);
1290-
const int ai = (int)std::floor((lab[i*3 + 1] - ABmin) / vs);
1291-
const int bi = (int)std::floor((lab[i*3 + 2] - ABmin) / vs);
1318+
// Bounded floor: drop points outside the Lab box rather than clamping their
1319+
// voxel onto the exterior face — a clamped solid voxel both clips the gamut to
1320+
// the box and can block a flood-fill seed on that face. Range-checking here also
1321+
// makes the float→int cast safe: GamutVolume filters only for finiteness, and a
1322+
// huge-but-finite coordinate is UB to cast (CWE-704).
1323+
const double lf = std::floor((lab[i*3] - Lmin) / vs);
1324+
const double af = std::floor((lab[i*3 + 1] - ABmin) / vs);
1325+
const double bf = std::floor((lab[i*3 + 2] - ABmin) / vs);
1326+
if (!(lf >= 0.0 && lf < nL) || !(af >= 0.0 && af < nA) || !(bf >= 0.0 && bf < nB))
1327+
continue;
1328+
const int li = (int)lf, ai = (int)af, bi = (int)bf;
12921329
for (int dl = -dilate; dl <= dilate; ++dl)
12931330
for (int da = -dilate; da <= dilate; ++da)
12941331
for (int db = -dilate; db <= dilate; ++db)
12951332
g[IDX(cl(li + dl, nL), cl(ai + da, nA), cl(bi + db, nB))] = 1;
12961333
}
12971334

1298-
std::vector<int> st;
1335+
std::vector<std::size_t> st; // linear voxel ids; size_t so a >INT_MAX grid can't truncate
12991336
auto push = [&](int l, int a, int b) {
13001337
const std::size_t id = IDX(l, a, b);
1301-
if (g[id] == 0) { g[id] = 2; st.push_back((int)id); }
1338+
if (g[id] == 0) { g[id] = 2; st.push_back(id); }
13021339
};
13031340
for (int a = 0; a < nA; ++a) for (int b = 0; b < nB; ++b) { push(0, a, b); push(nL - 1, a, b); }
13041341
for (int l = 0; l < nL; ++l) for (int b = 0; b < nB; ++b) { push(l, 0, b); push(l, nA - 1, b); }
13051342
for (int l = 0; l < nL; ++l) for (int a = 0; a < nA; ++a) { push(l, a, 0); push(l, a, nB - 1); }
13061343
while (!st.empty()) {
1307-
const int id = st.back(); st.pop_back();
1308-
const int b = id % nB, a = (id / nB) % nA, l = (int)(id / ((std::size_t)nB * nA));
1344+
const std::size_t id = st.back(); st.pop_back();
1345+
const int b = (int)(id % nB), a = (int)((id / nB) % nA), l = (int)(id / ((std::size_t)nB * nA));
13091346
if (l > 0) push(l - 1, a, b);
13101347
if (l < nL - 1) push(l + 1, a, b);
13111348
if (a > 0) push(l, a - 1, b);
@@ -1316,16 +1353,16 @@ double voxelEnclosedVolume(const std::vector<float>& lab, double vs,
13161353

13171354
const std::size_t total = (std::size_t)nL * nA * nB;
13181355
for (int pass = 0; pass < dilate; ++pass) {
1319-
std::vector<int> add;
1356+
std::vector<std::size_t> add;
13201357
for (std::size_t id = 0; id < total; ++id) {
13211358
if (g[id] == 2) continue;
13221359
const int b = (int)(id % nB), a = (int)((id / nB) % nA), l = (int)(id / ((std::size_t)nB * nA));
13231360
if ((l > 0 && g[IDX(l - 1, a, b)] == 2) || (l < nL - 1 && g[IDX(l + 1, a, b)] == 2) ||
13241361
(a > 0 && g[IDX(l, a - 1, b)] == 2) || (a < nA - 1 && g[IDX(l, a + 1, b)] == 2) ||
13251362
(b > 0 && g[IDX(l, a, b - 1)] == 2) || (b < nB - 1 && g[IDX(l, a, b + 1)] == 2))
1326-
add.push_back((int)id);
1363+
add.push_back(id);
13271364
}
1328-
for (int id : add) g[id] = 2;
1365+
for (std::size_t id : add) g[id] = 2;
13291366
}
13301367
std::size_t ext = 0;
13311368
for (std::size_t i = 0; i < total; ++i) if (g[i] == 2) ++ext;
@@ -1336,6 +1373,11 @@ double voxelEnclosedVolume(const std::vector<float>& lab, double vs,
13361373
// Device-cube 2-skeleton (boundary-face) samples in 0..1 (IccProfLib device
13371374
// convention): for each free-axis pair (di,dj) swept 0..S, all 2^(N-2) corner
13381375
// combinations of the remaining axes. Flat buffer, N floats per point.
1376+
//
1377+
// NOTE: for N==3 the 2-skeleton IS the full cube surface, but for N>=4 the gamut
1378+
// boundary also includes the interiors of the 3-faces (three free coordinates),
1379+
// which this does not sample — so for CMYK+ the enclosed volume is biased low.
1380+
// This is the sampling caveat referenced by voxelEnclosedVolume() above.
13391381
std::vector<float> boundaryDeviceSamples(int N, int S) {
13401382
std::vector<float> out;
13411383
int fixed[kMaxInkChannels];
@@ -1639,18 +1681,23 @@ GamutVolumeResult GamutVolume(CIccProfile* pIcc, icTagSignature aToBTag,
16391681
if (N < 1 || N > kMaxInkChannels) { delete x; return fail("unsupported device channel count"); }
16401682
if (dstCh < 3) { delete x; return fail("PCS output has < 3 channels"); }
16411683

1642-
// Sampling params: auto-pick any left at their sentinel (≤0).
1684+
// Sampling params: auto-pick any left at their sentinel (≤0), then bound the
1685+
// caller-supplied values so a crafted/typo argument can't drive a huge Lab grid
1686+
// or an unbounded dilate/erode (CWE-400/CWE-190). `!(vs > 0)` also rejects NaN.
16431687
int S = samplesPerAxis, dl = dilate;
16441688
double vs = voxelSize;
16451689
{
16461690
int aS, aDl; double aVs;
16471691
gamutVolumeParams(N, aS, aVs, aDl);
16481692
if ((S <= 0 || dl <= 0) && aS < 0) { delete x; return fail("too many device channels for volume"); }
1649-
if (S <= 0) S = aS;
1650-
if (vs <= 0) vs = aVs;
1651-
if (dl <= 0) dl = aDl;
1693+
if (S <= 0) S = aS;
1694+
if (!(vs > 0.0)) vs = aVs;
1695+
if (dl <= 0) dl = aDl;
16521696
}
16531697
if (S < 2) S = 2;
1698+
if (vs < kMinVoxelSize) vs = kMinVoxelSize; // floor the Lab grid resolution
1699+
if (dl < 0) dl = 0;
1700+
if (dl > kMaxGamutDilate) dl = kMaxGamutDilate; // clamp the structuring element
16541701
// Hard ceiling on total boundary points (CWE-400 guard against a crafted profile).
16551702
const double faces = (N * (N - 1) / 2.0) * std::pow(2.0, std::max(0, N - 2));
16561703
if (faces * (double)(S + 1) * (S + 1) > 3000000.0) { delete x; return fail("device boundary too large for volume"); }
@@ -1684,10 +1731,15 @@ GamutVolumeResult GamutVolume(CIccProfile* pIcc, icTagSignature aToBTag,
16841731
delete ap;
16851732
delete x;
16861733

1734+
// Degeneracy floor: need >=3 finite boundary points to enclose any volume. NOTE
1735+
// a boundary that survives this but has collapsed toward a point/plane still
1736+
// yields a small, technically-ok volume with no distinct "degenerate" signal —
1737+
// a caller cannot tell "genuinely tiny gamut" from "sampling collapsed".
16871738
if (lab.size() < 9) return fail("no finite boundary samples");
16881739

16891740
long long cells = 0;
16901741
r.volume = voxelEnclosedVolume(lab, vs, dl, cells);
1742+
if (cells < 0) return fail("voxel grid too large for volume"); // x/ap already freed above
16911743
r.voxels = cells;
16921744
r.samplesPerAxis = S;
16931745
r.voxelSize = vs;
@@ -1729,7 +1781,11 @@ RoundTripResult RoundTripDE(CIccProfile* pIcc, icRenderingIntent intent,
17291781
if (pcsSp != icSigLabData && pcsSp != icSigXYZData) { delete xA; delete xB; return fail("PCS is not Lab/XYZ"); }
17301782
if (N < 1 || N > kMaxInkChannels) { delete xA; delete xB; return fail("unsupported device channel count"); }
17311783
if (nPcs < 3) { delete xA; delete xB; return fail("PCS has < 3 channels"); }
1732-
if (xB->GetNumSrcSamples() < 3 || xB->GetNumDstSamples() != N) { delete xA; delete xB; return fail("BToA transform shape mismatch"); }
1784+
// xB reads GetNumSrcSamples() floats from pcs1 (sized nPcs) and writes
1785+
// GetNumDstSamples() into dev2 (sized N); require an exact match on BOTH so Apply
1786+
// can never over-read pcs1 (a crafted B2A declaring >nPcs input channels would
1787+
// otherwise read past the PCS buffer) or over-write dev2.
1788+
if (xB->GetNumSrcSamples() != nPcs || xB->GetNumDstSamples() != N) { delete xA; delete xB; return fail("BToA transform shape mismatch"); }
17331789

17341790
int S = samplesPerAxis > 0 ? samplesPerAxis : roundTripSteps(N);
17351791
if (S < 2) S = 2;

0 commit comments

Comments
 (0)