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

Missing Free Call for duress_hash in is_valid_duress_file #34

Closed zakuArbor closed 2 years ago

zakuArbor commented 2 years ago

https://github.com/nuvious/pam-duress/blob/04e607f9ab674c8dbbf27ea8bb59158178a72f69/src/duress.c#L141

I was skimming the code and noticed you forgot to free duress_hash in the function is_valid_duress_file since sha_256_sum allocates memory in the heap, it should be freed before calling return

zakuArbor commented 2 years ago

Confirmed using asan:

$ sudo pam_test $USER
Credentials accepted.
Password: 
AddressSanitizer:DEADLYSIGNAL
=================================================================
==39682==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x000000000000 bp 0x7ffcd3d02e30 sp 0x7ffcd3d025b8 T0)
==39682==Hint: pc points to the zero page.
==39682==The signal is caused by a READ memory access.
==39682==Hint: address points to the zero page.
    #0 0x0  (<unknown module>)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV (<unknown module>) 
==39682==ABORTING

To replicate:

$ git diff Makefile
diff --git a/Makefile b/Makefile
index 6065001..445900e 100644
--- a/Makefile
+++ b/Makefile
@@ -47,18 +47,18 @@ install: $(BIN_DIR)/pam_duress.so $(BIN_DIR)/duress_sign $(BIN_DIR)/pam_test

 $(OBJS): $(OBJ_DIR)/%.o : $(SRC_DIR)/%.c
        mkdir -p $(OBJ_DIR)
-       $(CC) -o $@ $(CFLAGS) -c $< $(LDINCLUDE)
+       $(CC) -g -fsanitize=address -o $@ $(CFLAGS) -c $< $(LDINCLUDE)

 $(BIN_DIR)/duress_sign: $(OBJ_DIR)/duress_sign.o $(OBJ_DIR)/util.o
        mkdir -p $(BIN_DIR)
-       $(CC) -o $@ $^ $(LDLIB)
+       $(CC) -fsanitize=address -g -o $@ $^ $(LDLIB)

 $(BIN_DIR)/pam_duress.so: $(OBJ_DIR)/duress.o $(OBJ_DIR)/util.o
        ld $(LDFLAGS) -o $@ $^ $(LDLIB)

 $(BIN_DIR)/pam_test: $(SRC_DIR)/pam_test.c
        mkdir -p $(BIN_DIR)
-       $(CC) -o $@ $^ $(LDLIB)
+       $(CC) -g -fsanitize=address -o $@ $^ $(LDLIB)

 .PHONY: uninstall
 uninstall:
zakuArbor commented 2 years ago

Using valgrind instead:

zaku@zaku:~/Documents/pam-duress$ valgrind pam_test $USER 2>&1 | tail -n 20
==40403== Warning: invalid file descriptor 1028 in syscall close()
==40403== Warning: invalid file descriptor 1029 in syscall close()
Credentials accepted.
Account is valid.
Authenticated
==40398== 
==40398== HEAP SUMMARY:
==40398==     in use at exit: 5,140 bytes in 17 blocks
==40398==   total heap usage: 5,716 allocs, 5,699 frees, 1,261,860 bytes allocated
==40398== 
==40398== LEAK SUMMARY:
==40398==    definitely lost: 51 bytes in 2 blocks
==40398==    indirectly lost: 0 bytes in 0 blocks
==40398==      possibly lost: 0 bytes in 0 blocks
==40398==    still reachable: 5,089 bytes in 15 blocks
==40398==         suppressed: 0 bytes in 0 blocks
==40398== Rerun with --leak-check=full to see details of leaked memory
==40398== 
==40398== For lists of detected and suppressed errors, rerun with: -s
==40398== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

With fix implemented:

zaku@zaku:~/Documents/pam-duress$ valgrind pam_test $USER 2>&1 | tail -n 20
==40892== Warning: invalid file descriptor 1028 in syscall close()
==40892== Warning: invalid file descriptor 1029 in syscall close()
Credentials accepted.
Account is valid.
Authenticated
==40887== 
==40887== HEAP SUMMARY:
==40887==     in use at exit: 5,108 bytes in 16 blocks
==40887==   total heap usage: 5,716 allocs, 5,700 frees, 1,261,860 bytes allocated
==40887== 
==40887== LEAK SUMMARY:
==40887==    definitely lost: 19 bytes in 1 blocks
==40887==    indirectly lost: 0 bytes in 0 blocks
==40887==      possibly lost: 0 bytes in 0 blocks
==40887==    still reachable: 5,089 bytes in 15 blocks
==40887==         suppressed: 0 bytes in 0 blocks
==40887== Rerun with --leak-check=full to see details of leaked memory
==40887== 
==40887== For lists of detected and suppressed errors, rerun with: -s
==40887== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Seems like there are more memory leaks.