Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix pam_get_data stack overwrite by saving a heap pointer instead #188

Merged
merged 1 commit into from
Mar 19, 2019

Conversation

nevun
Copy link
Contributor

@nevun nevun commented Mar 19, 2019

The previous code was using a trick of saving the actual retval value
as the "pointer". The problem with that was when pam_get_data copied
it out it treated it as a void* which is 8 byte on 64 bit operating
system which meant it copied 8 byte to a 4 byte location and overwrote
the stack with 4 bytes.

The fix is using a heap pointer instead, influenced by the official
code in https://github.com/linux-pam/linux-pam/blob/master/modules/pam_unix/pam_unix_auth.c

See this gdb session for "proof" of corruption with the previous code:

Thread 1 "login" hit Breakpoint 3, 0x00007ffff5b25645 in pam_sm_acct_mgmt (pamh=0x5555557632e0, flags=0, argc=3, argv=0x5555557a02e0)
    at pam_yubico.c:1330
1330      int rc = pam_get_data(pamh, "yubico_setcred_return", (const void**)&retval);
1: x/10i $rip
=> 0x7ffff5b25645 <pam_sm_acct_mgmt+72>:        call   0x7ffff5b1d1e0 <pam_get_data@plt>
   0x7ffff5b2564a <pam_sm_acct_mgmt+77>:        mov    DWORD PTR [rbp-0xc],eax
   0x7ffff5b2564d <pam_sm_acct_mgmt+80>:        mov    rcx,QWORD PTR [rbp-0x8]
   0x7ffff5b25651 <pam_sm_acct_mgmt+84>:        mov    rdx,QWORD PTR [rbp-0x118]
   0x7ffff5b25658 <pam_sm_acct_mgmt+91>:        mov    esi,DWORD PTR [rbp-0x110]
   0x7ffff5b2565e <pam_sm_acct_mgmt+97>:        mov    eax,DWORD PTR [rbp-0x10c]
   0x7ffff5b25664 <pam_sm_acct_mgmt+103>:       mov    edi,eax
   0x7ffff5b25666 <pam_sm_acct_mgmt+105>:       call   0x7ffff5b216d3 <parse_cfg>
   0x7ffff5b2566b <pam_sm_acct_mgmt+110>:       cmp    DWORD PTR [rbp-0xc],0x0
   0x7ffff5b2566f <pam_sm_acct_mgmt+114>:       jne    0x7ffff5b2570b <pam_sm_acct_mgmt+270>
(gdb) x/4x $rbp-0xf8
0x7fffffffb9a8: 0x00000000      0x00000000      0x00000000      0x00000000
(gdb) p retval
$2 = 0
(gdb) p &retval
$3 = (int *) 0x7fffffffb9ac
(gdb) set* 0x7fffffffb9a8 = 0x41414141
(gdb) set* 0x7fffffffb9ac = 0x42424242
(gdb) set* 0x7fffffffb9b0 = 0x43434343      
(gdb) 
(gdb) p &retval
$4 = (int *) 0x7fffffffb9ac
(gdb) p retval
$5 = 1111638594
(gdb) x/4x $rbp-0xf8
0x7fffffffb9a8: 0x41414141      0x42424242      0x43434343      0x00000000
(gdb) c
Continuing.

Thread 1 "login" hit Breakpoint 2, 0x00007ffff5b2564a in pam_sm_acct_mgmt (pamh=0x5555557632e0, flags=0, argc=3, argv=
    at pam_yubico.c:1330
1330      int rc = pam_get_data(pamh, "yubico_setcred_return", (const void**)&retval);
1: x/10i $rip
=> 0x7ffff5b2564a <pam_sm_acct_mgmt+77>:        mov    DWORD PTR [rbp-0xc],eax
   0x7ffff5b2564d <pam_sm_acct_mgmt+80>:        mov    rcx,QWORD PTR [rbp-0x8]
   0x7ffff5b25651 <pam_sm_acct_mgmt+84>:        mov    rdx,QWORD PTR [rbp-0x118]
   0x7ffff5b25658 <pam_sm_acct_mgmt+91>:        mov    esi,DWORD PTR [rbp-0x110]
   0x7ffff5b2565e <pam_sm_acct_mgmt+97>:        mov    eax,DWORD PTR [rbp-0x10c]
   0x7ffff5b25664 <pam_sm_acct_mgmt+103>:       mov    edi,eax
   0x7ffff5b25666 <pam_sm_acct_mgmt+105>:       call   0x7ffff5b216d3 <parse_cfg>
   0x7ffff5b2566b <pam_sm_acct_mgmt+110>:       cmp    DWORD PTR [rbp-0xc],0x0
   0x7ffff5b2566f <pam_sm_acct_mgmt+114>:       jne    0x7ffff5b2570b <pam_sm_acct_mgmt+270>
   0x7ffff5b25675 <pam_sm_acct_mgmt+120>:       mov    eax,DWORD PTR [rbp-0xf4]
(gdb) x/4x $rbp-0xf8
0x7fffffffb9a8: 0x41414141      0x00000000      0x00000000      0x00000000
                                               ^^^^^^^ this should not have been overwritten
(gdb) l
1325    pam_sm_acct_mgmt(pam_handle_t *pamh, int flags, int argc, const char **argv)
1326    {
1327      struct cfg cfg_st;
1328      struct cfg *cfg = &cfg_st; /* for DBG macro */
1329      int retval;
1330      int rc = pam_get_data(pamh, "yubico_setcred_return", (const void**)&retval);
1331
1332      parse_cfg (flags, argc, argv, cfg);
1333      if (rc == PAM_SUCCESS && retval == PAM_SUCCESS) {
1334        DBG ("pam_sm_acct_mgmt returning PAM_SUCCESS");
(gdb) p cfg
$6 = (struct cfg *) 0x7fffffffb9b0
(gdb) x/4x $rbp-0xf8
0x7fffffffb9a8: 0x41414141      0x00000000      0x00000000      0x00000000
(gdb) p sizeof(retval)
$7 = 4
(gdb) 

The previous code was using a trick of saving the actual retval value
as the "pointer". The problem with that was when pam_get_data copied
it out it treated it as a void* which is 8 byte on 64 bit operating
system which meant it copied 8 byte to a 4 byte location and overwrote
the stack with 4 bytes.

The fix is using a heap pointer instead, influenced by the official
code in https://github.com/linux-pam/linux-pam/blob/master/modules/pam_unix/pam_unix_auth.c

With feedback from pedro martelletto, thanks.
@klali klali merged commit 9531bc3 into master Mar 19, 2019
@nevun nevun deleted the fix_stack_overwrite branch March 19, 2019 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants