Mailing List Archive: [PATCH] do not free string returned by login

[PATCH] do not free string returned by login_getcapstr

Feb 15, 2021, 10:35 AM

Post #1 of 6 (832 views)

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().

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@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

Re: [PATCH] do not free string returned by login_getcapstr [ In reply to ]

dtucker at dtucker

Feb 15, 2021, 3:19 PM

Post #2 of 6 (832 views)

Permalink

On Tue, 16 Feb 2021 at 05:49, Ed Maste <emaste@freebsd.org> 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().

That seems to be a difference between FreeBSD and OpenBSD, since the
latter says:

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.

--
Darren Tucker (dtucker at dtucker.net)
GPG key 11EAA6FA / A86E 3E07 5B19 5880 E860 37F4 9357 ECEF 11EA A6FA (new)
Good judgement comes with experience. Unfortunately, the experience
usually comes from bad judgement.
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

Re: [PATCH] do not free string returned by login_getcapstr [ In reply to ]

djm at mindrot

Feb 15, 2021, 3:50 PM

Post #3 of 6 (832 views)

Permalink

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@mindrot.org
> https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
>
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

Re: [PATCH] do not free string returned by login_getcapstr [ In reply to ]

djm at mindrot

Feb 15, 2021, 3:57 PM

Post #4 of 6 (832 views)

Permalink

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
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

Re: [PATCH] do not free string returned by login_getcapstr [ In reply to ]

t.glaser at tarent

Feb 15, 2021, 4:08 PM

Post #5 of 6 (832 views)

Permalink

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!
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

Re: [PATCH] do not free string returned by login_getcapstr [ In reply to ]

emaste at freebsd

Feb 16, 2021, 7:18 AM

Post #6 of 6 (829 views)

Permalink

On Mon, 15 Feb 2021 at 18:58, Damien Miller <djm@mindrot.org> wrote:
>
> > 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...)

I'd assume MidnightBSD as another reply suggested.

> actually, this is in the child process so the leak doesn't matter here.
> I think this fix is fine.

That sounds reasonable to me. Probably with a comment pointing out
that we're intentionally ignoring the leak due to OS differences as a
hint to folks looking at static analysis results in the future. (While
looking at this issue on FreeBSD I saw that Coverity has a false
positive flagging it as a leak.)
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev