CVE Patch Review

GHSA-VF33-6R7X-66XX: Root-Cause Guard for ImageMagick Morphology Binomial Kernel Overflow

GHSA-VF33-6R7X-66XX · Updated 2026-05-22 Root-cause

Summary

The patch adds an explicit upper bound on the binomial kernel order before factorial-based computation in MagickCore/morphology.c. This prevents size_t overflow in fact() and the downstream divide-by-zero condition described in the advisory, converting a crashable path into early rejection for oversized kernels.

Analysis

Vulnerability

GHSA-VF33-6R7X-66XX describes a denial-of-service condition in ImageMagick morphology processing where an excessively large binomial kernel radius causes an unbounded integer overflow in factorial calculation. The vulnerable logic relied on factorial-style computation for kernel coefficients, using a size_t-backed conversion of tgamma(). Once the kernel order exceeded the representable range, the factorial result overflowed, which then propagated into invalid arithmetic and a mathematical division by zero during kernel generation.

The issue is fundamentally an input-validation failure around a numerically unsafe code path: the implementation accepted kernel widths whose derived order exceeded the safe domain for fact() as represented in size_t. Because kernel width is attacker-influenced through image processing inputs, this becomes a reachable crash vector.

#if 1
#elif 1 /* glibc floating point alternatives */
#define fact(n) (CastDoubleToSizeT(tgamma((double) n+1)))
#else
#define fact(n) (CastDoubleToSizeT(lgamma((double) n+1)))
#endif

References: official patch commit, GitHub Security Advisory, third-party report.

Patch

The patch introduces a hard upper bound for the binomial kernel order before any factorial-based computation occurs. The bound is architecture-aware: 20 for platforms where sizeof(size_t) > 4, and 12 for 32-bit platforms. If kernel->width - 1 exceeds that maximum order, the function aborts kernel construction and returns a destroyed kernel object instead of continuing into overflow-prone arithmetic.

const size_t
          max_order = (sizeof(size_t) > 4) ? 20 : 12;

          /* Check if kernel order (width-1) would overflow fact() */
        if ((kernel->width-1) > max_order)
          return(DestroyKernelInfo(kernel));

This is a direct guard on the root numerical hazard identified by the advisory. Rather than attempting to make the factorial macro itself saturating or exception-safe, the patch constrains callers to a range where the existing implementation remains valid for the target integer width.

Review

Pros

  • The fix addresses the vulnerable precondition directly by rejecting kernel orders that would overflow fact(), matching the advisory's stated root cause.
  • The bound is simple, low-risk, and cheap to evaluate, making it appropriate for a hot image-processing path.
  • The architecture-specific thresholds acknowledge that the safe factorial range depends on size_t width, which is materially relevant to the overflow behavior.
  • Early return via DestroyKernelInfo(kernel) prevents the downstream divide-by-zero path from being reached.

Cons

  • The patch is narrowly scoped to this call path and does not harden the fact() abstraction itself; other future uses of the same pattern could reintroduce similar overflow risks if not similarly bounded.
  • The safety limits are encoded as constants without in-code derivation or explanatory rationale beyond the comment, so maintainers must trust that 20 and 12 are the correct maxima for all supported builds.
  • The behavior on invalid oversized input is rejection rather than graceful degradation or alternative coefficient generation, which may affect callers expecting large kernels to be accepted.

Verdict

Root-cause.

The patch blocks the vulnerable condition at the correct boundary: it validates the kernel order before invoking overflow-prone factorial computation. Given the advisory's description that excessive kernel radius triggers integer overflow leading to division by zero, this precondition check is a technically sound remediation rather than a downstream crash suppression. The main residual concern is maintainability of the hard-coded thresholds, not completeness of the security fix for the documented issue.

Sources