Open Source Security Foundation (OpenSSF) best practices: Dynamic code analysis

[aclark4life]

We are only one best practice away from transitioning our OpenSSF badge from in-progress to passing! Thanks @hugovk and @radarhere for re-raising this in #7610.

I remember discussing this in the past and if I recall correctly, we never gained a consensus. At a glance, I'm not sure I fully understand what the challenges to declaring this "met" are. Here's the best practice details:

It is SUGGESTED that the project use a configuration for at least some dynamic analysis (such as testing or fuzzing) which enables many assertions. In many cases these assertions should not be enabled in production builds. [dynamic_analysis_enable_assertions] This criterion does not suggest enabling assertions during production; that is entirely up to the project and its users to decide. This criterion's focus is instead to improve fault detection during dynamic analysis before deployment. Enabling assertions in production use is completely different from enabling assertions during dynamic analysis (such as testing). In some cases enabling assertions in production use is extremely unwise (especially in high-integrity components). There are many arguments against enabling assertions in production, e.g., libraries should not crash callers, their presence may cause rejection by app stores, and/or activating an assertion in production may expose private data such as private keys. Beware that in many Linux distributions NDEBUG is not defined, so C/C++ assert() will by default be enabled for production in those environments. It may be important to use a different assertion mechanism or defining NDEBUG for production in those environments.

So, ignoring the "using in production" aspect, what does "use a configuration for at least some dynamic analysis which enables many assertions" require in our case?

Are we talking specifically about assert statements and if so do any of these count? Assuming some are security related, maybe we just need an "on/off". (Actually I assume none are security related… and when I say security-related I mean corresponding to a CVE fix.)

src/libImaging/SunRleDecode.c:                break;  // assert
src/libImaging/Draw.c:                assert(k1 >= 0);
src/libImaging/Draw.c:                assert(k2 >= 0);
src/libImaging/Draw.c:        assert(t != NULL);
src/libImaging/TgaRleEncode.c:#include <assert.h>
src/libImaging/TgaRleEncode.c:            assert(state->x <= state->xsize);
src/libImaging/TgaRleEncode.c:        assert(bytes >= 0);
src/libImaging/TgaRleEncode.c:        assert(state->count > 0);
src/libImaging/TgaRleEncode.c:        assert(state->x > 0);
src/libImaging/TgaRleEncode.c:        assert(state->count <= state->x * bytesPerPixel);
src/libImaging/TgaRleDecode.c:                break;  // assert
src/_webp.c:    assert(err <= WEBP_MUX_NOT_FOUND && err >= WEBP_MUX_NOT_ENOUGH_DATA);
src/PIL/MpoImagePlugin.py:        # Note that the following assertion will only be invalid if something
src/PIL/MpoImagePlugin.py:        assert self.n_frames == len(self.__mpoffsets)
src/PIL/XbmImagePlugin.py:        assert self.fp is not None
src/PIL/PcxImagePlugin.py:        assert self.fp is not None
src/PIL/PcxImagePlugin.py:    assert fp.tell() == 128
src/PIL/PcxImagePlugin.py:        assert im.im is not None
src/PIL/SunImagePlugin.py:        assert self.fp is not None
src/PIL/ImageFile.py:        assert image is not None
src/PIL/ImageFile.py:        assert self.data is None, "cannot reuse parsers"
src/PIL/MpegImagePlugin.py:        assert self.fp is not None
src/PIL/PixarImagePlugin.py:        assert self.fp is not None
src/PIL/MspImagePlugin.py:        assert self.fp is not None
src/PIL/MspImagePlugin.py:        assert self.fd is not None
src/PIL/ImtImagePlugin.py:        assert self.fp is not None
src/PIL/PcdImagePlugin.py:        assert self.fp is not None
src/PIL/PcdImagePlugin.py:            assert self.im is not None
src/PIL/TgaImagePlugin.py:        assert self.fp is not None
src/PIL/TgaImagePlugin.py:            assert self.im is not None
src/PIL/TgaImagePlugin.py:        assert im.im is not None
src/PIL/ImageMorph.py:        assert len(permutation) == 9
src/PIL/ImageMorph.py:        assert self.lut is not None
src/PIL/ImageOps.py:    # Initial asserts
src/PIL/ImageOps.py:    assert image.mode == "L"
src/PIL/ImageOps.py:        assert 0 <= blackpoint <= whitepoint <= 255
src/PIL/ImageOps.py:        assert 0 <= blackpoint <= midpoint <= whitepoint <= 255
src/PIL/GdImageFile.py:        assert self.fp is not None
src/PIL/McIdasImagePlugin.py:        assert self.fp is not None
src/PIL/FitsImagePlugin.py:        assert self.fp is not None
src/PIL/DdsImagePlugin.py:    assert item.name is not None
src/PIL/DdsImagePlugin.py:    assert item1.name is not None
src/PIL/DdsImagePlugin.py:    assert item2.name is not None
src/PIL/DdsImagePlugin.py:    assert item3.name is not None
src/PIL/Image.py:        assert BmpImagePlugin
src/PIL/Image.py:        assert GifImagePlugin
src/PIL/Image.py:        assert JpegImagePlugin
src/PIL/Image.py:        assert PpmImagePlugin
src/PIL/Image.py:        assert PngImagePlugin
src/PIL/XVThumbImagePlugin.py:        assert self.fp is not None
src/PIL/SgiImagePlugin.py:        assert self.fp is not None
src/PIL/SgiImagePlugin.py:    # assert we've got the right number of bands.
src/PIL/SgiImagePlugin.py:        assert self.fd is not None
src/PIL/SgiImagePlugin.py:        assert self.im is not None
src/PIL/ImageFont.py:          assert hello_world == font.getlength("HelloWorld")  # may fail
src/PIL/ImageFont.py:          assert hello_world == font.getlength("HelloWorld")  # True
src/PIL/ImageFont.py:          assert hello_world == draw.textlength("HelloWorld", font, features=["-kern"])
src/PIL/PpmImagePlugin.py:        assert self.fp is not None
src/PIL/PpmImagePlugin.py:        assert self.fp is not None
src/PIL/PpmImagePlugin.py:        assert self.fp is not None
src/PIL/PpmImagePlugin.py:        assert self.fd is not None
src/PIL/PpmImagePlugin.py:        assert self.fd is not None
src/PIL/FtexImagePlugin.py:        assert format_count == 1
src/thirdparty/raqm/raqm.c:#include <assert.h>
src/thirdparty/raqm/raqm.c:  assert (run);
src/thirdparty/raqm/raqm.c:  assert (types);
src/thirdparty/raqm/raqm.c:  assert (levels);
src/thirdparty/raqm/raqm.c:  assert (rq->base_dir < sizeof (dir_names));
src/thirdparty/raqm/raqm.c:  assert (rq->resolved_dir < sizeof (dir_names));

[aclark4life]

From IRC,

bjs: aclark: if you have tests, and you run those tests without -O0, and the tests run code with assert statements in it, then you should be passing the bar as the text is written.

cchianel: aclark, see https://docs.python.org/3/using/cmdline.html#cmdoption-O

bjs: aclark: if your test environment runs Python with -O then my reading of that text would say you wouldn't meet the requirement, for example. As that would disable the assertion checks. I think it'd be more debatable whether the sprinkling of if ...: raise ... checks that pillow undoubtedly has would also count towards this requirement

bjs: I would argue yes, but others may dissent :)

bjs: aclark: I think the rationale is clear though: they want you to make the code do a bunch of extra safety/sanity checks at runtime while it runs the tests (separate to the test case itself), and it's okay if these checks are only ran during testing and users don't get them (or are disabled) when they import the library.

Thanks IRC folks! And because I can't help myself, here's what ChatGPT thinks

from PIL import Image
import os

# Define a configuration variable to enable dynamic analysis assertions
dynamic_analysis_enable_assertions = True

# Define a function to perform dynamic analysis
def perform_dynamic_analysis(image_path):
    if dynamic_analysis_enable_assertions:
        # Perform assertions or other checks during dynamic analysis
        assert os.path.exists(image_path), "Image file does not exist"
        assert os.path.splitext(image_path)[1].lower() in ['.jpg', '.jpeg', '.png'], "Unsupported image format"
    # Load the image using Pillow
    image = Image.open(image_path)
    # Further processing...
    # Example: image.show()

# Example usage
if __name__ == "__main__":
    # Path to the image file
    image_path = "example_image.jpg"
    # Perform dynamic analysis on the image
    perform_dynamic_analysis(image_path)

bjs: aclark: I don't know what ChatGPT was smoking SnoopJ: reddit, probably Jefren laughs out loud

[aclark4life]

@python-pillow/pillow-team Can anyone point me in some tangible direction to try to do some work on this one? Thank you!

[hugovk]

You could see how other Python projects have handled it:

https://github.com/sethmlarson/pypi-scorecards