Ideas for more testfiles

[ktmf01]

Mostly as a note to self or TODO list, here are a few ideas I have for more testfiles. These are not for the subset testbench, but perhaps for a security related one, or one specific for fuzzing.

• FLAC files that have a fake sync code in the header
  • Creating FLAC files with samplerates of 65528Hz, 65529Hz, 655280Hz and 655290Hz. With these, a valid sync code (0xFFF8 or 0xFFF9) appear in the samplerate bits
  • Creating FLAC files with blocksizes of 65529 or 65530. Again, the sync code appears
  • Creating FLAC files with a blocksize of 65535 and a blocksize of 248 or 249. Once more, the sync code appears
  • Creating FLAC files with a samplerate of 256kHz or 256Hz. Chance of 1 in 128 that a sync code appears with the right CRC.
• FLAC files where the samplerate, number of channels or bit depth changes halfway a stream
• Extracting all frames in the testbench FLAC files to separate files, and running afl-cmin on them to create a nice fuzzing testset

I'll probably add some more if an idea comes up. Feel free to add ideas.

[kleinesfilmroellchen]

When loading pictures, make sure that the picture's claimed size matches the actual data size, or else the picture decoder is fed data past the end of the picture block. In the SerenityOS FLAC decoder, this lead to a heap buffer overflow in the past. A (probably invalid) file exercising this was found by oss-fuzz here: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=53684&q=label%3AProj-serenity%20Type%3DBug-Security%20FuzzFlacLoader&can=1

[ktmf01]

Yes, this is explicitly stated in the security considerations of the latest draft: https://www.ietf.org/archive/id/draft-ietf-cellar-flac-13.html#name-security-considerations

Various kinds of metadata blocks contain length fields or field counts. While reading a block following these lengths or counts, a decoder MUST make sure higher-level lengths or counts (most importantly, the length field of the metadata block itself) are not exceeded. As some of these length fields code string lengths, memory for which must be allocated, parsers MUST first verify that a block is valid before allocating memory based on its contents, except when explicitly instructed to salvage data from a malformed file.

Still, a test file would be helpful.

[wader]

For reference ffmpeg's flac demuxer used to have a similar issue FFmpeg/FFmpeg@af97c98

[H2Swine]

Maybe think twice before adding a file just because some application fails it, but on the other hand ffmpeg is a big player and - hopefully - when it is fixed, it would be a test on whether applications that either use ffmpeg directly or code from it, do the job updating it:
https://trac.ffmpeg.org/ticket/10982 , https://hydrogenaud.io/index.php/topic,125848 . I managed to provoke this flaw inside Subset.

A few more thoughts:

• As for the first post, on fake sync codes: Metadata could have fake sync codes, fake frames with CRC, or even paste a verbatim FLAC file into a picture ... whatever endianness the picture format might have ...
• While I'm at metadata: Test for treatment of non-nulls in PADDING block? Put a "fake first frame" there!
• Zero-length metadata blocks or fields, are they covered? PADDING of length zero plus the four block header bytes; PICTURE with zero size specified; SEEKTABLE with zero seek points or referencing a target frame with zero samples. Also for SEEKTABLE: nonsense values like referencing a target with more samples than max set in STREAMINFO - and nonsense values could also be a test on whether "placeholder" seekpoints are parsed correctly?
• "Unsigned fields exceeding half the max" to provoke errors if they are mistakenly declared as signed. Some metadata variables are u(32). Since s(36) isn't gonna happen (?) it might not be necessary to try to provoke that in the total sample count.

(... in retrospect: I would have wished for all the test files reading the file numbers aloud at the beginning.)