https://bugs.kde.org/show_bug.cgi?id=524564
Bug ID: 524564
Summary: KisGbrBrush::init and KisImagePipeBrush::initFromData
crash on GBR and GIH files with unchecked header
fields
Classification: Applications
Product: krita
Version First 6.0.3
Reported In:
Platform: Ubuntu
OS: Linux
Status: REPORTED
Severity: crash
Priority: NOR
Component: Resource Management
Assignee: [email protected]
Reporter: [email protected]
Target Milestone: ---
Created attachment 195356
--> https://bugs.kde.org/attachment.cgi?id=195356&action=edit
Please see the end of the Description text
KisGbrBrush::init and KisImagePipeBrush::initFromData crash on GBR and GIH
files with unchecked header fields
Attach: 01_gbr_overflow_mask_line236.bin (45 bytes),
02_gbr_overflow_color_line256.bin (37 bytes), 04_gbr_name_underflow_line188.bin
(28 bytes), 11_gih_lastBrush_empty_pipe.bin (2 bytes)
Loading a GBR or GIH brush whose header declares more data than the file
contains crashes Krita: the two size guards in `KisGbrBrush::init()` compute
the expected size in 32-bit arithmetic and then cast it to `qint32`, so any
expected size above `INT_MAX` reads as negative and the guard passes; the brush
name length in the same function is computed by an unchecked subtraction; and
`KisImagePipeBrush::initFromData()` keys its tail off the brush count written
in the file rather than the number of brushes it actually parsed.
| # | reproducer (format) | crash site | what the sanitizer says |
|---|---|---|---|
| 1 | `01_gbr_overflow_mask_line236.bin` (GBR) |
`libs/brush/kis_gbr_brush.cpp:236` | heap-buffer-overflow, read of size 1 |
| 2 | `02_gbr_overflow_color_line256.bin` (GBR) |
`libs/brush/kis_gbr_brush.cpp:256` | heap-buffer-overflow, read of size 1 |
| 3 | `04_gbr_name_underflow_line188.bin` (GBR) |
`libs/brush/kis_gbr_brush.cpp:188` | allocation of 0xffffffffffffffff bytes
requested |
| 4 | `11_gih_lastBrush_empty_pipe.bin` (GIH) |
`libs/brush/kis_brushes_pipe.h:47` | SEGV on address 0x18, write |
## error log
Trailing frames are cut at the `...` marks; every line shown is as printed.
```
=== 01_gbr_overflow_mask_line236.bin (KRITA_FUZZ_FORMAT=gbr)
==12==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x5070000236f6
at pc 0x7fa18f7e4849 bp 0x7ffca698f2b0 sp 0x7ffca698f2a8
READ of size 1 at 0x5070000236f6 thread T0
#0 0x7fa18f7e4848 in QByteArray::at(int) const
/usr/include/x86_64-linux-gnu/qt5/QtCore/qbytearray.h:500:47
#1 0x7fa18f7e4848 in KisGbrBrush::init()
/w/krita-6.0.3/libs/brush/kis_gbr_brush.cpp:236:63
#2 0x7fa18f7e68fe in KisGbrBrush::loadFromDevice(QIODevice*,
QSharedPointer<KisResourcesInterface>)
/w/krita-6.0.3/libs/brush/kis_gbr_brush.cpp:129:12
#3 0x55b3d42528c5 in main /w/harness/h_brush.cpp:81:15
...
0x5070000236f6 is located 0 bytes after 70-byte region
[0x5070000236b0,0x5070000236f6)
=== 02_gbr_overflow_color_line256.bin (KRITA_FUZZ_FORMAT=gbr)
==15==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x50600009611e
at pc 0x7f1bab48e819 bp 0x7ffc14ea8850 sp 0x7ffc14ea8848
READ of size 1 at 0x50600009611e thread T0
#0 0x7f1bab48e818 in QByteArray::at(int) const
/usr/include/x86_64-linux-gnu/qt5/QtCore/qbytearray.h:500:47
#1 0x7f1bab48e818 in KisGbrBrush::init()
/w/krita-6.0.3/libs/brush/kis_gbr_brush.cpp:256:40
#2 0x7f1bab4908fe in KisGbrBrush::loadFromDevice(QIODevice*,
QSharedPointer<KisResourcesInterface>)
/w/krita-6.0.3/libs/brush/kis_gbr_brush.cpp:129:12
#3 0x55ea8a89f8c5 in main /w/harness/h_brush.cpp:81:15
...
0x50600009611e is located 0 bytes after 62-byte region
[0x5060000960e0,0x50600009611e)
=== 04_gbr_name_underflow_line188.bin (KRITA_FUZZ_FORMAT=gbr)
==18==ERROR: AddressSanitizer: requested allocation size 0xffffffffffffffff
(0x800 after adjustments for alignment, red zones etc.) exceeds maximum
supported size of 0x10000000000 (thread T0)
...
#2 0x7f721cda4717 in QString::QString(int, Qt::Initialization)
(/lib/x86_64-linux-gnu/libQt5Core.so.5+0x164717) (BuildId:
0fb7b71b7cd236a3ac7fefcc17069ec1f5a93d5f)
...
#4 0x7f721cda91c8 in QString::fromUtf8_helper(char const*, int)
(/lib/x86_64-linux-gnu/libQt5Core.so.5+0x1691c8) (BuildId:
0fb7b71b7cd236a3ac7fefcc17069ec1f5a93d5f)
#5 0x7f7220dd022e in QString::fromUtf8(char const*, int)
/usr/include/x86_64-linux-gnu/qt5/QtCore/qstring.h:703:16
#6 0x7f7220dd022e in KisGbrBrush::init()
/w/krita-6.0.3/libs/brush/kis_gbr_brush.cpp:188:16
#7 0x7f7220dd68fe in KisGbrBrush::loadFromDevice(QIODevice*,
QSharedPointer<KisResourcesInterface>)
/w/krita-6.0.3/libs/brush/kis_gbr_brush.cpp:129:12
...
=== 11_gih_lastBrush_empty_pipe.bin (KRITA_FUZZ_FORMAT=gih)
==33==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000018 (pc
0x7fbb33ca4e32 bp 0x7ffc4dfdf630 sp 0x7ffc4dfdf4e0 T0)
==33==The signal is caused by a WRITE memory access.
==33==Hint: address points to the zero page.
...
#3 0x7fbb33ca4e32 in QSharedPointer<KisGbrBrush>::ref() const
/usr/include/x86_64-linux-gnu/qt5/QtCore/qsharedpointer_impl.h:509:44
#4 0x7fbb33ca4e32 in
QSharedPointer<KisGbrBrush>::QSharedPointer(QSharedPointer<KisGbrBrush> const&)
/usr/include/x86_64-linux-gnu/qt5/QtCore/qsharedpointer_impl.h:333:14
#5 0x7fbb33ca4e32 in KisBrushesPipe<KisGbrBrush>::lastBrush() const
/w/krita-6.0.3/libs/brush/kis_brushes_pipe.h:47:16
#6 0x7fbb33ca4e32 in KisImagePipeBrush::initFromData(QByteArray const&)
/w/krita-6.0.3/libs/brush/kis_imagepipe_brush.cpp:328:35
#7 0x7fbb33ca322d in KisImagePipeBrush::loadFromDevice(QIODevice*,
QSharedPointer<KisResourcesInterface>)
/w/krita-6.0.3/libs/brush/kis_imagepipe_brush.cpp:273:12
...
```
In the first trace the 70-byte region is the `QByteArray` that
`KisGbrBrush::loadFromDevice()` fills from the device at
`kis_gbr_brush.cpp:128` — that is, the file's own bytes.
## context
* Krita 6.0.3, release tarball `krita-6.0.3.tar.gz`, sha256
`06397997a3b6700d5167068dc0f50cc356d82018611ed37087a1566790b721a8`. All line
numbers below are that tarball's; they were not re-checked against the
development branch.
* Affected files: `libs/brush/kis_gbr_brush.cpp`,
`libs/brush/kis_imagepipe_brush.cpp`, `libs/brush/kis_brushes_pipe.h`.
* Build: CMake, `-DCMAKE_BUILD_TYPE=RelWithDebInfo`, `-DBUILD_WITH_QT6=OFF`,
`-DBUILD_TESTING=OFF`, `-DBUILD_COVERAGE=OFF`, `-DENABLE_CLAZY=OFF`,
`-DENABLE_BSYMBOLICFUNCTIONS=OFF`, `CMAKE_CXX_FLAGS='-fno-omit-frame-pointer
-g'`.
* Platform: x86_64 Linux, Ubuntu 24.04, Qt 5.15.13, KF5 5.115.
* Two builds were used: one built through AFL++'s clang wrapper with
`AFL_USE_ASAN=1`, which produced the output above, and one built with
`/usr/bin/clang++` with neither the address sanitizer nor coverage
instrumentation, used for confirmation.
## how to reproduce
1. Build Krita 6.0.3 as above and compile the program below against
`libs/brush`. It is the whole test program, quoted here in full apart from its
header comment: it defines no Krita code, and `loadFromDevice()` is the same
call Krita makes on a brush file at `libs/resources/KisFolderStorage.cpp:116`.
Add `-fno-operator-names` to the compile line; Krita's own compile lines carry
it and the program does not build against the Qt headers without it.
```cpp
#include <QtGlobal>
#include <QGuiApplication>
#include <QBuffer>
#include <QByteArray>
#include <QFile>
#include <QString>
#include <kis_abr_brush_collection.h>
#include <kis_gbr_brush.h>
#include <kis_imagepipe_brush.h>
#include <KisGlobalResourcesInterface.h>
#include <cstdio>
#include <cstdlib>
#include <cstring>
#ifdef __AFL_HAVE_MANUAL_CONTROL
# define FUZZ_INIT() __AFL_INIT()
#else
# define FUZZ_INIT() do { } while (0)
#endif
enum Format { FMT_ABR, FMT_GBR, FMT_GIH };
static Format format_from_env()
{
const char *f = getenv("KRITA_FUZZ_FORMAT");
if (f && strcmp(f, "gbr") == 0) return FMT_GBR;
if (f && strcmp(f, "gih") == 0) return FMT_GIH;
return FMT_ABR;
}
int main(int argc, char **argv)
{
// Krita's brush code builds QImages, so it needs a QGuiApplication. The
// offscreen platform keeps that off any display.
qputenv("QT_QPA_PLATFORM", "offscreen");
qputenv("QT_LOGGING_RULES", "*=false");
QGuiApplication app(argc, argv);
const Format fmt = format_from_env();
FUZZ_INIT();
if (argc < 2) {
fprintf(stderr, "usage: %s <file> (KRITA_FUZZ_FORMAT=abr|gbr|gih)\n",
argv[0]);
return 1;
}
QFile in(QString::fromLocal8Bit(argv[1]));
if (!in.open(QIODevice::ReadOnly))
return 0;
QByteArray bytes = in.readAll();
in.close();
QBuffer dev(&bytes);
if (!dev.open(QIODevice::ReadOnly))
return 0;
// The filename passed to the constructor is only a label Krita carries on
// the resource; the bytes come from the device.
switch (fmt) {
case FMT_GBR: {
KisGbrBrush brush(QStringLiteral("fuzz.gbr"));
brush.loadFromDevice(&dev, KisGlobalResourcesInterface::instance());
break;
}
case FMT_GIH: {
KisImagePipeBrush brush(QStringLiteral("fuzz.gih"));
brush.loadFromDevice(&dev, KisGlobalResourcesInterface::instance());
break;
}
case FMT_ABR:
default: {
KisAbrBrushCollection coll(QStringLiteral("fuzz.abr"));
coll.loadFromDevice(&dev);
break;
}
}
return 0;
}
```
2. Run it on each attached file:
```
KRITA_FUZZ_FORMAT=gbr ./h_brush 01_gbr_overflow_mask_line236.bin
KRITA_FUZZ_FORMAT=gbr ./h_brush 02_gbr_overflow_color_line256.bin
KRITA_FUZZ_FORMAT=gbr ./h_brush 04_gbr_name_underflow_line188.bin
KRITA_FUZZ_FORMAT=gih ./h_brush 11_gih_lastBrush_empty_pipe.bin
```
3. `KRITA_FUZZ_FORMAT` picks the reader, exactly as the file's extension does
in Krita: the three files named `gbr` are GBR brushes and go through
`KisGbrBrush::loadFromDevice()`, the one named `gih` is a GIH brush pipe and
goes through `KisImagePipeBrush::loadFromDevice()`. Given the wrong reader the
bytes are not parsed and the program exits 0.
All four are deterministic: on the build with neither the address sanitizer nor
coverage instrumentation, each one terminated on a signal 5 runs out of 5
against 6.0.3.
## more
**Crashes 1 and 2 — the expected-size guards go negative.** `bh.width` and
`bh.height` are `quint32` (`kis_gbr_brush.cpp:45-46`), so the expected size is
computed in 32-bit unsigned arithmetic and then narrowed to a signed 32-bit
value before the comparison:
```cpp
223: if (static_cast<qint32>(k + bh.width * bh.height) > d->data.size())
{
```
```cpp
244: if (static_cast<qint32>(k + (bh.width * bh.height * 4)) >
d->data.size()) {
```
Reproducer 1 declares `width = 0x00ff3030` (16,724,016) and `height =
0x000030f4` (12,532) with `bytes = 1`. The product is 209,585,368,512, which
wraps to 3,426,938,304; adding `k` (the 45-byte `header_size`) and casting to
`qint32` gives −868,028,947, so the guard passes on a 45-byte file. The pixel
loop then walks off the array:
```cpp
233: for (quint32 y = 0; y < bh.height; y++) {
234: uchar *pixel = reinterpret_cast<uchar *>(image.scanLine(y));
235: for (quint32 x = 0; x < bh.width; x++, k++) {
236: qint32 val = 255 - static_cast<uchar>(d->data.at(k));
```
Reproducer 2 is the same guard on the `bytes == 4` path, and it does not even
need the multiplication to wrap: `width = 0x01303030` (19,935,280) and `height
= 48` give `width * height * 4 = 3,827,573,760`, which fits in `quint32` but is
−467,393,502 once `k` is added and the sum is cast to `qint32`. Line 256 then
reads four bytes at a time past the end of a 37-byte file. Doing the arithmetic
in 64 bits fixes both:
```diff
- if (static_cast<qint32>(k + bh.width * bh.height) > d->data.size()) {
+ if (qint64(k) + qint64(bh.width) * qint64(bh.height) >
qint64(d->data.size())) {
```
```diff
- if (static_cast<qint32>(k + (bh.width * bh.height * 4)) >
d->data.size()) {
+ if (qint64(k) + qint64(bh.width) * qint64(bh.height) * 4 >
qint64(d->data.size())) {
```
**Crash 3 — the name length underflows.** The only check on `header_size` is an
upper bound:
```cpp
172: if (bh.header_size > (uint)d->data.size() || bh.header_size == 0) {
```
Nothing requires `header_size` to be at least as large as the header it
describes, and the name length is then computed by subtraction:
```cpp
188: name = QString::fromUtf8(d->data.constData() +
sizeof(GimpBrushHeader),
189: bh.header_size - sizeof(GimpBrushHeader) -
1);
```
Reproducer 3 is 28 bytes with `header_size = 25`, and `sizeof(GimpBrushHeader)`
is 28, so `25 - 28 - 1` is evaluated in unsigned 64-bit arithmetic as
0xfffffffffffffffc and narrows to −4 in the `int` length parameter of
`QString::fromUtf8`. The trace shows that reaching `QString::QString(int,
Qt::Initialization)` and asking the allocator for 0xffffffffffffffff bytes.
Line 183 has the same subtraction for version-1 files with `GimpBrushV1Header`.
A lower bound on `header_size` covers both, and also subsumes the existing `==
0` test:
```diff
- if (bh.header_size > (uint)d->data.size() || bh.header_size == 0) {
+ const quint32 minHeaderSize = bh.version == 1
+ ? quint32(sizeof(GimpBrushV1Header))
+ : quint32(sizeof(GimpBrushHeader));
+
+ if (bh.header_size > (uint)d->data.size() || bh.header_size <=
minHeaderSize) {
```
**Crash 4 — an empty brush pipe is still dereferenced.** `initFromData()` reads
a brush count from the file's second line, loads brushes while bytes remain,
and then keys the whole tail of the function off the declared count rather than
off what was loaded:
```cpp
316: for (int brushIndex = d->brushesPipe.sizeBrush();
317: brushIndex < numOfBrushes && i < data.size(); brushIndex++) {
```
```cpp
326: if (numOfBrushes > 0) {
327: setValid(true);
328: setSpacing(d->brushesPipe.lastBrush()->spacing());
```
Reproducer 4 is the two bytes `0a 38` — an empty name line, then `8` as the
brush count. After the two line scans `i` is 3 and `data.size()` is 2, so the
loop body never runs and no brush is added, but `numOfBrushes` is 8 and line
328 calls `lastBrush()`, which is an unguarded `QVector::last()`:
```cpp
46: QSharedPointer<BrushType> lastBrush() const {
47: return m_brushes.last();
48: }
```
On an empty vector that reads a `QSharedPointer` from before the first element;
copying it out increments a reference count through the pointer it holds, which
is the write to 0x18 the sanitizer reports. Testing what was parsed instead of
what was declared fixes it, and also makes a truncated pipe a clean load
failure:
```diff
- if (numOfBrushes > 0) {
+ if (d->brushesPipe.sizeBrush() > 0) {
setValid(true);
setSpacing(d->brushesPipe.lastBrush()->spacing());
...
}
- return true;
+ return d->brushesPipe.sizeBrush() > 0;
}
```
A guard in `lastBrush()` and `firstBrush()` themselves would be worth having
too, since both call `QVector::last()`/`QVector::first()` with no emptiness
check and are reachable from other callers.
file description
01_gbr_overflow_mask_line236.bin 45-byte GBR brush file — out-of-bounds
read in kis_gbr_brush.cpp:236
02_gbr_overflow_color_line256.bin 37-byte GBR brush file — out-of-bounds
read in kis_gbr_brush.cpp:256
04_gbr_name_underflow_line188.bin 28-byte GBR brush file — name length
underflows to a 16 EiB allocation, kis_gbr_brush.cpp:188
11_gih_lastBrush_empty_pipe.bin 2-byte GIH brush file — empty brush pipe
dereferenced in kis_brushes_pipe.h:47
--
You are receiving this mail because:
You are watching all bug changes.
