https://bugs.kde.org/show_bug.cgi?id=524567
Bug ID: 524567
Summary: convertToQImage writes through a null pointer when an
ABR brush declares out-of-range bounds
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 195358
--> https://bugs.kde.org/attachment.cgi?id=195358&action=edit
07_abr_convertToQImage_line48.bin 51-byte ABR brush file — null
dereference in kis_abr_brush_collection.cpp:48
convertToQImage writes through a null pointer when an ABR brush declares
out-of-range bounds
Opening an ABR brush file whose bounds fields give a width and height that
QImage cannot allocate crashes Krita: `abr_brush_load_v12()` derives the
dimensions from the file's `left`/`right`/`top`/`bottom` and passes them to
`convertToQImage()` with only an upper bound on the height checked, the
`QImage` built there is null, and the pixel loop stores through the null
pointer `QImage::scanLine()` hands back.
## error log
```
AddressSanitizer:DEADLYSIGNAL
=================================================================
==27==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc
0x7f7107ada25b bp 0x7ffcdbc0fe10 sp 0x7ffcdbc0fd20 T0)
==27==The signal is caused by a WRITE memory access.
==27==Hint: address points to the zero page.
#0 0x7f7107ada25b in convertToQImage(char*, int, int)
/w/krita-6.0.3/libs/brush/kis_abr_brush_collection.cpp:48:22
#1 0x7f7107ada25b in
KisAbrBrushCollection::abr_brush_load_v12(QDataStream&, AbrInfo*, QString, int,
int) /w/krita-6.0.3/libs/brush/kis_abr_brush_collection.cpp:474:36
#2 0x7f7107adc507 in KisAbrBrushCollection::abr_brush_load(QDataStream&,
AbrInfo*, QString, int, int)
/w/krita-6.0.3/libs/brush/kis_abr_brush_collection.cpp:511:20
#3 0x7f7107ae12f1 in KisAbrBrushCollection::loadFromDevice(QIODevice*)
/w/krita-6.0.3/libs/brush/kis_abr_brush_collection.cpp:593:20
#4 0x558103a41bca in main /w/harness/h_brush.cpp:92:14
#5 0x7f71033a71c9 (/lib/x86_64-linux-gnu/libc.so.6+0x2a1c9) (BuildId:
328820b908de8ea1ef79afa8995e302e819163d7)
#6 0x7f71033a728a in __libc_start_main
(/lib/x86_64-linux-gnu/libc.so.6+0x2a28a) (BuildId:
328820b908de8ea1ef79afa8995e302e819163d7)
#7 0x5581039655e4 in _start (/w/harness/h_brush+0x2e5e4) (BuildId:
3d4bca5d48c4bd96e8648f4826cd656ad8b103a1)
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV
/w/krita-6.0.3/libs/brush/kis_abr_brush_collection.cpp:48:22 in
convertToQImage(char*, int, int)
==27==ABORTING
```
That is the whole report; the sanitizer had nothing to add for a fault at
address 0.
## context
* Krita 6.0.3, release tarball `krita-6.0.3.tar.gz`, sha256
`06397997a3b6700d5167068dc0f50cc356d82018611ed37087a1566790b721a8`, which is
tag `v6.0.3` (858d352). All line numbers below are that source's; they were not
re-checked against the development branch.
* Affected file: `libs/brush/kis_abr_brush_collection.cpp`.
* 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
`KisAbrBrushCollection::loadFromDevice()` is the same call Krita makes on an
`.abr` file, from `KisAbrBrushCollection::load()` at
`kis_abr_brush_collection.cpp:555`, which is what `KisAbrStorage` drives. 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 the attached file:
```
KRITA_FUZZ_FORMAT=abr ./h_brush 07_abr_convertToQImage_line48.bin
```
Which reader runs is decided by the file the user opens; here it is chosen by
the variable so one binary serves all three, and `abr` is also the default.
Given the wrong reader the bytes are not parsed and the program exits 0.
3. It is deterministic: on the build with neither the address sanitizer nor
coverage instrumentation, the process terminated on a signal 5 runs out of 5
against 6.0.3. That confirmation was made at the time against 6.0.3; the build
no longer exists here, so there is no more recent re-run to report.
## more
The sampled-brush path reads the bounds and the depth straight out of the file
and derives the buffer size from them:
```cpp
454: width = right - left;
455: height = bottom - top;
456: size = width * (depth >> 3) * height;
```
The only thing tested is an upper bound on the height:
```cpp
458: /* FIXME: support wide brushes */
459: if (height > 16384) {
```
`width` is not checked at all, neither dimension is required to be positive,
and nothing checks that the reads that produced them succeeded.
In this reproducer the file's last four bytes are `right`, so `depth` and
`compression` are read past the end. `QDataStream` leaves `depth` at 0, and
`getChar()` at line 452 leaves `compression` untouched, that is uninitialised.
The four bounds read are `top = 0`, `left = 0x00303030`, `bottom = 0x00003030`,
`right = 0x30303030`, so `width` is 805306368 and `height` is 12336 — which
clears the 16384 test — and `size` is `width * (0 >> 3) * height`, that is 0.
`convertToQImage()` then builds an image of those dimensions and walks it:
```cpp
41: QImage img(width, height, QImage::Format_RGB32);
44: for (int y = 0; y < height; y++) {
45: QRgb *pixel = reinterpret_cast<QRgb *>(img.scanLine(y));
46: for (int x = 0; x < width; x++, pos++) {
47: value = 255 - buffer[pos];
48: pixel[x] = qRgb(value, value , value);
```
One scanline of 805306368 RGB32 pixels is 3221225472 bytes, more than Qt
accepts for a scanline, so `img` is null. `scanLine()` returns a null pointer
for a null image, the loop bounds are the parameters rather than `img.width()`
and `img.height()` so the body runs anyway, and line 48 stores at address 0 —
the write to the zero page in the trace. The image is not tested with
`isNull()` after it comes back either: `brushTipImage.save()` at line 482 and
`setBrushTipImage()` at line 486 are reached with it.
Two guards cover this. `convertToQImage()` should not walk an image it failed
to create:
```diff
QImage img(width, height, QImage::Format_RGB32);
+ if (img.isNull()) {
+ return img;
+ }
int pos = 0;
```
and `abr_brush_load_v12()` should skip the brush whenever the header did not
parse into usable dimensions, rather than only when it is tall:
```diff
- /* FIXME: support wide brushes */
- if (height > 16384) {
- warnKrita << "WARNING: wide brushes not supported";
+ if (abr.status() != QDataStream::Ok
+ || width <= 0 || height <= 0 || height > 16384) {
+ warnKrita << "WARNING: unusable brush dimensions, skipping";
abr.device()->seek(next_brush);
}
```
The `abr.status()` test also covers `compression`, which line 466 uses whether
or not line 452 managed to read it.
--
You are receiving this mail because:
You are watching all bug changes.
