You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by bn...@apache.org on 2026/10/19 21:15:14 UTC
(trafficserver) 11/22: magick: require signing key and allowlist convert operators
This is an automated email from the ASF dual-hosted git repository.
bneradt pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
commit ac7b5b6f3d02cba386eeac7f7ab822c1b6cc9997
Author: Serris Santos <se...@gmail.com>
AuthorDate: Mon Aug 24 08:57:47 2026 -0700
magick: require signing key and allowlist convert operators
Co-authored-by: Serris Santos <ls...@apache.org>
---
plugins/experimental/magick/CMakeLists.txt | 6 +-
plugins/experimental/magick/README | 90 +++++-
plugins/experimental/magick/magick.cc | 275 ++++++++++++++----
plugins/experimental/magick/magick_args.cc | 307 +++++++++++++++++++++
plugins/experimental/magick/magick_args.h | 43 +++
.../magick/{ => unit_tests}/CMakeLists.txt | 13 +-
.../magick/unit_tests/test_magick_args.cc | 239 ++++++++++++++++
7 files changed, 888 insertions(+), 85 deletions(-)
diff --git a/plugins/experimental/magick/CMakeLists.txt b/plugins/experimental/magick/CMakeLists.txt
index e7bcf8b42e..414dc90923 100644
--- a/plugins/experimental/magick/CMakeLists.txt
+++ b/plugins/experimental/magick/CMakeLists.txt
@@ -15,13 +15,17 @@
#
#######################
-add_atsplugin(magick magick.cc)
+add_atsplugin(magick magick.cc magick_args.cc)
# This creates the magic import target if necessary (older cmake)
include(magick_target)
target_link_libraries(magick PRIVATE OpenSSL::Crypto ImageMagick::MagickWand ImageMagick::MagickCore ts::tscppapi)
+if(BUILD_TESTING)
+ add_subdirectory(unit_tests)
+endif()
+
# There is a periodic issue with asan errors and imagemagick so disable this test for asan builds to reduce noise
if(NOT ENABLE_ASAN)
verify_global_plugin(magick ${CMAKE_CURRENT_SOURCE_DIR}/image_magic_dlopen_leak_suppression.txt)
diff --git a/plugins/experimental/magick/README b/plugins/experimental/magick/README
index 88da10ff53..48e36f216b 100644
--- a/plugins/experimental/magick/README
+++ b/plugins/experimental/magick/README
@@ -1,16 +1,72 @@
-ats-magick is a transformation plug-in which allows to run any ImageMagick's convert command while processing images from the origin.
+ats-magick is a transformation plug-in which runs a restricted set of ImageMagick's convert operators on images coming from the origin.
Ideas for how to use this plug-in are, but not limited to:
- convert from multiple formats to a single image format.
- - do simple operatins like resize, crop, etc.
- - add watermarks or automated labels.
- - transform images in a very radical way.
+ - do simple operations like resize, crop, rotate, etc.
+ - strip metadata from images.
Once transformed, the image can (and should) be stored into ATS's cache.
The input for the plug-in's request is the query parameter "magick" which contains a url escaped, base64 encoded version of the parameters passed to ImageMagick's convert command line utility. When this global plug-in is enabled, it will first look into the `Content-Type` response header in all transactions, to then check this query parameter in order to decide to do the transformation.
-To avoid being exploited, the plug-in accepts one configuration argument specified on `plugin.config`; a OpenSSL's RSA256 public key file used to verify if the raw content of the "magick" parameter is valid and authorized. When it is configured, an additional query parameter "magickSig" is required on each request.
+To avoid being exploited, the plug-in requires one configuration argument specified on `plugin.config`; an OpenSSL's RSA256 public key file used to verify if the raw content of the "magick" parameter is valid and authorized. This argument is mandatory: the plug-in refuses to load without it, because an unsigned deployment would let any client run arbitrary ImageMagick convert operators on image responses. When configured, an additional query parameter "magickSig" is required on each request.
+
+## Command shape
+
+As a second layer of defense, even signature-verified requests are restricted. The decoded command must have exactly this shape:
+
+```
+convert mpr:b <operators...> mpr:a
+```
+
+`mpr:b` and `mpr:a` are the plug-in's own in-memory registers, holding the origin image and the transformed image respectively. `mpr:b` is mandatory rather than merely position-constrained: without an input image the conversion fails, and because ImageMagick's MPR registry is process-global, `mpr:a` would still hold another request's image. Anything else in those two positions is rejected with a 403.
+
+## Allowed operators
+
+Operators taking no value:
+
+```
+-auto-orient -flip -flop -magnify -strip -transpose -transverse -trim
+```
+
+Operators taking exactly one value, with the value type each one accepts:
+
+| Operator | Value |
+| --- | --- |
+| `-background` | color (`red`, `#ff0000`, `rgb(255,0,0)`, `gray50`) |
+| `-blur` | geometry |
+| `-colorspace` | enum token (`sRGB`) |
+| `-crop` | geometry |
+| `-density` | geometry |
+| `-depth` | integer |
+| `-extent` | geometry |
+| `-filter` | enum token (`Lanczos`) |
+| `-gaussian-blur` | geometry |
+| `-gravity` | enum token (`NorthWest`) |
+| `-interpolate` | enum token |
+| `-quality` | integer |
+| `-resize` | geometry |
+| `-rotate` | geometry |
+| `-sample` | geometry |
+| `-scale` | geometry |
+| `-sharpen` | geometry |
+| `-thumbnail` | geometry |
+
+Any operator not in these two lists is rejected with a 403. That includes `-write`, bare `@file` references, and the `msl:`/`url:`/`ephemeral:` coders, but also everything else, such as `-draw`, `-annotate`, `-fill`, `-pointsize` and `-font`; watermarking and text labelling are not possible with this plug-in.
+
+The `+` spellings (`+resize`, `+flip`, ...) are all rejected, deliberately. ImageMagick's option table gives `+resize` an arity of 1 while `ConvertImageCommand` consumes zero arguments for it, so accepting `+` forms would desynchronize the plug-in's position model from convert's actual parsing.
+
+## What the validator does and does not guarantee
+
+Both halves of an operator are checked: the operator name against the lists above, and its value against the type in the table. Value typing is not optional hardening. `MagickCommandGenesis()` pre-scans the whole of argv before the convert command runs and has no concept of option arity, so a value position is reachable as an option; `-bench` and `-distribute-cache` in particular are acted on during that pre-scan. The value types are therefore chosen so that no option name can fit in a v [...]
+
+The safety of the position model rests on the arity in the two lists above matching what ImageMagick's `CommandOptions` table declares. Nothing in this tree verifies that external invariant, and the two failure directions are not symmetric: listing a genuinely one-argument option as zero-argument causes a rejection, but the reverse makes the validator skip a token that ImageMagick will then interpret. `unit_tests/test_magick_args.cc` locks the behaviour described here.
+
+## Failure handling
+
+If the conversion fails for any reason, the plug-in emits no body, the response becomes a 502 rather than a 200 carrying stale or empty content, the origin's freshness and validator headers are stripped in favour of `Cache-Control: no-store`, and the transformed response is not written to the cache. The reason ImageMagick reported is logged along with the request URL.
+
+## Keys
The size of the keys directly impact how easy to compute (and break) the signature as well as the length of it. Generate a private & public pair of RSA256 keys using the following OpenSSL commands:
@@ -22,16 +78,22 @@ openssl genrsa -out rsa256-private 512;
openssl rsa -in rsa256-private -pubout > rsa256-public;
```
-The `test.sh` script, helps verifying ImageMagick's convert works. The script outputs the right query parameter for you.
+## Examples
-`
+The `test.sh` script helps verify that a command works. It takes the operators only, wraps them in the required shape, and outputs the signed query parameters for you. It does not consult the allowlist, so it will happily sign a command that the plug-in rejects.
-##ImageMagick's convert examples:
-
-`convert input.jpg -flip output.jpg;`
-
-`convert input.jpg -rotate 90 output.jpg;`
+```
+./test.sh -flip;
+./test.sh -rotate 90;
+./test.sh -resize 128x128;
+./test.sh -auto-orient -resize 800x600 -quality 85 -strip;
+```
-`convert input.jpg -resize 128x128 output.jpg;`
+The commands those produce, which is what the "magick" query parameter carries once decoded:
-`convert input.jpg output.png;`
+```
+convert mpr:b -flip mpr:a;
+convert mpr:b -rotate 90 mpr:a;
+convert mpr:b -resize 128x128 mpr:a;
+convert mpr:b -auto-orient -resize 800x600 -quality 85 -strip mpr:a;
+```
diff --git a/plugins/experimental/magick/magick.cc b/plugins/experimental/magick/magick.cc
index bbe84bdee0..42dc088d7f 100644
--- a/plugins/experimental/magick/magick.cc
+++ b/plugins/experimental/magick/magick.cc
@@ -18,9 +18,11 @@
#include "ts/apidefs.h"
#include <algorithm>
+#include <atomic>
#include <chrono>
#include <condition_variable>
#include <functional>
+#include <iterator>
#include <list>
#include <map>
#include <memory>
@@ -38,6 +40,8 @@
#include <ts/ts.h>
+#include "magick_args.h"
+
#include <openssl/err.h>
#include <openssl/evp.h>
#include <openssl/pem.h>
@@ -60,9 +64,8 @@
using std::string;
using namespace atscppapi;
-using CharVector = std::vector<char>;
-using CharPointerVector = std::vector<char *>;
-using StringViewVector = std::vector<std::string_view>;
+// CharVector and CharPointerVector come from magick_args.h.
+using StringViewVector = std::vector<std::string_view>;
using byte = unsigned char;
@@ -259,6 +262,28 @@ struct Exception {
}
Exception() : info(AcquireExceptionInfo()) { assert(nullptr != info); }
+
+ /** The reason ImageMagick recorded, if any, so a failure can be diagnosed from
+ * the error log instead of a fixed string.
+ */
+ std::string
+ text() const
+ {
+ assert(nullptr != info);
+ std::string result;
+
+ if (nullptr != info->reason) {
+ result += info->reason;
+ }
+ if (nullptr != info->description) {
+ if (!result.empty()) {
+ result += ": ";
+ }
+ result += info->description;
+ }
+
+ return result;
+ }
};
struct Image {
@@ -281,13 +306,31 @@ struct Wand {
{
assert(nullptr != wand);
wand = DestroyMagickWand(wand);
- if (nullptr == blob) {
+ if (nullptr != blob) {
blob = MagickRelinquishMemory(blob);
}
}
Wand() : wand(NewMagickWand()) { assert(nullptr != wand); }
+ /** The reason the last wand operation failed, so a failure can be diagnosed from
+ * the error log instead of a fixed string.
+ */
+ std::string
+ exceptionText() const
+ {
+ assert(nullptr != wand);
+ ExceptionType severity = UndefinedException;
+ std::string result;
+
+ if (char *const description = MagickGetException(wand, &severity); nullptr != description) {
+ result = description;
+ MagickRelinquishMemory(description);
+ }
+
+ return result;
+ }
+
void
clear() const
{
@@ -414,9 +457,11 @@ bool
QueryParameterToCharVector(CharVector &v)
{
{
- std::size_t s = 0;
+ std::size_t s = 0;
+ // TSStringPercentDecode always reports success, so this cannot currently fail;
+ // the check is here so it does the right thing if that ever changes.
const TSReturnCode rc = TSStringPercentDecode(v.data(), v.size(), v.data(), v.size(), &s);
- assert(TS_SUCCESS == rc);
+
if (rc != TS_SUCCESS) {
return false;
}
@@ -426,7 +471,10 @@ QueryParameterToCharVector(CharVector &v)
{
std::size_t s = 0;
const TSReturnCode rc = TSBase64Decode(v.data(), v.size(), reinterpret_cast<unsigned char *>(v.data()), v.size(), &s);
- assert(TS_SUCCESS == rc);
+
+ // This rejects any input too short to be valid base64, which is client
+ // controlled and reachable before signature verification, so it must return a
+ // failure rather than assert.
if (rc != TS_SUCCESS) {
return false;
}
@@ -436,39 +484,6 @@ QueryParameterToCharVector(CharVector &v)
return true;
}
-CharPointerVector
-QueryParameterToArguments(CharVector &v)
-{
- CharPointerVector result;
- result.reserve(32);
-
- std::size_t i = 0, j = 0;
- bool quote = false;
-
- for (; i < v.size(); ++i) {
- char &c = v[i];
- assert('\0' != c);
- if ('"' == c) {
- if (i > j) {
- result.push_back(&v[j]);
- }
- c = '\0';
- j = i + 1;
- quote = !quote;
- } else if (!quote && ' ' == c) {
- if (i > j) {
- result.push_back(&v[j]);
- }
- c = '\0';
- j = i + 1;
- }
- }
- if (i > j) {
- result.push_back(&v[j]);
- }
- return result;
-}
-
struct ImageTransform : TransformationPlugin {
~ImageTransform() override = default;
@@ -476,9 +491,55 @@ struct ImageTransform : TransformationPlugin {
: TransformationPlugin(t, TransformationPlugin::RESPONSE_TRANSFORMATION),
arguments_(std::move(a)),
argumentMap_(std::move(m)),
- threadPool_(p)
+ threadPool_(p),
+ txn_(static_cast<TSHttpTxn>(t.getAtsHandle())),
+ // fail() runs on a pool thread, where touching the transaction to read the
+ // URL is not safe, so capture it here while still on the transaction thread.
+ url_(t.getClientRequest().getUrl().getUrlString())
{
Dbg(dbg_ctl, "ImageTransform");
+ TransformationPlugin::registerHook(HOOK_SEND_RESPONSE_HEADERS);
+ }
+
+ /** A failed conversion must not be served as a 200. The client response headers
+ * are not built until setOutputComplete, so the failure recorded by the background
+ * thread can still be turned into an error here.
+ *
+ * This only rewrites the client response. It does not stop the cache write, because
+ * the headers destined for the cache were already snapshotted into
+ * cache_info.transform_store by HttpTransact::handle_transform_ready, which runs
+ * off the first TSVConnWrite that setOutputComplete issues. fail() disables the
+ * transformed-response cache write directly for that reason.
+ *
+ * This runs before the response body is committed only because the success path
+ * has exactly one produce() and the failure path has none: SEND_RESPONSE_HDR is
+ * gated on the first TSVConnWrite, latched once via TransformVConnection's
+ * m_called_user, not on setOutputComplete. A produce()-then-fail() path would
+ * commit the 200 header first and silently stop the 502 from working.
+ */
+ void
+ handleSendResponseHeaders(Transaction &t) override
+ {
+ if (failed_) {
+ Response &response = t.getClientResponse();
+ response.setStatusCode(HTTP_STATUS_BAD_GATEWAY);
+ response.setReasonPhrase("Bad Gateway");
+ // This is an empty error response, not an image.
+ Headers &headers = response.getHeaders();
+
+ headers.erase("Content-Type");
+
+ // The origin's freshness and validator headers describe an image that was
+ // never produced. Leaving them would let a downstream cache or browser store
+ // this 502 against the URL.
+ headers.erase("Cache-Control");
+ headers.erase("Expires");
+ headers.erase("ETag");
+ headers.erase("Last-Modified");
+ headers.erase("Vary");
+ headers.set("Cache-Control", "no-store");
+ }
+ t.resume();
}
void
@@ -498,18 +559,48 @@ struct ImageTransform : TransformationPlugin {
magick::Exception exception;
magick::Wand wand;
- assert(!this->blob_.empty());
+ // An empty body is origin controlled, so it has to fail rather than assert.
+ if (this->blob_.empty()) {
+ this->fail("origin returned an empty body");
+ return;
+ }
- wand.readBlob(this->blob_);
- wand.write("mpr:b");
+ /** Every step is checked. ImageMagick's MPR registry is process-global rather
+ * than per-wand, so on a failed conversion mpr:a still holds whatever a
+ * previous transform wrote, from any thread. Producing unconditionally would
+ * serve that unrelated image under this request's URL, status and
+ * Content-Type, and cache it.
+ */
+ if (!wand.readBlob(this->blob_)) {
+ this->fail("failed to decode the origin image", wand.exceptionText());
+ return;
+ }
- MagickCommandGenesis(image.info, ConvertImageCommand, this->argumentMap_.size(), this->argumentMap_.data(), nullptr,
- exception.info);
+ if (!wand.write("mpr:b")) {
+ this->fail("failed to write the input MPR register", wand.exceptionText());
+ return;
+ }
+
+ if (MagickCommandGenesis(image.info, ConvertImageCommand, this->argumentMap_.size(), this->argumentMap_.data(), nullptr,
+ exception.info) != MagickTrue) {
+ this->fail("convert command failed", exception.text());
+ return;
+ }
wand.clear();
- wand.read("mpr:a");
+
+ if (!wand.read("mpr:a")) {
+ this->fail("failed to read the output MPR register", wand.exceptionText());
+ return;
+ }
const std::string_view output = wand.get();
+
+ if (output.empty()) {
+ this->fail("conversion produced an empty image", wand.exceptionText());
+ return;
+ }
+
this->produce(output);
Dbg(dbg_ctl, "Background transformation is done, resuming continuation (%p)", this);
@@ -520,10 +611,43 @@ struct ImageTransform : TransformationPlugin {
Dbg(dbg_ctl, "Scheduling background transformation (%p)", this);
}
+ /** Emit no body, suppress the cache write, and let handleSendResponseHeaders turn
+ * the response into a 502.
+ *
+ * Setting failed_ and disabling the cache write before setOutputComplete() is
+ * load-bearing, not incidental ordering. setOutputComplete() is what drives the
+ * transform read to completion, and therefore what makes the transaction thread
+ * observe both. api_info.cache_transformed is read by
+ * HttpTransact::handle_transform_ready, which runs off that same write.
+ */
+ void
+ fail(const char *const reason, const std::string &detail = std::string())
+ {
+ if (detail.empty()) {
+ Dbg(dbg_ctl, "transformation failed (%p): %s [%s]", this, reason, url_.c_str());
+ TSError("[" PLUGIN_TAG "] %s: %s", reason, url_.c_str());
+ } else {
+ Dbg(dbg_ctl, "transformation failed (%p): %s: %s [%s]", this, reason, detail.c_str(), url_.c_str());
+ TSError("[" PLUGIN_TAG "] %s: %s: %s", reason, detail.c_str(), url_.c_str());
+ }
+
+ failed_ = true;
+
+ TSHttpTxnTransformedRespCache(txn_, 0);
+
+ setOutputComplete();
+ }
+
CharVector arguments_;
CharPointerVector argumentMap_;
CharVector blob_;
ThreadPool &threadPool_;
+ TSHttpTxn txn_ = nullptr;
+ std::string url_;
+
+ /// Written on a pool thread in fail(), read on an ET_NET thread in
+ /// handleSendResponseHeaders().
+ std::atomic<bool> failed_ = false;
};
struct GlobalHookPlugin : GlobalPlugin {
@@ -588,32 +712,58 @@ struct GlobalHookPlugin : GlobalPlugin {
const auto &view = magickQueryParameter.front();
CharVector magick(view.data(), view.data() + view.size());
- bool verified = nullptr == key_;
+ // Fail closed: a request is only trusted when a signing key is
+ // configured AND the accompanying signature verifies.
+ bool verified = false;
- if (!verified) {
+ if (nullptr != key_) {
const auto &magickSigQueryParameter = queryMap["magickSig"];
if (!magickSigQueryParameter.empty()) {
const auto &view2 = magickSigQueryParameter.front();
CharVector magickSig(view2.data(), view2.data() + view2.size());
magickSig.insert(magickSig.end(), '\0');
Dbg(dbg_ctl, "Magick Signature: %s", magickSig.data());
- QueryParameterToCharVector(magickSig);
- verified = magick::verify(reinterpret_cast<const byte *>(magick.data()), magick.size(),
- reinterpret_cast<const byte *>(magickSig.data()), magickSig.size(), key_->key);
+ if (QueryParameterToCharVector(magickSig)) {
+ verified = magick::verify(reinterpret_cast<const byte *>(magick.data()), magick.size(),
+ reinterpret_cast<const byte *>(magickSig.data()), magickSig.size(), key_->key);
+ }
}
}
if (verified) {
magick.insert(magick.end(), '\0');
- QueryParameterToCharVector(magick);
- Dbg(dbg_ctl, "ImageMagick's syntax: %s", magick.data());
- CharPointerVector argumentMap = QueryParameterToArguments(magick);
- t.addPlugin(new ImageTransform(t, std::move(magick), std::move(argumentMap), threadPool_));
+ const bool decoded = QueryParameterToCharVector(magick);
+
+ // Keep the terminator within size() so every argv token, including the
+ // last, is a NUL-terminated C string without depending on the decoders
+ // leaving one in the vector's unused capacity.
+ CharPointerVector argumentMap;
+ std::string reason;
+
+ if (decoded) {
+ magick.push_back('\0');
+ Dbg(dbg_ctl, "ImageMagick's syntax: %s", magick.data());
+ argumentMap = QueryParameterToArguments(magick);
+ }
+
+ if (decoded && ValidateMagickArguments(argumentMap, &reason)) {
+ t.addPlugin(new ImageTransform(t, std::move(magick), std::move(argumentMap), threadPool_));
+ } else {
+ Dbg(dbg_ctl, "disallowed ImageMagick operator in magick arguments: %s", reason.c_str());
+ TSError("[" PLUGIN_TAG "] disallowed ImageMagick operator in magick arguments: %s", reason.c_str());
+ t.setStatusCode(HTTP_STATUS_FORBIDDEN);
+ t.error();
+ // error() is a terminal reenable; falling through to t.resume() below
+ // would reenable the transaction a second time.
+ return;
+ }
} else {
Dbg(dbg_ctl, "signature verification failed.");
TSError("[" PLUGIN_TAG "] signature verification failed.");
t.setStatusCode(HTTP_STATUS_FORBIDDEN);
t.error();
+ // error() is a terminal reenable; see above.
+ return;
}
}
}
@@ -629,12 +779,15 @@ TSPluginInit(int argc, const char **argv)
return;
}
- const char *key = nullptr;
-
- if (1 < argc) {
- // first argument is the path to the public key used to verify query parameter magick's content.
- key = argv[1];
+ // Fail closed: Refuses to load without a key
+ if (argc < 2) {
+ TSError("[" PLUGIN_TAG "] a public key file argument is required; refusing to load without a signing key");
+ Dbg(dbg_ctl, "no public key configured; refusing to load");
+ return;
}
+ // first argument is the path to the public key used to verify query parameter magick's content.
+ const char *const key = argv[1];
+
new GlobalHookPlugin(key);
}
diff --git a/plugins/experimental/magick/magick_args.cc b/plugins/experimental/magick/magick_args.cc
new file mode 100644
index 0000000000..7be728f918
--- /dev/null
+++ b/plugins/experimental/magick/magick_args.cc
@@ -0,0 +1,307 @@
+/**
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#include "magick_args.h"
+
+#include <algorithm>
+#include <string_view>
+
+CharPointerVector
+QueryParameterToArguments(CharVector &v)
+{
+ CharPointerVector result;
+ result.reserve(32);
+
+ // The caller NUL-terminates the decoded buffer so the last token is a valid C
+ // string; the terminator is not part of the command text.
+ std::size_t size = v.size();
+
+ while (size > 0 && '\0' == v[size - 1]) {
+ --size;
+ }
+
+ std::size_t i = 0, j = 0;
+ bool quote = false;
+
+ for (; i < size; ++i) {
+ char &c = v[i];
+ if ('"' == c) {
+ if (i > j) {
+ result.push_back(&v[j]);
+ }
+ c = '\0';
+ j = i + 1;
+ quote = !quote;
+ } else if (!quote && ' ' == c) {
+ if (i > j) {
+ result.push_back(&v[j]);
+ }
+ c = '\0';
+ j = i + 1;
+ }
+ }
+ if (i > j) {
+ result.push_back(&v[j]);
+ }
+ return result;
+}
+
+namespace
+{
+/** What a given operator's value is allowed to look like.
+ *
+ * Value positions used to be skipped without inspection, on the theory that
+ * ConvertImageCommand consumes a declared parameter verbatim and never treats it
+ * as a filename or coder. That is true of ConvertImageCommand, but it is not true
+ * of MagickCommandGenesis(), which pre-scans the whole of argv[1..argc-2] with no
+ * concept of option arity before the command ever runs. So a value position is
+ * reachable as an option, and every value has to be typed.
+ */
+enum class ValueKind {
+ Geometry, ///< 128x128, 50%, +10+20, -90, 100x100!
+ Integer, ///< 90, 8
+ Token, ///< an enum name such as sRGB, Lanczos, NorthWest
+ Color, ///< red, #ff0000, rgb(1,2,3), gray50
+};
+
+struct OneArgOption {
+ std::string_view name;
+ ValueKind kind;
+};
+
+/** Operators that consume no value.
+ *
+ * Only the `-` spelling is listed. The `+` spellings are deliberately rejected:
+ * ImageMagick's option table gives, for example, `+resize` an arity of 1 while
+ * ConvertImageCommand consumes zero arguments for it, so allowing `+` forms would
+ * desynchronize this validator's position model from convert's actual parsing.
+ *
+ * The arity in these two tables must match what ImageMagick's CommandOptions table
+ * declares, and nothing in this tree checks that external invariant. The mistake
+ * directions are not symmetric: listing a genuinely one-argument option here rejects
+ * (safe), but listing a genuinely zero-argument option in oneArgOptions below makes
+ * this code skip a token that ImageMagick will then treat as an operator or filename
+ * (fails open). When in doubt, put an option here or leave it out entirely.
+ */
+constexpr std::string_view zeroArgOptions[] = {
+ "-auto-orient", "-flip", "-flop", "-magnify", "-strip", "-transpose", "-transverse", "-trim",
+};
+
+constexpr OneArgOption oneArgOptions[] = {
+ {"-background", ValueKind::Color },
+ {"-blur", ValueKind::Geometry},
+ {"-colorspace", ValueKind::Token },
+ {"-crop", ValueKind::Geometry},
+ {"-density", ValueKind::Geometry},
+ {"-depth", ValueKind::Integer },
+ {"-extent", ValueKind::Geometry},
+ {"-filter", ValueKind::Token },
+ {"-gaussian-blur", ValueKind::Geometry},
+ {"-gravity", ValueKind::Token },
+ {"-interpolate", ValueKind::Token },
+ {"-quality", ValueKind::Integer },
+ {"-resize", ValueKind::Geometry},
+ {"-rotate", ValueKind::Geometry},
+ {"-sample", ValueKind::Geometry},
+ {"-scale", ValueKind::Geometry},
+ {"-sharpen", ValueKind::Geometry},
+ {"-thumbnail", ValueKind::Geometry},
+};
+
+/** Tokens MagickCommandGenesis() acts on while pre-scanning argv, before the
+ * command callback runs. See MagickWand/magick-cli.c (ImageMagick 7) and
+ * wand/mogrify.c (ImageMagick 6). The 6 loop compares option+1, so both the `-`
+ * and `+` spellings match there.
+ *
+ * The value typing below already rejects all of these, because none of them fits
+ * any ValueKind. This list is a redundant, explicit backstop so that widening a
+ * ValueKind later cannot silently reopen the hole.
+ */
+constexpr std::string_view genesisScannedOptions[] = {
+ "-bench", "+bench", "-concurrent", "+concurrent", "-debug", "+debug", "-distribute-cache", "+distribute-cache",
+ "-duration", "+duration", "-regard-warnings", "+regard-warnings",
+};
+
+/// Values longer than this are rejected outright; no legitimate operator value is.
+constexpr std::size_t maxValueLength = 64;
+
+bool
+inSet(std::string_view allowed, const char c)
+{
+ return allowed.find(c) != std::string_view::npos;
+}
+
+bool
+isDigit(const char c)
+{
+ return '0' <= c && c <= '9';
+}
+
+bool
+isAlnum(const char c)
+{
+ return isDigit(c) || ('a' <= c && c <= 'z') || ('A' <= c && c <= 'Z');
+}
+
+/** A geometry may lead with `-` (as in `-rotate -90`), so a bare "no leading dash"
+ * test is not usable here. Instead the character set excludes every letter, which
+ * is what keeps an option name out of a geometry position, and `@` is excluded so a
+ * value can never become an @file reference even though `@` is valid area geometry.
+ */
+bool
+isGeometry(const std::string_view value)
+{
+ constexpr std::string_view punctuation = "xX%+-!<>^.,";
+ bool digit = false;
+
+ for (const char c : value) {
+ if (isDigit(c)) {
+ digit = true;
+ } else if (!inSet(punctuation, c)) {
+ return false;
+ }
+ }
+
+ return digit;
+}
+
+bool
+isInteger(const std::string_view value)
+{
+ return !value.empty() && std::all_of(value.begin(), value.end(), isDigit);
+}
+
+bool
+isToken(const std::string_view value)
+{
+ if (value.empty() || !isAlnum(value.front())) {
+ return false;
+ }
+
+ return std::all_of(value.begin(), value.end(), [](const char c) { return isAlnum(c) || '-' == c || '_' == c; });
+}
+
+bool
+isColor(const std::string_view value)
+{
+ constexpr std::string_view punctuation = "#%(),.";
+
+ if (value.empty() || (!isAlnum(value.front()) && '#' != value.front())) {
+ return false;
+ }
+
+ return std::all_of(value.begin(), value.end(), [&](const char c) { return isAlnum(c) || inSet(punctuation, c); });
+}
+
+bool
+isValidValue(const ValueKind kind, const std::string_view value)
+{
+ if (value.empty() || value.size() > maxValueLength) {
+ return false;
+ }
+
+ if (std::find(std::begin(genesisScannedOptions), std::end(genesisScannedOptions), value) != std::end(genesisScannedOptions)) {
+ return false;
+ }
+
+ switch (kind) {
+ case ValueKind::Geometry:
+ return isGeometry(value);
+ case ValueKind::Integer:
+ return isInteger(value);
+ case ValueKind::Token:
+ return isToken(value);
+ case ValueKind::Color:
+ return isColor(value);
+ }
+
+ return false;
+}
+
+} // namespace
+
+/** Even a signed request must not reach ImageMagick's dangerous argv surface
+ * (-write, @file, and the msl:/url:/ephemeral: coders), so only vetted convert
+ * operators are allowed and the only "file" tokens permitted are the plugin's own
+ * mpr:a/mpr:b registers.
+ *
+ * The shape is pinned to exactly `convert mpr:b <operators...> mpr:a`. mpr:b is
+ * mandatory, not merely position constrained: without it the command has no input
+ * image, convert fails, and mpr:a still holds whatever a previous transform left in
+ * the process-global MPR registry.
+ *
+ * Both an operator and its value are checked. Checking the operator alone is not
+ * sufficient, because MagickCommandGenesis() pre-scans argv without regard for
+ * arity and so can pick up a value token as an option.
+ */
+bool
+ValidateMagickArguments(const CharPointerVector &arguments, std::string *reason)
+{
+ const auto reject = [reason](std::string &&message) {
+ if (nullptr != reason) {
+ *reason = std::move(message);
+ }
+ return false;
+ };
+
+ if (arguments.size() < 3) {
+ return reject("expected `convert mpr:b <operators> mpr:a`");
+ }
+
+ const std::size_t last = arguments.size() - 1;
+
+ for (const char *const argument : arguments) {
+ if (nullptr == argument) {
+ return reject("null argument");
+ }
+ }
+
+ // ImageMagick treats argv[0] as the command name. The input register must be
+ // read first and the output register written last.
+ if (std::string_view(arguments[0]) != "convert" || std::string_view(arguments[1]) != "mpr:b" ||
+ std::string_view(arguments[last]) != "mpr:a") {
+ return reject("expected `convert mpr:b <operators> mpr:a`");
+ }
+
+ for (std::size_t i = 2; i < last; ++i) {
+ const std::string_view token(arguments[i]);
+
+ if (std::find(std::begin(zeroArgOptions), std::end(zeroArgOptions), token) != std::end(zeroArgOptions)) {
+ continue;
+ }
+
+ const auto option =
+ std::find_if(std::begin(oneArgOptions), std::end(oneArgOptions), [token](const OneArgOption &o) { return o.name == token; });
+
+ if (option == std::end(oneArgOptions)) {
+ return reject("disallowed operator: " + std::string(token));
+ }
+
+ // Consume the operator's value by position, matching convert's own argv
+ // handling. The value may not swallow the trailing mpr:a.
+ if (++i >= last) {
+ return reject("missing value for operator: " + std::string(token));
+ }
+
+ if (const std::string_view value(arguments[i]); !isValidValue(option->kind, value)) {
+ return reject("disallowed value for " + std::string(token) + ": " + std::string(value));
+ }
+ }
+
+ return true;
+}
diff --git a/plugins/experimental/magick/magick_args.h b/plugins/experimental/magick/magick_args.h
new file mode 100644
index 0000000000..4050b6188a
--- /dev/null
+++ b/plugins/experimental/magick/magick_args.h
@@ -0,0 +1,43 @@
+/**
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#pragma once
+
+#include <string>
+#include <vector>
+
+using CharVector = std::vector<char>;
+using CharPointerVector = std::vector<char *>;
+
+/** Split a decoded `magick` query parameter into an argv vector.
+ *
+ * Tokens are separated by spaces, with double quotes grouping. The separators are
+ * overwritten with NUL in place, so the returned pointers alias @a v and are only
+ * valid while @a v lives and is not reallocated. Any trailing NUL bytes the caller
+ * appended are ignored rather than turned into empty tokens.
+ */
+CharPointerVector QueryParameterToArguments(CharVector &v);
+
+/** Defense-in-depth allowlist for the argv handed to ImageMagick.
+ *
+ * @param arguments argv as produced by QueryParameterToArguments().
+ * @param reason if non-null, set to a human-readable rejection reason on failure.
+ * @return true when every token is vetted and the command shape is exactly
+ * `convert mpr:b <operators...> mpr:a`.
+ */
+bool ValidateMagickArguments(const CharPointerVector &arguments, std::string *reason = nullptr);
diff --git a/plugins/experimental/magick/CMakeLists.txt b/plugins/experimental/magick/unit_tests/CMakeLists.txt
similarity index 63%
copy from plugins/experimental/magick/CMakeLists.txt
copy to plugins/experimental/magick/unit_tests/CMakeLists.txt
index e7bcf8b42e..b732265a05 100644
--- a/plugins/experimental/magick/CMakeLists.txt
+++ b/plugins/experimental/magick/unit_tests/CMakeLists.txt
@@ -15,14 +15,9 @@
#
#######################
-add_atsplugin(magick magick.cc)
+add_executable(test_magick_args test_magick_args.cc ${CMAKE_CURRENT_SOURCE_DIR}/../magick_args.cc)
-# This creates the magic import target if necessary (older cmake)
-include(magick_target)
+target_include_directories(test_magick_args PRIVATE "${CMAKE_CURRENT_SOURCE_DIR}/..")
+target_link_libraries(test_magick_args PRIVATE Catch2::Catch2WithMain)
-target_link_libraries(magick PRIVATE OpenSSL::Crypto ImageMagick::MagickWand ImageMagick::MagickCore ts::tscppapi)
-
-# There is a periodic issue with asan errors and imagemagick so disable this test for asan builds to reduce noise
-if(NOT ENABLE_ASAN)
- verify_global_plugin(magick ${CMAKE_CURRENT_SOURCE_DIR}/image_magic_dlopen_leak_suppression.txt)
-endif()
+add_catch2_test(NAME test_magick_args COMMAND test_magick_args)
diff --git a/plugins/experimental/magick/unit_tests/test_magick_args.cc b/plugins/experimental/magick/unit_tests/test_magick_args.cc
new file mode 100644
index 0000000000..00a4144476
--- /dev/null
+++ b/plugins/experimental/magick/unit_tests/test_magick_args.cc
@@ -0,0 +1,239 @@
+/**
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#include "magick_args.h"
+
+#include <catch2/catch_test_macros.hpp>
+
+#include <string>
+
+namespace
+{
+/** Tokenize and validate a command exactly as the plugin does, so the tests exercise
+ * the real path from decoded query parameter to verdict.
+ */
+bool
+accepts(const std::string &command, std::string *reason = nullptr)
+{
+ CharVector buffer(command.begin(), command.end());
+
+ buffer.push_back('\0');
+
+ const CharPointerVector arguments = QueryParameterToArguments(buffer);
+
+ return ValidateMagickArguments(arguments, reason);
+}
+} // namespace
+
+TEST_CASE("ValidateMagickArguments accepts allowlisted commands")
+{
+ // Without at least one happy-path case, a validator that rejected everything
+ // would ship green.
+ SECTION("no operators")
+ {
+ REQUIRE(accepts("convert mpr:b mpr:a"));
+ }
+ SECTION("zero-argument operators")
+ {
+ REQUIRE(accepts("convert mpr:b -flip mpr:a"));
+ REQUIRE(accepts("convert mpr:b -auto-orient -strip -trim mpr:a"));
+ }
+ SECTION("one-argument operators")
+ {
+ REQUIRE(accepts("convert mpr:b -resize 128x128 mpr:a"));
+ REQUIRE(accepts("convert mpr:b -crop 100x100+10+20 mpr:a"));
+ REQUIRE(accepts("convert mpr:b -quality 90 mpr:a"));
+ REQUIRE(accepts("convert mpr:b -colorspace sRGB mpr:a"));
+ REQUIRE(accepts("convert mpr:b -filter Lanczos -resize 50% mpr:a"));
+ REQUIRE(accepts("convert mpr:b -background none mpr:a"));
+ REQUIRE(accepts("convert mpr:b -background #ff0000 mpr:a"));
+ }
+ SECTION("a geometry may lead with a minus")
+ {
+ // A blanket "values may not start with -" test would break this.
+ REQUIRE(accepts("convert mpr:b -rotate -90 mpr:a"));
+ }
+ SECTION("mixed")
+ {
+ REQUIRE(accepts("convert mpr:b -auto-orient -resize 800x600 -quality 85 -strip mpr:a"));
+ }
+}
+
+TEST_CASE("ValidateMagickArguments enforces the command shape")
+{
+ SECTION("too few arguments")
+ {
+ REQUIRE_FALSE(accepts(""));
+ REQUIRE_FALSE(accepts("convert"));
+ REQUIRE_FALSE(accepts("convert mpr:a"));
+ }
+ SECTION("argv[0] must be convert")
+ {
+ REQUIRE_FALSE(accepts("mogrify mpr:b -flip mpr:a"));
+ REQUIRE_FALSE(accepts("identify mpr:b mpr:a"));
+ }
+ SECTION("the mpr:b input register is mandatory")
+ {
+ // Without an input image convert fails, and mpr:a still holds whatever a
+ // previous transform left in the process-global MPR registry.
+ REQUIRE_FALSE(accepts("convert -flip mpr:a"));
+ REQUIRE_FALSE(accepts("convert input.jpg -flip mpr:a"));
+ REQUIRE_FALSE(accepts("convert mpr:c -flip mpr:a"));
+ }
+ SECTION("mpr:a must be last")
+ {
+ REQUIRE_FALSE(accepts("convert mpr:b mpr:a -flip"));
+ REQUIRE_FALSE(accepts("convert mpr:b -flip output.png"));
+ REQUIRE_FALSE(accepts("convert mpr:b -flip mpr:a extra"));
+ }
+ SECTION("a value may not swallow the trailing mpr:a")
+ {
+ REQUIRE_FALSE(accepts("convert mpr:b -resize mpr:a"));
+ REQUIRE_FALSE(accepts("convert mpr:b -flip -quality mpr:a"));
+ }
+}
+
+TEST_CASE("ValidateMagickArguments rejects the dangerous argv surface")
+{
+ SECTION("-write")
+ {
+ REQUIRE_FALSE(accepts("convert mpr:b -write /tmp/owned.png mpr:a"));
+ }
+ SECTION("@file")
+ {
+ REQUIRE_FALSE(accepts("convert mpr:b @/etc/passwd mpr:a"));
+ }
+ SECTION("coders")
+ {
+ REQUIRE_FALSE(accepts("convert mpr:b msl:/tmp/x.msl mpr:a"));
+ REQUIRE_FALSE(accepts("convert mpr:b url:http://example.com/x mpr:a"));
+ REQUIRE_FALSE(accepts("convert mpr:b ephemeral:/etc/passwd mpr:a"));
+ }
+ SECTION("non-allowlisted operators")
+ {
+ REQUIRE_FALSE(accepts("convert mpr:b -draw \"text 0,0 hi\" mpr:a"));
+ REQUIRE_FALSE(accepts("convert mpr:b -annotate 0 hi mpr:a"));
+ REQUIRE_FALSE(accepts("convert mpr:b -process x mpr:a"));
+ }
+ SECTION("+ spellings are rejected")
+ {
+ // ImageMagick's option table gives +resize an arity of 1 while
+ // ConvertImageCommand consumes zero arguments for it, so accepting + forms
+ // would desynchronize the position model from convert's actual parsing.
+ REQUIRE_FALSE(accepts("convert mpr:b +resize 128x128 mpr:a"));
+ REQUIRE_FALSE(accepts("convert mpr:b +flip mpr:a"));
+ }
+ SECTION("operators dropped from the allowlist")
+ {
+ // -minify is not an ImageMagick CLI option in either version, so it reaches
+ // UnrecognizedOption and fails the conversion.
+ REQUIRE_FALSE(accepts("convert mpr:b -minify mpr:a"));
+ // -format buys nothing (the output coder is fixed by the mpr:a filename) and
+ // its value reaches InterpretImageProperties, which handles @path and %[fx:].
+ REQUIRE_FALSE(accepts("convert mpr:b -format png mpr:a"));
+ REQUIRE_FALSE(accepts("convert mpr:b -format \"%[fx:1]\" mpr:a"));
+ }
+}
+
+TEST_CASE("ValidateMagickArguments rejects MagickCommandGenesis pre-scanned tokens")
+{
+ // MagickCommandGenesis walks argv[1..argc-2] with no concept of option arity
+ // before ConvertImageCommand ever runs, so a value position is reachable as an
+ // option. These two commands satisfy the shape and arity rules exactly and must
+ // still be rejected on the value.
+ SECTION("regression: -distribute-cache carried in a value position")
+ {
+ REQUIRE_FALSE(accepts("convert mpr:b -format -distribute-cache mpr:a"));
+ REQUIRE_FALSE(accepts("convert mpr:b -colorspace -distribute-cache mpr:a"));
+ REQUIRE_FALSE(accepts("convert mpr:b -quality -distribute-cache mpr:a"));
+ REQUIRE_FALSE(accepts("convert mpr:b -background -distribute-cache mpr:a"));
+ REQUIRE_FALSE(accepts("convert mpr:b -resize -distribute-cache mpr:a"));
+ }
+ SECTION("regression: -bench carried in a value position")
+ {
+ // -bench with a non-numeric value yields zero iterations, so Genesis returns
+ // its initial MagickTrue without ever running the command, defeating both the
+ // return-value check and the mandatory mpr:b input.
+ REQUIRE_FALSE(accepts("convert mpr:b -format -bench mpr:a"));
+ REQUIRE_FALSE(accepts("convert mpr:b -filter -bench mpr:a"));
+ REQUIRE_FALSE(accepts("convert mpr:b -rotate -bench mpr:a"));
+ }
+ SECTION("every Genesis-scanned token, in both spellings")
+ {
+ for (const std::string token : {"-bench", "-concurrent", "-debug", "-distribute-cache", "-duration", "-regard-warnings"}) {
+ REQUIRE_FALSE(accepts("convert mpr:b -colorspace " + token + " mpr:a"));
+ REQUIRE_FALSE(accepts("convert mpr:b -colorspace +" + token.substr(1) + " mpr:a"));
+ REQUIRE_FALSE(accepts("convert mpr:b " + token + " mpr:a"));
+ }
+ }
+}
+
+TEST_CASE("ValidateMagickArguments type-checks operator values")
+{
+ SECTION("integers")
+ {
+ REQUIRE(accepts("convert mpr:b -quality 90 mpr:a"));
+ REQUIRE_FALSE(accepts("convert mpr:b -quality 90x90 mpr:a"));
+ REQUIRE_FALSE(accepts("convert mpr:b -quality high mpr:a"));
+ REQUIRE_FALSE(accepts("convert mpr:b -depth @/etc/passwd mpr:a"));
+ }
+ SECTION("geometries reject letters, so no option name fits")
+ {
+ REQUIRE(accepts("convert mpr:b -resize 100x100! mpr:a"));
+ REQUIRE(accepts("convert mpr:b -resize 100x100^ mpr:a"));
+ REQUIRE_FALSE(accepts("convert mpr:b -resize abc mpr:a"));
+ // A geometry must contain a digit, so punctuation alone is not a value.
+ REQUIRE_FALSE(accepts("convert mpr:b -resize -- mpr:a"));
+ // @ is valid area geometry in ImageMagick but is excluded so a value can never
+ // become an @file reference.
+ REQUIRE_FALSE(accepts("convert mpr:b -resize @1000000 mpr:a"));
+ }
+ SECTION("enum tokens may not lead with a dash")
+ {
+ REQUIRE(accepts("convert mpr:b -gravity NorthWest mpr:a"));
+ REQUIRE_FALSE(accepts("convert mpr:b -gravity -NorthWest mpr:a"));
+ REQUIRE_FALSE(accepts("convert mpr:b -filter @file mpr:a"));
+ }
+ SECTION("colors")
+ {
+ REQUIRE(accepts("convert mpr:b -background rgb(255,0,0) mpr:a"));
+ REQUIRE(accepts("convert mpr:b -background gray50 mpr:a"));
+ REQUIRE_FALSE(accepts("convert mpr:b -background @/etc/passwd mpr:a"));
+ REQUIRE_FALSE(accepts("convert mpr:b -background -red mpr:a"));
+ }
+ SECTION("overlong values")
+ {
+ REQUIRE_FALSE(accepts("convert mpr:b -colorspace " + std::string(65, 'a') + " mpr:a"));
+ }
+}
+
+TEST_CASE("ValidateMagickArguments reports a reason")
+{
+ std::string reason;
+
+ REQUIRE_FALSE(accepts("convert mpr:b -write /tmp/x mpr:a", &reason));
+ REQUIRE(reason.find("-write") != std::string::npos);
+
+ reason.clear();
+ REQUIRE_FALSE(accepts("convert mpr:b -quality bad mpr:a", &reason));
+ REQUIRE(reason.find("-quality") != std::string::npos);
+
+ reason.clear();
+ REQUIRE(accepts("convert mpr:b -flip mpr:a", &reason));
+ REQUIRE(reason.empty());
+}