Address CodeQL findings: fix real overflows, harden test file modes - #5
Merged
Conversation
Both seer_odbc_pos_update (exec.c) and SQLGetTypeInfo (catalog.c) build a SQL statement with the "o += snprintf(buf + o, sizeof buf - o, ...)" idiom. snprintf returns the length it WOULD have written, so once the text exceeds the fixed buffer, o passes sizeof buf, "sizeof buf - o" underflows to a huge size_t, and the next write runs out of bounds. In exec.c the appended text is schema-derived (base table + bound column names + count), so a wide updatable table can genuinely exceed the 4096 buffer and trip the overflow. In catalog.c the input is the static TYPEINFO table (bounded, not attacker-influenced), so it is safe today, but the idiom is identical and worth hardening. Guard every append: check snprintf's return against the remaining space before advancing, and fail with a diagnostic if the statement does not fit, so the offset can never pass the buffer end. Addresses CodeQL cpp/overflowing-snprintf (exec.c:779, catalog.c:212). Verified: unit suite green; live integration under ASan on 11g passes the FOR UPDATE / positioned-update path (40/0/11), no sanitizer report. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
seer_iconv computed its output buffer as `in_len * 2 + 16` and grew it with `cap * 2`, both of which wrap size_t for pathological in_len/cap. A wrapped, undersized allocation would then be overflowed by the iconv loop writing into it. Reject sizes whose computation cannot be represented in size_t before allocating (initial cap and the realloc doubling). These guards reject only unrepresentable sizes - never a real in-memory string length - so no legitimate conversion is affected. Addresses CodeQL cpp/uncontrolled-allocation-size at charset.c:26 and charset.c:44. The other alerts in that rule (dalc-derived mallocs in stmt.c / marshal.c / lob.c) are bounded by seer_reader_bytes against the bytes actually received (and by uint8_t/uint16_t length fields), so they are false positives handled separately, not with fabricated caps. Verified: unit suite green; live integration under ASan on 11g clean (40/0/11), exercising the national-charset / Unicode conversion paths. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
setup_browse_dsn wrote the temporary odbcinst.ini / odbc.ini via fopen(path, "w"), which creates them with the umask-default mode (0666, i.e. world-readable/0644 under a typical umask). They live inside a 0700 mkdtemp dir so this was never exploitable, but a config file has no business being world-accessible. Add a small fopen_private() helper (open O_CREAT with mode 0600, then fdopen) and use it for both files. Verified the created files are 0600. Addresses CodeQL cpp/world-writable-file-creation (test_integration.c :1304, :1306). Test-only code, not shipped in the driver. Verified: SQLBrowseConnect check passes under ASan on 11g (40/0/11). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Works through the open CodeQL code-scanning alerts. Each commit groups similar findings and was validated locally (unit suite + live integration under AddressSanitizer). The full 5-server matrix (10g/11g/21c/23ai/9i) is green under ASan on the branch tip, unchanged from baseline.
Commits — the genuinely actionable findings
odbc: guard snprintf accumulation against buffer overflow—cpp/overflowing-snprintfatexec.c:779andcatalog.c:212. Theo += snprintf(buf+o, sizeof buf - o, ...)idiom overflows once the text exceeds the buffer (opassessizeof,sizeof buf - ounderflows to a hugesize_t, next write is out of bounds). Inexec.cthe text is schema-derived (wide updatable table) so this is a real latent overflow;catalog.cis bounded by a static type table but shares the idiom. Every append is now guarded.common: guard iconv buffer-size arithmetic against size_t overflow—cpp/uncontrolled-allocation-sizeatcharset.c:26and:44.in_len*2+16andcap*2could wrapsize_tinto an undersized buffer that the iconv loop then overflows. Rejects only unrepresentable sizes, so no real conversion is affected.test: create browse-DSN config files mode 0600—cpp/world-writable-file-creationattest_integration.c:1304/1306. Test-only config files were created with the umask-default mode; now0600via anopen(…,0600)+fdopenhelper.Not fixed here — triaged as non-bugs (dismissed with justification)
After reading each site, the remaining alerts are false positives, protocol requirements, or harmless style:
mallocbetween thefreeand the use. False positive.SQLSetStmtAttr/SQLBindCol. False positive.seer_reader_bytesagainst bytes actually received, or byuint8/uint16length fields;writer.cis a growable-buffer size hint. False positive (no fabricated caps that would break largeLONG/LOB values).argv[++i]idiom, and a commented boolean. Not bugs.🤖 Generated with Claude Code