https://bugs.kde.org/show_bug.cgi?id=524565
Bug ID: 524565
Summary: Compression::uncompress allocates the declared length
unchecked, and readVirtualArrayList writes into a null
QImage
Classification: Applications
Product: krita
Version First 6.0.3
Reported In:
Platform: Ubuntu
OS: Linux
Status: REPORTED
Severity: crash
Priority: NOR
Component: File formats
Assignee: [email protected]
Reporter: [email protected]
Target Milestone: ---
Created attachment 195357
--> https://bugs.kde.org/attachment.cgi?id=195357&action=edit
Please see the end of the Description text
Compression::uncompress allocates the declared length unchecked, and
readVirtualArrayList writes into a null QImage
Attach: 12_psd_rle_excessive_alloc_line90.bin (14 bytes),
13_psd_zip_excessive_alloc_line367.bin (16 bytes),
09_asl_readVirtualArrayList_line427.bin (16,632 bytes)
Krita's `libs/psdutils` code uses sizes that come out of the file without
checking them against the bytes that are actually there:
`Compression::uncompress()` hands the declared uncompressed length straight to
`QByteArray` as an allocation size, and `readVirtualArrayList()` sizes a
pattern `QImage` from a rectangle read out of an `.asl` file and then writes
into that image without checking it was allocated, so a declared length of
`0x7fffffff` or an inverted rectangle crashes the process.
| # | reproducer | crash site | what the sanitizer says |
|---|---|---|---|
| 1 | `12_psd_rle_excessive_alloc_line90.bin` (14 bytes) |
`libs/psdutils/compression.cpp:90`, `KisRLE::decompress` |
allocation-size-too-big, 0xffffffffffffffff bytes requested |
| 2 | `13_psd_zip_excessive_alloc_line367.bin` (16 bytes) |
`libs/psdutils/compression.cpp:367`, `KisZip::decompress` |
allocation-size-too-big, 0xffffffffffffffff bytes requested |
| 3 | `09_asl_readVirtualArrayList_line427.bin` (16,632 bytes) |
`libs/psdutils/asl/kis_asl_reader.cpp:427`, `readVirtualArrayList` | SEGV on
address 0x0, write |
## error log
```
=== 12_psd_rle_excessive_alloc_line90.bin
==15==ERROR: AddressSanitizer: requested allocation size 0xffffffffffffffff
(0x800 after
adjustments for alignment, red zones etc.) exceeds maximum supported size of
0x10000000000 (thread T0)
#0 malloc
#1 QArrayData::allocate(unsigned long, unsigned long, unsigned long,
QFlags<QArrayData::AllocationOption>)
#2 QByteArray::resize(int)
#3 KisRLE::decompress(QByteArray const&, int)
krita-6.0.3/libs/psdutils/compression.cpp:90:12
#4 Compression::uncompress(int, QByteArray, psd_compression_type, int, int)
krita-6.0.3/libs/psdutils/compression.cpp:385:16
#5 main h_psd.cpp:93:11
SUMMARY: AddressSanitizer: allocation-size-too-big in malloc
=== 13_psd_zip_excessive_alloc_line367.bin
==18==ERROR: AddressSanitizer: requested allocation size 0xffffffffffffffff
(0x800 after
adjustments for alignment, red zones etc.) exceeds maximum supported size of
0x10000000000 (thread T0)
#0 malloc
#1 QArrayData::allocate(unsigned long, unsigned long, unsigned long,
QFlags<QArrayData::AllocationOption>)
#2 QByteArray::QByteArray(int, char)
#3 KisZip::decompress(QByteArray const&, int)
krita-6.0.3/libs/psdutils/compression.cpp:367:16
#4 Compression::uncompress(int, QByteArray, psd_compression_type, int, int)
krita-6.0.3/libs/psdutils/compression.cpp:387:16
#5 KisZip::psd_unzip_with_prediction(QByteArray const&, int, int, int)
krita-6.0.3/libs/psdutils/compression.cpp:287:26
#6 Compression::uncompress(int, QByteArray, psd_compression_type, int, int)
krita-6.0.3/libs/psdutils/compression.cpp:389:16
#7 main h_psd.cpp:93:11
SUMMARY: AddressSanitizer: allocation-size-too-big in malloc
=== 09_asl_readVirtualArrayList_line427.bin
==36==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc
0x7f21054493c7 T0)
==36==The signal is caused by a WRITE memory access.
==36==Hint: address points to the zero page.
#0 QImage Private::readVirtualArrayList<(psd_byte_order)0>(QIODevice&, int,
QVector<unsigned int> const&)
krita-6.0.3/libs/psdutils/asl/kis_asl_reader.cpp:427:27
#1 long long Private::readPattern<(psd_byte_order)0>(QIODevice&,
QDomElement*, QDomDocument*)
krita-6.0.3/libs/psdutils/asl/kis_asl_reader.cpp:582:31
#2 Private::readFileImpl(QIODevice&)
krita-6.0.3/libs/psdutils/asl/kis_asl_reader.cpp:649:36
#3 KisAslReader::readFile(QIODevice&)
krita-6.0.3/libs/psdutils/asl/kis_asl_reader.cpp:700:15
#4 main h_asl.cpp:57:35
SUMMARY: AddressSanitizer: SEGV in readVirtualArrayList
```
## context
* Krita 6.0.3, git tag `v6.0.3`, commit
`858d352e52e68831693067763b9cdaf8bb9a05ce`; release tarball
`krita-6.0.3.tar.gz`, sha256
`06397997a3b6700d5167068dc0f50cc356d82018611ed37087a1566790b721a8`. All line
numbers below are that tag's.
* Affected files: `libs/psdutils/compression.cpp`,
`libs/psdutils/asl/kis_asl_reader.cpp`. The lengths involved are computed by
the callers in `libs/psd/psd_pixel_utils.cpp`.
* Build: CMake + Ninja, `-DCMAKE_BUILD_TYPE=RelWithDebInfo`,
`-DBUILD_WITH_QT6=OFF` (so Qt 5), `-DBUILD_TESTING=OFF`,
`-DBUILD_COVERAGE=OFF`, `-DENABLE_CLAZY=OFF`,
`-DENABLE_BSYMBOLICFUNCTIONS=OFF`, `CXXFLAGS='-g -fno-omit-frame-pointer'`.
* Platform: x86_64 Linux, Ubuntu 24.04, clang 18, Qt 5.15.13, KF5 5.115.
* Two builds were used: one with clang's address sanitizer for the output
above, and one with neither the address sanitizer nor coverage instrumentation
for confirmation.
## how to reproduce
Reproducers 1 and 2 go through `Compression::uncompress()` directly, not
through the `.psd` importer — the five arguments that a document normally
supplies are packed into the reproducer file so each one is controlled
explicitly. Reproducer 3 is an `.asl` file read through the public
`KisAslReader::readFile()`, the same entry point Krita's own ASL tests call.
1. Build Krita 6.0.3 as above.
2. Compile this program against `libs/psdutils` (it is the whole test program;
it defines no Krita code):
```cpp
#include <compression.h>
#include <psd.h>
#include <QByteArray>
#include <cstdint>
#include <fstream>
#include <sstream>
static int32_t le32(const unsigned char *p)
{
return static_cast<int32_t>(static_cast<uint32_t>(p[0]) |
(static_cast<uint32_t>(p[1]) << 8)
| (static_cast<uint32_t>(p[2]) << 16) |
(static_cast<uint32_t>(p[3]) << 24));
}
int main(int argc, char **argv)
{
if (argc < 2) return 1;
std::ifstream in(argv[1], std::ios::in | std::ios::binary);
if (!in) return 0;
std::ostringstream buf; buf << in.rdbuf();
const std::string s = buf.str();
if (s.size() < 14) return 0;
// bytes 0..3 unpacked_len, byte 4 compression type, bytes 5..8 row_size,
// bytes 9..12 color_depth, bytes 13.. the compressed payload
const unsigned char *p = reinterpret_cast<const unsigned char *>(s.data());
static const psd_compression_type kTypes[4] = { Uncompressed, RLE, ZIP,
ZIPWithPrediction };
QByteArray payload(reinterpret_cast<const char *>(p + 13),
static_cast<int>(s.size() - 13));
(void)Compression::uncompress(le32(p), payload, kTypes[p[4] & 0x03], le32(p
+ 5), le32(p + 9));
return 0;
}
```
The type selector is reduced modulo four on purpose:
`Compression::uncompress()` ends in `default: qFatal("Cannot uncompress layer
data: invalid compression type")`, which is Krita deliberately refusing an
unknown type and is not a defect.
3. Compile a second program that calls the ASL reader on the file's bytes —
`QGuiApplication app(argc, argv);` with `QT_QPA_PLATFORM=offscreen`, then a
`QBuffer` over the file contents, then `KisAslReader().readFile(dev)` inside a
`try { } catch (...) { return 0; }` so that `ASLParseException`, which is Krita
correctly rejecting a malformed file, is not counted as a crash.
4. Run each on its reproducer:
```
./h_psd 12_psd_rle_excessive_alloc_line90.bin
./h_psd 13_psd_zip_excessive_alloc_line367.bin
./h_asl 09_asl_readVirtualArrayList_line427.bin
```
All three are deterministic: on the build with neither the address sanitizer
nor coverage instrumentation, each one crashed 5 runs out of 5 against 6.0.3.
## more
**Crashes 1 and 2 — the declared uncompressed length is the allocation size.**
`Compression::uncompress()` checks the compressed input, never the length the
document claims it expands to:
```cpp
376:QByteArray Compression::uncompress(int unpacked_len, QByteArray bytes,
psd_compression_type compressionType, int row_size, int color_depth)
377:{
378: if (bytes.size() < 1)
379: return QByteArray();
380:
381: switch (compressionType) {
```
That length reaches `QByteArray` unaltered in both decompressors:
```cpp
87:QByteArray decompress(const QByteArray &input, int unpacked_len)
88:{
89: QByteArray output;
90: output.resize(unpacked_len);
```
```cpp
365:QByteArray decompress(const QByteArray &data, int expected_length)
366:{
367: QByteArray output(expected_length, '\0');
```
Both reproducers declare `unpacked_len = 0x7fffffff` with a payload of 1 and 3
bytes; reproducer 1 selects RLE, reproducer 2 selects ZIPWithPrediction, which
is why its trace passes through `psd_unzip_with_prediction` at line 287 before
reaching line 367. `0x7fffffff` is past what Qt will allocate, so Qt's
block-size calculation saturates and `malloc` is called with
`0xffffffffffffffff`.
The callers compute that length from the layer rectangle in the document, in
32-bit arithmetic that can also wrap:
```cpp
303: const int uncompressedLength = width * channelSize;
```
```cpp
352: const int numPixels = channelSize * layerRect.width() *
layerRect.height();
```
(`libs/psd/psd_pixel_utils.cpp`, feeding lines 320 and 361 respectively.) We
drove `Compression::uncompress()` directly rather than through those two call
sites, so the sizes come from the reproducer rather than from a `.psd` file,
but the parameter is the same one and nothing between the file and the
allocation bounds it. Bounding it once, where every reader meets, fixes both
crash sites:
```diff
QByteArray Compression::uncompress(int unpacked_len, QByteArray bytes,
psd_compression_type compressionType, int row_size, int color_depth)
{
if (bytes.size() < 1)
return QByteArray();
+ // The unpacked length is a field of the document. Check it against what
the
+ // compressed input could possibly expand to before it becomes an
allocation
+ // size: deflate's worst case is 1032:1, packbits' is 64:1.
+ static constexpr qint64 maxExpansionRatio = 1032;
+ if (unpacked_len <= 0 || qint64(unpacked_len) > qint64(bytes.size()) *
maxExpansionRatio)
+ return QByteArray();
+
switch (compressionType) {
```
Making `numPixels` and `uncompressedLength` `qint64` at the two call sites
above is worth doing as well, so a large layer rectangle cannot turn a valid
length negative before it gets here.
**Crash 3 — the pattern image is written into whether or not it was
allocated.** The rectangle is built from four `quint32` read out of the file,
with nothing checked:
```cpp
256: SAFE_READ_EX(byteOrder, device, y0);
257: SAFE_READ_EX(byteOrder, device, x0);
258: SAFE_READ_EX(byteOrder, device, y1);
259: SAFE_READ_EX(byteOrder, device, x1);
260: QRect arrayRect(x0, y0, x1 - x0, y1 - y0);
```
The reproducer declares `y0 = 0x30000000`, `x0 = 0`, `y1 = 128`, `x1 = 128`, so
`arrayRect` is 128 wide and `128 - 805306368 = -805306240` high. `QImage`
cannot be built from that size, so it is null and `image.bits()` is a null
pointer:
```cpp
405: QImage image(arrayRect.size(), format);
...
412: const int dataLength = arrayRect.width() * arrayRect.height();
413:
414: if (format == QImage::Format_ARGB32) {
415: quint8 *dstPtr = image.bits();
```
The loop bound is then computed from the same unchecked rectangle: `128 *
-805306240` is `-103079198720`, which truncates to `16384` in `int`, so the
loop runs 16,384 times and the first store is the SEGV at address 0:
```cpp
418: for (int i = 0; i < dataLength; i++) {
419: for (int j = 2; j >= 0; j--) {
...
427: *dstPtr++ = dataPlanes[plane][i];
```
The same truncation is what gets the file past the plane check: `dataLength` at
line 337 wraps to 16,384 too, the file supplies exactly 16,384 uncompressed
bytes for its single plane, and the equality test at line 380 passes. Rejecting
the rectangle where it is read stops all of it:
```diff
QRect arrayRect(x0, y0, x1 - x0, y1 - y0);
+ if (!arrayRect.isValid() || arrayRect.isEmpty()) {
+ throw ASLParseException("VAList: pattern rectangle is empty or
inverted!");
+ }
+
quint32 numberOfChannels = GARBAGE_VALUE_MARK;
```
Two more changes are worth making in the same function, since a rectangle can
be valid and still too large to allocate: compute `dataLength` at line 412 as
`qint64(arrayRect.width()) * arrayRect.height()`, and check the image itself
before writing into it.
```diff
QImage image(arrayRect.size(), format);
+ if (image.isNull()) {
+ throw ASLParseException("VAList: could not allocate the pattern
image!");
+ }
+
if (format == QImage::Format_Indexed8) {
```
`readFileImpl()` already catches `ASLParseException` around the pattern loop at
lines 646-654, so all three throws end in the existing "could not read the
pattern" path rather than in a failed load.
12_psd_rle_excessive_alloc_line90.bin 14-byte PSD file — RLE unpacked length
taken from the file, 16 EiB allocation at compression.cpp:90
13_psd_zip_excessive_alloc_line367.bin 16-byte PSD file — ZIP expected length
taken from the file, 16 EiB allocation at compression.cpp:367
09_asl_readVirtualArrayList_line427.bin 16,632-byte ASL layer-style file — null
dereference in kis_asl_reader.cpp:427
--
You are receiving this mail because:
You are watching all bug changes.
