Bug#1094991: bookworm-pu: package dcmtk/3.6.7-9~deb12u2

[Étienne Mollier]

Package: release.debian.org
Severity: normal
Tags: bookworm
X-Debbugs-Cc: ***@packages.debian.org, ***@debian.org
Control: affects -1 + src:dcmtk
User: ***@packages.debian.org
Usertags: pu

Hi stable release managers,

[ Reason ]
I would like to upload dcmtk version 3.6.7-9~deb12u2 in order to
fix a series of CVEs rated no-DSA affecting it. The precise
list of issues fixed is:

* CVE-2024-27628, or bug #1074483, which is about a risk of
buffer overflow;
* CVE-2024-34508 and CVE-2024-34509, which are both about a
segmentation fault, there are two references to the same
issue because one affects local I/O and the other network
I/O, I have not identified relevant bugs in the BTS;
* CVE-2024-47796, or bug #1093043, which is about an
out-of-bound write;
* CVE-2024-52333, or bug #1093047, which is another
out-of-bound write.

CVE-2024-28130 is notably /not/ fixed for now as I'm not that
comfortable with the codebase and changes were a bit involved.
I note that there has been a port of the fix for Debian LTS
bullseye, so I believe this should be manageable. There is
still room to amend the debdiff before upload, if someone comes
up with a fix, or to upload a later package version to fix that
last item.

[ Impact ]
Users of dcmtk and reverse dependencies in Debian bookworm will
remain affected by these multiple known issues if the package is
not updated. Also, users upgrading from Debian bullseye would
experience a regression in their security coverage, as all the
issues are already addressed in LTS.

[ Tests ]
dcmtk ships with extensive unit test, it also includes
autopkgtest, which I ran on amd64. In addition, I also made
sure that the set of changes did not introduce regressions in
reverse dependencies by running their autopkgtest when available
and by rebuilding reverse build-dependencies. I have seen no
regressions.

[ Risks ]
Except for CVE-2024-28130 which is left alone for now, the patch
set was not too involved: most upstream patches applied without
too much fuzz.

There may be a risk from 0011-CVE-2024-34508-34509_bis.patch as
it might be seen as hiding a problem by adjusting the test
suite after a regression from 0010-CVE-2024-34508-34509.patch,
and I'm not entirely confident about what happened for it to be
suddenly needed.

I identified 19 reverse dependencies and 34 reverse build
dependencies to dcmtk in bookworm, so an uncaught regression
could have some blast radius; I failed to identify anything
obvious by running autopkgtests and rebuilds though. All that
being said, dcmtk is not a key package.

[ Checklist ]
[*] *all* changes are documented in the d/changelog
[*] I reviewed all changes and I approve them
[*] attach debdiff against the package in (old)stable
[*] the issue is verified as fixed in unstable

[ Changes ]
This dcmtk package update introduces a number of patches to
address the vulnerability issues aforementioned.

0007-CVE-2024-47796.patch is an adjustment of upstream commit
89a6e399f1e17d08a8bc8cdaa05b2ac9a50cd4f6, commit which looks to
replace a default zero padding by another padding value that
ought to be valid and avoid the incident as far as I understood.

0008-CVE-2024-52333.patch is an adjustment of upstream commit
03e851b0586d05057c3268988e180ffb426b2e03 which looks to
introduce supplemental sanity checks to avoid the out-of-bound
write incident when handling malformed DICOM files.

0009-CVE-2024-27628.patch is an adjustment of upstream commit
ec52e99e1e33fc39810560421c0833b02da567b3 which looks to add
further sanity checks to issue errors instead of risking
overflows. It is worth noting that a good chunk of the changes
introduce an new set of unit tests to make sure the regression
is gone for good.

0010-CVE-2024-34508-34509.patch is an adjustment of upstream
commit c78e434c0c5f9d932874f0b17a8b4ce305ca01f5 which looks to
fix the segmentation fault by properly raising errors when DIMSE
messages are truncated instead of passing through. First hunk
defines the missing check and the second hunk injects the check
at the necessary location.

0011-CVE-2024-34508-34509_bis.patch is an adjustment of upstream
commit 66c317feae446deda1a389226aa24c95a0eeac4c. This commit
fixes test regressions which turn out to be introduced by
0010-CVE-2024-34508-34509.patch. I'm still somewhat wary that
this change is needed, because it feels like some API changed,
but I'm comforted by the fact that the two upstream commits have
been issued on 2024-03-13 and that there were no regressions in
build reverse-dependencies. Why the patch 0010 introduces such
regression in the first place still escapes my understanding.

[ Other info ]
As mentioned in the Reason paragraph, with only these changes,
CVE-2024-28130 would still affect dcmtk in bookworm. Also,
fixes exist for bullseye, so I guess something can be achieved
for bookworm too. The issue was tracked in #1070207 and history
suggests that given how involved the patch is, it has been
non-trivial to get the fixes applied to dcmtk in unstable before
availability of the newer upstream release; there was notably an
accidental ABI breakage.


Thank you for making it this far, this update felt more involved
than my average targeted fixes to bring to stable, so I thought
better to err on the side of caution and being explicit.

Have a nice day, :)

[Debian Bug Tracking System]

Post by Étienne Mollier
affects -1 + src:dcmtk

Bug #1094991 [release.debian.org] bookworm-pu: package dcmtk/3.6.7-9~deb12u2
Added indication that 1094991 affects src:dcmtk

[Debian Bug Tracking System]

tag -1 confirmed

Bug #1094991 [release.debian.org] bookworm-pu: package dcmtk/3.6.7-9~deb12u2
Added tag(s) confirmed.

[Jonathan Wiltshire]

Control: tag -1 confirmed

Please go ahead.

Thanks,

[Étienne Mollier]

Hi Jonathan,

Post by Jonathan Wiltshire
Please go ahead.

Thanks for the go, I went ahead.

For the sake of transparency, we received today the bug #1095072
against dcmtk in bullseye, but I have taken the time to make
sure that the version on its way to the archive was not
affected.

Have a nice day, :)

[Adrian Bunk]

Post by Étienne Mollier
...
For the sake of transparency, we received today the bug #1095072
against dcmtk in bullseye, but I have taken the time to make
sure that the version on its way to the archive was not
affected.
...

FTR, both the CVE fix that caused the regression and the regression fix
are already included in the 3.6.7 upstream version in bookworm.

cu
Adrian

[Étienne Mollier]

Hi Adrian,

Post by Adrian Bunk
FTR, both the CVE fix that caused the regression and the regression fix
are already included in the 3.6.7 upstream version in bookworm.

Thank you, this is additional reassurance for the upcoming
bookworm-pu request for dcmtk/3.6.7-9~deb12u3 including the fix
for CVE-2024-28130.

Have a nice day, :)

[Adrian Bunk]

Post by Étienne Mollier
...
CVE-2024-28130 is notably /not/ fixed for now as I'm not that
comfortable with the codebase and changes were a bit involved.
I note that there has been a port of the fix for Debian LTS
bullseye, so I believe this should be manageable. There is
still room to amend the debdiff before upload, if someone comes
up with a fix, or to upload a later package version to fix that
last item.
...
[ Other info ]
As mentioned in the Reason paragraph, with only these changes,
CVE-2024-28130 would still affect dcmtk in bookworm. Also,
fixes exist for bullseye, so I guess something can be achieved
for bookworm too. The issue was tracked in #1070207 and history
suggests that given how involved the patch is, it has been
non-trivial to get the fixes applied to dcmtk in unstable before
availability of the newer upstream release; there was notably an
accidental ABI breakage.
...

I've attached a version for bookworm.

The non-ABI-breaking variant is not pretty since instead of
stack.top()->isElement()
it is now doing
dynamic_cast<DcmElement*>(stack.top()) != nullptr
in many places.

I am not claiming that my track record with DCMTK regressions is good,
but the original segfault from the CVE is fixed, the ABI regression
is fixed, and noone has complained in stretch/buster/bullseye about
this change. I also remember from initially developing it that the
upstream testsuite has some coverage of the changes.

cu
Adrian

[Étienne Mollier]

Hi Adrian,

Post by Adrian Bunk
The non-ABI-breaking variant is not pretty since instead of
stack.top()->isElement()
it is now doing
dynamic_cast<DcmElement*>(stack.top()) != nullptr
in many places.

Ouch, I opened #1095854 and only saw your proposal after that.
I put the bookworm-pu update on hold for now. I may amend my
work later on, or you can push your changes yourself, however
you prefer. Evidently my C++ knowledge is too limited and I
didn't anticipate that the addition of check methods would
result in an ABI break.

Thanks for liaising at the right time,

[Adrian Bunk]

Post by Étienne Mollier
Hi Adrian,

Hi Étienne,

Post by Étienne Mollier

Post by Adrian Bunk
The non-ABI-breaking variant is not pretty since instead of
stack.top()->isElement()
it is now doing
dynamic_cast<DcmElement*>(stack.top()) != nullptr
in many places.

Ouch, I opened #1095854 and only saw your proposal after that.
I put the bookworm-pu update on hold for now. I may amend my
work later on, or you can push your changes yourself, however
you prefer.
...

I also don't care, feel free to amend your work with my patches.

cu
Adrian

[Étienne Mollier]

Hi Adrian,

Post by Adrian Bunk

Post by Étienne Mollier
Ouch, I opened #1095854 and only saw your proposal after that.
I put the bookworm-pu update on hold for now. I may amend my
work later on, or you can push your changes yourself, however
you prefer.
...

I also don't care, feel free to amend your work with my patches.

Okay, it is in the pipeline.

Have a nice day, :)

[Jonathan Wiltshire]

package release.debian.org
tags 1094991 = bookworm pending
thanks

Hi,

The upload referenced by this bug report has been flagged for acceptance into the proposed-updates queue for Debian bookworm.

Thanks for your contribution!

Upload details
==============

Package: dcmtk
Version: 3.6.7-9~deb12u2

Explanation: fix issue when rendering an invalid monochrome DICOM image [CVE-2024-47796]; ensure: HighBit < BitsAllocated [CVE-2024-52333]; fix possible overflows when allocating memory [CVE-2024-27628]; fix two segmentation faults [CVE-2024-34508 CVE-2024-34509]

[Debian Bug Tracking System]

Post by Jonathan Wiltshire
package release.debian.org

Limiting to bugs with field 'package' containing at least one of 'release.debian.org'
Limit currently set to 'package':'release.debian.org'

Post by Jonathan Wiltshire
tags 1094991 = bookworm pending

Bug #1094991 [release.debian.org] bookworm-pu: package dcmtk/3.6.7-9~deb12u2
Added tag(s) pending; removed tag(s) confirmed.

Post by Jonathan Wiltshire
thanks

Stopping processing here.

Please contact me if you need assistance.