Fix Time-of-check time-of-use filesystem race condition and Likely overrunning write

[odaysec]

foundationdb/fdbrpc/HTTP.actor.cpp

Line 49 in c289917

sprintf(buf, "%%%.02X", c);

Fix the issue should replace the unsafe sprintf call with snprintf, which allows us to specify the maximum number of characters to write to the buffer, including the null terminator. This ensures that the buffer is not overrun. Specifically:

1. Replace sprintf(buf, "%%%.02X", c); with snprintf(buf, sizeof(buf), "%%%.02X", c); to limit the write operation to the size of the buf array.
2. Ensure that the buffer size is sufficient for the format string and the null terminator. The current size of 4 bytes is adequate for the format "%%%.02X".

The program performs a buffer copy or write operation with no upper limit on the size of the copy. By analyzing the bounds of the expressions involved, it appears that certain inputs will cause a buffer overflow to occur in this case. In addition to causing program instability, techniques exist which may allow an attacker to use this vulnerability to execute arbitrary code.

int sayHello(uint32_t userId)
{
	char buffer[17];

	if (userId > 9999) return USER_ID_OUT_OF_BOUNDS;

	// BAD: this message overflows the buffer if userId >= 1000,
	// as no space for the null terminator was accounted for
	sprintf(buffer, "Hello, user %d!", userId);

	MessageBox(hWnd, buffer, "New Message", MB_OK);

	return SUCCESS;
}

In this the call to sprintf writes a message of 14 characters (including the terminating null) plus the length of the string conversion of userId into a buffer with space for just 17 characters. While userId is checked to occupy no more than 4 characters when converted, there is no space in the buffer for the terminating null character if userId >= 1000. In this case, the null character overflows the buffer resulting in undefined behavior.

To fix this issue these changes should be made:

• Control the size of the buffer by declaring it with a compile time constant.
• Preferably, replace the call to sprintf with snprintf, using the defined constant size of the buffer or sizeof(buffer) as maximum length to write. This will prevent the buffer overflow.
• Increasing the buffer size to account for the full range of userId and the terminating null character.

References

STR31-C. Guarantee that storage for strings has sufficient space for character data and the null terminator STR50-CPP. Guarantee that storage for strings has sufficient space for character data and the null terminator

---

foundationdb/fdbrpc/libeio/eio.c

Lines 2235 to 2236 in c289917

	case EIO_RMDIR:
	req->result = rmdir(path);

Fix the TOCTOU vulnerability use a safer approach that operates on file descriptors rather than file names wherever possible. For rmdir, this can be achieved by opening the directory first and then performing operations on the file descriptor. If the platform does not support such operations, additional measures like locking or verifying the state of the file immediately before the operation can be implemented.

The best fix for this issue involves:

1. Using open to obtain a file descriptor for the directory.
2. Performing the rmdir operation using the file descriptor, ensuring the operation applies to the intended directory.
3. Closing the file descriptor after the operation.

The following shows a case where a file is opened and then, if the opening was successful, its permissions are changed with chmod. However, an attacker might change the target of the file name between the initial opening and the permissions change, potentially changing the permissions of a different file.

char *file_name;
FILE *f_ptr;

/* Initialize file_name */

f_ptr = fopen(file_name, "w");
if (f_ptr == NULL)  {
  /* Handle error */
}

/* ... */

if (chmod(file_name, S_IRUSR) == -1) {
  /* Handle error */
}

fclose(f_ptr);

This can be avoided by using fchmod with the file descriptor that was received from opening the file. This ensures that the permissions change is applied to the very same file that was opened.

char *file_name;
int fd;

/* Initialize file_name */

fd = open(
  file_name,
  O_WRONLY | O_CREAT | O_EXCL,
  S_IRWXU
);
if (fd == -1) {
  /* Handle error */
}

/* ... */

if (fchmod(fd, S_IRUSR) == -1) {
  /* Handle error */
}

close(fd);

References

FIO01-C. Be careful using functions that use file names for identification

Replace this text with your description here...

Code-Reviewer Section

The general pull request guidelines can be found here.

Please check each of the following things and check all boxes before accepting a PR.

• The PR has a description, explaining both the problem and the solution.
• The description mentions which forms of testing were done and the testing seems reasonable.
• Every function/class/actor that was touched is reasonably well documented.

For Release-Branches

If this PR is made against a release-branch, please also check the following:

• This change/bugfix is a cherry-pick from the next younger branch (younger release-branch or main if this is the youngest branch)
• There is a good reason why this PR needs to go into a release branch and this reason is documented (either in the description above or in a linked GitHub issue)

[xis19]

Again, thanks for the contribution! For the sprintf issue, yes, we should use something like snprintf, but if we switch this to fmt::format might be better. Also the example you have provided is pretty much from Windows programming as I see 'hWndandMB_OK` which I would assume most people have already forgotten how to write MFC code these days.

I have no experience in libeio, so I will leave the change there to more experienced people.

[jzhou77]

Performing the rmdir operation using the file descriptor, ensuring the operation applies to the intended directory.

I'm confused. The dirfd is not used?

This doesn't seem to have the TOCTOU problem.