Discovering an SQL injection in Firefish
2025/03/06 note:
We've originally planned to show the actual payloads, however we believe it would be somewhat irresponsible showing genuine destructive payloads now, so we may add them in a month or two, when we can be *sure* everyone has migrated to a version that is not impacted with this vulnerability.
If you're reading this and run Firefish and/or Iceshrimp.js, please update _immediately_.
We work on our own ActivityPub-enabled software in my free time. It is called Magnetar.
Iceshrimp.js, mentioned in this article, shares some legacy code from Firefish (f.k.a. Calckey, both abandonned) and is maintained by Laura (zotan).
Some parts of the code had its indentation reduced for readability.
Introduction
On the 5th of March, I started my day trying to look into transcoding of images, possibly trimming some more legacy code from the Firefish codebase we hard forked and slowly rewrite bit-by-bit, since we “dog-food” Magnetar.
Skimming through the codebase, the safeForSql
function caught my eye pretty much immediately. Not necessarily because it'd be wrong, but because it is a strong canary of a very smelly codebase.
It is a very simple one-line predicate that doesn't do anything interesting on its own except checking for the presence of any character that is not safe interpolating into an SQL query string.
export function safeForSql(text: string): boolean {
return !/[\0\x08\x09\x1a\n\r"'\\\%]/g.test(text);
}
We quickly skimmed through all — well — two usages, and found the very unpleasant-looking endpoint /packages/backend/src/server/api/endpoints/notes/search-by-tag.ts
. It accepts either a single hashtag name, or string[][]
, where the outer array is combined using SQL OR
, and the inner one using SQL AND
.
The most interesting part looks like this:
if (ps.tag) {
if (!safeForSql(normalizeForSearch(ps.tag))) throw "Injection";
query.andWhere(`'{"${normalizeForSearch(ps.tag)}"}' <@ note.tags`);
} else {
query.andWhere(
new Brackets((qb) => {
for (const tags of ps.query!) {
qb.orWhere(
new Brackets((qb) => {
for (const tag of tags) {
if (!safeForSql(normalizeForSearch(ps.tag)))
throw "Injection";
qb.andWhere(`'{"${normalizeForSearch(tag)}"}' <@ note.tags`);
}
}),
);
}
}),
);
}
This immediately caught my eye. The safeForSql
condition accepts the wrong input in the else branch! However, the server uses a JSON-schema validator (the ubiquitous Ajv) and worst case the code would throw on a null-pointer error, because the tag
and query
request fields are supposed to be mutually exclusive.
anyOf: [
{
properties: {
tag: {type: "string", minLength: 1},
},
required: ["tag"],
},
{
properties: {
query: {
type: "array",
description:
"The outer arrays are chained with OR, the inner arrays are chained with AND.",
items: {
type: "array",
items: {
type: "string",
minLength: 1,
},
minItems: 1,
},
minItems: 1,
},
},
required: ["query"],
},
]
However, we quickly realized something... There is one possible input for the tag
field that is falsy
, but still passes the safeForSql
check.
The Realization
That input is an empty string. Ajv fails to accept the tag
branch, but happily accepts the request JSON in the query
branch, even if the tag
string is invalid, since its minimum length is supposed to be 1
.
At this point I contacted Laura and shared this finding with her.
After a bit of fiddling and fighting the request validator, we finally realized how the endpoint works, and crafted a payload.
curl --insecure
-H "Authorizaton: Bearer ..."
-X POST
--json '{"query": ["\""], "tag": " "}'
"<host>/api/notes/search-by-tag"
And immediately, boom.
ERR 1 [api] Internal error occurred in notes/search-by-tag: malformed array literal: "{"""}"
It seems that the code path was indeed accessible if we trick Ajv into accepting an empty string for the tag
field.
Possible payloads
Once we have discovered this SQL injection, we have realized the scope of the vulnerability. After some fights with brace pairing and syntax errors, we had a payload that could execute arbitrary SQL.
Our first idea was, well, what if we make the condition always true... Well, to show the scope of this exploit, we quickly wrote one that lists every single note in the database.
[to be disclosed later]
And since Laura reminded us this endpoint is fully unauthenticated, we tried this against our running instance and also filtered by note visibility, and yes, it allows arbitrary data reads and writes.
[to be disclosed later]
Laura did the calculations and our CVSS 9.7 unauthenticated SQL injection was born.
How did this happen?
Well, it seems that the change was introduced over two years ago, in commit 265701
with an extremely ironic commit message.
commit 26570158fd6720e66e1631822445d00959406066
Author: [REDACTED]
Date: Sat Feb 4 12:38:46 2023 -0800
fix: :lock: improve tag search security
diff --git a/packages/backend/src/server/api/endpoints/notes/search-by-tag.ts b/packages/backend/src/server/api/endpoints/notes/search-by-tag.ts
index 8cf9ce8fb0..8993237421 100644
--- a/packages/backend/src/server/api/endpoints/notes/search-by-tag.ts
+++ b/packages/backend/src/server/api/endpoints/notes/search-by-tag.ts
@@ -93,7 +93,7 @@ export default define(meta, paramDef, async (ps, me) => {
try {
if (ps.tag) {
- if (!safeForSql(ps.tag)) throw new Error("Injection");
+ if (!safeForSql(normalizeForSearch(ps.tag))) throw 'Injection';
query.andWhere(`'{"${normalizeForSearch(ps.tag)}"}' <@ note.tags`);
} else {
query.andWhere(
@@ -102,7 +102,7 @@ export default define(meta, paramDef, async (ps, me) => {
qb.orWhere(
new Brackets((qb) => {
for (const tag of tags) {
- if (!safeForSql(tag)) throw new Error("Injection");
+ if (!safeForSql(normalizeForSearch(ps.tag))) throw 'Injection';
qb.andWhere(`'{"${normalizeForSearch(tag)}"}' <@ note.tags`);
}
}),
Whoops.
Aftermath
To our knowledge, this SQL injection has not been exploited by anyone, especially since it resides in a very obscure branch and cannot be triggered accidentally, without crafting a JSON payload like that.
We have coordinated a fix for both Magnetar and Iceshrimp.js the very same day, with this patch for Magnetar:
if (ps.tag) {
- if (!safeForSql(normalizeForSearch(ps.tag))) throw "Injection";
- query.andWhere(`'{"${normalizeForSearch(ps.tag)}"}' <@ note.tags`);
+ query.andWhere(":tag = ANY(note.tags)", { tag: normalizeForSearch(ps.tag) });
} else {
query.andWhere(
new Brackets((qb) => {
for (const tags of ps.query!) {
- qb.orWhere(
- new Brackets((qb) => {
- for (const tag of tags) {
- if (!safeForSql(normalizeForSearch(ps.tag)))
- throw "Injection";
- qb.andWhere(`'{"${normalizeForSearch(tag)}"}' <@ note.tags`);
- }
- }),
- );
+ qb.orWhere("ARRAY[:...tags]::varchar[] <@ note.tags", { tags: tags.map(normalizeForSearch) });
}
}),
);
We entirely removed the need for the safeForSql
function, removing the possibility to mess up like this entirely.
Hopefully we're free from this kind of “jank” in Magnetar for good.
It's quite ironic this likely would've never been caught if we weren't writing Magnetar and didn't spot this.
...
Not to mention the tags
DB (Postgres) column for some reason uses a BTree index instead of a GIN index, which would allow for faster array operations.