How buffer overflow via sprintf() happens in C++ settings parsing and how to fix it

How buffer overflow via sprintf() happens in C++ settings parsing and how to fix it

Introduction

In app/src/main/cpp/samp/settings.cpp, a critical buffer overflow vulnerability was discovered at line 15 inside the CSettings::CSettings() constructor. The code declares a 127-byte stack buffer (char buff[0x7F]) and then uses sprintf() to format a file path into it—concatenating the global g_pszStorage pointer with the hardcoded suffix "SAMP/settings.ini". Because sprintf() performs no bounds checking, an attacker who can influence g_pszStorage (through memory corruption, initialization manipulation, or controlled storage paths) can overflow the buffer, corrupt the stack, and potentially achieve arbitrary code execution.

This pattern is deceptively common in C/C++ codebases—especially in Android NDK projects where native code handles file path construction. The suffix "SAMP/settings.ini" alone consumes 18 bytes plus a null terminator (19 bytes total), meaning any g_pszStorage value longer than 107 characters will overflow the 127-byte buffer.

The Vulnerability Explained

Here is the vulnerable code from settings.cpp:

CSettings::CSettings()
{
    FLog("Loading settings..");

    char buff[0x7F];
    sprintf(buff, "%sSAMP/settings.ini", g_pszStorage);

    INIReader reader(buff);
    // ...

    // client
    size_t length = 0;
    sprintf(buff, "__android_%d%d", rand() % 1000, rand() % 1000);
    // ...
}

Why this is dangerous:

1. Fixed-size buffer: char buff[0x7F] allocates exactly 127 bytes on the stack.
2. Unbounded write: sprintf() will write as many bytes as the format string produces—there is no mechanism to stop at 127 bytes.
3. External input in the format: g_pszStorage is a global pointer set during application initialization. Its value derives from the Android storage path, which could be manipulated.

Exploitation scenario:

Consider an attacker who can influence g_pszStorage—for example, through a prior memory corruption vulnerability, a malicious intent that sets the storage directory, or by exploiting a race condition during initialization:

g_pszStorage = "/data/data/com.example.app/files/AAAAAAAAAAAA...AAAA/"  // 120+ chars

When sprintf(buff, "%sSAMP/settings.ini", g_pszStorage) executes:
- The output would be 120 + 18 + 1 (null) = 139 bytes
- The buffer only holds 127 bytes
- The remaining 12 bytes overwrite adjacent stack memory

This stack corruption can overwrite:
- The saved return address → arbitrary code execution
- Local variables → logic manipulation
- Stack canaries (if present) → crash/denial of service

The second sprintf() call at line 28 (sprintf(buff, "__android_%d%d", rand() % 1000, rand() % 1000)) is less exploitable since rand() % 1000 produces at most 3 digits, making the maximum output "__android_999999" (16 bytes). However, it still uses an unsafe pattern that should be corrected for defense-in-depth.

The Fix

The fix replaces both sprintf() calls with snprintf(), passing sizeof(buff) as the maximum number of bytes to write (including the null terminator):

Before:

char buff[0x7F];
sprintf(buff, "%sSAMP/settings.ini", g_pszStorage);

After:

char buff[0x7F];
snprintf(buff, sizeof(buff), "%sSAMP/settings.ini", g_pszStorage);

Before (line 28):

sprintf(buff, "__android_%d%d", rand() % 1000, rand() % 1000);

After (line 28):

snprintf(buff, sizeof(buff), "__android_%d%d", rand() % 1000, rand() % 1000);

How this solves the problem:

snprintf(buff, sizeof(buff), ...) guarantees that at most sizeof(buff) - 1 characters are written, followed by a null terminator. If the formatted string would exceed 127 bytes, it is truncated rather than overflowing into adjacent memory.

• sizeof(buff) evaluates to 0x7F (127) at compile time
• snprintf returns the number of characters that would have been written (useful for detecting truncation)
• The buffer is always null-terminated within its declared bounds

The security invariant is now enforced: buffer writes never exceed the declared length of 127 bytes, regardless of the content of g_pszStorage.

Trade-off: If g_pszStorage is too long, the path will be truncated and INIReader will fail to open the file. This is a safe failure mode—the application reports an error rather than executing attacker-controlled code.

Prevention & Best Practices

1. Never use sprintf() with external or variable-length input. Always prefer snprintf() in C or std::string/std::format in C++.

2. Use sizeof() rather than magic numbers. Writing snprintf(buff, sizeof(buff), ...) ensures the limit stays correct even if the buffer size changes later.

3. Enable compiler warnings. Both GCC and Clang support -Wformat-overflow which can detect some sprintf() overflows at compile time.

4. Enable stack protectors. Compile with -fstack-protector-strong to detect stack buffer overflows at runtime (though this is a mitigation, not a fix).

5. Use static analysis tools. Semgrep, clang-tidy (bugprone-not-null-terminated-result), and Coverity can all flag sprintf() usage on fixed-size buffers.

6. Consider using C++ string types. For path construction, std::string path = std::string(g_pszStorage) + "SAMP/settings.ini"; eliminates buffer management entirely.

7. Audit all sprintf() calls. If one exists in a codebase, there are likely more. The PR notes that line 28 also needed the same fix.

Key Takeaways

• sprintf() on a char buff[0x7F] with g_pszStorage concatenation is exploitable when the storage path exceeds 107 bytes—the 18-byte suffix "SAMP/settings.ini" plus null terminator leaves minimal headroom.
• Both sprintf() calls in CSettings::CSettings() needed fixing—even the second one at line 28 that formats random numbers, because unsafe patterns should never remain in security-sensitive code.
• snprintf(buff, sizeof(buff), ...) is a drop-in replacement that preserves the existing logic while enforcing the buffer boundary at the cost of truncation on overflow.
• Android NDK native code is particularly vulnerable because storage paths vary by device and can be longer on custom ROMs or rooted devices.
• A truncated path that fails to open is always preferable to a buffer overflow—fail safely, not catastrophically.

How Orbis AppSec Detected This

• Source: The g_pszStorage global variable, populated during application initialization with the Android storage directory path
• Sink: sprintf(buff, "%sSAMP/settings.ini", g_pszStorage) at app/src/main/cpp/samp/settings.cpp:15
• Missing control: No bounds checking between the variable-length source (g_pszStorage) and the fixed-size destination (char buff[0x7F])
• CWE: CWE-120 — Buffer Copy without Checking Size of Input
• Fix: Replaced sprintf() with snprintf(buff, sizeof(buff), ...) to enforce the 127-byte buffer limit

Orbis AppSec automatically detected this vulnerability and opened a pull request with the fix. Try Orbis AppSec on your repositories to find and fix issues like this automatically.

Conclusion

Buffer overflows via sprintf() remain one of the most dangerous and prevalent vulnerability classes in C/C++ code. In this case, a 127-byte stack buffer and an unbounded string format created a directly exploitable condition in production mobile application code. The fix—switching to snprintf() with sizeof(buff)—is minimal, backward-compatible, and definitively eliminates the overflow. If your codebase contains any sprintf() calls targeting fixed-size buffers, audit them now. The cost of the fix is negligible; the cost of exploitation is not.

References

• CWE-120: Buffer Copy without Checking Size of Input
• OWASP Buffer Overflow
• snprintf() — POSIX man page
• Semgrep rules for sprintf overflow
• fix: add buffer-length check in settings.cpp (PR link)
• Android NDK Security Best Practices