Bug 40729 - xdm causes SEGVs setting up pam_response structure
Summary: xdm causes SEGVs setting up pam_response structure
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Red Hat Linux
Classification: Retired
Component: XFree86
Version: 7.1
Hardware: i386
OS: Linux
medium
high
Target Milestone: ---
Assignee: Mike A. Harris
QA Contact: David Lawrence
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2001-05-15 15:07 UTC by Ben Harris
Modified: 2007-04-18 16:33 UTC (History)
1 user (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2001-11-01 13:28:13 UTC
Embargoed:


Attachments (Terms of Use)
This patch fixes the problem for me. (437 bytes, patch)
2001-05-15 15:08 UTC, Ben Harris
no flags Details | Diff


Links
System ID Private Priority Status Summary Last Updated
Red Hat Product Errata RHSA-2003:066 0 high SHIPPED_LIVE : Updated XFree86 packages provide security and bug fixes 2003-06-25 04:00:00 UTC

Description Ben Harris 2001-05-15 15:07:05 UTC
Description of Problem:
Under some circumstances, xdm can be persuaded to catch a SIGSEGV.  I found
this while using some locally-developed modules (though it was pam_lastlog
that caught the SEGV) and I suspect that crafting a test case using the stock
modules will be tricky since it depends on the state of the malloc arena.

How Reproducible:
Easily if you have the right modules.  I can find the right set if it's really
necessary.

Additional Information:
The problem is in PAM_conv, in xc/programs/xdm/greeter/verify.c:

static int PAM_conv (int num_msg,
                     const struct pam_message **msg,
                     struct pam_response **resp,
                     void *appdata_ptr) {
        int replies = 0;
        struct pam_response *reply = NULL;

        reply = calloc(num_msg, sizeof(struct pam_response));
        if (!reply) return PAM_CONV_ERR;
#define COPY_STRING(s) (s) ? strdup(s) : NULL

        for (replies = 0; replies < num_msg; replies++) {
                switch (msg[replies]->msg_style) {
                case PAM_PROMPT_ECHO_OFF:
                        /* wants password */
                        reply[replies].resp_retcode = PAM_SUCCESS;
                        reply[replies].resp = COPY_STRING(PAM_password);
                        break;
                case PAM_TEXT_INFO:
                        /* ignore the informational mesage */
                        break;
                case PAM_PROMPT_ECHO_ON:
                        /* user name given to PAM already */
                        /* fall through */
                default:
                        /* unknown or PAM_ERROR_MSG */
                        free (reply);
                        return PAM_CONV_ERR;
                }
        }

#undef COPY_STRING
        *resp = reply;
        return PAM_SUCCESS;
}

There are two bugs here.  First, there are references to reply[replies], where
replies can be as large as num_msg-1, even though reply is only allocated
large enough to hold one pam_response.  This could cause heap corruption,
though I've not observed this.

The second problem is that in the case of a PAM_TEXT_INFO message, the resp
field of reply[replies] remains uninitialised, and thus when pam_lastlog
calls _pam_drop_reply() on the structure, it tries to free a random pointer,
with predictably nasty results.

A simple fix is to use calloc rather than malloc, and to pass it the number of
structures we need.  I'll attach a patch for this.

Comment 1 Ben Harris 2001-05-15 15:08:35 UTC
Created attachment 18450 [details]
This patch fixes the problem for me.

Comment 2 Mike A. Harris 2001-05-16 16:40:46 UTC
Nalin, can you look at this patch and comment on it.  Seems ok to me,
but I'd like security guru eyes to pass over it first.

Comment 3 Mike A. Harris 2001-11-01 13:28:07 UTC
I have sent this patch upstream to XFree86 maintainers for application
to the CVS HEAD.  If accepted, it should appear in XFree86 4.2.0.

Comment 4 Mike A. Harris 2002-08-08 07:59:34 UTC
Did not appear in XFree86 4.2.0.  However that area of code did change,
and in an ugly (IMHO) way.

Another bug is that the return pointer from realloc() inside the GETMEM
macro in the new code, gets thrown away instead of being reassigned
to "reply", which assumes the realloc()'d memory will be in the same
location as the originally allocated block.

I've fixed both bugs with a new patch in rawhide 4.2.0-60.4 and later


Comment 5 Mark J. Cox 2003-06-25 15:52:40 UTC
An errata has been issued which should help the problem described in this bug report. 
This report is therefore being closed with a resolution of ERRATA. For more information
on the solution and/or where to find the updated files, please follow the link below. You may reopen 
this bug report if the solution does not work for you.

http://rhn.redhat.com/errata/RHSA-2003-066.html



Note You need to log in before you can comment on or make changes to this bug.