Justin Pryzby wrote:
On Thu, Mar 24, 2005 at 03:55:06PM +0300, Michael Tokarev wrote:
Package: libpam-ssh
Version: 1.91.0-5
Severity: critical
A long time ago (circa 1998 or so) I looked at pam-ssh project and
noticied several problems with it. And since it's now in Debian,
the same problems applies to Debian too.
Here's one.
in pam_sm_authenticate() routine, pam_ssh saves struct passwd as
a pam variable, this way (error checking removed for simplicitly):
Are any other getpwnam()-type functions actually called, allowing the
buffers to be overwritten?
Well, all pam modules and libpam itself does alot of getpw*() calls.
An application may call them too ofcourse, between pam auth and
session calls.
Documentation is not clear about how the fields of the struct are
allocated; a minimal test indicates that they are malloc()ated for the
first call only, but that implementation could change; I don't know if
there are relevant standards, and it may be that it is intentionally
"opaque".
It's really implementation-dependant. In glibc, they're implemented
as wrappers around getpwent_r(); a buffer "sufficiently" large will
be allocated on the first call, and if that buffer isn't sufficient
for the next line in /etc/passwd, it will be reallocated (while
scanning the file). Other implementations may use diffefent techniques.
It does seem like best-practice would be to copy the entire contents
of the structure, and not just the pointers.
Well, basically yes. With the only small "issue": other systems may
have additional fields in the structure, which will not be copied
without quite a bit of autoconf magic. I can't say from memory,
but I've seen HP/UX or NetBSD had several "non-standard" fields in
struct passwd. So, at least, such copying have to be done with
care.
The struct passwd pointer in pam_sm_open_session() is passed into
openpam_borrow_cred() -- and nothing stops this last routine from
using those additional fields on a platform which actually has
them. I think... ;)
Well, ok, as I don't have any direct reference to those "extra fields"..
maybe the problem isn't a problem after all ;)
Luckly, most (depending on the other modules in the PAM stack) getpw*
calls will be the same as this module does, and hence the problem
will not occur.
I think you are implying that every libpam module shares memory space
with every other libpam module, which AIUI is incorrect. libpam are
ELF shared objects, and every process that links with such an object
(at runtime) will get its own private copy of the data section, but
the text section will be shared (mmaped, really, it is backed by
disk).
I mean that other modules in the stack will most likely call getpwnam()
with the same argument, the same username which is being logged into.
So, provided the user info has not changed since pam-ssh call, the
whole structure/fields layout will be the same again. But if some
other module or application itself will call getpwnam() with different
username, the memory layout will change.
Does your claim still stand? Does *that* module call getpwnam()-type
functions multiple times, without memcpy the pointers, and then reuse
the top-level pointer?
It does not matter which module performs any calls to getpw* routines.
I pointed this problem out to the author the same time I looked at
the module, but instead of an ACK he replied with something like
"If you don't like my program write your own". Later on, he changed
logic a bit -- previously he where saving the pwent pointer, now
he saves the whole structure (as pwent_keep), but the same problem
is still here.
Right; it is a static buffer, and multiple calls to getpwnam() return
the same pointer, *and* the pointer structure fields always point to
the same place.
Well.. not into the same place, if it's the different username.
The line from /etc/passwd (whatever) is read into that buffer,
and when parsed into fields. And pointers are initialized to
point to the found fields, to whatever place they're at.
If you parse the same line, the pointers will be exactly the
same. If you parse different line, the pointers will be
different. One way or another, no one guaranteed that content
of the memory in question will be valid and the same after
other module work. The content of the structure and pointed-to
memory is valid only up to the next call to getpw*() routine.
It seems that your request can be easily satisfied by using the
reentrant versions of these functions, like getpwnam_r. I'm including
a test file I've been playing with, which indicates that a patch, if
necessary, would be unintrusive.
Yes, getpwnam_r will work. It's a PITA to deal with, and it may
not be sufficiently portable, but it will work.
But: why are you trynig to save the pwent pointer in the first
place? To remove extra getpwnam() call? It will be done by other
modules and by the application too, anyway... Why not just call
it again in pam_sm_open_session(), based on pam_get_user() ?
BTW, there was a discussion on linux-pam list several years ago,
concerning this very issue -- calling all those get{pw,gr}*()
routines multiple times and mixing up the buffers between
modules and application. No real proposal emerged, the discussion
was largely about using getpwnam_r() and implementing pam_getpwnam()..
which turned out to be too ugly... ;)
/mjt
--
To UNSUBSCRIBE, email to [EMAIL PROTECTED]
with a subject of "unsubscribe". Trouble? Contact [EMAIL PROTECTED]
