openssh unix dev - Feb 2021 - [PATCH] do not free string returned by login_getcapstr

On Mon, 15 Feb 2021, Ed Maste wrote:
> From the login_getcapstr man page,
> > Note that with all functions in this group, you should not call
free(3)
> > on any pointers returned.  Memory allocated during retrieval or
> > processing of capability tags is automatically reused by subsequent
calls
> > to functions in this group, or deallocated on calling login_close().
This seems to be a divergence between FreeBSD and OpenBSD. OpenBSD has
> CAVEATS
>     The string returned by login_getcapstr() is allocated via
>     malloc(3) when the specified capability is present and thus
>     it is the responsibility of the caller to free() this space.
>     However, if the capability was not found or an error
>     occurred and def or err (whichever is relevant) are non-NULL
>     the returned value is simply what was passed in to
>     login_getcapstr().  Therefore it is not possible to blindly
>     free() the return value without first checking it against
>     def and err.
NetBSD is idential to OpenBSD. I guess we'll need to special-case FreeBSD
and anything else that derives from that codebase. Does anyone know what
else does it the FreeBSD way? (I'm guessing Dragonfly...)

-d
> From FreeBSD d93a896ef959 by des@, "Upgrade to OpenSSH 7.5p1."
> 
> Differential Revision:	https://reviews.freebsd.org/D28617
> ---
>  session.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/session.c b/session.c
> index 27ca8a104..ec1755d21 100644
> --- a/session.c
> +++ b/session.c
> @@ -1279,7 +1279,8 @@ static void
>  do_nologin(struct passwd *pw)
>  {
>  	FILE *f = NULL;
> -	char buf[1024], *nl, *def_nl = _PATH_NOLOGIN;
> +	const char *nl;
> +	char buf[1024], *def_nl = _PATH_NOLOGIN;
>  	struct stat sb;
>  
>  #ifdef HAVE_LOGIN_CAP
> @@ -1291,11 +1292,8 @@ do_nologin(struct passwd *pw)
>  		return;
>  	nl = def_nl;
>  #endif
> -	if (stat(nl, &sb) == -1) {
> -		if (nl != def_nl)
> -			free(nl);
> +	if (stat(nl, &sb) == -1)
>  		return;
> -	}
>  
>  	/* /etc/nologin exists.  Print its contents if we can and exit. */
>  	logit("User %.100s not allowed because %s exists",
pw->pw_name, nl);
> -- 
> 2.30.0
> 
> _______________________________________________
> openssh-unix-dev mailing list
> openssh-unix-dev at mindrot.org
> https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
>

On Tue, 16 Feb 2021, Damien Miller wrote:
> On Mon, 15 Feb 2021, Ed Maste wrote:
> 
> > From the login_getcapstr man page,
> > > Note that with all functions in this group, you should not call
free(3)
> > > on any pointers returned.  Memory allocated during retrieval or
> > > processing of capability tags is automatically reused by
subsequent calls
> > > to functions in this group, or deallocated on calling
login_close().
> 
> This seems to be a divergence between FreeBSD and OpenBSD. OpenBSD has
> 
> > CAVEATS
> >     The string returned by login_getcapstr() is allocated via
> >     malloc(3) when the specified capability is present and thus
> >     it is the responsibility of the caller to free() this space.
> >     However, if the capability was not found or an error
> >     occurred and def or err (whichever is relevant) are non-NULL
> >     the returned value is simply what was passed in to
> >     login_getcapstr().  Therefore it is not possible to blindly
> >     free() the return value without first checking it against
> >     def and err.
> 
> NetBSD is idential to OpenBSD. I guess we'll need to special-case
FreeBSD
> and anything else that derives from that codebase. Does anyone know what
> else does it the FreeBSD way? (I'm guessing Dragonfly...)
actually, this is in the child process so the leak doesn't matter here.
I think this fix is fine.

-d

On Tue, 16 Feb 2021, Damien Miller wrote:
> On Mon, 15 Feb 2021, Ed Maste wrote:
> 
> > From the login_getcapstr man page,
> > > Note that with all functions in this group, you should not call
free(3)
> > > on any pointers returned.  Memory allocated during retrieval or
> > > processing of capability tags is automatically reused by
subsequent calls
> > > to functions in this group, or deallocated on calling
login_close().
> 
> This seems to be a divergence between FreeBSD and OpenBSD. OpenBSD has
> 
> > CAVEATS
> >     The string returned by login_getcapstr() is allocated via
> >     malloc(3) when the specified capability is present and thus
> >     it is the responsibility of the caller to free() this space.
> >     However, if the capability was not found or an error
> >     occurred and def or err (whichever is relevant) are non-NULL
> >     the returned value is simply what was passed in to
> >     login_getcapstr().  Therefore it is not possible to blindly
> >     free() the return value without first checking it against
> >     def and err.
> 
> NetBSD is idential to OpenBSD. I guess we'll need to special-case
FreeBSD
> and anything else that derives from that codebase. Does anyone know what
> else does it the FreeBSD way? (I'm guessing Dragonfly...)
MidnightBSD, perhaps? laffer1, can you comment?

bye,
//mirabilos
-- 
?MyISAM tables -will- get corrupted eventually. This is a fact of life. ?
?mysql is about as much database as ms access? ? ?MSSQL at least descends
from a database? ?it's a rebranded SyBase? ?MySQL however was born from a
flatfile and went downhill from there? ? ?at least jetDB doesn?t claim to
be a database?	(#nosec)    ??? Please let MySQL and MariaDB finally die!