[FFmpeg-devel] [PATCH] [fateserver] Cleanup and security strengthening

[Chad Fraleigh]

On 8/22/2021 11:18 AM, Michael Niedermayer wrote:
> On Sun, Aug 15, 2021 at 11:24:47AM +0200, Nicolas George wrote:
>> Nicolas George (12021-08-08):
>>> Here is a patch series for fateserver, to fix warnings and enable Perl's
>>> taint checks, thus protecting against a whole class of security issues.
>>
>> I would appreciate somebody looks at the code before I deploy it and we
>> re-enable the server.
> 
> noone in the whole community cares about the server or everyone trusts you
> to never make a mistake. At least when limited to people knowing perl and
> having time.
> 
> seriously, if someone has time and knows perl, please look over this,
> even if by the time you do, this has already been applied.
> 
> The sooner someone goes over this the sooner the fateserver is online
> again

It mostly looks good (from a perl perspective).

Just 3 questionable items..

-<>-<>-

-if ($ENV{HTTP_ACCEPT_ENCODING} =~ /gzip/) {
-    print "Content-Encoding: gzip\r\n";
+if (ready_for_gzip) {
      $cat = 'cat';
  }

The old code outputs "\r\n", where ready_for_gzip() outputs "\r\n\r\n". 
Will this be an issue, or should it have done that in the first place?

-<>-<>-

+sub ready_for_gzip() {
+    my $ae = $ENV{HTTP_ACCEPT_ENCODING};
+    if (defined($ae) && $ae =~ /gzip/) {

It is checking for 'gzip' as a substring, rather than a whole word. If 
it was passed a [hypothetical] encoding type contains the substring gzip 
(e.g. "bigzip"), it could trigger in incompatible output encoding. 
However, it's not any worse than it was previously.

Perhaps changing it to match /\bgzip\b/ ?

-<>-<>-

  sub ready_for_gzip() {
+    # Under CGI, $PATH is safe
+    ($ENV{PATH}) = $ENV{PATH} =~ /(.*)/s;

It is untainting the PATH as "hidden" side effect of calling 
ready_for_gzip(). While technically it works, it feels a little kludgy 
compared to untainting it at the beginning of each taint-enabled script.