Page MenuHomePhabricator

Security review for fluidsynth
Closed, ResolvedPublic

Description

Project Information

Description of the tool/project

A MIDI to audio converter (.mp3 and .ogg)

Description of how the tool will be used at WMF

As a replacement to the obsolete TiMidity++ that served the same purpose, but only towards .ogg.

Dependencies

From apt-cache depends fluidsynth:

fluidsynth
  Depends: libfluidsynth1
  Depends: libc6
  Conflicts: <iiwusynth>
  Recommends: qsynth
  Replaces: <iiwusynth>
    fluidsynth

Please note that a soundfont would also be downloaded. (such as fluid)

Has this project been reviewed before?

Cursory comparison done by @Legoktm: {T181897#3806030}

Sounds good to me at first glance, we should make sure to evaluate the performance of fluidsynth (CPU, memory, wall clock time) versus timidity. Having a more active upstream (and Debian maintainer) all sound great. I tried finding any past CVEs/security issues in fluidsynth but didn't find any, which was a bit surprising to me at least (that said, the only CVEs I could find for timidity were all local crashes/DoS).

Working test environment

Done simply through apt-get. The extension will default to Fluidsynth if found.

Patchset implementing Fluidsynth compatibility: rESCRb173336b9634: Migrate TiMidity++ to fluidsynth

Post-deployment

Event Timeline

Reedy updated the task description. (Show Details)Jul 13 2018, 11:56 AM
Reedy added a subscriber: Reedy.Jul 13 2018, 12:00 PM

I honestly don't know if we've got anyone to particularly review C code... And do we have a history of reviewing stuff like this?

Lego's comments are probably more the sort of thing we want to be doing, looking more holistically at the package, it's upstream, packagers and stuff

I honestly don't know if we've got anyone to particularly review C code... And do we have a history of reviewing stuff like this?

I don't know if @tstarling ever did any kind of review of lilypond or timidity.

Lego's comments are probably more the sort of thing we want to be doing, looking more holistically at the package, it's upstream, packagers and stuff

FWIW basic benchmarking of fluidsynth vs timidity was done in T181897#3873809 by @divadsn.

@Legoktm those benchmark results are a bit old, while fluidsynth might got an update, I can update them if required :)

@Legoktm those benchmark results are a bit old, while fluidsynth might got an update, I can update them if required :)

Pretty sure the versions in debian stable aren't going to be newer than what you used ;)

I don't know if @tstarling ever did any kind of review of lilypond or timidity.

Lilypond is written in Scheme, I mostly just read the docs which have plenty of information about script injection and the like. I don't think I reviewed Timidity, although it probably would have been wise. I do have a Timidity source tree on my hard drive dated 2013 but I don't recall reviewing it. I did review the C code of Philip's Music Writer, which was proposed as an alternative to Lilypond. It was horrifying, full of buffer overflows. I tried to explain the (many) issues to the author but ended up just recommending to Debian that they drop the package (they didn't). This is why we don't have Philip's Music Writer.

Speaking of C reviews in general, I also did a security review of librsvg, and I wrote some patches to secure it. I don't think it's necessary to review external projects completely, line by line, but I think it's prudent to open up the source and see if it makes your eyes bleed, and to brainstorm any modes of attack which might be especially relevant to us. These kinds of projects are usually not written with adversarial input in mind.

In my opinion, DoS is not worth worrying about, unless it is something exotic like ephemeral port exhaustion. We have time and memory limits and there's plenty of ways to hit them.

Ebe123 closed this task as Resolved.Aug 31 2018, 1:19 PM
Ebe123 claimed this task.

As fluidsynth is now in puppet and the comments above, think this can be set as resolved.

Didn't hear of Philip's Music Writer before; guess that was for the better.

Ebe123 removed Ebe123 as the assignee of this task.Aug 31 2018, 1:20 PM
sbassett moved this task from Backlog to Done on the Security-Team board.Jun 11 2019, 6:30 PM