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

Missing Free Call for duress_hash in is_valid_duress_file #34

Closed
zakuArbor opened this issue Jun 24, 2022 · 2 comments
Closed

Missing Free Call for duress_hash in is_valid_duress_file #34

zakuArbor opened this issue Jun 24, 2022 · 2 comments

Comments

@zakuArbor
Copy link
Contributor

unsigned char *duress_hash =

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

zakuArbor added a commit to zakuArbor/pam-duress that referenced this issue Jun 24, 2022
Signed-off-by: Ju Hong Kim <nabimoon1234@gmail.com>
zakuArbor added a commit to zakuArbor/pam-duress that referenced this issue Jun 24, 2022
Signed-off-by: Ju Hong Kim <nabimoon1234@gmail.com>
zakuArbor added a commit to zakuArbor/pam-duress that referenced this issue Jun 24, 2022
…eeded nested else

Signed-off-by: Ju Hong Kim <nabimoon1234@gmail.com>
nuvious added a commit that referenced this issue Jun 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant