You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by ez...@apache.org on 2026/10/19 22:03:14 UTC
(trafficserver) 10/30: magick: require a signing key and allowlist convert operators
This is an automated email from the ASF dual-hosted git repository.
ezelkow1 pushed a commit to branch 9.2.x
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
commit 08085f8cd7b0c7a0a2c30027abeb3d65351884f5
Author: Evan Zelkowitz <e_...@apple.com>
AuthorDate: Wed Aug 26 17:22:22 2026 -0600
magick: require a signing key and allowlist convert operators
The plug-in treated an absent signing key as "verified", so a deployment
configured without a key ran client-supplied ImageMagick operators
unauthenticated. Refuse to load without a key, require the signature,
and accept only an allowlist of convert operators with typed values.
Client-controlled input reachable before signature verification now
returns a failure instead of asserting, and a failed conversion emits no
body and becomes an uncacheable 502 rather than serving whatever the
process-global MPR register still held.
---
plugins/experimental/magick/Makefile.inc | 13 +-
plugins/experimental/magick/README | 93 ++++++-
plugins/experimental/magick/magick.cc | 267 +++++++++++++++----
plugins/experimental/magick/magick_args.cc | 295 +++++++++++++++++++++
plugins/experimental/magick/magick_args.h | 43 +++
.../magick/unit_tests/test_magick_args.cc | 226 ++++++++++++++++
6 files changed, 866 insertions(+), 71 deletions(-)
diff --git a/plugins/experimental/magick/Makefile.inc b/plugins/experimental/magick/Makefile.inc
index b28b723396..978beead70 100644
--- a/plugins/experimental/magick/Makefile.inc
+++ b/plugins/experimental/magick/Makefile.inc
@@ -21,7 +21,8 @@ experimental_magick_magick_la_CXXFLAGS = $(AM_CXXFLAGS) -Wno-unused-variable
pkglib_LTLIBRARIES += experimental/magick/magick.la
experimental_magick_magick_la_SOURCES = \
- experimental/magick/magick.cc
+ experimental/magick/magick.cc \
+ experimental/magick/magick_args.cc
experimental_magick_magick_la_LDFLAGS = \
$(AM_LDFLAGS) \
@@ -30,3 +31,13 @@ experimental_magick_magick_la_LDFLAGS = \
experimental_magick_magick_la_LIBADD = \
-ltscppapi \
$(LIBMAGICKCPP_LIBS)
+
+# magick_args.cc depends only on <string>/<vector>, not on ImageMagick, so the
+# argument validator is unit testable even where ImageMagick is absent.
+check_PROGRAMS += experimental/magick/test_magick_args
+
+experimental_magick_test_magick_args_CPPFLAGS = $(AM_CPPFLAGS) -I$(abs_top_srcdir)/tests/include
+
+experimental_magick_test_magick_args_SOURCES = \
+ experimental/magick/unit_tests/test_magick_args.cc \
+ experimental/magick/magick_args.cc
diff --git a/plugins/experimental/magick/README b/plugins/experimental/magick/README
index 88da10ff53..c658b3490d 100644
--- a/plugins/experimental/magick/README
+++ b/plugins/experimental/magick/README
@@ -1,16 +1,74 @@
-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
+
+As a second layer of defense, even signature-verified requests are restricted to an allowlist.
+
+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.
+
+## 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 +80,27 @@ 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.
+The `test.sh` script, helps verifying ImageMagick's convert works. The script outputs the right query parameter for you. It does not consult the allowlist, so it will happily sign a command that the plug-in rejects.
-`
+## Examples
-##ImageMagick's convert examples:
+The `test.sh` script takes the operators only, wraps them in the required shape, and outputs the signed query parameters. It does not consult the allowlist, so it will happily sign a command the plug-in rejects.
-`convert input.jpg -flip 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 -rotate 90 output.jpg;`
+The commands those produce, which is what the "magick" query parameter carries once decoded:
-`convert input.jpg -resize 128x128 output.jpg;`
+```
+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
+```
-`convert input.jpg output.png;`
+A bare `convert input.jpg -flip output.jpg` is rejected: filenames in the input and
+output positions are not accepted, only the `mpr:b` / `mpr:a` registers.
diff --git a/plugins/experimental/magick/magick.cc b/plugins/experimental/magick/magick.cc
index 4efcbf7703..654a4f51b5 100644
--- a/plugins/experimental/magick/magick.cc
+++ b/plugins/experimental/magick/magick.cc
@@ -17,6 +17,7 @@
*/
#include <algorithm>
+#include <atomic>
#include <chrono>
#include <condition_variable>
#include <functional>
@@ -25,6 +26,7 @@
#include <mutex>
#include <string>
#include <vector>
+#include <string_view>
#include <cassert>
#include <cstring>
@@ -35,6 +37,8 @@
#include <ts/ts.h>
+#include "magick_args.h"
+
#include <openssl/err.h>
#include <openssl/evp.h>
#include <openssl/pem.h>
@@ -57,9 +61,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;
@@ -251,6 +254,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 {
@@ -273,13 +298,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
{
@@ -408,53 +451,31 @@ QueryParameterToCharVector(CharVector &v)
{
std::size_t s = 0;
const TSReturnCode rc = TSStringPercentDecode(v.data(), v.size(), v.data(), v.size(), &s);
- assert(TS_SUCCESS == rc);
+
+ // This input is client controlled and reachable before signature
+ // verification, so it must return a failure rather than assert.
+ if (rc != TS_SUCCESS) {
+ return false;
+ }
v.resize(s);
}
{
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;
+ }
v.resize(s);
}
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;
@@ -462,9 +483,42 @@ 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())
{
TSDebug(PLUGIN_TAG, "ImageTransform");
+ TransformationPlugin::registerHook(HOOK_SEND_RESPONSE_HEADERS);
+ }
+
+ /** A failed conversion must not be served as a 200. The client response headers
+ * still describe the origin image at this point, so rewrite them here.
+ */
+ 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
@@ -484,18 +538,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;
+ }
+
+ if (!wand.write("mpr:b")) {
+ this->fail("failed to write the input MPR register", wand.exceptionText());
+ return;
+ }
- const bool result = MagickCommandGenesis(image.info, ConvertImageCommand, this->argumentMap_.size(),
- this->argumentMap_.data(), nullptr, exception.info) == MagickTrue;
+ 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);
TSDebug(PLUGIN_TAG, "Background transformation is done, resuming continuation (%p)", this);
@@ -506,10 +590,43 @@ struct ImageTransform : TransformationPlugin {
TSDebug(PLUGIN_TAG, "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()) {
+ TSDebug(PLUGIN_TAG, "transformation failed (%p): %s [%s]", this, reason, url_.c_str());
+ TSError("[" PLUGIN_TAG "] %s: %s", reason, url_.c_str());
+ } else {
+ TSDebug(PLUGIN_TAG, "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 {
@@ -580,32 +697,59 @@ 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. Treating an absent
+ // key as "verified" would run client-supplied operators unauthenticated.
+ 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');
TSDebug(PLUGIN_TAG, "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);
- TSDebug(PLUGIN_TAG, "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');
+ TSDebug(PLUGIN_TAG, "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 {
+ TSDebug(PLUGIN_TAG, "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 {
TSDebug(PLUGIN_TAG, "signature verification failed.");
TSError("[" PLUGIN_TAG "] signature verification failed.");
t.setStatusCode(HTTP_STATUS_FORBIDDEN);
t.error();
+ // error() is a terminal reenable; see above.
+ return;
}
}
}
@@ -623,10 +767,17 @@ TSPluginInit(int argc, const char **argv)
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: refuse to load without a key. Without one, the signature check
+ // below is skipped and client-supplied ImageMagick operators would be run
+ // unauthenticated.
+ if (2 > argc) {
+ TSError("[" PLUGIN_TAG "] a public key file argument is required; refusing to load without a signing key");
+ TSDebug(PLUGIN_TAG, "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.
+ key = argv[1];
+
plugin = 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..aca3c8c6dd
--- /dev/null
+++ b/plugins/experimental/magick/magick_args.cc
@@ -0,0 +1,295 @@
+/**
+ 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/unit_tests/test_magick_args.cc b/plugins/experimental/magick/unit_tests/test_magick_args.cc
new file mode 100644
index 0000000000..d87f4b8a27
--- /dev/null
+++ b/plugins/experimental/magick/unit_tests/test_magick_args.cc
@@ -0,0 +1,226 @@
+/**
+ 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.
+ */
+
+#define CATCH_CONFIG_MAIN /* include main function */
+
+#include <catch.hpp> /* catch unit-test framework */
+
+#include "../magick_args.h"
+
+#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());
+}