Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fluidsynth crashes when loading malformed sf2 file #808

Closed
veritas501 opened this issue Mar 14, 2021 · 11 comments
Closed

fluidsynth crashes when loading malformed sf2 file #808

veritas501 opened this issue Mar 14, 2021 · 11 comments
Labels
Milestone

Comments

@veritas501
Copy link

veritas501 commented Mar 14, 2021

version: master(v.2.1.1), ubuntu18.04(v1.1.9), ubuntu20.04(v.2.1.1), ....

https://github.com/FluidSynth/fluidsynth/blob/master/src/sfloader/fluid_sffile.c#L1952
It says Gen_SampleId is the last gen, and then set level to 3 and break.

else if(genid == Gen_SampleId)
{
    /* sample is last gen */
    level = 3;
    READW(sf, genval.uword);
    ((SFZone *)(p2->data))->instsamp = FLUID_INT_TO_POINTER(genval.uword + 1);
    break; /* break out of generator loop */
}

but if a malformed sf2 doesn't contain Gen_SampleId, it will finally goto here:
https://github.com/FluidSynth/fluidsynth/blob/master/src/sfloader/fluid_sffile.c#L2041

if(level == 3) // level == 2 because no Gen_SampleId found
{
    SLADVREM(z->gen, p3);    /* zone has sample? */
}
else // enter here
{
    /* its a global zone */
    if(!gzone) // not the first time enter here, so gzone == TRUE, skip
    {
        gzone = TRUE;

        /* if global zone is not 1st zone, relocate */
        if(*hz != p2)
        {
            void *save = p2->data;
            FLUID_LOG(FLUID_WARN, "Instrument '%s': Global zone is not first zone",
                      ((SFPreset *)(p->data))->name);
            SLADVREM(*hz, p2);
            *hz = fluid_list_prepend(*hz, save);
            continue;
        }
    }
    else // finally enter here and free it, but actually it's gzone. so it will cause an use-after-free later ...
    {
        /* previous global zone exists, discard */
        FLUID_LOG(FLUID_WARN, "Instrument '%s': Discarding invalid global zone",
                  ((SFInst *)(p->data))->name);
        *hz = fluid_list_remove(*hz, p2->data);
        delete_zone((SFZone *)fluid_list_get(p2));
    }
}

it will be freed again at: https://github.com/FluidSynth/fluidsynth/blob/master/src/sfloader/fluid_sffile.c#L2293

entry = zone->gen;

while(entry)
{
    FLUID_FREE(fluid_list_get(entry)); // free at here
    entry = fluid_list_next(entry);
}

fluid_synth_sfload() -> fluid_defsfloader_load() -> fluid_defsfont_load() -> fluid_sffile_close() -> delete_inst() -> delete_zone()


HERE is an example that trigger this vuln: vuln.zip

image

image

Programs like VLC that use this library are affected by this vulnerability:

image

@veritas501 veritas501 added the bug label Mar 14, 2021
@derselbst derselbst added this to the 2.1 milestone Mar 14, 2021
@derselbst
Copy link
Member

Thanks for the detailed report. The problem here is that fluid_list_remove(*hz, p2->data); should receive the beginning of the list, so it can adjust p2s predecessor. Unfortunately, it receives *hz, which in this case points to p2, which in turn is the element about to be removed. Hence there is no chance to remove the item from the list.

The next time it would be accessed is in fixup_igen. This is where you get the Invalid sample reference error, which erroneously causes the whole soundfont to be dropped.

Looking at the code, I think we'll have a similar flaw when parsing the presets. I'll make a PR for that in a second and mention you, so you can test it with potentially other malformed fonts you have around. If you have no time to test, pls. let me know, because I wanted to release 2.1.8 today or tomorrow.

@derselbst
Copy link
Member

Fixed for 2.1.8 and merged to master. Thanks!

mawe42 added a commit that referenced this issue Apr 3, 2021
 - parser_test_valid: good case with simple test sf2
 - pz_duplicate_gen_vel: duplicate VelRange generator in preset zone
 - pz_key_vel_wrong_order: invalid key/vel range generator order in pz
 - pz_two_global: two global zones, regression for #808
mawe42 added a commit that referenced this issue Apr 3, 2021
 - iz_duplicate_gen_vel: duplicate VelRange generator in inst zone
 - iz_key_vel_wrong_order: invalid key/vel range generator order in iz
 - iz_two_global: two global zones, regression for #808
mawe42 added a commit that referenced this issue Apr 3, 2021
 - iz_duplicate_gen_vel: duplicate VelRange generator in inst zone
 - iz_key_vel_wrong_order: invalid key/vel range generator order in iz
 - iz_two_global: two global zones, regression for #808
@ajakk
Copy link

ajakk commented Apr 13, 2021

CVE-2021-28421 was assigned for this.

@utkarsh2102
Copy link

utkarsh2102 commented Apr 24, 2021

Hello, I wonder if it's also affecting v1.1.11? static int load_pgen definition exists in src/sfloader/fluid_defsfont.c there which is not way too different than what it is atm. Could somebody confirm if this bug/CVE also applies to v1.1.11?

Thanks!

@derselbst
Copy link
Member

I wonder if it's also affecting v1.1.11?

Pls. refer to the changelog: https://github.com/FluidSynth/fluidsynth/wiki/ChangeLog#fluidsynth-218

@utkarsh2102
Copy link

I wonder if it's also affecting v1.1.11?

Pls. refer to the changelog: https://github.com/FluidSynth/fluidsynth/wiki/ChangeLog#fluidsynth-218

Eeks, sorry for not checking earlier. And thanks!

@bynt
Copy link

bynt commented May 5, 2021

CVE-2021-28421 was assigned for this.

@ajakk:

GHSA-6fcq-pxhc-jxc9 mentions CVE-2021-21417 but references #808. Is this a duplicate?

CVE-2021-21417 was assigned by GitHub, maybe automatically.

@derselbst
Copy link
Member

CVE-2021-21417 is the one I've originally filed via Github security advice. Which actually worked quite smooth, but unfortunately, the CVE stayed "reserved" even after the advice was published on Mar 31. But it seems like it was finally published a few days ago. No clue why it stayed reserved for so long. The best guess I have is that veritas501 hasn't accepted the credit for this CVE. Although according to GH documentation this shouldn't matter... weird.

@bynt
Copy link

bynt commented May 6, 2021

CVE-2021-21417 is the one I've originally filed via Github security advice.

Thanks for clarifying. I filed an update request for CVE-2021-28421 with Mitre (Duplicate of CVE-2021-21417).

@mawe42
Copy link
Member

mawe42 commented May 6, 2021

Hey Martin, funny meeting you here :-)

@bynt
Copy link

bynt commented May 6, 2021

Hey Martin, funny meeting you here :-)

haha, indeed! Fedora brought me here :)
https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2021-f17367545f

buildroot-auto-update pushed a commit to buildroot/buildroot that referenced this issue Sep 13, 2021
Fixes the CVE-2021-21417 security issue:
GHSA-6fcq-pxhc-jxc9
FluidSynth/fluidsynth#808

See changelog since v2.1.5:
- https://github.com/FluidSynth/fluidsynth/releases/tag/v2.1.6
- https://github.com/FluidSynth/fluidsynth/releases/tag/v2.1.7
- https://github.com/FluidSynth/fluidsynth/releases/tag/v2.1.8
- https://github.com/FluidSynth/fluidsynth/releases/tag/v2.1.9

./utils/test-pkg --package fluidsynth
6 builds, 2 skipped, 0 build failed, 0 legal-info failed

Signed-off-by: Julien Olivain <ju.o@free.fr>
Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants