*Binary is SUID Root*
The binary is setuid root, so any vulnerability within the program acts as a root-level exploit and the file is writing the local filesystem with root permissions. Although this isn't directly a vulnerability, it makes all vulnerabilities detailed below have high impact.
*File Creation Race Condition*
*Symbolic Link Arbitrary File Write*
Two vulnerabilities exist here, but are mitigated by the same fix.
The call to `access()` is being used to check for the existance of the file prior to opening it for writing. If `MY_TMP_FILE` is a symbolic link, this allows for an attacker to change the path checked vs. what path is written between these operations. This allows an attacker to bypass the check for 'does this file exist', and create arbitrary files on the system for writing. Such an attack is commonly used to write cron jobs or similiar to allow for code execution as root.
```
// Both function calls follow symbolic links
if(!access(MY_TMP_FILE, F_OK)){
// Race for file existing occurs here
tmpFile = fopen(MY_TMP_FILE, "w");
```
This allows for two attack scenarios:
- Arbitrary creation of a file by racing the `access()` check and moving the symbolink link
- `MY_TMP_FILE` being a symolic link allowing for arbitary file writes
Recommedation: Additional flags need to be added to prevent symbolic link opening and to make sure the file exists within our fopen call, so we can remove the `access()` call. Specifying `O_NOFOLLOW` as a flag to prevent following the symbolic link and the `O_CREAT`should *not* be used (This is inversed with fopen). The usage of `access()` should be removed to prevent the race condition, as our call to `fopen()` will already validate the existence of the file. For fopen, the `l` flag is equivelent to O_NOFOLLOW, and `x` is `!O_CREAT`
Example:
```fopen(MY_TMP_FILE, "wlx")```
*Format String Vulnerability*
Passing of arbitary user supplied data `argv[1]` allows for an attacker to provide any format string specifiers to the format-string aware function fprintf. On various platforms this can lead to code execution
Recommendation: Utilize `fwrite()` to avoid the format string usage. Another alternative is FormatGuard if glibc is used.
*Return Values not checked on `fwrite` and `fclose`*
The return values on each operation writing the file are not checked, so in teh case of a failure during this process it may enter an undefined state, or the data will not be written correctly.
Recommendation: Check appropriate return values for success and fail safely along the way.
------------
Remediated Code:
```
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>
#include <limits.h> // This is included for ARG_MAX
#define MY_TMP_FILE "/tmp/file.tmp"
int main(int argc, char **argv){
int tmpFile;
int writeLen;
if (2 != argc){
return EXIT_FAILURE;
}
// Access check removed since our flags with fopen resolve this
tmpFile = fopen(MY_TMP_FILE, "wlx");
if(tmpFile == NULL){
printf("fopen failed\n");
return EXIT_FAILURE;
}
// This strnlen isn't neccessary, as the data will be ARG_MAX at most
// but added for safety and I never use strlen.
// This is fine since argv is always null terminated
// Store the amount, in bytes, we want to write
writeLen = strnlen(argv[1], ARG_MAX);
// fwrite doesn't garuntee full or sucessful writes, so check here
// we write n 1-byte elements so we can compare our writeLen as bytes
// We can alternatively loop here on successful partial writes,
// to complete the full write in multiple fwrite() calls
if(fwrite(tmpFile, argv[1], 1, writeLen) != writeLen) {
printf("fwrite failed to write all values successfully\n");
fclose(tmpFile); // We do not check fclose success here, as we are already bailing
return EXIT_FAILURE;
}
if(fprintf(tmpFile, "\n") != 1) {
printf("fwrite failed\n");
// We do not check fclose success here, as we are already bailing
fclose(tmpFile);
return EXIT_FAILURE;
}
if(fclose(tmpFile) != 0) {
printf("fclose() failed, write may have failed.");
return EXIT_FAILURE;
}
return EXIT_SUCCESS;
}
```