search

Efferent-Health / fo-dicom.Codecs

Cross-platform Dicom native codecs for fo-dicom
Other
65 stars 23 forks source link

Fuzzing resulted in a number of fatal errors #78

Closed tboby closed 1 week ago

tboby commented 3 months ago

Describe the bug As part of wider fuzzing I included fuzzing of the fo-dicom.Codecs image rendering. This found a number of different fatal, mostly uncatchable, exceptions which take down the host process. These were found across multiple codecs.

As these dicoms almost all crash the application without a stack trace I'm unable to confirm if these are all the same issue, how realistic they are, etc.

While I understand these dicoms are all certainly corrupt, a corrupt dicom shouldn't take down the server if at all possible.

To Reproduce

var myFIle = DicomFile.Open(file);  
var myImage = new DicomImage(myFIle.Dataset);  
myImage.RenderImage(0);

I have generated this set using the acceptance testing files as the starting point so they're not confidential, but I won't post them publicly. Please let me know how I can share them with you.

Environment:

jaime-olivares commented 3 months ago

It is not possible to avoid crashes with a corrupted pixel data. The conversion is performed by unmanaged code, so no managed exception at all. To handle that situation, you can run the transcoding portion or the entire DICOM handling portion in a separate process. You can trap the crash report in that case. Thus, there is nothing we would fix regarding that situation.

jaime-olivares commented 2 months ago

Closing for the reasons explained. Also provided a possible solution alternative.

tboby commented 2 months ago

Thanks for the response, and sorry for the delayed follow-up. I've been doing some further investigation into how other DICOM libraries handle these files.

Other libraries

I've tested:

dcmtk and gdcm gracefully failed all but one, printing an error with the JPEG issue. microdicom simply failed to convert all but one, but the failures did not interrupt the batch processing of the rest. opencv swallowed the internal jpeg library exceptions and silently returned an empty Mat (default behaviour for opencv) pydicom simply threw exceptions and continued: I ran both decoders on all files in the same execution.

In summary: all other libraries gracefully handled this set of corrupt jpegs without crashing the entire process. To my knowledge, none of them use a separate process. It's also likely that at least one of them is using the same underlying jpeg source as this repo (there aren't that many implementations of j2k), although I haven't checked.

Mitigation

To handle that situation, you can run the transcoding portion or the entire DICOM handling portion in a separate process. You can trap the crash report in that case.

I understand that could work in a batch processing situation, but how would this work for C-GET? fo-dicom automatically transcodes as part of sending the returning C-STORE triggered by a C-GET.

A client can C-STORE a corrupt DICOM, then C-GET the same DICOM with a different presentation context and crash the server.

Is there any workaround possible at all?

If there really is no way to fix this, it might be a good idea to add a security warning to this repo and the fo-dicom repo warning of this vulnerability (@gofal).

jaime-olivares commented 2 months ago

We will analyze where we can trap errors. Not sure if we can trap all of them. That will be a major enhancement, so it will take some time.

jaime-olivares commented 1 month ago

@tboby Please email your dataset to cbeltran at efferenthealth.com

tboby commented 1 month ago

@tboby Please email your dataset to cbeltran at efferenthealth.com

Done!

cbeltran1306 commented 1 month ago

Hi @tboby, we have some improvements at J2K codecs to support fuzzing test, please you cant test it using 5.15.0-beta1 nuget version (https://www.nuget.org/packages/fo-dicom.Codecs/5.15.0-beta1) for J2K dicom files. Make sure replace the transcoder manager in your test code as below. 

var myFIle = DicomFile.Open(file);  
var myImage = new DicomImage(myFIle.Dataset);

new DicomSetupBuilder()
  .RegisterServices(s => s.AddFellowOakDicom().AddTranscoderManager<FellowOakDicom.Imaging.NativeCodec.NativeTranscoderManager>())
  .SkipValidation()
  .Build();  
myImage.RenderImage(0);

We are improving the other codecs to support the fuzzing test.

tboby commented 1 month ago

Hi @tboby, we have some improvements at J2K codecs to support fuzzing test, please you cant test it using 5.15.0-beta1 nuget version (https://www.nuget.org/packages/fo-dicom.Codecs/5.15.0-beta1) for J2K dicom files. Make sure replace the transcoder manager in your test code as below.

I can confirm that the previous J2K cases no longer cause fatal errors (just exceptions). I did a little more fuzzing and it didn't immediately find other problems, but I'd expect that.

To properly test I need to spin up some compute and leave it running for a while. It'll be a lot easier if I don't need to try and restrict the codec as I can continue from my previous run. If you think you've found the similar issues in other codecs and it'll be (relatively) quick to fix then it might be better for me to wait for that.

If you're not sure how long the other codecs will take then I can try and start a J2K test from scratch instead.

Thanks for the quick investigation!

jaime-olivares commented 1 month ago

Hi @tboby We are starting to review all our code to find points where we can put some guards. We have started with JP2K but will continue with other codecs. Any test result will help.

cbeltran1306 commented 1 month ago

Hi @tboby, we did more improvements at J2K decoding, please use this beta version 5.15.0-beta2 https://www.nuget.org/packages/fo-dicom.Codecs/5.15.0-beta2

jaime-olivares commented 1 week ago

I am closing this issue for now, as we haven't received more feedback