Skip to content

SecUploadFileMode parsed as decimal instead of octal; temp files have no read permission #3580

@LeShaunJ

Description

@LeShaunJ

Description

Multipart upload temporary files (the ones referenced by FILES_TMPNAMES) are created with incorrect permissions. With the default SecUploadFileMode 0600, the resulting temp file has mode 1130 (---x-wx--T) instead of 0600 (-rw-------). All read bits are stripped, so @inspectFile scripts, and any other consumer of the temp file, cannot read the uploaded file.

Root Cause

The octal string 0600 is being parsed as the decimal number 600, and 600(decimal) == 1130(octal). The value is then passed straight to fchmod().

This is a regression: it works on v3.0.14 and is broken on v3.0.15.

Logs & Dumps

Note

Debug logs don't help here, as they simply show everything working fine.

To Reproduce

Steps to reproduce the behavior:

  1. modsecurity.conf:
    SecRequestBodyAccess On
    SecUploadDir /tmp/
    SecTmpDir /tmp/
    SecUploadKeepFiles On
    SecUploadFileMode 0600
    SecRule FILES_TMPNAMES "@inspectFile /path/to/script.sh" \
        "id:1234,phase:2,deny"
  2. Upload a file:
    curl -F "file=@sample.txt" http://localhost/
  3. Inspect the temp file in SecUploadDir:
    ls -l /tmp/*-file-*
    -> mode is 1130, not 0600.
  4. The @inspectFile script cannot read the file (no read bit), so inspection can either fail or behave unexpected.

Expected Behavior

The temp file is created with the configured octal mode (0600 -> -rw-------), as documented ("The default mode (0600) only grants read/write access to the account writing the file"), so @inspectFile and other consumers can read it.

Server (please complete the following information):

Rule Set (please complete the following information):

  • Running any public or commercial rule set? Not required to reproduce; a single custom @inspectFile rule is enough.
  • What is the version number? N/A

Additional Context

Confirmed against current v3/master, the mode is applied verbatim:

int mode = m_transaction->m_rules->m_uploadFileMode.m_value;
if ((m_tmp_file_fd != -1) && (mode != 0)) {
#ifndef WIN32
if (fchmod(m_tmp_file_fd, mode) == -1) {
#else
if (_chmod(m_tmp_file_name.c_str(), mode) == -1) {
#endif
m_tmp_file_fd = -1;
}
}

fchmod() treats mode as literal permission bits, so m_uploadFileMode.m_value must contain octal 0600 (384(decimal)). Instead it contains 600 decimal (or 01130(octal)), which is exactly "0600" parsed in base 10.

The mis-parse is in the directive handling, in src/parser/seclang-parser.yy:

  • SecAuditLogFileMode is parsed as octal:
    driver.m_auditLog->setFileMode(strtol($1.c_str(), NULL, 8));
  • SecUploadFileMode is parsed via the generic integer config (base 10):
    if (driver.m_uploadFileMode.parse(std::string($1), &errmsg) != true) {
    driver.error(@0, "Failed to parse SecUploadFileMode: " + errmsg);
    YYERROR;
    }

    (immediately adjacent to m_uploadFileLimit.parse(...), where base 10 is
    correct because it is a count, not a permission mask.)

So "0600" -> 600(decimal) -> fchmod(fd, 600) -> 01130(octal). Besides breaking readers of FILES_TMPNAMES, this silently produces unintended/insecure permission bits for any configured SecUploadFileMode value (here: sticky bit set, group gains write+execute, owner loses read/write).

Workaround

Any @inspectFile script can simply ensure the file at the path passed to it has read access. Despite the discrepancy, it nonetheless has the authority to do so.

Suggested Fix

Parse SecUploadFileMode as octal: mirror SecAuditLogFileMode's strtol(..., NULL, 8), or have the upload-file-mode config parse with base 8 (or base 0 to honor a leading "0").

Metadata

Metadata

Assignees

No one assigned

    Labels

    3.xRelated to ModSecurity version 3.x

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions