Opened 7 weeks ago

#24081 new defect

Torsocks logging to a file can cause a crash or corrupt application files.

Reported by: cpwc Owned by: dgoulet
Priority: High Milestone:
Component: Core Tor/Torsocks Version:
Severity: Major Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

I ran into problems using torsocks with OpenSSH and tracked down several bugs in torsocks related to logging to a file. Here's a description, a test program to demonstrate the problem, and a patch.

The initial problem was triggered by OpenSSH closing all file descriptors above the first three when it starts up. Some programs do that for good programming hygiene and security. See the BSD closefrom() function for background. When a program that does that is run with torsocks configured to log to a file, the log file descriptor is closed but torsocks doesn't notice. It keeps trying to write to the closed descriptor via the log file pointer as the program runs and doesn't detect the resulting write errors. If the file descriptor isn't reused then the log messages are silently lost. But if the descriptor is later opened for writing by the program then the torsocks log messages are written to the program's file!

There is code intended to detect log write errors in log.c function _log_write(), but it doesn't work. The code looks at the return value from fprintf(), but fprintf() always returns success because the file is opened in buffered mode and the return value from a subsequent fflush() is ignored. So the write errors aren't detected.

I added a call to setbuf() in log_init() to set the log file to unbuffered mode and that exposed further problems. Then log_write() sees fprintf() return an error and calls log_destroy() to clean up. log_destroy() calls fclose() to close the logconfig.fp file pointer and frees the malloced logconfig.filepath but doesn't zero them out, so subsequent logging calls still try to write to the closed fp, log_destroy() gets called again, the filepath is free()'d again (double free). Also the fclose() call goes to torsocks_fclose() which may generate another log message, causing a call loop and crash (SEGV due to stack overrun).

The attached test_log_fd_close.c demonstrates the problem and file corruption. It simply closes file descriptors 3-5, creates some test files and closes them without writing to them, then verifies that they are in fact empty. Compile and link the program with the torsocks library and run it with:

test_log_fd_close

to see normal non-failing behavior, or run it with torsocks logging to a file:

TORSOCKS_LOG_FILE_PATH=./log TORSOCKS_LOG_LEVEL=5 test_log_fd_close

to activate the bugs and see application file corruption.

The attached log_fd_close_notify.patch fixes the problems. The changes are:

  • Add a function log_fd_close_notify() that can be called to notify the log system when an fd is being closed, so it can gracefully close out the log if necessary. Call it from torsocks_close().
  • Add a call to setbuf() in log_init() to make the torsocks log file unbuffered.
  • Don't call fclose() in log_destroy() and instead simply zero out the file pointer to stop future accesses to it. Also zero out the filepath pointer after freeing it to eliminate the potential for references to the freed pointer.
  • Eliminate another call to fclose() in log_init() just by reordering the existing code. A loop there is unlikely but it seems like a good change to make.

I don't understand the torsocks test system to well enough to add test_log_fd_close.c to it. If it's worthwhile maybe someone else can do that.

Child Tickets

Attachments (2)

test_log_fd_close.c (2.0 KB) - added by cpwc 7 weeks ago.
log_fd_close_notify.patch (2.2 KB) - added by cpwc 7 weeks ago.

Download all attachments as: .zip

Change History (2)

Changed 7 weeks ago by cpwc

Attachment: test_log_fd_close.c added

Changed 7 weeks ago by cpwc

Attachment: log_fd_close_notify.patch added
Note: See TracTickets for help on using tickets.