[vlc-devel] [PATCH v2 12/13] record: use vlc_MakeTmpFile (thus mkstemp) instead of tempnam

[Lyndon Brown]

On Tue, 2020-10-06 at 17:58 +0300, Rémi Denis-Courmont wrote:
> Le tiistaina 6. lokakuuta 2020, 9.01.44 EEST Lyndon Brown a écrit :
> > From: Lyndon Brown <jnqnfe at gmail.com>
> > Date: Tue, 6 Oct 2020 02:51:48 +0100
> > Subject: record: use vlc_MakeTmpFile (thus mkstemp) instead of
> > tempnam
> > 
> > resolves warning about insecure function
> > 
> > diff --git a/modules/stream_out/record.c
> > b/modules/stream_out/record.c
> > index ce2ce958e7..e0cf0e4345 100644
> > --- a/modules/stream_out/record.c
> > +++ b/modules/stream_out/record.c
> > @@ -463,14 +463,16 @@ static void OutputStart( sout_stream_t
> > *p_stream )
> >          for( unsigned i = 0; i < sizeof(ppsz_muxers) /
> > sizeof(*ppsz_muxers); i++ ) {
> >              char *psz_file;
> > -            int i_es;
> > -
> > -            psz_file = tempnam( NULL, "vlc" );
> > -            if( !psz_file )
> > +            int fd = vlc_MakeTmpFile(&psz_file, PACKAGE_NAME"-
> > rec.XXXXXX",
> > NULL); +            if( fd == -1 )
> > +            {
> > +                msg_Warn( p_stream, "failed to create temporary
> > file" );
> >                  continue;
> > +            }
> > 
> >              msg_Dbg( p_stream, "probing muxer %s",
> > ppsz_muxers[i][0] );
> > -            i_es = OutputNew( p_stream, ppsz_muxers[i][0],
> > psz_file, NULL
> > ); +            int i_es = OutputNew( p_stream, ppsz_muxers[i][0],
> > psz_file, NULL );
> > +            vlc_close( fd );
> 
> All of that rework and in the end, the real ToCToU bug is still not
> fixed.

I had no doubt you'd catch any such problem here if there was one. I'm
not trying to hide the fact that when it came down to completing this
commit I had some doubts around dealing with the extra FD, with proper
evaluation requiring a greater understanding that I currently poses of
this code. It would be very helpful and appreciated if you could
briefly detail what is needed.

My very rough assessment was that:
 1. We exclusively create and open the file safely via the helper.
 2. We give the path to OutputNew() which results in the file being
opened again, which presumably works fine, and is presumably safe
because we're already holding it open exclusive to the process.
 3. Having done that, we can then close the extra handle we're holding
from step 1.

Is this wrong? Can you please explain. Does step 3 leave us vulnerable
because the other handle is not created with O_EXCL and we need that
attribute to persist as long as we're using the file?

I did consider modifying the functions to pass the existing FD we've
obtained through, but that's a rather intrusive change and hits a
problem in psz_output construction in OutputNew(), though I recall
there being an access module that works with FDs.

Do I just need to capture the FD outside of the loop along with the
'best' vars and then close it much later in the function?

> >              if( i_es < 0 )
> >              {
> > 
> > _______________________________________________
> > vlc-devel mailing list
> > To unsubscribe or modify your subscription options:
> > https://mailman.videolan.org/listinfo/vlc-devel
> 
>