Fix several memory-safety issues in the C API surface - #219
Open
djw8605 wants to merge 1 commit into
Open
Conversation
- scitoken_free_string_list read value[idx] after freeing value[idx-1], running one element past the allocation for an empty (terminator-only) list; rewrite as a bounded loop and make null input a no-op. - convert_acls left the ACL array tail (including the terminator) uninitialized while filling it; on a strdup failure the cleanup path handed uninitialized pointers to enforcer_acl_free. Use calloc, check the allocation, and check each strdup before moving on. - scitoken_get_claim_string_list: check the list allocation and use calloc so partially-built lists are always terminated. - scitoken_get_claim_string dereferenced a NULL token handle (every sibling function rejects NULL); also reject null key/value pointers. - scitoken_create(NULL) bound a C++ reference through a null pointer (undefined behavior); construct with an empty key instead, matching the documented 'NULL for unsigned token' usage. - SciToken stored its signing key as a reference; the internal key in scitoken_deserialize lives on the stack, leaving a dangling reference in the returned token (serialize() after deserialize read freed stack memory). Own the key by value. - SciToken::get_claim*/get_claim_list used operator[], silently inserting empty claims into the token on lookup misses; use find(). - sqlite3_column_text results were passed to std::string's const char* constructor without a null check in get_public_keys_from_db and get_jwks_metadata (undefined behavior on OOM/NULL). - Enforcer::scope_validator walked the iterator past scope.end() on trailing whitespace and then emitted a bogus ACL with an empty authorization string. Adds regression tests for the trailing-whitespace scope and the empty string list round-trip. Co-Authored-By: Claude Fable 5 <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.
A bundle of small, related memory-safety fixes found while auditing the C API:
Out-of-bounds / uninitialized memory
scitoken_free_string_list: thedo/whilereadvalue[idx]after the terminating NULL — for an empty (terminator-only) list this reads one element past the allocation. Rewritten as a bounded loop; NULL input is now a no-op.convert_acls:malloc'd array was filled left-to-right with the tail (including the terminator) uninitialized; onstrdupfailure the cleanup handed uninitialized pointers toenforcer_acl_free(wild frees). Now uses checkedcallocand checks eachstrdupbefore proceeding.scitoken_get_claim_string_list: uncheckedmalloc→ checkedcalloc(partially-built lists always terminated).Null-pointer UB
scitoken_get_claim_stringdereferenced a NULL token handle — the only getter without the standard NULL check. Also rejects nullkey/value.scitoken_create(NULL)bound a C++ reference through a null pointer (UB; the test suite itself calls this). Now constructs with an empty key, matching the header's documented "NULL for unsigned token".sqlite3_column_textresults were passed tostd::string(const char*)without a null check inget_public_keys_from_dbandget_jwks_metadata(the third call site already checks).Dangling reference
SciTokenstored its signing key asSciTokenKey&.scitoken_deserializepasses a stack-local key, so the returned token held a dangling reference —scitoken_serializeafter deserialize read dead stack memory. Same for callers destroying their key handle before serializing. The key is now owned by value (four small strings; keys have no mutators, so no behavior change).SciToken::get_claim*usedoperator[], silently inserting empty claims into the token on every lookup miss; now usesfind().Iterator UB
Enforcer::scope_validatorwalked its iterator pastscope.end()on trailing whitespace, and then emitted a bogus ACL with an empty authorization string intogenerate_aclsoutput.Testing
EnforcerScopeTrailingSpaceTest(exactly one ACL from"read:/foo ") andEmptyStringListTest(empty list round-trip + free).ctestunit, env_config, and monitoring suites pass.🤖 Generated with Claude Code