owasp-modsecurity / owasp-modsecurity/ModSecurity
SecUploadFileMode parsed as decimal instead of octal; temp files have no read permission
Nobody has claimed this yet.
- Dominant language
- C++
- Stars
- 9.8k
- Forks
- 1.8k
- Avg merge
- 2h 46m
- Merged PRs (30d)
- 1
Description
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:
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"- Upload a file:
curl -F "file=@sample.txt" http://localhost/ - Inspect the temp file in SecUploadDir:
-> mode isls -l /tmp/*-file-*1130, not0600. - The
@inspectFilescript 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):
- ModSecurity version (and connector):
ModSecurity-v3.0.15ModSecurity-nginx-v1.0.4
- WebServer:
nginx-v1.30.1 - OS (and Distro):
Alpine Linux 3.23.4
Rule Set (please complete the following information):
- Running any public or commercial rule set? Not required to reproduce; a single custom
@inspectFilerule is enough. - What is the version number? N/A
Additional Context
Confirmed against current v3/master, the mode is applied verbatim:
https://github.com/owasp-modsecurity/ModSecurity/blob/f5a6fcd37aea3a7618408b7d1f03b94de9164637/src/request_body_processor/multipart.cc#L89-L98
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:
SecAuditLogFileModeis parsed as octal:
https://github.com/owasp-modsecurity/ModSecurity/blob/f5a6fcd37aea3a7618408b7d1f03b94de9164637/src/parser/seclang-parser.yy#L777SecUploadFileModeis parsed via the generic integer config (base 10):
https://github.com/owasp-modsecurity/ModSecurity/blob/f5a6fcd37aea3a7618408b7d1f03b94de9164637/src/parser/seclang-parser.yy#L861-L864
(immediately adjacent tom_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").
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in src/parser/seclang-parser.yy around the SecUploadFileMode handling and compare it with the nearby SecAuditLogFileMode parsing. Reproduce the configuration with SecUploadFileMode 0600 and an upload, then verify that the temporary file mode is 0600 and readable by the @inspectFile consumer.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- security
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 70/100