nuvious / pam-duress

A Pluggable Authentication Module (PAM) which allows the establishment of alternate passwords that can be used to perform actions to clear sensitive data, notify IT/Security staff, close off sensitive network connections, etc if a user is coerced into giving a threat actor a password.
GNU Lesser General Public License v3.0
1.33k stars 39 forks source link

[#34] Clean memory leaks #37

Closed zakuArbor closed 2 years ago

zakuArbor commented 2 years ago

Summary: There are some memory leaks in the code that have been squashed and some additional checks are written to handle if malloc fails. Issue: #34 Test:

$ pam_test $USER
Credentials accepted.
Password: 
Hello World
Account is valid.
Authenticated

Valgrind Results:

$ 2>&1 valgrind pam_test $USER > /dev/null | tail -n 14 | head -n 9
==44655== HEAP SUMMARY:
==44655==     in use at exit: 5,089 bytes in 15 blocks
==44655==   total heap usage: 5,716 allocs, 5,701 frees, 1,261,870 bytes allocated
==44655== 
==44655== LEAK SUMMARY:
==44655==    definitely lost: 0 bytes in 0 blocks
==44655==    indirectly lost: 0 bytes in 0 blocks
==44655==      possibly lost: 0 bytes in 0 blocks
==44655==    still reachable: 5,089 bytes in 15 blocks
zakuArbor commented 2 years ago

Commit: zakuarbor/pam-duress@79b0d31


Tests

bad usage:

$ pam_test
Usage: app [username]

bad password:

zaku@zaku:~/Documents/pam-duress$ pam_test $USER
Credentials accepted.
Password: 
Not Authenticated

duress password:

zaku@zaku:~/Documents/pam-duress$ pam_test $USER
Credentials accepted.
Password: 
Hello World
Account is valid.
Authenticated

real password:

zaku@zaku:~/Documents/pam-duress$ pam_test $USER
Credentials accepted.
Password: 
Account is valid.
Authenticated

Resolves: Memory leak in duress-sign:

... cut ...
Password: 
Confirm: 
Password did not match. Aborting.
==2631== 
==2631== HEAP SUMMARY:
==2631==     in use at exit: 126 bytes in 2 blocks
==2631==   total heap usage: 7 allocs, 5 frees, 10,286 bytes allocated
==2631== 
==2631== LEAK SUMMARY:
==2631==    definitely lost: 6 bytes in 1 blocks

to

zaku@zaku:~/.duress$ valgrind duress_sign ~/.duress/hello.sh
...cut...
Password: 
Confirm: 
Password did not match. Aborting.
==2719== 
==2719== HEAP SUMMARY:
==2719==     in use at exit: 120 bytes in 1 blocks
==2719==   total heap usage: 7 allocs, 6 frees, 10,286 bytes allocated
==2719== 
==2719== LEAK SUMMARY:
==2719==    definitely lost: 0 bytes in 0 blocks
nuvious commented 2 years ago

Thanks for the diligent work, write ups and explanation. Made the review very easy. My c experience is definitely weaker and I hadn't had time to audit for memory leaks so I greatly appreciate your work.

DusanLesan commented 2 years ago

@zakuArbor Why did you revert shebang?

zakuArbor commented 2 years ago

My bad, I wanted to keep PR #36 separate from this PR but I should have realized that would be a bad idea if #36 was merged before #37. I forgot to make my changes in another branch when updating the document so when I was working on #37 I removed it. @DusanLesan Good catch.

@nuvious would it be possible to reopen PR #36 and re-merge the changes? i.e. I reverted the change on this PR without considering #36 would be merged before #37. An error on my part.

zakuArbor commented 2 years ago

Alternatively, someone can make a new PR and push the shellbang back to the documentation.

nuvious commented 2 years ago

Alternatively, someone can make a new PR and push the shellbang back to the documentation.

Hey, just catching up to this thread and I'll see if I can open a PR today under #35